fix(linter): keep node-level lint fixes when the fixed document is written

linter/fix/engine.go

@@ -3,8 +3,10 @@ package fix
 import (
 	"context"
 	"errors"
+	"fmt"
 	"sort"
 
+	"github.com/speakeasy-api/openapi/marshaller"
 	"github.com/speakeasy-api/openapi/openapi"
 	"github.com/speakeasy-api/openapi/validation"
 	"go.yaml.in/yaml/v3"
@@ -158,7 +160,9 @@ func (e *Engine) ProcessErrors(ctx context.Context, doc *openapi.OpenAPI, errs [
 	result := &Result{}
 	modified := make(map[conflictKey]bool)
 	skippedRules := make(map[string]bool)
+	var nodeFixes []pendingNodeFix
 
+fixLoop:
 	for _, fe := range fixable {
 		fix := fe.fix
 		vErr := fe.vErr
@@ -225,7 +229,7 @@ func (e *Engine) ProcessErrors(ctx context.Context, doc *openapi.OpenAPI, errs [
 					continue
 				}
 				if errors.Is(err, validation.ErrExitInteractive) {
-					return result, nil
+					break fixLoop
 				}
 				result.Failed = append(result.Failed, FailedFix{
 					Error: vErr, Fix: fix, FixError: err,
@@ -241,19 +245,20 @@ func (e *Engine) ProcessErrors(ctx context.Context, doc *openapi.OpenAPI, errs [
 			}
 		}
 
-		// Apply the fix
-		var applyErr error
-		if nodeFix, ok := fix.(validation.NodeFix); ok {
-			rootNode := doc.GetRootNode()
-			if rootNode != nil {
-				applyErr = nodeFix.ApplyNode(rootNode)
-			} else {
-				applyErr = fix.Apply(doc)
+		// Node fixes are deferred until all model fixes have been applied, as
+		// marshalling syncs the model back over the YAML nodes.
+		if nodeFix, ok := fix.(validation.NodeFix); ok && doc.GetRootNode() != nil {
+			if key.Line >= 0 {
+				modified[key] = true
 			}
-		} else {
-			applyErr = fix.Apply(doc)
+			nodeFixes = append(nodeFixes, pendingNodeFix{vErr: vErr, fix: nodeFix, index: len(result.Applied)})
+			result.Applied = append(result.Applied, makeAppliedFix(vErr, fix))
+			continue
 		}
 
+		// Apply the fix
+		applyErr := fix.Apply(doc)
+
 		if applyErr != nil {
 			result.Failed = append(result.Failed, FailedFix{
 				Error: vErr, Fix: fix, FixError: applyErr,
@@ -269,9 +274,67 @@ func (e *Engine) ProcessErrors(ctx context.Context, doc *openapi.OpenAPI, errs [
 		result.Applied = append(result.Applied, makeAppliedFix(vErr, fix))
 	}
 
+	if err := applyNodeFixes(ctx, doc, result, nodeFixes); err != nil {
+		return result, err
+	}
+
 	return result, nil
 }
 
+type pendingNodeFix struct {
+	vErr  *validation.Error
+	fix   validation.NodeFix
+	index int // position of the fix in Result.Applied
+}
+
+// applyNodeFixes applies fixes that edit the YAML node tree directly. The model
+// is synced to the nodes first so earlier model fixes are kept, and the model is
+// rebuilt from the edited nodes afterwards so the edits survive marshalling.
+func applyNodeFixes(ctx context.Context, doc *openapi.OpenAPI, result *Result, nodeFixes []pendingNodeFix) error {
+	if len(nodeFixes) == 0 {
+		return nil
+	}
+
+	rootNode, err := marshaller.Sync(ctx, doc)
+	if err != nil {
+		return fmt.Errorf("failed to sync document before applying node fixes: %w", err)
+	}
+
+	failed := make(map[int]bool)
+	for _, nf := range nodeFixes {
+		if err := nf.fix.ApplyNode(rootNode); err != nil {
+			failed[nf.index] = true
+			result.Failed = append(result.Failed, FailedFix{
+				Error: nf.vErr, Fix: nf.fix, FixError: err,
+			})
+		}
+	}
+
+	if len(failed) > 0 {
+		applied := make([]AppliedFix, 0, len(result.Applied)-len(failed))
+		for i, af := range result.Applied {
+			if !failed[i] {
+				applied = append(applied, af)
+			}
+		}
+		result.Applied = applied
+	}
+
+	if len(failed) == len(nodeFixes) {
+		return nil
+	}
+
+	config := doc.GetCore().GetConfig()
+	*doc = openapi.OpenAPI{}
+	doc.InitCache()
+	doc.GetCore().SetConfig(config)
+	if _, err := marshaller.UnmarshalNode(ctx, "", rootNode, doc); err != nil {
+		return fmt.Errorf("failed to rebuild document after applying node fixes: %w", err)
+	}
+
+	return nil
+}
+
 func makeAppliedFix(vErr *validation.Error, fix validation.Fix) AppliedFix {
 	af := AppliedFix{Error: vErr, Fix: fix}
 	if cd, ok := fix.(validation.ChangeDescriber); ok {

linter/fix/engine_test.go

@@ -1,7 +1,9 @@
 package fix_test
 
 import (
+	"bytes"
 	"errors"
+	"strings"
 	"testing"
 
 	"github.com/speakeasy-api/openapi/linter/fix"
@@ -612,3 +614,118 @@ func TestEngine_SortsByLocation(t *testing.T) {
 	}
 	assert.Equal(t, []string{"fix-line2", "fix-line5", "fix-line10"}, order, "fixes should be applied in location order")
 }
+
+// setNodeValueFix is a node fix that replaces the value of a YAML node.
+type setNodeValueFix struct {
+	mockFix
+	node  *yaml.Node
+	value string
+}
+
+func (f *setNodeValueFix) ApplyNode(_ *yaml.Node) error {
+	f.node.Value = f.value
+	return nil
+}
+
+// modelFix is a fix that edits the document model.
+type modelFix struct {
+	mockFix
+	apply func(doc *openapi.OpenAPI)
+}
+
+func (f *modelFix) Apply(doc any) error {
+	f.apply(doc.(*openapi.OpenAPI))
+	return nil
+}
+
+func TestEngine_NodeFix_SurvivesMarshal(t *testing.T) {
+	t.Parallel()
+	ctx := t.Context()
+
+	doc, _, err := openapi.Unmarshal(ctx, strings.NewReader(`openapi: 3.1.0
+info:
+  title: Test
+  version: 1.0.0
+servers:
+  - url: https://api.example.com/
+paths:
+  /pets/:
+    get:
+      responses:
+        "200":
+          description: ok
+`))
+	require.NoError(t, err, "unmarshal should succeed")
+
+	urlNode := doc.Servers[0].GetCore().URL.ValueNode
+	pathKeyNode := doc.Paths.GetCore().GetMapKeyNodeOrRoot("/pets/", doc.Paths.GetRootNode())
+
+	engine := fix.NewEngine(fix.Options{Mode: fix.ModeAuto}, nil, nil)
+	result, err := engine.ProcessErrors(ctx, doc, []error{
+		makeError("version-rule", 4, 12, "version", &modelFix{
+			mockFix: mockFix{description: "bump version"},
+			apply:   func(doc *openapi.OpenAPI) { doc.Info.Version = "2.0.0" },
+		}),
+		makeError("url-rule", 6, 10, "url", &setNodeValueFix{
+			mockFix: mockFix{description: "trim url"},
+			node:    urlNode,
+			value:   "https://api.example.com",
+		}),
+		makeError("path-rule", 8, 3, "path", &setNodeValueFix{
+			mockFix: mockFix{description: "trim path"},
+			node:    pathKeyNode,
+			value:   "/pets",
+		}),
+	})
+	require.NoError(t, err, "ProcessErrors should not fail")
+	assert.Len(t, result.Applied, 3, "should apply all fixes")
+
+	var buf bytes.Buffer
+	require.NoError(t, openapi.Marshal(ctx, doc, &buf), "marshal should succeed")
+	assert.Equal(t, `openapi: 3.1.0
+info:
+  title: Test
+  version: 2.0.0
+servers:
+  - url: https://api.example.com
+paths:
+  /pets:
+    get:
+      responses:
+        "200":
+          description: ok
+`, buf.String(), "marshalled document should keep both model and node fixes")
+}
+
+// failingNodeFix is a node fix that always fails.
+type failingNodeFix struct {
+	mockFix
+}
+
+func (f *failingNodeFix) ApplyNode(_ *yaml.Node) error {
+	return errors.New("node fix failed")
+}
+
+func TestEngine_NodeFix_FailureIsReported(t *testing.T) {
+	t.Parallel()
+	ctx := t.Context()
+
+	doc, _, err := openapi.Unmarshal(ctx, strings.NewReader(`openapi: 3.1.0
+info:
+  title: Test
+  version: 1.0.0
+paths: {}
+`))
+	require.NoError(t, err, "unmarshal should succeed")
+
+	engine := fix.NewEngine(fix.Options{Mode: fix.ModeAuto}, nil, nil)
+	result, err := engine.ProcessErrors(ctx, doc, []error{
+		makeError("rule-a", 1, 1, "a", &failingNodeFix{mockFix: mockFix{description: "failing node fix"}}),
+		makeError("rule-b", 2, 1, "b", &mockFix{description: "model fix"}),
+	})
+	require.NoError(t, err, "ProcessErrors should not fail")
+	require.Len(t, result.Applied, 1, "should apply only the model fix")
+	assert.Equal(t, "model fix", result.Applied[0].Fix.Description(), "applied fix should be the model fix")
+	require.Len(t, result.Failed, 1, "should report the failed node fix")
+	assert.EqualError(t, result.Failed[0].FixError, "node fix failed", "failure should carry the node fix error")
+}