Repository navigation
Fix composed schema aliases, empty Arazzo extensions, and header examples - #701
daveshanley wants to merge 5 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #701 +/- ##
===========================================
- Coverage 100.00% 99.99% -0.01%
===========================================
Files 315 315
Lines 39762 39765 +3
===========================================
+ Hits 39762 39763 +1
- Misses 0 1 +1
- Partials 0 1 +1
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
daveshanley
left a comment
There was a problem hiding this comment.
Solid set of fixes. The header scope change is exactly right (and now matches parameter.go and media_type.go), the redundant BuildModel in buildSchema was dead weight (Schema.Build already calls it), and the sibling alias fixture is a tight reproduction.
One real regression to fix before merge (the inline bundler can now store a nil schema), one guard that's narrower than the panic it's dodging, and one question on refPointer. Details inline.
| _ = low.BuildModel(node, &schema) | ||
| ctx := context.Background() | ||
| err := schema.Build(ctx, node, idx) | ||
| if err := schema.Build(ctx, node, idx); err != nil { |
There was a problem hiding this comment.
Returning the error here is the right call, but there's a second caller that wasn't updated. inline_references.go:71 does:
schema, _ := buildSchema(target.node, target.index)
model.Components.Schemas.Set(componentName, schema)Before this change schema was never nil. Now a schema that fails to build drops a nil *SchemaProxy into components, and the error is thrown away. That's a panic waiting for render time.
Handle the error there (return it, or skip the target), and add a test that pushes an unresolvable ref through the inline path.
Small nit while you're in here: line 643 still returns a non-nil proxy alongside a sch.Build error. Make both paths behave the same way.
There was a problem hiding this comment.
Fixed in c765799. The inline bundler now returns the buildSchema error with the target name attached, so nothing nil goes into components. Covered by TestPrepareCollectedInlineReferences_SchemaBuildErrorReturned.
Retracting the nit about line 643. SchemaProxy.Build always returns nil, so that error can never be non-nil and a guard would be dead code.
| continue | ||
| } | ||
| } | ||
| // Mapped references identify targets, not the outgoing $ref on an alias. |
There was a problem hiding this comment.
I like this. Mapped refs describe targets, so they shouldn't be touching the alias's outgoing $ref. It's a cleaner fix than the rewiredRefNodes guard it replaces.
One side effect got dropped though. rewireRef also seeds pr.refPointer when it's empty, and inlineProcessRef (bundler.go:790) reads it in the "inline anything that could not be recomposed" pass. Mapped refs no longer go through that path.
If every processRef is always reachable from at least one sequenced ref, we're fine. Is that guaranteed? If it is, a one-line comment saying so would help. If it isn't, seed refPointer here too.
There was a problem hiding this comment.
Fixed in c765799. Pulled the seeding into seedRefPointer and call it from both loops. I could not construct a target that is only reachable through a mapped ref, so this closes the gap without a test proving the gap was real.
| if err = valueNode.Encode(value); err != nil { | ||
| // Encoding an empty null node as a standalone document panics in go-yaml. | ||
| // The value is already cloned, so retain its comments and source metadata. | ||
| if n, ok := value.(*yaml.Node); ok && n != nil && n.Kind == yaml.ScalarNode && n.Tag == "!!null" && n.Value == "" { |
There was a problem hiding this comment.
This guard is narrower than the bug. I checked against pb33f/go-yaml v0.1.1:
| node | Encode |
|---|---|
{Kind: Scalar, Tag: "!!null"} |
panic |
{Kind: Scalar} (no tag) |
panic |
{Kind: Scalar, Tag: "!!null", Value: "~"} |
ok |
{Kind: Scalar, Tag: "!!str"} |
ok |
So an empty scalar built in code without a tag still goes bang. Drop the n.Tag == "!!null" check, or just pass every cloned *yaml.Node straight through (it's already a clone, and that skips a pointless re-encode on the hot path too).
Also, this is a nil deref inside our fork. Worth fixing it in go-yaml as well, so the next caller that encodes an empty node doesn't trip over it.
There was a problem hiding this comment.
Fixed in c765799. isEmptyNullScalar now catches untagged empty scalars as well as !!null, and the test runs both. I kept the guard narrow rather than passing every node through, to avoid changing how other node kinds render. The fork still deserves its own fix.
…e encoding - Return buildSchema errors from the inline bundler instead of storing a nil schema in components. - Treat untagged empty scalars like empty nulls when marshalling ordered maps, avoiding the go-yaml encode panic. - Seed refPointer from mapped references as well as sequenced ones, so the inline fallback can always follow alias targets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #692, fixes #693, fixes #695.
Composed bundling now preserves a cross-document alias's outgoing reference instead of rewriting it to itself. The reported fixture retains
price, theamount → numberchain, and both descriptions. Schema-build errors are returned instead of discarded.Empty Arazzo extension values render without a panic and retain their node metadata. Header examples are read only from the header's own fields, so a nested schema example does not become a header example.
Validation:
go test ./...and focused race tests for the changed packages passed. Regression tests cover both composed-bundle APIs and root configurations, missing alias targets, empty extensions at three Arazzo levels, and absent/explicit/null header examples. Independent correctness and de-slop reviews found no actionable defects.Risk: Medium. The bundler change affects shared reference remapping; existing alias, collision, discriminator, sibling-reference, and diff tests pass. No public API or dependency changes.