refactor(schema): unify sibling-ref handling across schema extraction paths

Consolidate the sibling $ref transformation logic into resolveSchemaBuildInput / transformSiblingRefNode so ExtractSchema, buildSchema, buildSchemaList, and buildPropertyMap all flow through the same helpers, and propagate the original ref node and scope node through buildSchemaProxy / prepareForResolvedBuild. Sort sibling keys deterministically when constructing the allOf wrapper, and add coverage for nested sibling refs across components, parameters, request bodies, arrays, and allOf compositions.

Author: daveshanley

datamodel/low/base/schema.go

@@ -2,8 +2,6 @@ package base
 
 import (
 	"context"
-	"errors"
-	"fmt"
 	"sync"
 
 	"github.com/pb33f/libopenapi/datamodel/low"
@@ -153,81 +151,33 @@ type Schema struct {
 // will specifically look for a key node named 'schema' and extract the value mapped to that key. If the operation
 // fails then no NodeReference is returned and an error is returned instead.
 func ExtractSchema(ctx context.Context, root *yaml.Node, idx *index.SpecIndex) (*low.NodeReference[*SchemaProxy], error) {
-	var schLabel, schNode *yaml.Node
 	errStr := "schema build failed: reference '%s' cannot be found at line %d, col %d"
 
-	refLocation := ""
-	var refNode *yaml.Node
-
-	foundIndex := idx
-	foundCtx := ctx
-	if rf, rl, rv := utils.IsNodeRefValue(root); rf {
-		// locate reference in index.
-		ref, fIdx, err, nCtx := low.LocateRefNodeWithContext(ctx, root, idx)
-		if ref != nil {
-			schNode = ref
-			schLabel = rl
-			foundCtx = nCtx
-			foundIndex = fIdx
-		} else if errors.Is(err, low.ErrExternalRefSkipped) {
-			refLocation = rv
-			schema := &SchemaProxy{kn: root, vn: root, idx: idx, ctx: ctx}
-			_ = schema.Build(ctx, root, root, idx)
-			n := &low.NodeReference[*SchemaProxy]{Value: schema, KeyNode: root, ValueNode: root}
-			n.SetReference(refLocation, root)
-			schema.SetReference(refLocation, root)
-			return n, nil
-		} else {
-			v := root.Content[1].Value
-			if root.Content[1].Value == "" {
-				v = "[empty]"
-			}
-			return nil, fmt.Errorf(errStr,
-				v, root.Content[1].Line, root.Content[1].Column)
-		}
-	} else {
-		_, schLabel, schNode = utils.FindKeyNodeFull(SchemaLabel, root.Content)
-		if schNode != nil {
-			h := false
-			if h, _, refLocation = utils.IsNodeRefValue(schNode); h {
-				ref, fIdx, lerr, nCtx := low.LocateRefNodeWithContext(foundCtx, schNode, foundIndex)
-				if ref != nil {
-					refNode = schNode
-					schNode = ref
-					if fIdx != nil {
-						foundIndex = fIdx
-					}
-					foundCtx = nCtx
-				} else if errors.Is(lerr, low.ErrExternalRefSkipped) {
-					refNode = schNode
-				} else {
-					v := schNode.Content[1].Value
-					if schNode.Content[1].Value == "" {
-						v = "[empty]"
-					}
-					return nil, fmt.Errorf(errStr,
-						v, schNode.Content[1].Line, schNode.Content[1].Column)
-				}
-			}
-		}
+	if rf, refLabel, _ := utils.IsNodeRefValue(root); rf {
+		return extractSchemaProxy(ctx, idx, refLabel, root, errStr)
 	}
 
+	_, schLabel, schNode := utils.FindKeyNodeFull(SchemaLabel, root.Content)
 	if schNode != nil {
-		// check if schema has already been built.
-		schema := &SchemaProxy{kn: schLabel, vn: schNode, idx: foundIndex, ctx: foundCtx}
-
-		// call Build to ensure transformation happens
-		_ = schema.Build(foundCtx, schLabel, schNode, foundIndex)
+		return extractSchemaProxy(ctx, idx, schLabel, schNode, errStr)
+	}
+	return nil, nil
+}
 
-		schema.SetReference(refLocation, refNode)
+func extractSchemaProxy(ctx context.Context, idx *index.SpecIndex, keyNode, valueNode *yaml.Node, errFormat string) (*low.NodeReference[*SchemaProxy], error) {
+	resolved, err := resolveSchemaBuildInput(ctx, valueNode, idx, errFormat)
+	if err != nil {
+		return nil, err
+	}
 
-		n := &low.NodeReference[*SchemaProxy]{
-			Value:     schema,
-			KeyNode:   schLabel,
-			ValueNode: schema.vn, // use transformed node
-		}
-		n.SetReference(refLocation, refNode)
-		return n, nil
+	built := buildSchemaProxy(resolved.ctx, resolved.idx, keyNode, resolved.valueNode, resolved.scopeNode, resolved.refNode, resolved.transformed, resolved.refLocation)
+	n := &low.NodeReference[*SchemaProxy]{
+		Value:     built.Value,
+		KeyNode:   keyNode,
+		ValueNode: built.ValueNode,
 	}
-	return nil, nil
+	if resolved.refLocation != "" {
+		n.SetReference(resolved.refLocation, resolved.refNode)
+	}
+	return n, nil
 }

datamodel/low/base/schema_build_coverage_test.go

@@ -82,6 +82,91 @@ func TestResolveSchemaBuildInput_NilAndRefFailures(t *testing.T) {
 	assert.Contains(t, err.Error(), "boom: ./missing.yaml#/Pet")
 }
 
+func TestTransformSiblingRefNode(t *testing.T) {
+	var siblingRef yaml.Node
+	require.NoError(t, yaml.Unmarshal([]byte("$ref: '#/components/schemas/Name'\ndeprecated: true"), &siblingRef))
+
+	transformed, ok := transformSiblingRefNode(siblingRef.Content[0], nil)
+	require.False(t, ok)
+	assert.Equal(t, siblingRef.Content[0], transformed)
+
+	cfg := index.CreateOpenAPIIndexConfig()
+	cfg.TransformSiblingRefs = true
+	idx := index.NewSpecIndexWithConfig(&siblingRef, cfg)
+
+	var refOnly yaml.Node
+	require.NoError(t, yaml.Unmarshal([]byte("$ref: '#/components/schemas/Name'"), &refOnly))
+	transformed, ok = transformSiblingRefNode(refOnly.Content[0], idx)
+	require.False(t, ok)
+	assert.Equal(t, refOnly.Content[0], transformed)
+
+	transformed, ok = transformSiblingRefNode(siblingRef.Content[0], idx)
+	require.True(t, ok)
+	require.NotNil(t, transformed)
+	require.Len(t, transformed.Content, 2)
+	assert.Equal(t, "allOf", transformed.Content[0].Value)
+}
+
+func TestResolveSchemaBuildInput_TransformsSiblingRefBeforeResolution(t *testing.T) {
+	spec := `openapi: 3.1.0
+info:
+  title: sibling refs
+  version: 1.0.0
+paths: {}
+components:
+  schemas:
+    Name:
+      type: string
+    Container:
+      type: object
+      properties:
+        foo:
+          $ref: '#/components/schemas/Name'
+          deprecated: true`
+
+	var root yaml.Node
+	require.NoError(t, yaml.Unmarshal([]byte(spec), &root))
+
+	cfg := index.CreateOpenAPIIndexConfig()
+	cfg.TransformSiblingRefs = true
+	idx := index.NewSpecIndexWithConfig(&root, cfg)
+
+	fooNode := findNestedSchemaTestNode(t, root.Content[0], "components", "schemas", "Container", "properties", "foo")
+	resolved, err := resolveSchemaBuildInput(context.Background(), fooNode, idx, "boom: %s")
+	require.NoError(t, err)
+	require.NotNil(t, resolved.valueNode)
+	assert.Equal(t, "allOf", resolved.valueNode.Content[0].Value)
+	assert.Equal(t, fooNode, resolved.scopeNode)
+	assert.Nil(t, resolved.refNode)
+	assert.Equal(t, fooNode, resolved.transformed)
+	assert.Empty(t, resolved.refLocation)
+	assert.Equal(t, idx, resolved.idx)
+
+	built := buildSchemaProxy(resolved.ctx, resolved.idx, fooNode, resolved.valueNode, resolved.scopeNode, resolved.refNode, resolved.transformed, resolved.refLocation)
+	assert.Equal(t, fooNode, built.Value.TransformedRef)
+}
+
+func findNestedSchemaTestNode(t *testing.T, node *yaml.Node, path ...string) *yaml.Node {
+	t.Helper()
+
+	current := node
+	for _, key := range path {
+		require.NotNil(t, current)
+		require.Equal(t, yaml.MappingNode, current.Kind)
+
+		var next *yaml.Node
+		for i := 0; i+1 < len(current.Content); i += 2 {
+			if current.Content[i].Value == key {
+				next = current.Content[i+1]
+				break
+			}
+		}
+		require.NotNil(t, next, "missing key %q", key)
+		current = next
+	}
+	return current
+}
+
 func TestSchemaBuild_BuildModelError(t *testing.T) {
 	var root yaml.Node
 	require.NoError(t, yaml.Unmarshal([]byte("type: string\n"), &root))

datamodel/low/base/schema_build_helpers.go

@@ -19,7 +19,9 @@ type resolvedSchemaBuildInput struct {
 	ctx         context.Context
 	idx         *index.SpecIndex
 	valueNode   *yaml.Node
+	scopeNode   *yaml.Node
 	refNode     *yaml.Node
+	transformed *yaml.Node
 	refLocation string
 }
 
@@ -41,7 +43,7 @@ func buildPropertyMap(ctx context.Context, parent *Schema, root *yaml.Node, idx
 			propertyMap.Set(low.KeyReference[string]{
 				KeyNode: currentProp,
 				Value:   currentProp.Value,
-			}, buildSchemaProxy(resolved.ctx, resolved.idx, currentProp, resolved.valueNode, resolved.refNode, resolved.refLocation != "", resolved.refLocation))
+			}, buildSchemaProxy(resolved.ctx, resolved.idx, currentProp, resolved.valueNode, resolved.scopeNode, resolved.refNode, resolved.transformed, resolved.refLocation))
 		}
 
 		return &low.NodeReference[*orderedmap.Map[low.KeyReference[string], low.ValueReference[*SchemaProxy]]]{
@@ -100,13 +102,9 @@ func (s *Schema) extractExtensions(root *yaml.Node) {
 }
 
 // buildSchemaProxy builds out a SchemaProxy for a single node.
-func buildSchemaProxy(ctx context.Context, idx *index.SpecIndex, kn, vn *yaml.Node, rf *yaml.Node, isRef bool, refLocation string) low.ValueReference[*SchemaProxy] {
+func buildSchemaProxy(ctx context.Context, idx *index.SpecIndex, kn, vn, scopeNode, rf, transformed *yaml.Node, refLocation string) low.ValueReference[*SchemaProxy] {
 	sp := new(SchemaProxy)
-	if isRef {
-		sp.prepareForResolvedBuild(ctx, kn, vn, idx, refLocation, rf)
-	} else {
-		sp.prepareForResolvedBuild(ctx, kn, vn, idx, "", nil)
-	}
+	sp.prepareForResolvedBuild(ctx, kn, vn, scopeNode, idx, refLocation, rf, transformed)
 	return low.ValueReference[*SchemaProxy]{
 		Value:     sp,
 		ValueNode: sp.vn,
@@ -130,7 +128,7 @@ func buildSchema(ctx context.Context, labelNode, valueNode *yaml.Node, idx *inde
 		return low.ValueReference[*SchemaProxy]{}, err
 	}
 
-	return buildSchemaProxy(resolved.ctx, resolved.idx, labelNode, resolved.valueNode, resolved.refNode, resolved.refLocation != "", resolved.refLocation), nil
+	return buildSchemaProxy(resolved.ctx, resolved.idx, labelNode, resolved.valueNode, resolved.scopeNode, resolved.refNode, resolved.transformed, resolved.refLocation), nil
 }
 
 // buildSchemaList builds out child schemas for a parent schema. Expected to be an array of schema objects.
@@ -152,7 +150,7 @@ func buildSchemaList(ctx context.Context, labelNode, valueNode *yaml.Node, idx *
 		if err != nil {
 			return nil, err
 		}
-		r := buildSchemaProxy(resolved.ctx, resolved.idx, resolved.valueNode, resolved.valueNode, resolved.refNode, resolved.refLocation != "", resolved.refLocation)
+		r := buildSchemaProxy(resolved.ctx, resolved.idx, resolved.valueNode, resolved.valueNode, resolved.scopeNode, resolved.refNode, resolved.transformed, resolved.refLocation)
 		results = append(results, r)
 	}
 
@@ -190,17 +188,25 @@ func resolveSchemaBuildInput(ctx context.Context, valueNode *yaml.Node, idx *ind
 		ctx:       ctx,
 		idx:       idx,
 		valueNode: valueNode,
+		scopeNode: valueNode,
 	}
 
 	if valueNode == nil {
 		return resolved, nil
 	}
 
+	if transformedValue, wasTransformed := transformSiblingRefNode(valueNode, idx); wasTransformed {
+		resolved.valueNode = transformedValue
+		resolved.transformed = valueNode
+		return resolved, nil
+	}
+
 	if hasRef, _, refLocation := utils.IsNodeRefValue(valueNode); hasRef {
 		ref, foundIdx, err, foundCtx := low.LocateRefNodeWithContext(ctx, valueNode, idx)
 		if ref != nil {
 			resolved.refNode = valueNode
 			resolved.valueNode = ref
+			resolved.scopeNode = ref
 			resolved.refLocation = refLocation
 			resolved.ctx = foundCtx
 			resolved.idx = foundIdx
@@ -211,8 +217,16 @@ func resolveSchemaBuildInput(ctx context.Context, valueNode *yaml.Node, idx *ind
 			resolved.refLocation = refLocation
 			return resolved, nil
 		}
-		return resolved, fmt.Errorf(errFormat, valueNode.Content[1].Value, valueNode.Content[1].Line, valueNode.Content[1].Column)
+		return resolved, schemaReferenceBuildError(errFormat, valueNode)
 	}
 
 	return resolved, nil
 }
+
+func schemaReferenceBuildError(errFormat string, valueNode *yaml.Node) error {
+	refValue := valueNode.Content[1].Value
+	if refValue == "" {
+		refValue = "[empty]"
+	}
+	return fmt.Errorf(errFormat, refValue, valueNode.Content[1].Line, valueNode.Content[1].Column)
+}

datamodel/low/base/schema_proxy.go

@@ -85,18 +85,9 @@ func (sp *SchemaProxy) Build(ctx context.Context, key, value *yaml.Node, idx *in
 
 	// transform sibling refs to allOf structure if enabled and applicable
 	// this ensures sp.vn contains the pre-transformed YAML as the source of truth
-	transformedValue := value
-	wasTransformed := false
-	if idx != nil && idx.GetConfig() != nil && idx.GetConfig().TransformSiblingRefs {
-		transformer := NewSiblingRefTransformer(idx)
-		if transformer.ShouldTransform(value) {
-			transformed, _ := transformer.TransformSiblingRef(value)
-			if transformed != nil {
-				transformedValue = transformed
-				wasTransformed = true
-				sp.TransformedRef = value // store original node that had the ref
-			}
-		}
+	transformedValue, wasTransformed := transformSiblingRefNode(value, idx)
+	if wasTransformed {
+		sp.TransformedRef = value // store original node that had the ref
 	}
 
 	sp.vn = transformedValue
@@ -117,14 +108,27 @@ func (sp *SchemaProxy) Build(ctx context.Context, key, value *yaml.Node, idx *in
 	return nil
 }
 
+func transformSiblingRefNode(value *yaml.Node, idx *index.SpecIndex) (*yaml.Node, bool) {
+	if idx == nil || idx.GetConfig() == nil || !idx.GetConfig().TransformSiblingRefs {
+		return value, false
+	}
+	transformer := NewSiblingRefTransformer(idx)
+	if !transformer.ShouldTransform(value) {
+		return value, false
+	}
+	transformed, _ := transformer.TransformSiblingRef(value)
+	return transformed, true
+}
+
 // prepareForResolvedBuild initializes proxy state when the caller has already resolved any reference metadata.
 // This avoids re-running the full Build ref-detection path for child-schema helpers that already did that work.
-func (sp *SchemaProxy) prepareForResolvedBuild(ctx context.Context, key, value *yaml.Node, idx *index.SpecIndex, refLocation string, refNode *yaml.Node) {
+func (sp *SchemaProxy) prepareForResolvedBuild(ctx context.Context, key, value, scopeNode *yaml.Node, idx *index.SpecIndex, refLocation string, refNode, transformed *yaml.Node) {
 	sp.kn = key
 	sp.idx = idx
 	sp.vn = value
-	sp.ctx = applySchemaIdScope(ctx, value, idx)
+	sp.ctx = applySchemaIdScope(ctx, scopeNode, idx)
 	sp.Reference = low.Reference{}
+	sp.TransformedRef = transformed
 	if refLocation != "" {
 		sp.SetReference(refLocation, refNode)
 	}

datamodel/low/base/sibling_ref_transformer.go

@@ -4,6 +4,8 @@
 package base
 
 import (
+	"sort"
+
 	"github.com/pb33f/libopenapi/index"
 	"github.com/pb33f/libopenapi/utils"
 	"go.yaml.in/yaml/v4"
@@ -53,7 +55,13 @@ func (srt *SiblingRefTransformer) CreateAllOfStructure(refValue string, siblings
 	// first element: schema with sibling properties (excluding $ref)
 	if len(siblings) > 0 {
 		siblingSchemaNode := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"}
-		for key, valueNode := range siblings {
+		keys := make([]string, 0, len(siblings))
+		for key := range siblings {
+			keys = append(keys, key)
+		}
+		sort.Strings(keys)
+		for _, key := range keys {
+			valueNode := siblings[key]
 			keyNode := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: key}
 			// create a copy of the value node to avoid modifying original
 			copiedValueNode := srt.copyNode(valueNode)