Repository navigation
feat: support OpenAPI Overlay 1.2 - #651
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #651 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 301 308 +7
Lines 38415 38781 +366
==========================================
+ Hits 38415 38781 +366
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
daveshanley
left a comment
There was a problem hiding this comment.
Needs changes before this merges. Four P2s and one P3, all inline. Reviewed at cddaee1a.
The core of this is good work. The merge rules finally match the spec (primitive replace, array append, recursive compatibility errors instead of quietly replacing containers). Snapshotting the copy source before mutating is the right call. Keeping $ref resolution local, with no loader and no index, is exactly how it should be. And the wide-merge fix (map only above the threshold, linear scan below it) gets the quadratic scan out without making small merges allocate. Tests are solid, and the overlay, datamodel/low/overlay and datamodel/high/overlay packages plus the root Overlay tests all pass locally.
The problem is strictness. Several new checks reject overlays that applied fine on main, and none of the rejected things affect the result. I ran the same probes through ApplyOverlayFromBytesToSpecBytes on main and on this head:
| input | main | this PR |
|---|---|---|
info.version: 1 or 1.0 |
applies | parse error |
overlay: 1.0 unquoted |
applies | parse error |
overlay: 1.3.0 |
applies | ErrUnsupportedVersion |
extends with a space in the path |
applies | URI error |
copy + update on one action |
copy, then update | no-op, 0 warnings |
The first four break consumers (vacuum apply-overlay included) on upgrade, for no gain. The last one is spec-correct, but it's silent, so people won't find out their overlay stopped doing anything.
A few things I checked that are fine:
- Security: no new I/O.
ResolveExtendsis pure, and references to other documents are rejected. No new exposure. - Performance: one
CloneYAMLNodeper copy action, the property map only exists for wide merges, the version regex compiles once, and an action without$refresolves to itself with no allocation. I didn't re-run the benchmarks, so the µs numbers in the description are yours, not mine. - Memory: nothing long-lived is added. Reusable
updatenodes are shared across references, but every insert path clones, so nothing from the overlay ends up aliased into the target document.
One thing that isn't this PR: YAML aliases in the target (allOf: [*base]). On main, an update through an alias quietly replaced it and lost type: object. Here it errors. That's better, but neither is right. Worth a follow-up to dereference with utils.NodeAlias.
cddaee1 to
d5849aa
Compare
|
Addressed all five review findings in df19201 and d5849aa, replied with evidence, and resolved every inline thread. The full local suite passes with 100% statement coverage and zero uncovered blocks; the three Overlay packages also pass with the race detector. Independent repair review found no issues. Rebased onto main ce94592, so this PR also includes the merged final-line coverage fix. The target YAML-alias behavior noted in the review remains outside these repairs, as the review identifies it as inherited behavior. @greptileai review |
Add Overlay 1.2 document identity and reusable actions, with parsing, rendering, hashing, local reference resolution, and public API coverage for YAML and JSON inputs.
Correct action semantics: primitive replacement, array appends, recursive type compatibility, removal precedence, and snapshot-based copies. Wide object merges use indexed lookup while small merges keep zero lookup allocations. Errors and warnings retain the original action and its source location.
Risk: Medium. These specification corrections also apply to existing 1.0/1.1 overlays. Incompatible container updates now fail; copy and update on one action suppress each other and emit a warning for matched targets; missing targets and malformed structures are rejected. Fixed fields use exact case, Info rejects
$ref, and application renders block YAML. Scalar string values, including unquoted numeric versions, remain accepted. Version strings accept 1. with an optional patch, including later minors, without claiming support for future features. Apply does not validate unused document identifiers; URI resolution validates them when needed. Compatibility details and URI behavior.Validation on d5849aa: full local suite with network tests enabled passed at 100% statement coverage, with zero uncovered blocks. All three Overlay packages also pass with the race detector and 100% coverage. Public API regressions and independent repair review passed. All five review findings are fixed and resolved. Hosted CI is rerunning on this head.
Six-sample wide-merge medians improved from 24.6 µs to 5.05 µs at 200 properties, and 3.33 ms to 72.5 µs at 2,000 properties. Small merges retain zero lookup allocations.