-
Notifications
You must be signed in to change notification settings - Fork 16
fix(yml): keep folded block scalars stable across encode round trips #244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4a6a06e
15da3d1
8f14af9
37c7fb9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| package openapi_test | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/speakeasy-api/openapi/openapi" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // The trailing table row is indented one space further than the rows above it, which | ||
| // makes yaml.v3 emit an extra line break before it on every encode. Marshalling has to | ||
| // stay a fixed point regardless of whether an overlay was involved. | ||
| const foldedScalarDocument = `openapi: 3.1.0 | ||
| info: | ||
| title: Test | ||
| version: 1.0.0 | ||
| description: >- | ||
| ### Widgets | ||
|
|
||
| | Name | Kind | | ||
| | ---- | ---- | | ||
| | acme | ` + "`petstore`" + ` | | ||
| paths: {} | ||
| ` | ||
|
|
||
| func TestMarshal_FoldedScalar_SurvivesRepeatedRoundTrips(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| ctx := context.Background() | ||
|
|
||
| doc, validationErrs, err := openapi.Unmarshal(ctx, strings.NewReader(foldedScalarDocument)) | ||
| require.NoError(t, err) | ||
| require.Empty(t, validationErrs) | ||
|
|
||
| want := doc.Info.GetDescription() | ||
| require.Contains(t, want, "| acme |") | ||
|
|
||
| current := foldedScalarDocument | ||
| for i := range 30 { | ||
| doc, _, err := openapi.Unmarshal(ctx, strings.NewReader(current)) | ||
| require.NoError(t, err) | ||
|
|
||
| var buf bytes.Buffer | ||
| require.NoError(t, openapi.Marshal(ctx, doc, &buf)) | ||
|
|
||
| current = buf.String() | ||
| assert.Equal(t, want, doc.Info.GetDescription(), "value changed after %d round trips", i+1) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ import ( | |
| "github.com/speakeasy-api/openapi/references" | ||
| "github.com/speakeasy-api/openapi/sequencedmap" | ||
| "github.com/speakeasy-api/openapi/system" | ||
| "github.com/speakeasy-api/openapi/yml" | ||
| "gopkg.in/yaml.v3" | ||
| ) | ||
|
|
||
|
|
@@ -581,6 +582,8 @@ func rewriteInternalReferences(content []byte, originalRef string, storage *loca | |
| } | ||
|
|
||
| // Marshal back to YAML | ||
| yml.StabilizeFoldedScalars(&node) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be worth adding a test fixture with a more-indented string in https://github.com/speakeasy-api/openapi/tree/main/openapi/testdata/localize |
||
|
|
||
| updatedContent, err := yaml.Marshal(&node) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal updated YAML: %w", err) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| package overlay | ||
|
|
||
| import ( | ||
| "github.com/speakeasy-api/openapi/yml" | ||
| ) | ||
|
|
||
| // stabilizeFoldedScalars restyles folded block scalars in the overlay's own update | ||
| // payloads so that serializing the overlay round trips unchanged. | ||
| func (o *Overlay) stabilizeFoldedScalars() { | ||
| if o == nil { | ||
| return | ||
| } | ||
|
|
||
| for i := range o.Actions { | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| yml.StabilizeFoldedScalars(&o.Actions[i].Update) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,158 @@ | ||||||
| package overlay_test | ||||||
|
|
||||||
| import ( | ||||||
| "bytes" | ||||||
| "strings" | ||||||
| "testing" | ||||||
|
|
||||||
| "github.com/speakeasy-api/openapi/overlay" | ||||||
| "github.com/stretchr/testify/assert" | ||||||
| "github.com/stretchr/testify/require" | ||||||
| "gopkg.in/yaml.v3" | ||||||
| ) | ||||||
|
|
||||||
| // The trailing table row is indented one space further than the rows above it, | ||||||
| // which is what triggers the yaml.v3 emitter defect these tests guard against. | ||||||
| const foldedWithMoreIndentedLine = `description: >- | ||||||
| ### Widgets | ||||||
|
|
||||||
| | Name | Kind | | ||||||
| | ---- | ---- | | ||||||
| | acme | ` + "`petstore`" + ` | | ||||||
| ` | ||||||
|
|
||||||
| func testOverlay() *overlay.Overlay { | ||||||
| return &overlay.Overlay{ | ||||||
| Version: "1.0.0", | ||||||
| Info: overlay.Info{Title: "Test", Version: "1.0.0"}, | ||||||
| Actions: []overlay.Action{ | ||||||
| { | ||||||
| Target: "$.title", | ||||||
| Update: yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: "Updated"}, | ||||||
| }, | ||||||
| }, | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| func decodeDescription(t *testing.T, doc string) string { | ||||||
| t.Helper() | ||||||
|
|
||||||
| var decoded struct { | ||||||
| Description string `yaml:"description"` | ||||||
| } | ||||||
| require.NoError(t, yaml.Unmarshal([]byte(doc), &decoded)) | ||||||
|
|
||||||
| return decoded.Description | ||||||
| } | ||||||
|
|
||||||
| func TestApplyToSurvivesRepeatedApplies(t *testing.T) { | ||||||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||||||
| t.Parallel() | ||||||
|
|
||||||
| o := testOverlay() | ||||||
|
|
||||||
| doc := "title: Original\n" + foldedWithMoreIndentedLine | ||||||
| want := decodeDescription(t, doc) | ||||||
|
|
||||||
| for i := range 30 { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: i don't think 30 iterations is necessary (multiple instances)
Suggested change
|
||||||
| var node yaml.Node | ||||||
| require.NoError(t, yaml.Unmarshal([]byte(doc), &node)) | ||||||
| require.NoError(t, o.ApplyTo(&node)) | ||||||
|
|
||||||
| out, err := yaml.Marshal(&node) | ||||||
| require.NoError(t, err) | ||||||
|
|
||||||
| doc = string(out) | ||||||
| assert.Equal(t, want, decodeDescription(t, doc), "value changed after %d applies", i+1) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| func TestApplyToStrictSurvivesRepeatedApplies(t *testing.T) { | ||||||
| t.Parallel() | ||||||
|
|
||||||
| o := testOverlay() | ||||||
|
|
||||||
| doc := "title: Original\n" + foldedWithMoreIndentedLine | ||||||
| want := decodeDescription(t, doc) | ||||||
|
|
||||||
| for i := range 30 { | ||||||
| var node yaml.Node | ||||||
| require.NoError(t, yaml.Unmarshal([]byte(doc), &node)) | ||||||
| _, err := o.ApplyToStrict(&node) | ||||||
| require.NoError(t, err) | ||||||
|
|
||||||
| out, err := yaml.Marshal(&node) | ||||||
| require.NoError(t, err) | ||||||
|
|
||||||
| doc = string(out) | ||||||
| assert.Equal(t, want, decodeDescription(t, doc), "value changed after %d applies", i+1) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // An overlay carries folded scalars of its own, in the update payloads it applies. | ||||||
| func TestFormatSurvivesRepeatedRoundTrips(t *testing.T) { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test name is slightly misleading as it invokes |
||||||
| t.Parallel() | ||||||
|
|
||||||
| src := `overlay: 1.0.0 | ||||||
| info: | ||||||
| title: Test | ||||||
| version: 1.0.0 | ||||||
| actions: | ||||||
| - target: $.info | ||||||
| update: | ||||||
| ` + indent(foldedWithMoreIndentedLine, " ") | ||||||
|
|
||||||
| want := updateDescription(t, src) | ||||||
|
|
||||||
| doc := src | ||||||
| for i := range 30 { | ||||||
| o, err := overlay.ParseReader(strings.NewReader(doc)) | ||||||
| require.NoError(t, err) | ||||||
|
|
||||||
| formatted, err := o.ToString() | ||||||
| require.NoError(t, err) | ||||||
|
|
||||||
| doc = formatted | ||||||
| assert.Equal(t, want, updateDescription(t, doc), "value changed after %d round trips", i+1) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // A nil overlay serializes as "null"; stabilizing must not change that. | ||||||
| func TestFormatToleratesNilOverlay(t *testing.T) { | ||||||
| t.Parallel() | ||||||
|
|
||||||
| var o *overlay.Overlay | ||||||
|
|
||||||
| formatted, err := o.ToString() | ||||||
| require.NoError(t, err) | ||||||
| assert.Equal(t, "null\n", formatted) | ||||||
|
|
||||||
| var buf bytes.Buffer | ||||||
| require.NoError(t, o.Format(&buf)) | ||||||
| assert.Equal(t, "null\n", buf.String()) | ||||||
| } | ||||||
|
|
||||||
| func indent(doc string, prefix string) string { | ||||||
| lines := strings.Split(strings.TrimSuffix(doc, "\n"), "\n") | ||||||
| for i, line := range lines { | ||||||
| if line != "" { | ||||||
| lines[i] = prefix + line | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return strings.Join(lines, "\n") + "\n" | ||||||
| } | ||||||
|
|
||||||
| func updateDescription(t *testing.T, doc string) string { | ||||||
| t.Helper() | ||||||
|
|
||||||
| o, err := overlay.ParseReader(strings.NewReader(doc)) | ||||||
| require.NoError(t, err) | ||||||
| require.Len(t, o.Actions, 1) | ||||||
|
|
||||||
| var decoded struct { | ||||||
| Description string `yaml:"description"` | ||||||
| } | ||||||
| require.NoError(t, o.Actions[0].Update.Decode(&decoded)) | ||||||
|
|
||||||
| return decoded.Description | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| package yml | ||
|
|
||
| import ( | ||
| "strings" | ||
|
|
||
| "gopkg.in/yaml.v3" | ||
| ) | ||
|
|
||
| // Block indentation is stripped on decode, so leading whitespace means more-indented. | ||
| func hasMoreIndentedLine(value string) bool { | ||
| for _, line := range strings.Split(value, "\n") { | ||
| if line == "" { | ||
| continue | ||
| } | ||
| if line[0] == ' ' || line[0] == '\t' { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| // StabilizeFoldedScalars restyles folded block scalars that contain a more-indented | ||
| // line as literal blocks, leaving every other node untouched. | ||
| // | ||
| // gopkg.in/yaml.v3 injects a line break before a more-indented line in a folded scalar | ||
| // on every encode, so a document that is decoded and re-encoded repeatedly accumulates | ||
| // blank lines inside such scalars. The break lands inside the scalar, so it becomes part | ||
| // of the decoded value rather than cosmetic whitespace. Literal blocks reproduce their | ||
| // value verbatim and round trip unchanged. | ||
| // | ||
| // Only the representation changes; the decoded value is identical. Call this immediately | ||
| // before encoding a node tree. | ||
| func StabilizeFoldedScalars(node *yaml.Node) { | ||
| if node == nil { | ||
| return | ||
| } | ||
|
|
||
| if node.Kind == yaml.ScalarNode && node.Style&yaml.FoldedStyle != 0 && hasMoreIndentedLine(node.Value) { | ||
| node.Style = node.Style&^yaml.FoldedStyle | yaml.LiteralStyle | ||
| } | ||
|
|
||
| for _, child := range node.Content { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: When the input is an alias or a subtree whose folded scalar is reachable only through Prompt for AI agents
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not reachable in practice, so leaving the traversal as is. An anchor definition is an ordinary node in the tree — a: &anchor >-
one
more indented
b: *anchoris a fixed point after 30 encode/decode cycles under this implementation. Added as The only shape where a folded scalar is reachable solely through |
||
| StabilizeFoldedScalars(child) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: