[change] Merging with wrong format now raises Validation error #351#355
Conversation
An attempt to merge config with wrong format will now raise a validation error instead of failing. Fixes openwisp#351
|
coverage: 99.143% (-0.04%) from 99.18% — stktyagi:master into openwisp:master |
It only prevents the original AttributeError crash and correctly lets the .validate() method handle all validation of the final merged configuration. Fixes openwisp#351
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughmerge_config in netjsonconfig/utils.py now only recurses when both existing and new values are dicts; list merging occurs only when both sides are lists (via merge_list). If existing and new values are incompatible container types (dict vs list), merge_config raises a local ValidationError that wraps a jsonschema.ValidationError with an expected/got type message. netjsonconfig/exceptions.py adds guards in _list_errors and omits schema details in str when no errors exist. Tests were added to verify list-overridden-by-scalar behavior and that dict-vs-list merges raise ValidationError. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@stktyagi the description says:
An attempt to merge config with wrong format will now raise a validation error instead of failing.
However, I don't see this in this implementation, nor in the test. No validation error is raised, it looks as if the error is silently ignored, which is not what I was expecting.
Why this inconsistency? Is this intentional?
nemesifier
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/openwrt/test_default.py`:
- Around line 249-272: The test test_merge_invalid_format currently calls
o.validate() without asserting failure; update it to expect a ValidationError by
wrapping the call in a pytest.raises(ValidationError) context (or using the test
framework's equivalent) and ensure ValidationError is imported from the module
that defines it; reference the OpenWrt class and its validate() call (i.e., wrap
OpenWrt(...).validate() in the raises block) so the test will fail if no
ValidationError is raised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f361efe-c095-4ed5-88cc-86211f95ba8a
📒 Files selected for processing (1)
tests/openwrt/test_default.py
Merge_config now raises ValidationError when templates have incompatible types for the same key (e.g. list vs dict), instead of silently overwriting. Test updated to assert ValidationError. Fixes openwisp#351
@nemesifier, I must have missed this while updating all my PRs in feb. These changes don't make sense. I apologise for the oversight, I've added changes as per requirements. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netjsonconfig/utils.py`:
- Around line 30-40: The current check in merge_config rejects any differing
scalar types; change it so scalar overrides are allowed and only container-shape
conflicts raise. In the block handling key/value merges (referencing
merge_config, result, key, value and merge_list), replace the broad
type-mismatch check with logic that only raises a ValidationError when one side
is a container (dict or list) and the other is not the same container type
(e.g., dict vs list or dict vs scalar), allowing differing primitive scalars
(int vs str vs bool) to overwrite the template.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f65b1a5-55aa-4fc8-b948-073e80837fd2
📒 Files selected for processing (2)
netjsonconfig/utils.pytests/openwrt/test_default.py
📜 Review details
🔇 Additional comments (1)
tests/openwrt/test_default.py (1)
250-273: Good regression coverage.This reproduces the dict-vs-list
dhcpclash from the bug report and now asserts the expectedValidationErrorat merge time.
Narrowed the incompatible type check in merge_config to only raise ValidationError when a dict or list conflicts with a different type. Scalar overrides (e.g. str to int) are still allowed as before. Fixes openwisp#351
There was a problem hiding this comment.
The direct reproduction from issue #351 now raises the project ValidationError, but I found two remaining problems that should be fixed before merge. See my comments below.
| and type(value) is not type(existing) | ||
| ): | ||
| raise ValidationError( | ||
| JsonSchemaError( |
There was a problem hiding this comment.
This constructs a JsonSchemaError with only a message. That gives the right exception type, but netjsonconfig.exceptions.ValidationError.__str__ later assumes the wrapped jsonschema error has iterable validator_value and context. For this path, str(exc) raises TypeError: 'Unset' object is not iterable, so an uncaught error is shown as <exception str() failed> instead of the validation message. Please either make the wrapper tolerate message only errors, or build the wrapped error with the fields that __str__ expects. The test should also assert that the raised error can be stringified.
| result[key] = merge_config(existing, value, list_identifiers) | ||
| elif isinstance(value, list) and isinstance(existing, list): | ||
| result[key] = merge_list(existing, value, list_identifiers) | ||
| elif ( |
There was a problem hiding this comment.
This guard applies at every recursive merge level, not only to the top level package shape conflict from issue 351. It now rejects cases where a matched custom UCI block overrides a list option with a scalar option, even though the merge contract says simple values in the config overwrite values in the template. For example, a template dnsmasq block with server: ["1.1.1.1"] and device config with server: "8.8.8.8" used to merge, but now raises because existing is a list and value is a string. Please scope the validation to the package shape conflict or other cases that would otherwise crash, unless this broader behavior change is intentional and documented.
Make NetJsonConfigException tolerant of message-only jsonschema errors to prevent TypeError during stringification. Scope type mismatch validation in merge_config strictly to complex shape conflicts (dict vs list).
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
Proposed change log entry: |
nemesifier
changed the title
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The implementation correctly fixes issue #351 by adding type validation during config merging. Highlights✅ Type safety: The new check properly detects incompatible types (dict vs list) and raises a clear ✅ Backward compatible: Scalar values can still override dicts/lists (existing behavior preserved) - only dict↔list mismatches are blocked ✅ Error handling: Added null checks in ✅ Well tested: Both unit tests ( ✅ Clear error messages: The error indicates which key has the problem and shows expected vs received types Files Reviewed (4 files)
The fix is minimal, focused, and addresses the exact issue described. Good work! 🎉 Reviewed by kimi-k2.5 · 148,794 tokens |
3 participants
An attempt to merge config with wrong format will now raise a validation error instead of failing.
Fixes #351
Checklist
Reference to Existing Issue
Closes #351
Description of Changes
Added conditionals to check if two template types can be merged and if not raise validation error instead of failing.