fix: normalize nil group param to prevent NoMethodError in version/di…#441
Open
makaiver wants to merge 9 commits into
Open
fix: normalize nil group param to prevent NoMethodError in version/di…#441makaiver wants to merge 9 commits into
makaiver wants to merge 9 commits into
Conversation
…ffs views When a request is made to /node/version/view or /node/version/diffs without a group= query parameter, params[:group] returns nil. The HAML templates check `@info[:group] != ''` which evaluates to true for nil, then attempt `nil + '/' + node` causing: NoMethodError: undefined method `+' for nil Fix by normalizing params[:group] to '' when absent using `|| ''`, so the existing guards in the templates behave correctly. Also pass nil to get_version when group is empty, preserving the original API contract. Verified against oxidized/oxidized:latest (oxidized-web 0.18.1): curl "http://localhost:8888/node/version/view?node=test&num=1" Before: HTTP 500 NoMethodError After: no crash
The /node/version/diffs route correctly normalises params[:group] to '' and builds a local `group` variable (nil or value) for nodes.version, but then passed @info[:group] (which is '' for ungrouped nodes) directly to both nodes.get_diff calls. In oxidized's yield_repo_and_path, an empty string is truthy, so the path was built as "/node-name" instead of "node-name", causing get_diff to silently return 'no diffs' for all ungrouped nodes. Fix: use the already-correct local `group` variable in both get_diff calls.
- spec/web/node/version_spec.rb: add three tests covering the case where group= is entirely absent from the query string for /node/version/view and /node/version/diffs (the original NoMethodError scenario) - CHANGELOG.md: add Fixed entry under [Unreleased] for the nil group param NoMethodError fix
…oderror-again Handle missing `group` param in version routes and align branch files with RuboCop
8 tasks
…oderror-another-one Guard remaining nil/invalid request params in version and config-search routes to prevent 500s
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pre-Request Checklist
rubocop --auto-correct)rake test)Description
Bug
Requesting
/node/version/viewor/node/version/diffswithout agroup=query parameter causes a 500 in the version view:params[:group]isnilwhen the query string omitsgroup=. Both route handlers inwebapp.rbassigned@info[:group] = params[:group]directly. The HAML templates then check@info[:group] != ''(which istruefornil) and enter the branch that does@info[:group] + '/' + @info[:node], crashing.Reproduction
Against
oxidized/oxidized:latest(shipsoxidized-web0.18.1):No authentication is required and any value for
nodetriggers it./node/version/diffsis affected the same way.A common workaround is to assign a
groupto every device inrouter.dbso the UI/API always emits&group=..., but that is a workaround, not a fix.Fix
Normalize
params[:group]to''when absent in bothGET /node/version/viewandGET /node/version/diffs, so the existing@info[:group] != ''guards inversion.hamlanddiffs.hamlbehave correctly:Tests
Added focused minitest cases in
spec/web/node/version_spec.rbfor both routes that issue a request with nogroup=parameter and assert a 200 plus the no-group branch of the template being rendered.bundle exec rakepasses (rubocop + full minitest suite).