🐛 Apply merge keys before schema validation#188
Conversation
frenck
added
the
bugfix
📝 WalkthroughWalkthroughChangesThis PR adds YAML merge-key ( Sequence Diagram(s)See hidden diagram above. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
There was a problem hiding this comment.
Pull request overview
This PR closes a schema-validation gap by applying YAML merge keys (<<) to the alias-expanded AST before running JSON Schema validation, so validation checks the same effective mapping shape that loads() returns.
Changes:
- Apply merge-key folding to the schema-validation AST after alias expansion (
src/schema/mod.rs). - Reuse the decoder’s key-signature logic for merge dedup by exposing the decode merge module to the crate (
src/decode/mod.rs). - Add Python and Rust regression tests covering merged-shape validation semantics (
tests/features/test_schema.py,src/schema/mod.rs).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/schema/mod.rs |
Applies << merge keys to the resolved AST prior to schema validation; adds Rust unit test coverage. |
src/decode/mod.rs |
Makes the decode merge module pub(crate) so schema validation can reuse key_sig. |
tests/features/test_schema.py |
Adds Python regression tests for merge-key behavior under schema validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #188 +/- ##
==========================================
+ Coverage 92.34% 92.39% +0.05%
==========================================
Files 40 40
Lines 11504 11606 +102
==========================================
+ Hits 10623 10723 +100
- Misses 881 883 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The schema validator ran on the alias-expanded AST but never applied `<<` merge keys, so it checked the literal `<<` mapping instead of the merged shape that `loads()` returns. A type violation carried in through a merge slipped past validation, a `required` property satisfied only by a merge was wrongly flagged, and the literal `<<` key tripped `additionalProperties: false`. Validation now folds merge keys in (in place, after alias expansion) with the same semantics as the fast path: explicit keys win, earlier merges win over later ones, a plain `<<` scalar or an `!!merge`-tagged key is a merge while a quoted `"<<"` stays a literal string. `merge_node_into` returns the leftover to preserve, mirroring the fast path's `merge_into`: a merge value that is a sequence merges its mapping elements and keeps every non-mergeable element under one literal `<<` (as a sequence), not just the first, so the validated shape matches `loads()`. Key signatures reuse the decoder's, so merge dedup here agrees with `loads()` (type-distinct: `1` and `"1"` differ). The decoder's `merge` module is made `pub(crate)` so the validator can share `key_sig`.
frenck
force-pushed
the
frenck/schema-merge-keys
branch
from
c56468d to
0eb6ebd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b7f92370-c699-4d86-8197-9bf83abc5ab8
📒 Files selected for processing (3)
src/decode/mod.rssrc/schema/mod.rstests/features/test_schema.py
| use std::collections::{HashMap, HashSet}; | ||
|
|
||
| mod merge; | ||
| pub(crate) mod merge; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the crate-visible module.
pub(crate) mod merge is now part of the crate-visible Rust API surface; add a /// rustdoc summary.
As per coding guidelines, src/**/*.rs: Keep public Rust items documented with /// rustdoc.
Proposed fix
+/// YAML merge-key helpers shared by the decoder and schema validator.
pub(crate) mod merge;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(crate) mod merge; | |
| /// YAML merge-key helpers shared by the decoder and schema validator. | |
| pub(crate) mod merge; |
Source: Coding guidelines
| def test_merge_key_violation_in_merged_value_is_caught(): | ||
| """A `<<` merge is applied before validation, so a type violation carried in | ||
| through the merge is caught (validation sees what `loads()` returns).""" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one-line test docstrings.
These new test docstrings span multiple lines; please collapse each to a single-line summary.
As per coding guidelines, tests/**/*.py: Every Python test has a one-line docstring describing what it verifies.
Proposed fix
- """A `<<` merge is applied before validation, so a type violation carried in
- through the merge is caught (validation sees what `loads()` returns)."""
+ """Verify merged type violations are caught before validation."""
...
- """`required` met only through a `<<` merge passes, and the literal `<<` key
- is consumed so it is not flagged by additionalProperties: false."""
+ """Verify merged keys satisfy required without tripping additionalProperties."""
...
- """An explicit key wins over the merged one, so the explicit (valid) value is
- what gets validated."""
+ """Verify explicit keys override merged values during validation."""
...
- """A `<<` sequence mixing a mapping and several non-mergeable elements merges
- the mapping and preserves every leftover (not just the first) under one
- literal `<<`, so the validator sees the same shape `loads()` returns."""
+ """Verify merge sequences preserve all leftovers for validation."""Also applies to: 575-577, 598-600, 609-612
Source: Coding guidelines
Breaking change
None for correct usage. Documents that use
<<merge keys are now validated against their merged shape instead of the literal<<mapping, which matches whatloads()returns. A schema that previously passed only because a merged-in violation was invisible will now (correctly) fail; that is fixing a validation hole, not changing valid behavior.Proposed change
The schema validator ran on the alias-expanded AST but never applied
<<merge keys, so it checked the literal<<mapping instead of the merged shapeloads()returns. Three concrete bugs followed: a type violation carried in through a merge slipped past validation, arequiredproperty satisfied only by a merge was wrongly flagged, and the literal<<key trippedadditionalProperties: false.Validation now folds merge keys in (in place, after alias expansion) with the same semantics as the fast path: explicit keys win, earlier merges win over later ones, a plain
<<scalar or an!!merge-tagged key is a merge while a quoted"<<"stays a literal string, and a non-mergeable value is preserved under the literal<<. Key signatures reuse the decoder'skey_sig, so merge dedup here agrees withloads()(type-distinct:1and"1"differ). The decoder'smergemodule is madepub(crate)so the validator can share that helper.Type of change
Additional information
Checklist
uv run pytestpasses locally. A pull request cannot be merged unless CI is green.uv run ruff check .anduv run ruff format --check .pass.cargo fmt --checkandcargo clippy --all-targets -- -D warningspass.If the change is user-facing:
docs/is added or updated, anddocs/verify_examples.pystill passes.