diff --git a/bundler/bundler_composer.go b/bundler/bundler_composer.go index eeb65fa7..cb5f7ef4 100644 --- a/bundler/bundler_composer.go +++ b/bundler/bundler_composer.go @@ -68,6 +68,9 @@ func handleIndex(c *handleIndexConfig) error { continue } mappedReference := mappedReferences[sequenced.FullDefinition] + if mappedReference == nil { + mappedReference, _ = c.idx.SearchIndexForReference(sequenced.FullDefinition) + } // Check for invalid sibling properties if strict validation is enabled if c.compositionConfig.StrictValidation && @@ -188,6 +191,14 @@ func processReference(model *v3.Document, pr *processRef, cf *handleIndexConfig) } model.Components = components } + // An alias contributes no component of its own. Inline its reference after + // the target has been composed, so both paths use the same component. + isAlias, _, _ := utils.IsNodeRefValue(pr.ref.Node) + isAlias = isAlias && len(pr.ref.Node.Content) == 2 + if !pr.fromDiscriminator && isAlias { + cf.inlineRequired = append(cf.inlineRequired, pr) + return nil + } var location []string if strings.Contains(pr.ref.FullDefinition, "#/") { @@ -198,7 +209,9 @@ func processReference(model *v3.Document, pr *processRef, cf *handleIndexConfig) // Bare-file imports need the sequenced absolute definition so composition // keys and later rewrites point at the same target. pr.ref.FullDefinition = pr.seqRef.FullDefinition - if importType, ok := inferComponentTypeFromSourcePath(pr.seqRef.SourcePath); ok && + if pr.fromDiscriminator && isAlias { + _, location = fileImportLocationForType(v3low.SchemasLabel, components, pr, cf) + } else if importType, ok := inferComponentTypeFromSourcePath(pr.seqRef.SourcePath); ok && canComposeContextualReference(importType, pr.ref.Node, true) { _, location = fileImportLocationForType(importType, components, pr, cf) } else if importType, ok := DetectOpenAPIComponentType(pr.ref.Node); ok { @@ -383,7 +396,8 @@ func enqueueDiscriminatorMappingTargets( mapKey := processRefMapKeyForComponent(ref, v3low.SchemasLabel) // Skip targets already queued for composition. - if cf.refMap.GetOrZero(mapKey) != nil { + if existing := cf.refMap.GetOrZero(mapKey); existing != nil { + existing.fromDiscriminator = true continue } diff --git a/bundler/composer_functions.go b/bundler/composer_functions.go index 432b551a..fb720a2c 100644 --- a/bundler/composer_functions.go +++ b/bundler/composer_functions.go @@ -52,7 +52,15 @@ func processRefMapKey(target, source *index.Reference) string { if fullDefinition == "" && source != nil { fullDefinition = source.FullDefinition } - return contextualProcessRefKey(fullDefinition, source) + key := contextualProcessRefKey(fullDefinition, source) + if key == fullDefinition && target != nil && !isExplicitComponentDefinition(fullDefinition) { + // A whole-file alias has no source path. Use the same detected bucket + // that composition uses, so it shares the target with contextual refs. + if componentType, ok := DetectOpenAPIComponentType(target.Node); ok { + return fullDefinition + contextualRefKeySeparator + componentType + } + } + return key } // processRefMapKeyForComponent scopes a target ref by an already-known component bucket. diff --git a/bundler/external_regression_test.go b/bundler/external_regression_test.go new file mode 100644 index 00000000..2b869089 --- /dev/null +++ b/bundler/external_regression_test.go @@ -0,0 +1,152 @@ +package bundler + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/pb33f/go-yaml" + "github.com/pb33f/libopenapi" + "github.com/pb33f/libopenapi/datamodel" + "github.com/pb33f/libopenapi/utils" + "github.com/pb33f/testify/require" +) + +func externalRegressionConfig(t *testing.T, files map[string]string) *datamodel.DocumentConfiguration { + t.Helper() + dir := t.TempDir() + for name, content := range files { + path := filepath.Join(dir, name) + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0755)) + require.NoError(t, os.WriteFile(path, []byte(content), 0600)) + } + return &datamodel.DocumentConfiguration{BasePath: dir, AllowFileReferences: true} +} + +func TestBundleBytesComposed_EncodedPathItem(t *testing.T) { + config := externalRegressionConfig(t, map[string]string{"paths.yaml": `openapi: 3.1.0 +info: {title: Paths, version: "1"} +paths: + /items/{id}: + get: + parameters: [{name: id, in: path, required: true, schema: {type: string}}] + responses: + '200': {description: ok} +`}) + root := []byte(`openapi: 3.1.0 +info: {title: Encoded paths, version: "1"} +paths: + /items/{id}: {$ref: './paths.yaml#/paths/~1items~1%7Bid%7D'} +`) + out, err := BundleBytesComposed(root, config, nil) + require.NoError(t, err) + var bundled yaml.Node + require.NoError(t, yaml.Unmarshal(out, &bundled)) + _, paths := utils.FindKeyNodeTop("paths", bundled.Content[0].Content) + require.NotNil(t, paths) + _, pathItem := utils.FindKeyNodeTop("/items/{id}", paths.Content) + require.NotNil(t, pathItem) + require.Nil(t, utils.GetRefValueNode(pathItem), "path item must contain the operation, not a self-reference") + doc, err := libopenapi.NewDocument(out) + require.NoError(t, err) + model, err := doc.BuildV3Model() + require.NoError(t, err) + item := model.Model.Paths.PathItems.GetOrZero("/items/{id}") + require.NotNil(t, item.Get) + require.Len(t, item.Get.Parameters, 1) +} + +func TestBundleBytesComposed_SharedAliasTarget(t *testing.T) { + config := externalRegressionConfig(t, map[string]string{ + "schemas/List.yaml": "type: object\nproperties:\n data:\n type: array\n items: {$ref: ./Item.yaml}\n", + "schemas/Alias.yaml": "$ref: ./Item.yaml\n", + "schemas/Item.yaml": "type: object\nproperties:\n id: {type: string}\n", + }) + root := []byte(`openapi: 3.1.0 +info: {title: Shared aliases, version: "1"} +paths: + /items: + get: + responses: + '200': + description: ok + content: {application/json: {schema: {$ref: ./schemas/List.yaml}}} + /item: + get: + responses: + '200': + description: ok + content: {application/json: {schema: {$ref: ./schemas/Alias.yaml}}} +`) + out, err := BundleBytesComposed(root, config, nil) + require.NoError(t, err) + doc, err := libopenapi.NewDocument(out) + require.NoError(t, err) + model, err := doc.BuildV3Model() + require.NoError(t, err) + list := model.Model.Components.Schemas.GetOrZero("List").Schema() + require.NotNil(t, list.Properties.GetOrZero("data"), string(out)) + require.Equal(t, 2, model.Model.Components.Schemas.Len(), string(out)) + item := list.Properties.GetOrZero("data").Schema().Items.A.Schema() + require.Equal(t, []string{"object"}, item.Type) + require.NotNil(t, item.Properties.GetOrZero("id")) +} + +func TestBundleBytes_RecursiveSchemaAtDifferentDepths(t *testing.T) { + for depth := 0; depth <= 5; depth++ { + t.Run(fmt.Sprint(depth), func(t *testing.T) { + files := map[string]string{ + "schemas/Step.yaml": "properties:\n steps:\n items: {$ref: ./Step.yaml}\n", + "schemas/V.yaml": "properties:\n steps:\n items: {$ref: ./Step.yaml}\n styles: {$ref: ./W.yaml}\n", + "schemas/W.yaml": "properties:\n c: {$ref: ./E.yaml}\n", + "schemas/E.yaml": "{}\n", + } + ref := "V.yaml" + for i := depth; i > 0; i-- { + name := fmt.Sprintf("A%d.yaml", i) + files["schemas/"+name] = "properties:\n p: {$ref: ./" + ref + "}\n" + ref = name + } + files["paths/item.yaml"] = "post:\n requestBody:\n content:\n application/json:\n schema: {$ref: ../schemas/" + ref + "}\n responses:\n '200': {description: ok}\n" + config := externalRegressionConfig(t, files) + root := []byte("openapi: 3.0.3\ninfo: {title: Nested recursion, version: '1'}\npaths:\n /items: {$ref: ./paths/item.yaml}\n") + out, err := BundleBytes(root, config) + require.NoError(t, err) + doc, err := libopenapi.NewDocument(out) + require.NoError(t, err) + model, err := doc.BuildV3Model() + require.NoError(t, err) + step := model.Model.Components.Schemas.GetOrZero("Step") + require.NotNil(t, step, string(out)) + require.Equal(t, "#/components/schemas/Step", step.Schema().Properties.GetOrZero("steps").Schema().Items.A.GetReference()) + }) + } +} + +func TestBundleBytesComposed_AliasDiscriminatorMapping(t *testing.T) { + cfg := externalRegressionConfig(t, map[string]string{"schemas/Alias.yaml": "$ref: ./Dog.yaml\n", "schemas/Dog.yaml": "type: object\nproperties:\n kind: {type: string, enum: [dog]}\n"}) + out, err := BundleBytesComposed([]byte(`openapi: 3.1.0 +info: {title: Aliases, version: '1'} +paths: {} +components: + schemas: + Animal: + oneOf: + - {$ref: ./schemas/Alias.yaml} + discriminator: + propertyName: kind + mapping: + dog: ./schemas/Alias.yaml +`), cfg, nil) + require.NoError(t, err) + + doc, err := libopenapi.NewDocument(out) + require.NoError(t, err) + model, err := doc.BuildV3Model() + require.NoError(t, err) + mapping := model.Model.Components.Schemas.GetOrZero("Animal").Schema().Discriminator.Mapping.GetOrZero("dog") + require.True(t, strings.HasPrefix(mapping, "#/components/schemas/"), mapping) + require.NotNil(t, model.Model.Components.Schemas.GetOrZero(strings.TrimPrefix(mapping, "#/components/schemas/")), mapping) +} diff --git a/datamodel/document_config.go b/datamodel/document_config.go index 3a431005..4f12dfb0 100644 --- a/datamodel/document_config.go +++ b/datamodel/document_config.go @@ -81,6 +81,11 @@ type DocumentConfiguration struct { // FileFilter should be used to limit the scope of the rolodex. AllowFileReferences bool + // AllowExcessiveAliases disables the YAML alias expansion limit during indexing. + // Use it for trusted documents with large amounts of shared YAML content. + // The limit remains enabled by default; cycle detection is always enabled. + AllowExcessiveAliases bool + // SkipExternalRefResolution will skip resolving external $ref references (those not starting with #). // When enabled, external references will be left as-is during model building. Schema proxies will // report IsReference()=true and GetReference() will return the ref string, but Schema() will return nil. diff --git a/datamodel/high/v2/path_item.go b/datamodel/high/v2/path_item.go index 619dcb34..375ab700 100644 --- a/datamodel/high/v2/path_item.go +++ b/datamodel/high/v2/path_item.go @@ -39,6 +39,7 @@ type PathItem struct { func NewPathItem(pathItem *lowV2.PathItem) *PathItem { p := new(PathItem) p.low = pathItem + p.Ref = pathItem.Ref.Value p.Extensions = high.ExtractExtensions(pathItem.Extensions) if !pathItem.Parameters.IsEmpty() { var params []*Parameter diff --git a/datamodel/low/base/schema_build.go b/datamodel/low/base/schema_build.go index a414f25f..dcbd2a9e 100644 --- a/datamodel/low/base/schema_build.go +++ b/datamodel/low/base/schema_build.go @@ -68,7 +68,7 @@ func (s *Schema) Build(ctx context.Context, root *yaml.Node, idx *index.SpecInde if !isTransformed { if h, _, _ := utils.IsNodeRefValue(root); h { - ref, foundIdx, err, fctx := low.LocateRefNodeWithContext(ctx, root, idx) + ref, foundIdx, err, fctx := low.LocateRefEnd(ctx, root, idx, 0) if ref != nil { root = utils.ResolveMergeNodes(ref) if fctx != nil { diff --git a/datamodel/low/integer_decimal.go b/datamodel/low/integer_decimal.go new file mode 100644 index 00000000..6ba94eca --- /dev/null +++ b/datamodel/low/integer_decimal.go @@ -0,0 +1,95 @@ +package low + +import ( + "math" + "strconv" + "strings" +) + +// parseDecimalInt64 converts a syntactically valid decimal without rounding. +// It scans the mantissa once and retains at most 19 digits, regardless of exponent. +func parseDecimalInt64(value string) (int64, bool) { + value = strings.ReplaceAll(value, "_", "") + negative := strings.HasPrefix(value, "-") + value = strings.TrimLeft(value, "+-") + mantissa := value + exponent := int64(0) + if i := strings.IndexAny(value, "eE"); i >= 0 { + mantissa = value[:i] + exponent, _ = strconv.ParseInt(value[i+1:], 10, 64) + } + var digits, leading, trailing, fractional int64 + nonzero, afterPoint := false, false + for _, digit := range mantissa { + if digit == '.' { + afterPoint = true + continue + } + digits++ + if afterPoint { + fractional++ + } + if digit == '0' { + trailing++ + if !nonzero { + leading++ + } + } else { + nonzero = true + trailing = 0 + } + } + if !nonzero { + return 0, true + } + limit := int64(math.MaxInt64) + if negative { + limit = math.MinInt64 + } + // Bound the exponent before subtracting the mantissa's decimal places. + if exponent > digits+19 { + return limit, true + } + if exponent < -digits { + return 0, false + } + scale := exponent - fractional + if scale < -trailing { + return 0, false + } + integerDigits := digits - leading + scale + if integerDigits > 19 { + return limit, true + } + maxMagnitude := uint64(math.MaxInt64) + if negative { + maxMagnitude++ + } + var result uint64 + remaining := integerDigits + for _, digit := range mantissa { + if digit == '.' { + continue + } + if leading > 0 { + leading-- + continue + } + if remaining == 0 { + break + } + result = result*10 + uint64(digit-'0') + remaining-- + } + for remaining > 0 { + result *= 10 + remaining-- + } + if result > maxMagnitude { + return limit, true + } + if negative { + return -int64(result), true + } + return int64(result), true +} diff --git a/datamodel/low/integer_decimal_test.go b/datamodel/low/integer_decimal_test.go new file mode 100644 index 00000000..f0a215de --- /dev/null +++ b/datamodel/low/integer_decimal_test.go @@ -0,0 +1,59 @@ +package low + +import ( + "math" + "strconv" + "testing" + + "github.com/pb33f/go-yaml" + "github.com/pb33f/testify/require" +) + +func TestBuildModel_ExactDecimalIntegers(t *testing.T) { + for _, tc := range []struct { + value string + want int64 + set bool + }{ + {"9007199254740993.0", 9007199254740993, true}, + {"9.007199254740993e15", 9007199254740993, true}, + {"-9.007199254740993e15", -9007199254740993, true}, + {"9007199254740992.5", 0, false}, + {"1.0000000000000001", 0, false}, + {"1e-999999999999999999999", 0, false}, + {"0e-999999999999999999999", 0, true}, + {"1e999999999999999999999", math.MaxInt64, true}, + {"-1e999999999999999999999", math.MinInt64, true}, + {"9223372036854775807.0", math.MaxInt64, true}, + {"-9223372036854775808.0", math.MinInt64, true}, + {"9223372036854775808.0", math.MaxInt64, true}, + {"-9223372036854775809.0", math.MinInt64, true}, + {"001.2300e2", 123, true}, + {"1_000.00", 1000, true}, + {"100.0e-2", 1, true}, + {"100.0e-3", 0, false}, + {".0", 0, true}, + {"Inf", math.MaxInt64, true}, + {"-Inf", math.MinInt64, true}, + {"0x1p4", 16, true}, + {"0x1p64", math.MaxInt64, true}, + {"-0x1p64", math.MinInt64, true}, + {"0x1.8p0", 0, false}, + } { + t.Run(tc.value, func(t *testing.T) { + // Explicit tags also exercise overflow literals that YAML's implicit + // float resolver classifies as strings. + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte("maxLength: !!float "+tc.value), &root)) + var model struct{ MaxLength NodeReference[int64] } + require.NoError(t, BuildModel(root.Content[0], &model)) + require.Equal(t, tc.set, !model.MaxLength.IsEmpty()) + require.Equal(t, tc.want, model.MaxLength.Value) + got, set := parseIntNode(tc.value) + require.Equal(t, tc.set, set) + if tc.set && strconv.IntSize == 64 { + require.Equal(t, tc.want, int64(got)) + } + }) + } +} diff --git a/datamodel/low/model_builder.go b/datamodel/low/model_builder.go index 5fbe332c..1e6bc7ad 100644 --- a/datamodel/low/model_builder.go +++ b/datamodel/low/model_builder.go @@ -544,17 +544,29 @@ func parseInt64Node(value string) (int64, bool) { return math.MinInt64, true } f, err := strconv.ParseFloat(value, 64) - switch { - case err != nil || math.IsNaN(f): + if (err != nil && !errors.Is(err, strconv.ErrRange)) || math.IsNaN(f) { return 0, false - case f >= math.MaxInt64: + } + if err == nil && math.IsInf(f, 0) { + if f < 0 { + return math.MinInt64, true + } return math.MaxInt64, true - case f <= math.MinInt64: - return math.MinInt64, true - case f != math.Trunc(f): - return 0, false } - return int64(f), true + // Keep the existing hexadecimal-float handling; decimal constraints must not + // pass through float64, which loses integer and fractional digits above 2^53. + if strings.ContainsAny(value, "xX") { + switch { + case f >= math.MaxInt64: + return math.MaxInt64, true + case f <= math.MinInt64: + return math.MinInt64, true + case f != math.Trunc(f): + return 0, false + } + return int64(f), true + } + return parseDecimalInt64(value) } // parseIntNode is parseInt64Node for an int field. Where int is 32 bits, strconv.Atoi clamps the diff --git a/datamodel/low/v2/path_item_test.go b/datamodel/low/v2/path_item_test.go index 702a0a6c..846dd2c2 100644 --- a/datamodel/low/v2/path_item_test.go +++ b/datamodel/low/v2/path_item_test.go @@ -12,7 +12,9 @@ import ( "github.com/pb33f/libopenapi/datamodel/low" "github.com/pb33f/libopenapi/index" "github.com/pb33f/libopenapi/orderedmap" + "github.com/pb33f/libopenapi/utils" "github.com/pb33f/testify/assert" + "github.com/pb33f/testify/require" ) func TestPathItem_Build_Params(t *testing.T) { @@ -32,6 +34,33 @@ func TestPathItem_Build_Params(t *testing.T) { assert.Error(t, err) } +func TestPathItem_Build_SkippedExternalReferenceAndSiblingOperation(t *testing.T) { + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(`swagger: '2.0' +info: {title: Pets, version: '1'} +paths: + /pets: + $ref: './shared.yaml#/paths/~1pets' + get: + operationId: listPets + responses: + '200': {description: Pet list} +`), &root)) + cfg := index.CreateOpenAPIIndexConfig() + cfg.SkipExternalRefResolution = true + idx := index.NewSpecIndexWithConfig(&root, cfg) + _, paths := utils.FindKeyNodeTop("paths", root.Content[0].Content) + _, source := utils.FindKeyNodeTop("/pets", paths.Content) + var item PathItem + require.NoError(t, item.Build(context.Background(), nil, source, idx)) + assert.Equal(t, "./shared.yaml#/paths/~1pets", item.Ref.Value) + assert.Same(t, source.Content[0], item.Ref.KeyNode) + assert.Same(t, source.Content[1], item.Ref.ValueNode) + assert.Empty(t, item.Parameters.Value) + require.NotNil(t, item.Get.Value) + assert.Equal(t, "listPets", item.Get.Value.OperationId.Value) +} + func TestPathItem_Build_MethodFail(t *testing.T) { yml := `post: $ref: break` diff --git a/datamodel/low/v2/paths.go b/datamodel/low/v2/paths.go index 7d68b443..742822a0 100644 --- a/datamodel/low/v2/paths.go +++ b/datamodel/low/v2/paths.go @@ -5,6 +5,7 @@ package v2 import ( "context" + "fmt" "hash/maphash" "sync" @@ -125,12 +126,25 @@ func (p *Paths) Build(ctx context.Context, _, root *yaml.Node, idx *index.SpecIn translateFunc := func(value buildInput) (pathBuildResult, error) { pNode := value.pathNode cNode := value.currentNode + pathCtx, pathIdx := ctx, idx + var originalRef low.NodeReference[string] + if ok, refKey, ref := utils.IsNodeRefValue(pNode); ok { + originalRef = low.NodeReference[string]{Value: ref, KeyNode: refKey, ValueNode: utils.GetRefValueNode(pNode)} + resolved, foundIdx, err, foundCtx := low.LocateRefEnd(ctx, pNode, idx, 0) + if err != nil { + return pathBuildResult{}, fmt.Errorf("path item build failed: %w", err) + } + pNode, pathIdx, pathCtx = resolved, foundIdx, foundCtx + } path := new(PathItem) _ = low.BuildModel(pNode, path) - err := path.Build(ctx, cNode, pNode, idx) + err := path.Build(pathCtx, cNode, pNode, pathIdx) if err != nil { return pathBuildResult{}, err } + if originalRef.Value != "" { + path.Ref = originalRef + } return pathBuildResult{ key: low.KeyReference[string]{ Value: cNode.Value, diff --git a/datamodel/low/v2/swagger.go b/datamodel/low/v2/swagger.go index 33656b3b..c7dc6f3b 100644 --- a/datamodel/low/v2/swagger.go +++ b/datamodel/low/v2/swagger.go @@ -146,6 +146,7 @@ func createDocument(info *datamodel.SpecInfo, config *datamodel.DocumentConfigur idxConfig.IgnorePolymorphicCircularReferences = config.IgnorePolymorphicCircularReferences idxConfig.AllowUnknownExtensionContentDetection = config.AllowUnknownExtensionContentDetection idxConfig.SkipExternalRefResolution = config.SkipExternalRefResolution + idxConfig.AllowExcessiveAliases = config.AllowExcessiveAliases idxConfig.ResolveNestedRefsWithDocumentContext = config.ResolveNestedRefsWithDocumentContext idxConfig.AvoidCircularReferenceCheck = true idxConfig.BaseURL = config.BaseURL @@ -236,6 +237,12 @@ func createDocument(info *datamodel.SpecInfo, config *datamodel.DocumentConfigur // set the index on the document. doc.Index = rolodex.GetRootIndex() doc.SpecInfo = info + for _, err := range roloErrs { + var resolvingErr *index.ResolvingError + if errors.As(err, &resolvingErr) && errors.Is(resolvingErr.ErrorRef, index.ErrReferenceChainCycle) { + return &doc, errors.Join(errs...) + } + } // build out swagger scalar variables. _ = low.BuildModel(info.RootNode.Content[0], &doc) diff --git a/datamodel/low/v2/swagger_test.go b/datamodel/low/v2/swagger_test.go index f66475ff..f9321ed8 100644 --- a/datamodel/low/v2/swagger_test.go +++ b/datamodel/low/v2/swagger_test.go @@ -519,3 +519,20 @@ paths: {} assert.NotNil(t, doc.Index.GetConfig()) assert.True(t, doc.Index.GetConfig().ResolveNestedRefsWithDocumentContext) } + +func TestCreateDocument_PathReferenceCycle(t *testing.T) { + info, err := datamodel.ExtractSpecInfo([]byte(`swagger: '2.0' +info: {title: Path cycle, version: '1'} +x-a: {$ref: '#/x-b'} +x-b: {$ref: '#/x-a'} +paths: + /a: {$ref: '#/x-a'} +`)) + require.NoError(t, err) + config := datamodel.NewDocumentConfiguration() + config.SkipCircularReferenceCheck = true + doc, err := CreateDocumentFromConfig(info, config) + require.ErrorContains(t, err, "circular reference chain") + require.NotNil(t, doc.Index) + require.Nil(t, doc.Paths.Value) +} diff --git a/datamodel/low/v3/components.go b/datamodel/low/v3/components.go index 12ac3c97..343791e9 100644 --- a/datamodel/low/v3/components.go +++ b/datamodel/low/v3/components.go @@ -278,6 +278,7 @@ func (co *Components) Build(ctx context.Context, root *yaml.Node, idx *index.Spe func extractComponentValues[T low.Buildable[N], N any](ctx context.Context, label string, root *yaml.Node, idx *index.SpecIndex, co *Components) (low.NodeReference[*orderedmap.Map[low.KeyReference[string], low.ValueReference[T]]], error) { var emptyResult low.NodeReference[*orderedmap.Map[low.KeyReference[string], low.ValueReference[T]]] _, nodeLabel, nodeValue := utils.FindKeyNodeFullTop(label, root.Content) + nodeValue = utils.ResolveMergeNodes(nodeValue) if nodeValue == nil { return emptyResult, nil } diff --git a/datamodel/low/v3/create_document.go b/datamodel/low/v3/create_document.go index 153cbb01..126dedc3 100644 --- a/datamodel/low/v3/create_document.go +++ b/datamodel/low/v3/create_document.go @@ -157,6 +157,7 @@ func createDocument(info *datamodel.SpecInfo, config *datamodel.DocumentConfigur idxConfig.AllowUnknownExtensionContentDetection = config.AllowUnknownExtensionContentDetection idxConfig.TransformSiblingRefs = config.TransformSiblingRefs idxConfig.SkipExternalRefResolution = config.SkipExternalRefResolution + idxConfig.AllowExcessiveAliases = config.AllowExcessiveAliases idxConfig.ResolveNestedRefsWithDocumentContext = config.ResolveNestedRefsWithDocumentContext idxConfig.AvoidCircularReferenceCheck = true @@ -301,6 +302,12 @@ func createDocument(info *datamodel.SpecInfo, config *datamodel.DocumentConfigur // set root index. doc.Index = rolodex.GetRootIndex() + for _, err := range roloErrs { + var resolvingErr *index.ResolvingError + if errors.As(err, &resolvingErr) && errors.Is(resolvingErr.ErrorRef, index.ErrReferenceChainCycle) { + return &doc, errors.Join(errs...) + } + } var wg sync.WaitGroup var cacheMap sync.Map diff --git a/datamodel/low/v3/create_document_test.go b/datamodel/low/v3/create_document_test.go index 5ffe00d2..bc67e32f 100644 --- a/datamodel/low/v3/create_document_test.go +++ b/datamodel/low/v3/create_document_test.go @@ -1483,3 +1483,20 @@ paths: {} assert.NotNil(t, doc.Index.GetConfig()) assert.True(t, doc.Index.GetConfig().ResolveNestedRefsWithDocumentContext) } + +func TestCreateDocument_PathReferenceCycle(t *testing.T) { + info, err := datamodel.ExtractSpecInfo([]byte(`openapi: 3.1.1 +info: {title: Path cycle, version: '1'} +x-a: {$ref: '#/x-b'} +x-b: {$ref: '#/x-a'} +paths: + /a: {$ref: '#/x-a'} +`)) + require.NoError(t, err) + config := datamodel.NewDocumentConfiguration() + config.SkipCircularReferenceCheck = true + doc, err := CreateDocumentFromConfig(info, config) + require.ErrorContains(t, err, "circular reference chain") + require.NotNil(t, doc.Index) + require.Nil(t, doc.Paths.Value) +} diff --git a/document_alias_limit_test.go b/document_alias_limit_test.go new file mode 100644 index 00000000..3b1264c4 --- /dev/null +++ b/document_alias_limit_test.go @@ -0,0 +1,144 @@ +package libopenapi + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/pb33f/libopenapi/datamodel" + "github.com/pb33f/testify/require" +) + +func sharedResponseDocument() string { + var source strings.Builder + source.WriteString("openapi: 3.1.1\ninfo: {title: Shared responses, version: '1'}\nx-common:\n errors: &errors\n") + for i := range 20 { + fmt.Fprintf(&source, " '%d':\n description: Error %d\n content:\n application/json:\n schema:\n type: object\n properties:\n code: {type: integer}\n message: {type: string}\n", 400+i, i) + } + source.WriteString("paths:\n") + for i := range 5000 { + fmt.Fprintf(&source, " /r%d:\n get:\n operationId: op%d\n responses:\n '200': {description: ok}\n <<: *errors\n", i, i) + } + return source.String() +} + +func TestBuildV3Model_AllowsTrustedSharedResponses(t *testing.T) { + source := sharedResponseDocument() + for _, external := range []bool{false, true} { + for _, allow := range []bool{false, true} { + t.Run(fmt.Sprintf("external=%t/allow=%t", external, allow), func(t *testing.T) { + config := datamodel.NewDocumentConfiguration() + input := source + if external { + config = externalRegressionConfig(t, map[string]string{"routes.yaml": source}) + input = "openapi: 3.1.1\ninfo: {title: Linked response, version: '1'}\npaths:\n /linked: {$ref: './routes.yaml#/paths/~1r4999'}\n" + } + config.AllowExcessiveAliases = allow + doc, err := NewDocumentWithConfiguration([]byte(input), config) + require.NoError(t, err) + model, err := doc.BuildV3Model() + if !allow { + require.ErrorContains(t, err, "AllowExcessiveAliases") + require.Nil(t, model) + return + } + require.NoError(t, err) + require.NotNil(t, model) + require.True(t, model.Index.GetConfig().AllowExcessiveAliases) + if external { + require.Equal(t, 1, model.Model.Paths.PathItems.Len()) + require.Equal(t, "op4999", model.Model.Paths.PathItems.GetOrZero("/linked").Get.OperationId) + require.Len(t, model.Index.GetRolodex().GetIndexes(), 1) + require.Equal(t, 5000, model.Index.GetRolodex().GetIndexes()[0].GetOperationCount()) + } else { + require.Equal(t, 5000, model.Model.Paths.PathItems.Len()) + } + for item := range model.Model.Paths.PathItems.ValuesFromOldest() { + require.Equal(t, 21, item.Get.Responses.Codes.Len()) + require.Equal(t, "Error 19", item.Get.Responses.Codes.GetOrZero("419").Description) + } + }) + } + } +} + +func TestBuildV2Model_PropagatesAliasPolicy(t *testing.T) { + config := datamodel.NewDocumentConfiguration() + config.AllowExcessiveAliases = true + doc, err := NewDocumentWithConfiguration([]byte("swagger: '2.0'\ninfo: {title: Alias policy, version: '1'}\npaths: {}\n"), config) + require.NoError(t, err) + model, err := doc.BuildV2Model() + require.NoError(t, err) + require.True(t, model.Index.GetConfig().AllowExcessiveAliases) + require.True(t, model.Index.GetConfig().ToDocumentConfiguration().AllowExcessiveAliases) +} + +func TestBuildV3ModelReportsAliasLimit(t *testing.T) { + // A small shared graph crosses the parser's alias ratio limit during indexing. + // It has no references, so only the traversal limit can report this error. + const source = `openapi: 3.1.1 +info: {title: Aliases, version: '1'} +paths: {} +x-a: &a [value] +x-b: &b [*a, *a] +x-c: &c [*b, *b] +x-d: &d [*c, *c] +x-e: &e [*d, *d] +x-f: &f [*e, *e] +x-g: &g [*f, *f] +x-h: &h [*g, *g] +x-i: &i [*h, *h] +x-j: &j [*i, *i] +x-k: &k [*j, *j] +` + for _, location := range []string{"root", "external"} { + t.Run(location, func(t *testing.T) { + for _, skipCircular := range []bool{false, true} { + config := datamodel.NewDocumentConfiguration() + config.SkipCircularReferenceCheck = skipCircular + input := source + if location == "external" { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "shared.yaml"), []byte(source+"components: {schemas: {Target: {type: string}}}\n"), 0600)) + config.BasePath = dir + config.AllowFileReferences = true + input = "openapi: 3.1.1\ninfo: {title: Aliases, version: '1'}\npaths: {}\ncomponents: {schemas: {Target: {$ref: './shared.yaml#/components/schemas/Target'}}}\n" + } + doc, err := NewDocumentWithConfiguration([]byte(input), config) + require.NoError(t, err) + for range 2 { + model, buildErr := doc.BuildV3Model() + require.ErrorContains(t, buildErr, "document contains excessive aliasing") + require.Nil(t, model) + } + } + }) + } +} + +func TestBuildV3ModelReportsMergeAliasLimit(t *testing.T) { + const source = `openapi: 3.1.1 +info: {title: Merge aliases, version: '1'} +paths: {} +x-a: &a {leaf: value} +x-b: &b {left: {<<: *a}, right: {<<: *a}} +x-c: &c {left: {<<: *b}, right: {<<: *b}} +x-d: &d {left: {<<: *c}, right: {<<: *c}} +x-e: &e {left: {<<: *d}, right: {<<: *d}} +x-f: &f {left: {<<: *e}, right: {<<: *e}} +x-g: &g {left: {<<: *f}, right: {<<: *f}} +x-h: &h {left: {<<: *g}, right: {<<: *g}} +x-i: &i {left: {<<: *h}, right: {<<: *h}} +x-j: &j {left: {<<: *i}, right: {<<: *i}} +x-k: &k {left: {<<: *j}, right: {<<: *j}} +` + doc, err := NewDocument([]byte(source)) + require.NoError(t, err) + for range 2 { + model, buildErr := doc.BuildV3Model() + require.ErrorContains(t, buildErr, "document contains excessive aliasing") + require.Nil(t, model) + } +} diff --git a/document_effective_index_test.go b/document_effective_index_test.go new file mode 100644 index 00000000..e7f48c11 --- /dev/null +++ b/document_effective_index_test.go @@ -0,0 +1,65 @@ +package libopenapi + +import ( + "testing" + + "github.com/pb33f/go-yaml" + "github.com/pb33f/libopenapi/utils" + "github.com/pb33f/testify/assert" + "github.com/pb33f/testify/require" +) + +func TestBuildV3ModelEffectiveComponentIndex(t *testing.T) { + const prefix = `openapi: 3.1.1 +info: {title: Index, version: '1'} +paths: + /bound: + get: + responses: + '200': + description: Bound + content: + application/json: + schema: {$ref: '#/components/schemas/Bound'} +` + for _, tc := range []struct{ name, components string }{ + {"plain", "components:\n schemas:\n Bound: {type: integer, minimum: 5}\n"}, + {"components merge", "x-base: &base\n schemas:\n Bound: {type: integer, minimum: 5}\ncomponents:\n <<: *base\n"}, + {"schemas merge", "x-base: &base\n Bound: {type: integer, minimum: 5}\ncomponents:\n schemas:\n <<: *base\n"}, + {"components alias", "x-base: &base\n schemas:\n Bound: {type: integer, minimum: 5}\ncomponents: *base\n"}, + {"schemas alias", "x-base: &base\n Bound: {type: integer, minimum: 5}\ncomponents:\n schemas: *base\n"}, + {"schema value merge", "x-base: &base {type: integer, minimum: 5}\ncomponents:\n schemas:\n Bound:\n <<: *base\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + doc, err := NewDocument([]byte(prefix + tc.components)) + require.NoError(t, err) + root := doc.GetSpecInfo().RootNode + before, err := yaml.Marshal(root) + require.NoError(t, err) + model, err := doc.BuildV3Model() + require.NoError(t, err) + proxy, ok := model.Model.Components.Schemas.Get("Bound") + require.True(t, ok) + schema := proxy.Schema() + require.NotNil(t, schema) + require.NotNil(t, schema.Minimum) + assert.Equal(t, float64(5), *schema.Minimum) + refs := model.Index.GetAllComponentSchemas() + require.Len(t, refs, 1) + ref := refs["#/components/schemas/Bound"] + require.NotNil(t, ref) + assert.Equal(t, "Bound", ref.KeyNode.Value) + _, min := utils.FindKeyNodeTop("minimum", utils.ResolveMergeNodes(ref.Node).Content) + assert.Same(t, min, schema.GoLow().Minimum.ValueNode) + after, err := yaml.Marshal(root) + require.NoError(t, err) + assert.Equal(t, before, after) + rendered, err := doc.Render() + require.NoError(t, err) + reloaded, err := NewDocument(rendered) + require.NoError(t, err) + _, err = reloaded.BuildV3Model() + require.NoError(t, err) + }) + } +} diff --git a/document_external_regression_test.go b/document_external_regression_test.go new file mode 100644 index 00000000..e08ae955 --- /dev/null +++ b/document_external_regression_test.go @@ -0,0 +1,23 @@ +// Copyright 2026 Dave Shanley / Quobix +// SPDX-License-Identifier: MIT + +package libopenapi + +import ( + "github.com/pb33f/libopenapi/datamodel" + "github.com/pb33f/testify/require" + "os" + "path/filepath" + "testing" +) + +func externalRegressionConfig(t *testing.T, files map[string]string) *datamodel.DocumentConfiguration { + t.Helper() + dir := t.TempDir() + for name, content := range files { + path := filepath.Join(dir, name) + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0755)) + require.NoError(t, os.WriteFile(path, []byte(content), 0600)) + } + return &datamodel.DocumentConfiguration{BasePath: dir, AllowFileReferences: true} +} diff --git a/document_named_example_test.go b/document_named_example_test.go new file mode 100644 index 00000000..b1486fc2 --- /dev/null +++ b/document_named_example_test.go @@ -0,0 +1,75 @@ +package libopenapi + +import ( + "fmt" + "strings" + "testing" + + "github.com/pb33f/libopenapi/datamodel" + "github.com/pb33f/testify/require" +) + +func TestBuildV3Model_NamedExampleSchemaCycles(t *testing.T) { + for _, name := range []string{"example", "examples"} { + for _, container := range []string{"components/schemas", "definitions", "$defs", "dependentSchemas", "external"} { + t.Run(container+"/"+name, func(t *testing.T) { + prefix := "#/" + container + "/" + if container == "external" { + prefix = "#/" + } + schemas := fmt.Sprintf(`%s: + type: object + required: [child] + properties: + child: {$ref: '%sNode'} +Node: + type: object + required: [back] + properties: + back: {$ref: '%s%s'} +`, name, prefix, prefix, name) + header := "openapi: 3.1.1\ninfo: {title: Named schema cycles, version: '1'}\npaths: {}\n" + config := datamodel.NewDocumentConfiguration() + var source string + switch container { + case "external": + config = externalRegressionConfig(t, map[string]string{"schemas.yaml": schemas}) + source = header + "components:\n schemas:\n Root: {$ref: './schemas.yaml#/" + name + "'}\n" + case "components/schemas": + source = header + "components:\n schemas:\n" + indentNamedSchema(schemas, 4) + default: + source = header + "components:\n schemas:\n Root: {$ref: '" + prefix + name + "'}\n" + container + ":\n" + indentNamedSchema(schemas, 2) + } + doc, err := NewDocumentWithConfiguration([]byte(source), config) + require.NoError(t, err) + _, err = doc.BuildV3Model() + require.ErrorContains(t, err, "infinite circular reference detected") + }) + } + } +} + +func indentNamedSchema(source string, spaces int) string { + indent := strings.Repeat(" ", spaces) + return indent + strings.ReplaceAll(strings.TrimSuffix(source, "\n"), "\n", "\n"+indent) + "\n" +} + +func TestBuildV3Model_ExternalNamedExampleObject(t *testing.T) { + config := externalRegressionConfig(t, map[string]string{ + "examples.yaml": "example:\n value:\n $ref: '#/payload-only'\n", + }) + doc, err := NewDocumentWithConfiguration([]byte(`openapi: 3.1.1 +info: {title: External example, version: '1'} +paths: {} +components: + examples: + External: {$ref: './examples.yaml#/example'} +`), config) + require.NoError(t, err) + model, err := doc.BuildV3Model() + require.NoError(t, err) + require.NotNil(t, model) + value := model.Model.Components.Examples.GetOrZero("External").Value + require.Equal(t, "$ref", value.Content[0].Value) + require.Equal(t, "#/payload-only", value.Content[1].Value) +} diff --git a/document_path_cycle_test.go b/document_path_cycle_test.go new file mode 100644 index 00000000..d3273c2d --- /dev/null +++ b/document_path_cycle_test.go @@ -0,0 +1,39 @@ +package libopenapi + +import ( + "fmt" + "testing" + + "github.com/pb33f/libopenapi/datamodel" + "github.com/pb33f/testify/require" +) + +func TestBuildModel_PathReferenceCycleReturnsError(t *testing.T) { + for _, version := range []string{"swagger: '2.0'", "openapi: 3.1.1"} { + for _, skipCircular := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/skipCircular=%t", version, skipCircular), func(t *testing.T) { + config := datamodel.NewDocumentConfiguration() + config.SkipCircularReferenceCheck = skipCircular + doc, err := NewDocumentWithConfiguration([]byte(version+` +info: {title: Path cycle, version: '1'} +x-a: {$ref: '#/x-b'} +x-b: {$ref: '#/x-a'} +paths: + /a: {$ref: '#/x-a'} +`), config) + require.NoError(t, err) + for range 2 { + if version == "swagger: '2.0'" { + model, buildErr := doc.BuildV2Model() + require.Nil(t, model) + require.ErrorContains(t, buildErr, "circular reference chain") + } else { + model, buildErr := doc.BuildV3Model() + require.Nil(t, model) + require.ErrorContains(t, buildErr, "circular reference chain") + } + } + }) + } + } +} diff --git a/document_resolver_regression_test.go b/document_resolver_regression_test.go new file mode 100644 index 00000000..5e74fd7d --- /dev/null +++ b/document_resolver_regression_test.go @@ -0,0 +1,132 @@ +// Copyright 2026 Dave Shanley / Quobix +// SPDX-License-Identifier: MIT + +package libopenapi + +import ( + "fmt" + "github.com/pb33f/testify/require" + "testing" +) + +func TestBuildV3Model_RecursiveRequiredAlias(t *testing.T) { + config := externalRegressionConfig(t, map[string]string{ + "schemas/id.yaml": "$ref: ./uuid.yaml\n", + "schemas/uuid.yaml": "type: string\nformat: uuid\n", + }) + doc, err := NewDocumentWithConfiguration([]byte(`openapi: 3.1.0 +info: {title: Recursive required alias, version: "1"} +paths: {} +components: + schemas: + Item: + type: object + required: [id] + properties: + id: {$ref: ./schemas/id.yaml} + children: + type: array + items: {$ref: '#/components/schemas/Item'} +`), config) + require.NoError(t, err) + model, err := doc.BuildV3Model() + require.NoError(t, err) + item := model.Model.Components.Schemas.GetOrZero("Item").Schema() + require.NotNil(t, item) + require.Equal(t, "uuid", item.Properties.GetOrZero("id").Schema().Format) +} + +func TestBuildV3Model_ReferencedExamplePayload(t *testing.T) { + doc, err := NewDocument([]byte(`openapi: 3.1.0 +info: {title: Example payload, version: "1"} +paths: + /items: + get: + responses: + '200': + description: ok + content: + application/json: + schema: {$ref: '#/components/schemas/Item'} +components: + schemas: + Item: + type: object + properties: + schema: {type: object} + example: + schema: + properties: + id: {$ref: '#/$defs/id'} + $defs: + id: {type: string} +`)) + require.NoError(t, err) + model, err := doc.BuildV3Model() + require.NoError(t, err) + require.NotNil(t, model) +} + +func TestBuildV3Model_ReferencedExampleObjects(t *testing.T) { + for _, version := range []string{"3.0.3", "3.1.0"} { + for _, field := range []string{"value", "dataValue"} { + t.Run(version+"/"+field, func(t *testing.T) { + response := fmt.Sprintf(`description: ok +content: + application/json: + schema: + type: object + properties: + example: {$ref: ./string.yaml} + examples: {$ref: ./string.yaml} + examples: + payload: + %s: + $ref: '#/payload-only' + nested: {$ref: '#/also-payload-only'} +`, field) + config := externalRegressionConfig(t, map[string]string{ + "responses/ok.yaml": response, + "responses/string.yaml": "type: string\n", + }) + root := fmt.Sprintf(`openapi: %s +info: {title: External example payload, version: "1"} +paths: + /items: + get: + responses: + '200': {$ref: ./responses/ok.yaml} +`, version) + doc, err := NewDocumentWithConfiguration([]byte(root), config) + require.NoError(t, err) + model, err := doc.BuildV3Model() + require.NoError(t, err) + media := model.Model.Paths.PathItems.GetOrZero("/items").Get.Responses.Codes.GetOrZero("200").Content.GetOrZero("application/json") + for _, name := range []string{"example", "examples"} { + proxy := media.Schema.Schema().Properties.GetOrZero(name) + require.Equal(t, []string{"string"}, proxy.Schema().Type) + require.NoError(t, proxy.GetBuildError()) + } + }) + } + } +} + +func TestBuildV3Model_ExamplePolymorphicPayload(t *testing.T) { + doc, err := NewDocument([]byte(`openapi: 3.1.0 +info: {title: Examples, version: '1'} +paths: {} +components: + schemas: + A: + type: object + example: + oneOf: + - {$ref: '#/components/schemas/A'} + Ref: {$ref: '#/components/schemas/A'} +`)) + require.NoError(t, err) + model, err := doc.BuildV3Model() + require.NoError(t, err) + require.Empty(t, model.Index.GetCircularReferences()) +} diff --git a/document_schema_alias_test.go b/document_schema_alias_test.go new file mode 100644 index 00000000..f616e0c8 --- /dev/null +++ b/document_schema_alias_test.go @@ -0,0 +1,41 @@ +// Copyright 2026 Dave Shanley / Quobix +// SPDX-License-Identifier: MIT + +package libopenapi + +import ( + "fmt" + "github.com/pb33f/testify/require" + "testing" +) + +func TestBuildV3Model_ExternalSchemaAliasChain(t *testing.T) { + for _, aliases := range []int{0, 1, 2, 5} { + t.Run(fmt.Sprint(aliases), func(t *testing.T) { + files := map[string]string{"schemas/target.yaml": "type: object\nproperties:\n id: {type: string}\n"} + ref := "./target.yaml" + for i := aliases; i > 0; i-- { + name := fmt.Sprintf("alias%d.yaml", i) + files["schemas/"+name] = "$ref: " + ref + "\n" + ref = "./" + name + } + config := externalRegressionConfig(t, files) + doc, err := NewDocumentWithConfiguration([]byte(fmt.Sprintf(`openapi: 3.1.0 +info: {title: Alias chains, version: "1"} +paths: {} +components: + schemas: + Item: {$ref: ./schemas/%s} +`, ref[2:])), config) + require.NoError(t, err) + model, err := doc.BuildV3Model() + require.NoError(t, err) + proxy := model.Model.Components.Schemas.GetOrZero("Item") + schema := proxy.Schema() + require.NoError(t, proxy.GetBuildError()) + require.NotNil(t, schema) + require.Equal(t, []string{"object"}, schema.Type) + require.NotNil(t, schema.Properties.GetOrZero("id")) + }) + } +} diff --git a/document_v2_path_reference_test.go b/document_v2_path_reference_test.go new file mode 100644 index 00000000..d239e86e --- /dev/null +++ b/document_v2_path_reference_test.go @@ -0,0 +1,51 @@ +// Copyright 2026 Dave Shanley / Quobix +// SPDX-License-Identifier: MIT + +package libopenapi + +import ( + "github.com/pb33f/testify/require" + "testing" +) + +func TestBuildV2Model_ExternalPathItems(t *testing.T) { + config := externalRegressionConfig(t, map[string]string{ + "paths/list.yaml": "get:\n responses:\n '200': {description: ok}\n", + "paths/item.yaml": "parameters:\n - {$ref: ./id.yaml}\nget:\n responses:\n '200': {description: ok}\n", + "paths/id.yaml": "name: id\nin: path\nrequired: true\ntype: string\n", + }) + doc, err := NewDocumentWithConfiguration([]byte(`swagger: "2.0" +info: {title: External paths, version: "1"} +paths: + /items: + $ref: ./paths/list.yaml + /items/{id}: + $ref: ./paths/item.yaml +`), config) + require.NoError(t, err) + model, err := doc.BuildV2Model() + require.NoError(t, err) + list := model.Model.Paths.PathItems.GetOrZero("/items") + require.NotNil(t, list.Get) + require.Empty(t, list.Parameters) + require.Equal(t, "./paths/list.yaml", list.Ref) + item := model.Model.Paths.PathItems.GetOrZero("/items/{id}") + require.NotNil(t, item.Get) + require.Len(t, item.Parameters, 1) + require.Equal(t, "id", item.Parameters[0].Name) + require.Equal(t, "./paths/item.yaml", item.Ref) +} + +func TestBuildV2Model_MissingExternalPathItem(t *testing.T) { + config := externalRegressionConfig(t, map[string]string{ + "paths.yaml": "paths: {}\n", + }) + doc, err := NewDocumentWithConfiguration([]byte(`swagger: "2.0" +info: {title: Missing path, version: "1"} +paths: + /items: {$ref: './paths.yaml#/paths/missing'} +`), config) + require.NoError(t, err) + _, err = doc.BuildV2Model() + require.Error(t, err) +} diff --git a/generator/sdk/gosdk/coverage_test.go b/generator/sdk/gosdk/coverage_test.go index 5347c16e..a4f17b5e 100644 --- a/generator/sdk/gosdk/coverage_test.go +++ b/generator/sdk/gosdk/coverage_test.go @@ -337,7 +337,8 @@ func TestPrepareResponsesContracts(t *testing.T) { {Status: "default", Content: jsonContent(schema("object", ""))}, }}, "Read", "[ThingsResource.Read]", &view) if err != nil || view.ResponseType != "string" || !reflect.DeepEqual(view.SuccessStatuses, []string{"200"}) || !view.DecodeAll || - len(view.ErrorCases) != 1 || !view.ErrorCases[0].Default || view.ErrorCases[0].Type != "ReadError" || view.ErrorSwitch != "response.StatusCode" { + len(view.ErrorCases) != 2 || view.ErrorCases[0].Case != "401" || view.ErrorCases[0].Type != "" || + !view.ErrorCases[1].Default || view.ErrorCases[1].Type != "ReadError" || view.ErrorSwitch != "response.StatusCode" { t.Fatalf("unexpected prepared responses: %#v %v", view, err) } view = operationView{} diff --git a/generator/sdk/gosdk/error_response_precedence_test.go b/generator/sdk/gosdk/error_response_precedence_test.go new file mode 100644 index 00000000..c616e8ad --- /dev/null +++ b/generator/sdk/gosdk/error_response_precedence_test.go @@ -0,0 +1,134 @@ +// Copyright 2026 Princess B33f Heavy Industries / Dave Shanley +// SPDX-License-Identifier: MIT + +package gosdk + +import ( + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/pb33f/libopenapi" + "github.com/pb33f/libopenapi/generator/sdk" +) + +func TestGeneratedErrorResponsesPreserveUntypedOverrides(t *testing.T) { + document, err := libopenapi.NewDocument([]byte(errorResponsePrecedenceSpec)) + if err != nil { + t.Fatal(err) + } + model, err := document.BuildV3Model() + if err != nil { + t.Fatal(err) + } + result, err := Generate(&model.Model, Options{PackageName: "example", Prepare: sdk.PrepareOptions{ + Names: map[string]sdk.OperationName{"getWidgets": {Resource: "Widgets", Method: "Get"}}, + }}) + if err != nil { + t.Fatal(err) + } + directory := t.TempDir() + for _, file := range result.Files { + if err := os.WriteFile(filepath.Join(directory, file.Path), file.Content, 0o600); err != nil { + t.Fatal(err) + } + } + for path, content := range map[string]string{ + "go.mod": "module generated.example/response-precedence\n\ngo 1.26.0\n", + "client_test.go": errorResponsePrecedenceRuntime, + } { + if err := os.WriteFile(filepath.Join(directory, path), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + command := exec.Command("go", "test", "./...") + command.Dir = directory + command.Env = append(os.Environ(), "GOWORK=off") + if output, err := command.CombinedOutput(); err != nil { + t.Fatalf("generated error handling failed: %v\n%s", err, output) + } +} + +const errorResponsePrecedenceSpec = `openapi: 3.1.0 +info: {title: Widget service, version: '1'} +paths: + /widgets: + get: + operationId: getWidgets + responses: + '200': {description: OK} + '400': + description: Invalid request + content: + application/json: + schema: {type: string} + '404': + description: Missing widget + content: + text/plain: + schema: {type: string} + '405': {description: Method not allowed} + '4XX': + description: Other client error + content: + application/json: + schema: {type: integer} + '5XX': + description: Server error + content: + text/plain: + schema: {type: string} + '503': + description: Service unavailable + content: + application/json: + schema: {type: boolean} + default: + description: Other error + content: + application/json: + schema: {type: boolean} +` + +const errorResponsePrecedenceRuntime = `package example + +import ( + "context" + "errors" + "io" + "net/http" + "strings" + "testing" +) + +type responseDoer struct { status int; body string } + +func (d responseDoer) Do(*http.Request) (*http.Response, error) { + return &http.Response{StatusCode: d.status, Header: make(http.Header), Body: io.NopCloser(strings.NewReader(d.body))}, nil +} + +func TestErrorResponsePrecedence(t *testing.T) { + for _, test := range []struct { name string; status int; body string; typed bool }{ + {"typed exact over typed range", 400, "\"invalid\"", true}, + {"untyped exact over typed range", 404, "missing", false}, + {"bodyless exact over typed range", 405, "", false}, + {"typed range over default", 418, "42", true}, + {"untyped range over default", 502, "unavailable", false}, + {"typed exact over untyped range", 503, "true", true}, + {"typed default", 302, "false", true}, + } { + t.Run(test.name, func(t *testing.T) { + client, err := NewClient("https://example.com", WithHTTPClient(responseDoer{test.status, test.body})) + if err != nil { t.Fatal(err) } + _, err = client.Widgets.Get(context.Background()) + var apiError *APIError + if !errors.As(err, &apiError) { t.Fatalf("error = %v", err) } + if apiError.StatusCode != test.status || string(apiError.Body) != test.body || apiError.Cause != nil { + t.Fatalf("response evidence = %#v", apiError) + } + if (apiError.Value != nil) != test.typed { t.Fatalf("decoded value = %#v", apiError.Value) } + }) + } +} +` diff --git a/generator/sdk/gosdk/generator.go b/generator/sdk/gosdk/generator.go index e93d3125..e73ab4d3 100644 --- a/generator/sdk/gosdk/generator.go +++ b/generator/sdk/gosdk/generator.go @@ -697,9 +697,8 @@ func (e *emitter) prepareResponses(operation *sdk.Operation, base, method string if _, err := statusCondition([]string{status}); err != nil { return fmt.Errorf("gosdk: operation %q error response %s: %w", operation.ID, status, err) } - if typeName != "" { - errorCases = append(errorCases, errorResponseView{Statuses: []string{status}, Type: typeName}) - } + // Untyped responses still override a typed range or default response. + errorCases = append(errorCases, errorResponseView{Statuses: []string{status}, Type: typeName}) } if len(view.SuccessStatuses) == 0 { return fmt.Errorf("gosdk: operation %q has no declared 2xx response", operation.ID) @@ -720,6 +719,9 @@ func (e *emitter) prepareResponses(operation *sdk.Operation, base, method string view.DecodeAll = view.DecodeCondition == view.SuccessCondition view.ErrorCases, view.ErrorSwitch = errorSwitch(errorCases) for _, errorCase := range view.ErrorCases { + if errorCase.Type == "" { + continue + } statuses := strings.Join(errorCase.Statuses, ", ") if errorCase.Default { statuses = "default" @@ -735,6 +737,7 @@ func (e *emitter) prepareResponses(operation *sdk.Operation, base, method string // ranges the switch is on the status code itself. func errorSwitch(responses []errorResponseView) ([]errorResponseView, string) { var exact, ranges, fallback []errorResponseView + hasTypedResponse := false group := func(cases []errorResponseView, response errorResponseView) []errorResponseView { for index := range cases { if cases[index].Type == response.Type { @@ -745,6 +748,7 @@ func errorSwitch(responses []errorResponseView) ([]errorResponseView, string) { return append(cases, response) } for _, response := range responses { + hasTypedResponse = hasTypedResponse || response.Type != "" switch status := response.Statuses[0]; { case status == "default": fallback = []errorResponseView{{Type: response.Type, Default: true}} @@ -754,6 +758,9 @@ func errorSwitch(responses []errorResponseView) ([]errorResponseView, string) { exact = group(exact, response) } } + if !hasTypedResponse { + return nil, "" + } for _, cases := range [][]errorResponseView{exact, ranges} { for index := range cases { sort.Strings(cases[index].Statuses) diff --git a/generator/sdk/gosdk/templates/resources.tmpl b/generator/sdk/gosdk/templates/resources.tmpl index f0e5d809..3eddb05f 100644 --- a/generator/sdk/gosdk/templates/resources.tmpl +++ b/generator/sdk/gosdk/templates/resources.tmpl @@ -118,7 +118,9 @@ var security{{$resource.Name}}{{.MethodName}} = [][]securityRequirement{ {{- else}} case {{.Case}}: {{- end}} +{{- if .Type}} decodeErrorValue[{{.Type}}](apiError) +{{- end}} {{- end}} } {{- end}} diff --git a/generator/sdk/gosdk/testdata/people_match_resources.golden.go b/generator/sdk/gosdk/testdata/people_match_resources.golden.go index d14f3334..90396d09 100644 --- a/generator/sdk/gosdk/testdata/people_match_resources.golden.go +++ b/generator/sdk/gosdk/testdata/people_match_resources.golden.go @@ -211,6 +211,7 @@ func (resource *PeopleResource) Match(ctx context.Context, input *PeopleMatchPar switch response.StatusCode { case 400: decodeErrorValue[PeopleMatchBadRequest](apiError) + case 401: case 429: decodeErrorValue[PeopleMatchTooManyRequests](apiError) } diff --git a/index/effective_yaml_test.go b/index/effective_yaml_test.go new file mode 100644 index 00000000..43a761a2 --- /dev/null +++ b/index/effective_yaml_test.go @@ -0,0 +1,178 @@ +package index + +import ( + "context" + "strings" + "testing" + + "github.com/pb33f/go-yaml" + "github.com/pb33f/testify/assert" + "github.com/pb33f/testify/require" +) + +func TestEffectiveReferenceOccurrences(t *testing.T) { + const source = `openapi: 3.1.1 +info: {title: Index, version: '1'} +paths: {} +x-ref: &ref {$ref: '#/components/schemas/Target'} +x-props: &props + nested: *ref +components: + schemas: + Target: {type: string} + First: {type: object, properties: *props} + Second: {type: object, properties: {<<: *props}} + Example: + type: object + example: {nested: *ref} + examples: + Named: + value: {nested: *ref} +` + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(source), &root)) + before, err := yaml.Marshal(&root) + require.NoError(t, err) + idx := NewSpecIndex(&root) + paths := map[string]bool{} + for _, ref := range idx.rawSequencedRefs { + paths[strings.Join(ref.SourcePath, "/")] = true + } + assert.Contains(t, paths, "components/schemas/First/properties/nested") + assert.Contains(t, paths, "components/schemas/Second/properties/nested") + for path := range paths { + assert.NotContains(t, path, "/Example/") + assert.NotContains(t, path, "/examples/Named/value/") + } + assert.Empty(t, idx.GetReferenceIndexErrors()) + after, err := yaml.Marshal(&root) + require.NoError(t, err) + assert.Equal(t, before, after) +} + +func TestEffectiveFragmentTraversal(t *testing.T) { + const source = `base: &base + escaped/key: &value {type: string} +components: {schemas: {<<: *base}} +array: [*value] +` + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(source), &root)) + for _, path := range []string{"#/components/schemas/escaped~1key/type", "#/array/0/type"} { + got := navigateToFragment(&root, path) + require.NotNil(t, got, path) + assert.Equal(t, "string", got.Value) + } + assert.Nil(t, navigateToFragment(&root, "#/components/schemas/missing")) + assert.Nil(t, navigateToFragment(&root, "#/array/1")) +} + +func TestEffectiveTraversalCycles(t *testing.T) { + for _, source := range []string{ + "x-map: &mapping {<<: *mapping, nested: *mapping, local: value}\n", + "x-sequence: &sequence [*sequence]\n", + } { + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(source), &root)) + idx := NewSpecIndex(&root) + assert.Empty(t, idx.GetReferenceIndexErrors()) + } +} + +func TestEffectiveComponentCollections(t *testing.T) { + const source = `openapi: 3.1.1 +info: {title: Index, version: '1'} +paths: {} +x-entry: &entry + Named: {description: inherited} +x-collections: &collections + schemas: {<<: *entry} + parameters: {<<: *entry} + requestBodies: {<<: *entry} + responses: {<<: *entry} + securitySchemes: {<<: *entry} + headers: {<<: *entry} + examples: {<<: *entry} + links: {<<: *entry} + callbacks: {<<: *entry} + pathItems: {<<: *entry} +components: *collections +` + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(source), &root)) + idx := NewSpecIndex(&root) + assert.Equal(t, 1, idx.GetComponentSchemaCount()) + assert.Equal(t, 1, idx.GetComponentParameterCount()) + for _, kind := range []string{"schemas", "parameters", "requestBodies", "responses", "securitySchemes", "headers", "examples", "links", "callbacks", "pathItems"} { + ref := idx.FindComponent(context.Background(), "#/components/"+kind+"/Named") + require.NotNil(t, ref, kind) + assert.Equal(t, "Named", ref.KeyNode.Value, kind) + assert.Nil(t, idx.FindComponent(context.Background(), "#/components/"+kind+"/<<"), kind) + } +} + +func TestEffectiveInlineSchemaCollections(t *testing.T) { + const source = `openapi: 3.1.1 +info: {title: Index, version: '1'} +paths: {} +x-schema: &schema {type: object, properties: {name: {type: string}}} +x-map: &map {child: *schema} +x-array: &array [*schema] +components: + schemas: + Single: {type: array, items: *schema} + Mapping: {type: object, properties: {<<: *map}} + Array: {allOf: *array} +` + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(source), &root)) + idx := NewSpecIndex(&root) + definitions := map[string]*Reference{} + for _, ref := range idx.allInlineSchemaObjectDefinitions { + definitions[ref.Definition] = ref + } + for _, definition := range []string{ + "#/components/schemas/Single/items", + "#/components/schemas/Mapping/properties/child", + "#/components/schemas/Array/allOf/0", + } { + assert.Contains(t, definitions, definition) + } +} + +func TestEffectiveSchemaIDScope(t *testing.T) { + const source = `openapi: 3.1.1 +info: {title: Index, version: '1'} +paths: {} +x-scope: &scope {$id: 'https://example.test/scoped', type: object} +x-property: &property {$ref: '#/components/schemas/Target'} +components: + schemas: + Target: {type: string} + Scoped: + <<: *scope + properties: {value: *property} + Ordinary: + type: object + properties: {value: *property} +` + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(source), &root)) + idx := NewSpecIndex(&root) + scopes := map[string]string{} + for _, ref := range idx.rawSequencedRefs { + scopes[strings.Join(ref.SourcePath, "/")] = ref.SchemaIdBase + } + assert.Equal(t, "https://example.test/scoped", scopes["components/schemas/Scoped/properties/value"]) + assert.Equal(t, "", scopes["components/schemas/Ordinary/properties/value"]) +} + +func TestEffectiveLookupWithAbsentNodes(t *testing.T) { + t.Run("missing collection mapping", func(t *testing.T) { + require.Nil(t, effectiveMappingValue(nil, "schemas")) + }) + t.Run("document with absent content node", func(t *testing.T) { + root := &yaml.Node{Kind: yaml.DocumentNode, Content: []*yaml.Node{nil}} + require.Nil(t, navigateToFragment(root, "#/type")) + }) +} diff --git a/index/extract_refs.go b/index/extract_refs.go index 0d0b642c..984bff0b 100644 --- a/index/extract_refs.go +++ b/index/extract_refs.go @@ -5,8 +5,12 @@ package index import ( "context" + "fmt" + "strings" "github.com/pb33f/go-yaml" + "github.com/pb33f/go-yaml/plugin/limit" + "github.com/pb33f/libopenapi/utils" ) func isSchemaContainingNode(v string) bool { @@ -35,8 +39,8 @@ func isArrayOfSchemaContainingNode(v string) bool { } // underOpenAPIExamplePath reports whether seenPath is under an OpenAPI example or examples -// keyword (sample data, not schema). A segment named "example" or "examples" that is preceded -// by "properties" or "patternProperties" is a schema property name, not an OpenAPI keyword. +// keyword (sample data, not schema). Entries in maps of named schemas are names, +// even when the name is "example" or "examples". func underOpenAPIExamplePath(seenPath []string) bool { for i := range seenPath { if isOpenAPIExampleKeywordSegment(seenPath, i) { @@ -50,12 +54,28 @@ func isOpenAPIExampleKeywordSegment(seenPath []string, idx int) bool { if idx < 0 || idx >= len(seenPath) { return false } - switch seenPath[idx] { - case "example", "examples": - return idx == 0 || (seenPath[idx-1] != "properties" && seenPath[idx-1] != "patternProperties") - default: + if seenPath[idx] != "example" && seenPath[idx] != "examples" { return false } + // A component or schema name cannot itself introduce another named map. + // For example, properties/definitions/example ends at a payload keyword. + nameSlot := false + for i, segment := range seenPath[:idx] { + if nameSlot { + nameSlot = false + continue + } + if i == 1 && seenPath[0] == "components" { + nameSlot = true + continue + } + switch segment { + case "properties", "patternProperties", "definitions", "$defs", "dependentSchemas", + "headers", "links", "callbacks", "encoding", "webhooks": + nameSlot = true + } + } + return !nameSlot } // underOpenAPIExamplePayloadPath reports whether seenPath points to raw example payload content. @@ -96,15 +116,133 @@ func isDirectOpenAPIExampleValuePath(seenPath []string) bool { return false } +// Limit plugins are immutable after construction and shared by extraction walks. +var defaultExtractRefsLimits limit.Plugin +var unlimitedExtractRefsLimits = limit.New(limit.AliasNone()) + // ExtractRefs will return a deduplicated slice of references for every unique ref found in the document. // The total number of refs, will generally be much higher, you can extract those from GetRawReferenceCount() +// Excessive YAML alias expansion stops extraction and is reported by GetReferenceIndexErrors. func (index *SpecIndex) ExtractRefs(ctx context.Context, node, parent *yaml.Node, seenPath []string, level int, poly bool, pName string) []*Reference { - if node == nil { + walk := &extractRefsTraversal{} + if index.config != nil && index.config.AllowExcessiveAliases { + walk.limits = unlimitedExtractRefsLimits + } + return index.extractRefs(ctx, node, parent, seenPath, level, poly, pName, walk) +} + +type extractRefsTraversal struct { + limits *limit.Plugin + ancestry map[*yaml.Node]struct{} + nodes int + aliasNodes int + aliasDepth int + stopped bool +} + +func (walk *extractRefsTraversal) visit() error { + walk.nodes++ + if walk.aliasDepth == 0 { return nil } + walk.aliasNodes++ + limits := walk.limits + if limits == nil { + limits = &defaultExtractRefsLimits + } + return limits.CheckAlias(walk.aliasNodes, walk.nodes) +} - state := index.initializeExtractRefsState(ctx, node, seenPath, level, poly, pName) - found := index.walkExtractRefs(node, parent, &state) +// Ancestry tracks canonical source nodes, not the shallow views created during +// merge expansion. A shared anchor is visited again at each distinct path. +func (index *SpecIndex) extractRefs(ctx context.Context, node, parent *yaml.Node, seenPath []string, level int, poly bool, pName string, walk *extractRefsTraversal) []*Reference { + if node == nil || walk.stopped { + return nil + } + err := walk.visit() + aliased := node.Kind == yaml.AliasNode && err == nil + if aliased { + walk.aliasDepth++ + node = utils.NodeAlias(node) + err = walk.visit() + } + if err != nil { + walk.stopped = true + // A stopped traversal cannot provide a complete model. + index.refErrors = append(index.refErrors, &ResolvingError{ + ErrorRef: fmt.Errorf("%w; set AllowExcessiveAliases for trusted documents", err), + Node: node, Path: "$." + strings.Join(seenPath, "."), + }) + if aliased { + walk.aliasDepth-- + } + return nil + } + if node == nil { + if aliased { + walk.aliasDepth-- + } + return nil + } + view, nodeID := resolveExtractRefsNode(node) + tracked := aliased || view != node + if tracked { + if _, active := walk.ancestry[node]; active { + if aliased { + walk.aliasDepth-- + } + return nil + } + if walk.ancestry == nil { + walk.ancestry = make(map[*yaml.Node]struct{}) + } + walk.ancestry[node] = struct{}{} + } + state := index.initializeExtractRefsState(ctx, nodeID, seenPath, level, poly, pName) + state.mergeStart = len(view.Content) + if view != node { + // Merge views retain explicit pairs first, then append inherited pairs. + // Only inherited descendants consume the alias expansion budget. + state.mergeStart = 0 + for i := 0; i < len(node.Content); i += 2 { + if key := node.Content[i]; key != nil && key.Tag != "!!merge" { + state.mergeStart += 2 + } + } + } + found := index.walkExtractRefs(view, parent, &state, walk) + if tracked { + delete(walk.ancestry, node) + } + if aliased { + walk.aliasDepth-- + } index.refCount = len(index.allRefs) return found } + +// resolveExtractRefsNode finds merge keys and the schema ID in one scan. Ordinary +// mappings retain their identity; merged mappings use the existing read-only view. +func resolveExtractRefsNode(node *yaml.Node) (*yaml.Node, string) { + if node.Kind != yaml.MappingNode && node.Tag != "!!map" { + return node, "" + } + var idNode *yaml.Node + for i := 0; i < len(node.Content); i += 2 { + key := node.Content[i] + if key == nil { + continue + } + if key.Tag == "!!merge" { + view := utils.ResolveMergeNodes(node) + return view, FindSchemaIdInNode(view) + } + if idNode == nil && node.Kind == yaml.MappingNode && key.Value == "$id" && i+1 < len(node.Content) && utils.IsNodeStringValue(node.Content[i+1]) { + idNode = node.Content[i+1] + } + } + if idNode != nil { + return node, idNode.Value + } + return node, "" +} diff --git a/index/extract_refs_alias_limit_test.go b/index/extract_refs_alias_limit_test.go new file mode 100644 index 00000000..956fceee --- /dev/null +++ b/index/extract_refs_alias_limit_test.go @@ -0,0 +1,173 @@ +package index + +import ( + "context" + "strings" + "testing" + + "github.com/pb33f/go-yaml" + "github.com/pb33f/testify/require" +) + +func TestExtractRefsStopsAtAliasLimit(t *testing.T) { + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte("type: string\n"), &root)) + idx := NewSpecIndex(&root) + // The parser starts checking the alias ratio after 1,000 node visits. + walk := &extractRefsTraversal{ + ancestry: make(map[*yaml.Node]struct{}), + nodes: 1000, aliasNodes: 1000, aliasDepth: 1, + } + require.Nil(t, idx.extractRefs(context.Background(), root.Content[0], &root, []string{"x-shared"}, 0, false, "", walk)) + require.True(t, walk.stopped) + require.Len(t, idx.GetReferenceIndexErrors(), 1) + err := idx.GetReferenceIndexErrors()[0].(*ResolvingError) + require.ErrorContains(t, err, "document contains excessive aliasing") + require.Same(t, root.Content[0], err.Node) + require.Equal(t, "$.x-shared", err.Path) + // Once stopped, later siblings do not continue work or add duplicate errors. + require.Nil(t, idx.extractRefs(context.Background(), root.Content[0], &root, nil, 0, false, "", walk)) + require.Len(t, idx.GetReferenceIndexErrors(), 1) + require.Equal(t, 1001, walk.nodes) +} + +func TestExtractRefsAbsentAliasTarget(t *testing.T) { + idx := NewSpecIndex(nil) + for _, node := range []*yaml.Node{nil, {Kind: yaml.AliasNode}} { + require.Nil(t, idx.ExtractRefs(context.Background(), node, nil, nil, 0, false, "")) + } + require.Empty(t, idx.GetReferenceIndexErrors()) +} + +func TestExtractRefsMergeCycleWithoutAliasInWalk(t *testing.T) { + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte("x: &x {b: {<<: *x}}\n"), &root)) + idx := NewSpecIndex(nil) + walk := &extractRefsTraversal{} + idx.extractRefs(context.Background(), &root, nil, nil, 0, false, "", walk) + require.False(t, walk.stopped) + require.Empty(t, idx.GetReferenceIndexErrors()) + require.Empty(t, walk.ancestry) + require.Zero(t, walk.aliasDepth) + require.Less(t, walk.nodes, 10) +} + +func TestExtractRefsPlainDocumentNeedsNoAncestry(t *testing.T) { + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(`openapi: 3.1.1 +components: + schemas: + Item: {type: object, properties: {id: {type: string}}} + Ref: {$ref: '#/components/schemas/Item'} +`), &root)) + idx := NewSpecIndex(nil) + walk := &extractRefsTraversal{} + refs := idx.extractRefs(context.Background(), &root, nil, nil, 0, false, "", walk) + require.Len(t, refs, 1) + require.Nil(t, walk.ancestry) + require.Zero(t, walk.aliasNodes) +} + +func TestExtractRefsCountsInheritedMergeDescendants(t *testing.T) { + const source = `shared: &shared {inherited: {nested: {type: string}}} +target: + before: {type: string} + <<: *shared + after: {type: integer} +` + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(source), &root)) + target := root.Content[0].Content[3] + t.Run("only inherited descendants", func(t *testing.T) { + idx := NewSpecIndex(nil) + walk := &extractRefsTraversal{ancestry: make(map[*yaml.Node]struct{}), nodes: 1000} + idx.extractRefs(context.Background(), target, nil, nil, 0, false, "", walk) + require.False(t, walk.stopped) + require.Empty(t, idx.GetReferenceIndexErrors()) + require.Equal(t, 1005, walk.nodes) + require.Equal(t, 2, walk.aliasNodes) + require.Zero(t, walk.aliasDepth) + }) + t.Run("inherited descendants reach limit", func(t *testing.T) { + idx := NewSpecIndex(nil) + walk := &extractRefsTraversal{ancestry: make(map[*yaml.Node]struct{}), nodes: 997, aliasNodes: 997} + idx.extractRefs(context.Background(), target, nil, []string{"target"}, 0, false, "", walk) + require.True(t, walk.stopped) + require.Len(t, idx.GetReferenceIndexErrors(), 1) + require.ErrorContains(t, idx.GetReferenceIndexErrors()[0], "document contains excessive aliasing") + require.Equal(t, 1001, walk.nodes) + require.Equal(t, 998, walk.aliasNodes) + require.Zero(t, walk.aliasDepth) + }) +} + +func TestExtractRefsAllowsOrdinaryAliases(t *testing.T) { + const source = `openapi: 3.1.1 +info: {title: Aliases, version: '1'} +paths: {} +x-meta: &meta {description: shared metadata} +x-ref: &ref {$ref: '#/components/schemas/Target'} +x-props: &props {value: *ref} +components: + schemas: + Target: {type: string} + First: {type: object, properties: {value: *ref}, x-meta: *meta} + Second: {type: object, properties: {value: *ref}, x-meta: *meta} + MergedFirst: {type: object, properties: {<<: *props}} + MergedSecond: {type: object, properties: {<<: *props}} +` + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(source), &root)) + idx := NewSpecIndex(&root) + require.Empty(t, idx.GetReferenceIndexErrors()) + paths := make(map[string]bool) + for _, ref := range idx.GetRawReferencesSequenced() { + paths[strings.Join(ref.SourcePath, "/")] = true + } + require.Contains(t, paths, "components/schemas/First/properties/value") + require.Contains(t, paths, "components/schemas/Second/properties/value") + require.Contains(t, paths, "components/schemas/MergedFirst/properties/value") + require.Contains(t, paths, "components/schemas/MergedSecond/properties/value") +} + +func TestExtractRefsTrustedAliasesStillStopCycles(t *testing.T) { + config := CreateClosedAPIIndexConfig() + config.AllowExcessiveAliases = true + idx := NewSpecIndexWithConfig(nil, config) + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(strings.ReplaceAll(excessiveAliasDocument, "./unvisited.yaml", "#/components/schemas/Target")), &root)) + require.NotEmpty(t, idx.ExtractRefs(context.Background(), &root, nil, nil, 0, false, "")) + require.Empty(t, idx.GetReferenceIndexErrors()) + require.True(t, idx.GetConfig().ToDocumentConfiguration().AllowExcessiveAliases) + var cycle yaml.Node + require.NoError(t, yaml.Unmarshal([]byte("x: &x {b: {<<: *x}}"), &cycle)) + require.Empty(t, idx.ExtractRefs(context.Background(), &cycle, nil, nil, 0, false, "")) + require.Empty(t, idx.GetReferenceIndexErrors()) +} + +func TestExtractRefsMergeViewPreservesSchemaID(t *testing.T) { + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(`shared: &shared {$id: 'https://example.com/shared', type: object} +withID: {$id: 'https://example.com/local', <<: *shared} +inherited: {<<: *shared} +`), &root)) + mapping := root.Content[0] + view, id := resolveExtractRefsNode(mapping.Content[3]) + require.Equal(t, "https://example.com/local", id) + require.NotSame(t, mapping.Content[3], view) + _, inherited := resolveExtractRefsNode(mapping.Content[5]) + require.Equal(t, "https://example.com/shared", inherited) + _, documentID := resolveExtractRefsNode(&root) + require.Empty(t, documentID) +} + +func TestExtractRefsSchemaIDKeepsFirstEmptyValue(t *testing.T) { + mapping := &yaml.Node{Kind: yaml.MappingNode, Content: []*yaml.Node{ + nil, {Kind: yaml.ScalarNode, Value: "ignored"}, + {Kind: yaml.ScalarNode, Value: "$id"}, {Kind: yaml.ScalarNode, Tag: "!!str"}, + {Kind: yaml.ScalarNode, Value: "$id"}, {Kind: yaml.ScalarNode, Tag: "!!str", Value: "https://example.com/later"}, + }} + view, id := resolveExtractRefsNode(mapping) + require.Same(t, mapping, view) + require.Empty(t, id) +} diff --git a/index/extract_refs_inline.go b/index/extract_refs_inline.go index 2cc2f35d..fe61707a 100644 --- a/index/extract_refs_inline.go +++ b/index/extract_refs_inline.go @@ -50,7 +50,7 @@ func (index *SpecIndex) collectInlineSchemaDefinition(parent, node *yaml.Node, s } keyNode := node.Content[keyIndex] - valueNode := node.Content[keyIndex+1] + valueNode := utils.ResolveMergeNodes(node.Content[keyIndex+1]) var jsonPath, definitionPath, fullDefinitionPath string if len(seenPath) > 0 || keyNode.Value != "" { @@ -90,7 +90,7 @@ func (index *SpecIndex) collectMapSchemaDefinitions(parent, node *yaml.Node, see } keyNode := node.Content[keyIndex] - propertiesNode := node.Content[keyIndex+1] + propertiesNode := utils.ResolveMergeNodes(node.Content[keyIndex+1]) if len(seenPath) > 0 { for _, p := range seenPath { @@ -108,6 +108,7 @@ func (index *SpecIndex) collectMapSchemaDefinitions(parent, node *yaml.Node, see continue } + prop = utils.ResolveMergeNodes(prop) var jsonPath, definitionPath, fullDefinitionPath string if len(seenPath) > 0 || keyNode.Value != "" && label != "" { if prefix == "" { @@ -146,10 +147,11 @@ func (index *SpecIndex) collectArraySchemaDefinitions(parent, node *yaml.Node, s } keyNode := node.Content[keyIndex] - arrayNode := node.Content[keyIndex+1] + arrayNode := utils.ResolveMergeNodes(node.Content[keyIndex+1]) prefix := "" for h, element := range arrayNode.Content { + element = utils.ResolveMergeNodes(element) var jsonPath, definitionPath, fullDefinitionPath string if len(seenPath) > 0 { if prefix == "" { diff --git a/index/extract_refs_test.go b/index/extract_refs_test.go index 43e888ab..271064bb 100644 --- a/index/extract_refs_test.go +++ b/index/extract_refs_test.go @@ -5,6 +5,7 @@ package index import ( "context" + "fmt" "runtime" "strings" "testing" @@ -1087,7 +1088,7 @@ properties: func TestExtractRefs_WalkHelpers(t *testing.T) { idx := NewTestSpecIndex().Load().(*SpecIndex) - state := idx.initializeExtractRefsState(context.Background(), nil, nil, 0, false, "") + state := idx.initializeExtractRefsState(context.Background(), "", nil, 0, false, "") node := &yaml.Node{ Kind: yaml.MappingNode, @@ -1226,3 +1227,46 @@ components: assert.True(t, rawRefs["#/components/examples/ReusableExample"]) assert.False(t, rawRefs["#/components/schemas/ShouldNotIndex"]) } + +// Copyright 2023 Princess B33f Heavy Industries / Dave Shanley +// SPDX-License-Identifier: MIT + +func TestExtractRefs_ExampleUnderSchemaKeywordName(t *testing.T) { + for _, name := range []string{"schemas", "definitions", "$defs", "dependentSchemas", "properties", "patternProperties"} { + t.Run(name, func(t *testing.T) { + source := fmt.Sprintf("openapi: 3.1.1\ninfo: {title: Payload, version: '1'}\npaths: {}\ncomponents:\n schemas:\n Thing:\n type: object\n properties:\n %s:\n type: object\n example:\n nested: {$ref: '#/not-a-schema'}\n", name) + var root yaml.Node + if err := yaml.Unmarshal([]byte(source), &root); err != nil { + t.Fatal(err) + } + idx := NewSpecIndex(&root) + if refs := idx.GetRawReferencesSequenced(); len(refs) != 0 { + t.Fatalf("example payload indexed: refs=%d first=%s path=%v errors=%v", len(refs), refs[0].Definition, refs[0].SourcePath, idx.GetReferenceIndexErrors()) + } + }) + } +} + +func TestExtractRefs_ExampleUnderComponentKeywordName(t *testing.T) { + for _, container := range []string{"parameters", "headers"} { + for _, name := range []string{"definitions", "$defs", "dependentSchemas", "properties", "patternProperties"} { + source := fmt.Sprintf(`openapi: 3.1.1 +info: {title: Component examples, version: '1'} +paths: {} +components: + %s: + %s: + name: test + in: query + schema: {type: object} + example: + nested: {$ref: '#/payload-only'} +`, container, name) + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(source), &root)) + idx := NewSpecIndex(&root) + require.Empty(t, idx.GetRawReferencesSequenced(), "%s/%s example is payload", container, name) + require.Empty(t, idx.GetReferenceIndexErrors()) + } + } +} diff --git a/index/extract_refs_walk.go b/index/extract_refs_walk.go index a9eb1717..02fd53c9 100644 --- a/index/extract_refs_walk.go +++ b/index/extract_refs_walk.go @@ -12,6 +12,7 @@ import ( ) type extractRefsState struct { + mergeStart int ctx context.Context scope *SchemaIdScope parentBaseURI string @@ -25,7 +26,7 @@ type extractRefsState struct { func (index *SpecIndex) initializeExtractRefsState( ctx context.Context, - node *yaml.Node, + nodeID string, seenPath []string, level int, poly bool, @@ -38,16 +39,14 @@ func (index *SpecIndex) initializeExtractRefsState( } parentBaseURI := scope.BaseUri - if node != nil && node.Kind == yaml.MappingNode && !underOpenAPIExamplePath(seenPath) { - if nodeID := FindSchemaIdInNode(node); nodeID != "" { - resolvedNodeID, _ := ResolveSchemaId(nodeID, parentBaseURI) - if resolvedNodeID == "" { - resolvedNodeID = nodeID - } - scope = scope.Copy() - scope.PushId(resolvedNodeID) - ctx = WithSchemaIdScope(ctx, scope) + if nodeID != "" && !underOpenAPIExamplePath(seenPath) { + resolvedNodeID, _ := ResolveSchemaId(nodeID, parentBaseURI) + if resolvedNodeID == "" { + resolvedNodeID = nodeID } + scope = scope.Copy() + scope.PushId(resolvedNodeID) + ctx = WithSchemaIdScope(ctx, scope) } return extractRefsState{ @@ -61,7 +60,7 @@ func (index *SpecIndex) initializeExtractRefsState( } } -func (index *SpecIndex) walkExtractRefs(node, parent *yaml.Node, state *extractRefsState) []*Reference { +func (index *SpecIndex) walkExtractRefs(node, parent *yaml.Node, state *extractRefsState, walk *extractRefsTraversal) []*Reference { if node == nil || len(node.Content) == 0 { return nil } @@ -77,7 +76,17 @@ func (index *SpecIndex) walkExtractRefs(node, parent *yaml.Node, state *extractR } if utils.IsNodeMap(n) || utils.IsNodeArray(n) { - found = append(found, index.walkChildExtractRefs(n, node, state)...) + inherited := i >= state.mergeStart + if inherited { + walk.aliasDepth++ + } + found = append(found, index.walkChildExtractRefs(n, node, state, walk)...) + if inherited { + walk.aliasDepth-- + } + if walk.stopped { + break + } } index.unwindExtractRefsPath(node, state, i) @@ -86,7 +95,7 @@ func (index *SpecIndex) walkExtractRefs(node, parent *yaml.Node, state *extractR return found } -func (index *SpecIndex) walkChildExtractRefs(node, parent *yaml.Node, state *extractRefsState) []*Reference { +func (index *SpecIndex) walkChildExtractRefs(node, parent *yaml.Node, state *extractRefsState, walk *extractRefsTraversal) []*Reference { if underOpenAPIExamplePayloadPath(state.seenPath) { return nil } @@ -100,10 +109,11 @@ func (index *SpecIndex) walkChildExtractRefs(node, parent *yaml.Node, state *ext state.polyName = state.prev } } - return index.ExtractRefs(state.ctx, node, parent, state.seenPath, state.level, state.poly, state.polyName) + return index.extractRefs(state.ctx, node, parent, state.seenPath, state.level, state.poly, state.polyName, walk) } func isDirectOpenAPIExampleRefNode(node *yaml.Node) bool { + node = utils.ResolveMergeNodes(node) return utils.IsNodeMap(node) && utils.GetRefValueNode(node) != nil && len(node.Content) == 2 } diff --git a/index/find_component_external.go b/index/find_component_external.go index 80b464f9..172e220c 100644 --- a/index/find_component_external.go +++ b/index/find_component_external.go @@ -89,10 +89,13 @@ func (index *SpecIndex) lookupRolodex(ctx context.Context, uri []string) *Refere } } - foundRef := FindComponent(ctx, parsedDocument, query, absoluteFileLocation, index) + foundRef := FindComponent(ctx, parsedDocument, query, absoluteFileLocation, idx) if foundRef != nil { foundRef.IsRemote = true foundRef.RemoteLocation = absoluteFileLocation + if source := index.allRefs[absoluteFileLocation+query]; source != nil { + foundRef.SourcePath = append([]string(nil), source.SourcePath...) + } return foundRef } index.logger.Debug("[lookupRolodex] FindComponent returned nil", diff --git a/index/index_model.go b/index/index_model.go index f10c2d0b..b0a81c01 100644 --- a/index/index_model.go +++ b/index/index_model.go @@ -85,6 +85,9 @@ func (rm *ReferenceMapped) MarshalJSON() ([]byte, error) { // allowing or disallowing remote or local file lookups. // - https://github.com/pb33f/libopenapi/issues/73 type SpecIndexConfig struct { + // AllowExcessiveAliases disables the YAML alias expansion limit for trusted + // documents with extensive shared content. Cycle detection remains enabled. + AllowExcessiveAliases bool // The BaseURL will be the root from which relative references will be resolved from if they can't be found locally. // // For example: @@ -282,6 +285,7 @@ func (s *SpecIndexConfig) ToDocumentConfiguration() *datamodel.DocumentConfigura BasePath: s.BasePath, SpecFilePath: s.SpecFilePath, AllowFileReferences: s.AllowFileLookup, + AllowExcessiveAliases: s.AllowExcessiveAliases, AllowRemoteReferences: s.AllowRemoteLookup, BypassDocumentCheck: s.SkipDocumentCheck, IgnorePolymorphicCircularReferences: s.IgnorePolymorphicCircularReferences, diff --git a/index/resolver_circular.go b/index/resolver_circular.go index 836f4f42..72c7a227 100644 --- a/index/resolver_circular.go +++ b/index/resolver_circular.go @@ -30,7 +30,8 @@ func (resolver *Resolver) buildCircularReferenceResult( journey []*Reference, loopIndex int, ) *CircularReferenceResult { - loop := append(journey, foundDup) + // Keep the recorded journey independent of later sibling traversal. + loop := append(journey[:len(journey):len(journey)], foundDup) visitedDefinitions := make(map[string]bool) isInfiniteLoop, _ := resolver.isInfiniteCircularDependency(foundDup, visitedDefinitions, nil) @@ -99,6 +100,9 @@ func (resolver *Resolver) isInfiniteCircularDependency( } for refDefinition := range ref.RequiredRefProperties { r, _ := resolver.specIndex.SearchIndexForReference(refDefinition) + if r == nil { + continue // Unresolved targets are reported by reference resolution. + } // Direct loop back to the original starting reference — infinite cycle. if initialRef != nil && initialRef.FullDefinition == r.FullDefinition { diff --git a/index/resolver_entry.go b/index/resolver_entry.go index a604c7ed..4c094005 100644 --- a/index/resolver_entry.go +++ b/index/resolver_entry.go @@ -12,6 +12,9 @@ import ( "github.com/pb33f/libopenapi/utils" ) +// ErrReferenceChainCycle indicates a reference-only chain with no terminal value. +var ErrReferenceChainCycle = errors.New("circular reference chain detected") + // ResolvingError represents an issue the resolver had trying to stitch the tree together. type ResolvingError struct { ErrorRef error diff --git a/index/resolver_polymorphic.go b/index/resolver_polymorphic.go index dc5adbad..1ccf4f5f 100644 --- a/index/resolver_polymorphic.go +++ b/index/resolver_polymorphic.go @@ -13,6 +13,7 @@ func (resolver *Resolver) extractPolymorphicRelatives( node, keywordNode *yaml.Node, state relativeWalkState, index int, + walkPath []string, ) []*Reference { var found []*Reference @@ -40,7 +41,7 @@ func (resolver *Resolver) extractPolymorphicRelatives( if d, _, l := utils.IsNodeRefValue(v); d { resolver.visitPolymorphicReference(ref, keywordNode.Value, node.Content[index], l, state, index) } else { - found = append(found, resolver.extractRelativesWithState(ref, v, keywordNode, state.descend())...) + found = append(found, resolver.extractRelativesWithState(ref, v, keywordNode, state.descend(), walkPath)...) } } } @@ -73,7 +74,7 @@ func (resolver *Resolver) visitPolymorphicReference( return } - loop := append(state.journey, mappedRefs) + loop := append(state.journey[:len(state.journey):len(state.journey)], mappedRefs) circRef := &CircularReferenceResult{ ParentNode: parentNode, Journey: loop, diff --git a/index/resolver_relatives.go b/index/resolver_relatives.go index bf0dd0f5..de75550c 100644 --- a/index/resolver_relatives.go +++ b/index/resolver_relatives.go @@ -24,14 +24,50 @@ func (resolver *Resolver) extractRelatives( schemaIDBase string, ) []*Reference { state := newRelativeWalkState(foundRelatives, journey, resolve, depth, schemaIDBase) - return resolver.extractRelativesWithState(ref, node, parent, state) + // Keep ordinary paths on this walk's stack. Child walks reuse capacity; + // unusually deep paths grow normally without changing the traversal limit. + var pathBuffer [32]string + walkPath := pathBuffer[:0] + if ref != nil { + if strings.HasPrefix(ref.Definition, "#/") { + for segment := range strings.SplitSeq(strings.TrimPrefix(ref.Definition, "#/"), "/") { + walkPath = append(walkPath, segment) + } + } else { + walkPath = append(walkPath, ref.SourcePath...) + } + if len(walkPath) > 0 && (walkPath[0] == "example" || walkPath[0] == "examples") && ref.Index != nil { + root := ref.Index.GetRootNode() + if root != nil { + if root.Kind == yaml.DocumentNode && len(root.Content) > 0 { + root = root.Content[0] + } + _, openapi := utils.FindKeyNodeTop("openapi", root.Content) + _, swagger := utils.FindKeyNodeTop("swagger", root.Content) + if openapi == nil && swagger == nil { + // An external fragment identifies the object being resolved. Its + // top-level name is not a keyword in the referencing document. + // Example fields encountered inside that object still mark payloads. + walkPath = append(pathBuffer[:0], ref.SourcePath...) + } + } + } + } + return resolver.extractRelativesWithState(ref, node, parent, state, walkPath) } func (resolver *Resolver) extractRelativesWithState( ref *Reference, node, parent *yaml.Node, state relativeWalkState, + walkPath []string, ) []*Reference { + if underOpenAPIExamplePayloadPath(walkPath) { + return nil + } + if isDirectOpenAPIExampleValuePath(walkPath) && !isDirectOpenAPIExampleRefNode(node) { + return nil + } // Guard against stack overflow from deeply nested or circular specs. // Journey tracks the reference chain (100 max); depth tracks recursive calls (500 max). if len(state.journey) > 100 { @@ -53,7 +89,14 @@ func (resolver *Resolver) extractRelativesWithState( } if utils.IsNodeMap(n) || utils.IsNodeArray(n) { - found = append(found, resolver.extractNestedRelatives(ref, node, n, state)...) + childState := state + childPath := walkPath + if node.Kind == yaml.MappingNode && i%2 == 1 { + childPath = append(walkPath, node.Content[i-1].Value) + } else if node.Kind == yaml.SequenceNode { + childPath = append(walkPath, "[]") + } + found = append(found, resolver.extractNestedRelatives(ref, node, n, childState, childPath)...) } if i%2 == 0 && n.Value == "$ref" && len(node.Content) > i%2+1 { @@ -67,7 +110,7 @@ func (resolver *Resolver) extractRelativesWithState( } if i%2 == 0 && shouldExtractPolymorphicRelatives(parent, n) { - found = append(found, resolver.extractPolymorphicRelatives(ref, node, n, state, i)...) + found = append(found, resolver.extractPolymorphicRelatives(ref, node, n, state, i, walkPath)...) skip = true } } @@ -88,7 +131,7 @@ func (resolver *Resolver) handleRelativeDepthLimit(ref *Reference, state relativ "reference", def) } - loop := append(state.journey, ref) + loop := append(state.journey[:len(state.journey):len(state.journey)], ref) circRef := &CircularReferenceResult{ Journey: loop, Start: ref, @@ -107,6 +150,7 @@ func (resolver *Resolver) extractNestedRelatives( ref *Reference, parent, node *yaml.Node, state relativeWalkState, + walkPath []string, ) []*Reference { childState := state.descend() foundRef, _, _ := resolver.searchReferenceWithContext(ref, ref) @@ -114,9 +158,9 @@ func (resolver *Resolver) extractNestedRelatives( if foundRef.Circular { return nil } - return resolver.extractRelativesWithState(foundRef, node, parent, childState) + return resolver.extractRelativesWithState(foundRef, node, parent, childState, walkPath) } - return resolver.extractRelativesWithState(ref, node, parent, childState) + return resolver.extractRelativesWithState(ref, node, parent, childState, walkPath) } func (resolver *Resolver) extractRelativeReference( diff --git a/index/resolver_required_alias_test.go b/index/resolver_required_alias_test.go new file mode 100644 index 00000000..faeae6a4 --- /dev/null +++ b/index/resolver_required_alias_test.go @@ -0,0 +1,63 @@ +package index + +import ( + "testing" + + "github.com/pb33f/go-yaml" + "github.com/pb33f/testify/require" +) + +func TestResolver_RequiredReferenceMissingTarget(t *testing.T) { + var node yaml.Node + require.NoError(t, yaml.Unmarshal([]byte("type: object\n"), &node)) + idx := NewSpecIndex(&node) + resolver := NewResolver(idx) + infinite, _ := resolver.isInfiniteCircularDependency(&Reference{ + FullDefinition: "#/Item", + RequiredRefProperties: map[string][]string{"#/Missing": {"id"}}, + }, make(map[string]bool), nil) + require.False(t, infinite) +} + +func TestResolver_CircularJourneySnapshot(t *testing.T) { + original := &Reference{FullDefinition: "#/Loop"} + journey := make([]*Reference, 1, 8) + journey[0] = original + resolver := &Resolver{} + result := resolver.buildCircularReferenceResult(original, original, journey, 0) + journey[0] = &Reference{FullDefinition: "#/Sibling"} + journey = append(journey, &Reference{FullDefinition: "#/Child"}) + require.Equal(t, []*Reference{original, original}, result.Journey) +} + +func TestRequiredAliasPreservesLocalFragment(t *testing.T) { + var node yaml.Node + require.NoError(t, yaml.Unmarshal([]byte("$ref: '#/$defs/ID'\n$defs:\n ID: {type: string}\n"), &node)) + for _, location := range []string{"/spec/schemas/alias.yaml", "https://example.com/schemas/alias.yaml"} { + targets := extractDefinitionRequiredRefProperties(node.Content[0], map[string][]string{}, location, nil) + require.Contains(t, targets, location+"#/$defs/ID") + } +} + +func TestRequiredPropertyWithoutDefinition(t *testing.T) { + var node yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(`type: object +required: [missing, id] +properties: + id: {$ref: '#/$defs/ID'} +`), &node)) + targets := extractDefinitionRequiredRefProperties(node.Content[0], map[string][]string{}, "https://example.com/schema.yaml", nil) + require.Equal(t, map[string][]string{"https://example.com/schema.yaml#/$defs/ID": {"id"}}, targets) +} + +func TestRelativeSchemaFileUsesRemoteOwner(t *testing.T) { + resolver := NewResolver(NewSpecIndex(nil)) + for _, ref := range []*Reference{ + {FullDefinition: "https://example.com/schemas/owner.yaml#/components/schemas/Pet"}, + {FullDefinition: "#/components/schemas/Pet", RemoteLocation: "https://example.com/schemas/owner.yaml"}, + } { + definition, full := resolver.buildRelativeLookupDefinitions(ref, "child.yaml", "") + require.Equal(t, "child.yaml", definition) + require.Equal(t, "https://example.com/schemas/child.yaml", full) + } +} diff --git a/index/resolver_test.go b/index/resolver_test.go index 7f55b8df..04a41a93 100644 --- a/index/resolver_test.go +++ b/index/resolver_test.go @@ -4,12 +4,14 @@ import ( "bytes" "errors" "fmt" + "github.com/pb33f/testify/require" "log/slog" "net/http" "net/http/httptest" "net/url" "os" "path/filepath" + "strings" "testing" "github.com/pb33f/jsonpath/pkg/jsonpath" @@ -2042,3 +2044,48 @@ func TestResolver_ResolveRelativeReference(t *testing.T) { assert.True(t, relative.Resolved) assert.Equal(t, foundNode.Content, relative.Node.Content) } + +func TestResolverNamedExternalExampleFragment(t *testing.T) { + for _, source := range []string{ + "example: {properties: {value: {$ref: '#/components/schemas/Target'}}}\ncomponents: {schemas: {Target: {type: string}}}", + "openapi: 3.1.1\nexample: {properties: {value: {$ref: '#/components/schemas/Target'}}}\ncomponents: {schemas: {Target: {type: string}}}", + } { + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(source), &root)) + idx := NewSpecIndex(&root) + _, node := utils.FindKeyNodeTop("example", root.Content[0].Content) + ref := &Reference{Definition: "#/example", FullDefinition: "#/example", Index: idx, Node: node} + resolver := NewResolver(idx) + found := resolver.extractRelatives(ref, node, nil, map[string]bool{}, nil, false, 0, "") + require.Empty(t, resolver.resolvingErrors) + require.NotNil(t, node) + if strings.HasPrefix(source, "openapi:") { + require.Empty(t, found, "a document example remains payload") + } else { + require.Len(t, found, 1, "the external fragment name does not turn its schema into payload") + require.Equal(t, "#/components/schemas/Target", found[0].Definition) + } + } +} + +func TestResolverDeepExamplePaths(t *testing.T) { + var source strings.Builder + source.WriteString("openapi: 3.1.1\ncomponents:\n schemas:\n Target: {type: string}\n Root:\n") + indent := " " + for range 24 { + source.WriteString(indent + "properties:\n" + indent + " item:\n") + indent += " " + } + source.WriteString(indent + "example: {nested: {$ref: '#/payload-only'}}\n") + source.WriteString(indent + "properties: {child: {$ref: '#/components/schemas/Target'}}\n") + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(source.String()), &root)) + idx := NewSpecIndex(&root) + ref, _ := idx.SearchIndexForReference("#/components/schemas/Root") + require.NotNil(t, ref) + resolver := NewResolver(idx) + found := resolver.extractRelatives(ref, ref.Node, nil, map[string]bool{}, nil, false, 0, "") + require.Empty(t, resolver.resolvingErrors) + require.Len(t, found, 1) + require.Equal(t, "#/components/schemas/Target", found[0].Definition) +} diff --git a/index/rolodex.go b/index/rolodex.go index 335427b1..46249e01 100644 --- a/index/rolodex.go +++ b/index/rolodex.go @@ -75,6 +75,7 @@ type Rolodex struct { indexingDuration time.Duration indexes []*SpecIndex indexMap map[string]*SpecIndex + ownedFiles map[string]*ownedRolodexFile // private copies of files indexed by another rolodex indexLock sync.Mutex rootIndex *SpecIndex rootNode *yaml.Node @@ -103,6 +104,7 @@ func (r *Rolodex) Release() { r.remoteFS = nil r.indexes = nil r.indexMap = nil + r.ownedFiles = nil r.rootIndex = nil r.indexLock.Unlock() @@ -247,6 +249,11 @@ func (r *Rolodex) GetIndexingDuration() time.Duration { return r.indexingDuration } +// HasCheckedForCircularReferences returns true once the rolodex has been checked for circular references. +func (r *Rolodex) HasCheckedForCircularReferences() bool { + return r.circChecked +} + // GetRootIndex returns the root index of the rolodex (the entry point, the main document) func (r *Rolodex) GetRootIndex() *SpecIndex { return r.rootIndex @@ -354,21 +361,41 @@ func (r *Rolodex) IndexTheRolodex(ctx context.Context) error { copiedConfig.Rolodex = r copiedConfig.SpecAbsolutePath = fullPath copiedConfig.AvoidBuildIndex = true // we will build out everything in two steps. - idx, err := idxFile.Index(&copiedConfig) - - if err == nil { // Index() does not throw an error anymore. - // for each index, we need a resolver - resolver := NewResolver(idx) - - // check if the config has been set to ignore circular references in arrays and polymorphic schemas - if copiedConfig.IgnoreArrayCircularReferences { - resolver.IgnoreArrayCircularReferences() + file := idxFile.(RolodexFile) + idx := file.GetIndex() + var err error + if idx == nil || idx.rolodex == r { + idx, err = idxFile.Index(&copiedConfig) + } + if err != nil || idx == nil { + if err == nil { + err = fmt.Errorf("indexing '%s' returned no index", fullPath) } - if copiedConfig.IgnorePolymorphicCircularReferences { - resolver.IgnorePolymorphicCircularReferences() + errChan <- err + return + } + if idx.rolodex != r { + owned, ownErr := r.ownIndexedFile(file) + if ownErr != nil { + errChan <- ownErr + return } - indexChan <- idx + // Only the batch worker waits. Recursive opens must remain non-blocking. + owned.WaitForIndexing() + idx = owned.GetIndex() } + + resolver := idx.GetResolver() + if resolver == nil { + resolver = NewResolver(idx) + } + if copiedConfig.IgnoreArrayCircularReferences { + resolver.IgnoreArrayCircularReferences() + } + if copiedConfig.IgnorePolymorphicCircularReferences { + resolver.IgnorePolymorphicCircularReferences() + } + indexChan <- idx } if lfs, ok := fs.(RolodexFS); ok { @@ -412,7 +439,6 @@ func (r *Rolodex) IndexTheRolodex(ctx context.Context) error { case <-doneChan: indexingCompleted++ case err := <-errChan: - indexingCompleted++ caughtErrors = append(caughtErrors, err) case idx := <-indexChan: indexBuildQueue = append(indexBuildQueue, idx) @@ -427,9 +453,11 @@ func (r *Rolodex) IndexTheRolodex(ctx context.Context) error { ) r.indexLock.Lock() - r.indexes = indexBuildQueue for _, idx := range indexBuildQueue { - r.indexMap[idx.specAbsolutePath] = idx + if r.indexMap[idx.specAbsolutePath] == nil { + r.indexes = append(r.indexes, idx) + r.indexMap[idx.specAbsolutePath] = idx + } } r.indexLock.Unlock() @@ -552,6 +580,16 @@ func (r *Rolodex) IndexTheRolodex(ctx context.Context) error { caughtErrors = append(caughtErrors, index.refErrors...) } } + // Root reference lookup can load additional files after the initial build queue. + // Include fatal extraction errors from those indexes before returning a model. + for _, idx := range r.GetIndexes() { + for _, err := range idx.refErrors { + var resolvingErr *ResolvingError + if errors.As(err, &resolvingErr) { + caughtErrors = append(caughtErrors, err) + } + } + } r.indexingDuration = time.Since(started) r.indexed = true r.caughtErrors = caughtErrors @@ -768,6 +806,18 @@ func (r *Rolodex) localPathForOpen(baseDir, fileLookup string, fileSystem fs.FS) func (r *Rolodex) asLocalFile(file fs.File, fileLookup string) (*LocalFile, []error) { var errorStack []error + if localFile, ok := file.(*LocalFile); ok && localFile == nil { + return nil, nil + } + if indexed, ok := file.(RolodexFile); ok { + if idx := indexed.GetIndex(); idx != nil && idx.rolodex != r { + owned, err := r.ownIndexedFile(indexed) + if err != nil { + return nil, []error{err} + } + return owned, nil + } + } if localFile, ok := file.(*LocalFile); ok { return localFile, nil } @@ -895,6 +945,9 @@ func (r *Rolodex) wrapLocalRolodexFile(localFile *LocalFile) (RolodexFile, error } func (r *Rolodex) wrapRemoteRolodexFile(remoteFile *RemoteFile) (RolodexFile, error) { + if idx := remoteFile.GetIndex(); idx != nil && idx.rolodex != r { + return r.ownIndexedFile(remoteFile) + } return &rolodexFile{ rolodex: r, location: remoteFile.fullPath, diff --git a/index/rolodex_alias_limit_test.go b/index/rolodex_alias_limit_test.go new file mode 100644 index 00000000..f97bc81e --- /dev/null +++ b/index/rolodex_alias_limit_test.go @@ -0,0 +1,79 @@ +package index + +import ( + "context" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "testing" + + "github.com/pb33f/go-yaml" + "github.com/pb33f/testify/require" +) + +const excessiveAliasDocument = `openapi: 3.1.1 +info: {title: Alias limit, version: '1'} +paths: {} +components: {schemas: {Target: {type: string}}} +x-a: &a [value] +x-b: &b [*a, *a] +x-c: &c [*b, *b] +x-d: &d [*c, *c] +x-e: &e [*d, *d] +x-f: &f [*e, *e] +x-g: &g [*f, *f] +x-h: &h [*g, *g] +x-i: &i [*h, *h] +x-j: &j [*i, *i] +x-k: &k [*j, *j] +x-tail: {$ref: './unvisited.yaml'} +` + +func TestExtractRefsAliasLimitSkipsRemainingSiblings(t *testing.T) { + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(excessiveAliasDocument), &root)) + idx := NewSpecIndex(nil) + require.Empty(t, idx.ExtractRefs(context.Background(), &root, nil, nil, 0, false, "")) + require.Len(t, idx.GetReferenceIndexErrors(), 1) + require.ErrorContains(t, idx.GetReferenceIndexErrors()[0], "document contains excessive aliasing") + require.Empty(t, idx.GetRawReferencesSequenced(), "the tail reference must not be visited after the limit") +} + +func TestRolodexReportsAliasLimitFromFileLoadedByRoot(t *testing.T) { + for _, skipCircular := range []bool{false, true} { + t.Run(fmt.Sprintf("skipCircular=%t", skipCircular), func(t *testing.T) { + cfg := CreateOpenAPIIndexConfig() + cfg.AvoidCircularReferenceCheck = skipCircular + cfg.BaseURL, _ = url.Parse("https://example.com/") + rolo := NewRolodex(cfg) + remote, err := NewRemoteFSWithConfig(cfg) + require.NoError(t, err) + remote.RemoteHandlerFunc = func(location string) (*http.Response, error) { + if location != "https://example.com/shared.yaml" { + return nil, fmt.Errorf("unexpected remote lookup: %s", location) + } + return &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader(excessiveAliasDocument))}, nil + } + rolo.AddRemoteFS("https://example.com/", remote) + require.Empty(t, remote.GetFiles(), "the external index must be loaded after the initial build queue") + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(`openapi: 3.1.1 +info: {title: Root, version: '1'} +paths: {} +components: + schemas: + Target: {$ref: 'https://example.com/shared.yaml#/components/schemas/Target'} +`), &root)) + rolo.SetRootNode(&root) + err = rolo.IndexTheRolodex(context.Background()) + require.ErrorContains(t, err, "document contains excessive aliasing") + require.Len(t, rolo.GetIndexes(), 1) + var resolvingErr *ResolvingError + require.ErrorAs(t, err, &resolvingErr) + require.NotNil(t, resolvingErr.Node) + require.Contains(t, rolo.GetCaughtErrors(), resolvingErr) + }) + } +} diff --git a/index/rolodex_file_loader.go b/index/rolodex_file_loader.go index 29a528f2..63e5cb1b 100644 --- a/index/rolodex_file_loader.go +++ b/index/rolodex_file_loader.go @@ -153,15 +153,15 @@ func (l *LocalFS) OpenWithContext(ctx context.Context, name string) (fs.File, er copiedCfg.SpecAbsolutePath = name copiedCfg.AvoidBuildIndex = true copiedCfg.SpecInfo = nil + if l.rolodex != nil { + copiedCfg.Rolodex = l.rolodex + } // Add this file to the context's indexing set to prevent deadlocks // when circular references cause the same file to be looked up recursively. indexingCtx := AddIndexingFile(ctx, name) idx, _ := extractedFile.IndexWithContext(indexingCtx, &copiedCfg) - if idx != nil && l.rolodex != nil { - idx.rolodex = l.rolodex - } if idx != nil { NewResolver(idx) diff --git a/index/rolodex_owned_file.go b/index/rolodex_owned_file.go new file mode 100644 index 00000000..247e8d7c --- /dev/null +++ b/index/rolodex_owned_file.go @@ -0,0 +1,68 @@ +package index + +import ( + "context" + + "github.com/pb33f/libopenapi/datamodel" +) + +type ownedRolodexFile struct { + file *LocalFile + err error + parsed chan struct{} +} + +// ownIndexedFile isolates a cached file from another document's mutable index. +// A pending file exposes its parsed nodes without waiting for indexing, so +// mutually referencing files cannot deadlock during recursive lookups. +func (r *Rolodex) ownIndexedFile(source RolodexFile) (*LocalFile, error) { + location := source.GetFullPath() + r.indexLock.Lock() + owned := r.ownedFiles[location] + if owned != nil { + r.indexLock.Unlock() + <-owned.parsed + return owned.file, owned.err + } + owned = &ownedRolodexFile{parsed: make(chan struct{})} + if r.ownedFiles == nil { + r.ownedFiles = make(map[string]*ownedRolodexFile) + } + r.ownedFiles[location] = owned + r.indexLock.Unlock() + + data := []byte(source.GetContent()) + info, err := datamodel.ExtractSpecInfoWithDocumentCheckSync(data, true) + if err != nil { + owned.err = err + close(owned.parsed) + return nil, err + } + file := &LocalFile{ + filename: source.Name(), name: source.Name(), fullPath: location, + extension: source.GetFileExtension(), lastModified: source.ModTime(), + data: data, parsed: info.RootNode, indexingComplete: make(chan struct{}), + } + owned.file = file + // Parsing cannot follow references. Release its waiters before indexing, + // where cross-file lookups must be able to return the pending tree. + close(owned.parsed) + + file.indexOnce.Do(func() { + cfg := *r.indexConfig + cfg.Rolodex = r + cfg.SpecAbsolutePath = location + cfg.SpecInfo = info + cfg.AvoidBuildIndex = true + ctx := AddIndexingFile(context.Background(), location) + idx := NewSpecIndexWithConfigAndContext(ctx, info.RootNode, &cfg) + resolver := NewResolver(idx) + resolver.IgnoreArray = cfg.IgnoreArrayCircularReferences + resolver.IgnorePoly = cfg.IgnorePolymorphicCircularReferences + idx.BuildIndex() + file.index.Store(idx) + r.AddIndex(idx) + }) + file.signalIndexingComplete() + return file, nil +} diff --git a/index/rolodex_owned_file_test.go b/index/rolodex_owned_file_test.go new file mode 100644 index 00000000..b66a4b82 --- /dev/null +++ b/index/rolodex_owned_file_test.go @@ -0,0 +1,317 @@ +package index + +import ( + "context" + "errors" + "io/fs" + "os" + "path/filepath" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/pb33f/go-yaml" + "github.com/pb33f/testify/require" +) + +type sharedIndexedFS struct { + files map[string]RolodexFile + lazy bool +} + +func TestRolodexMissingLocalFile(t *testing.T) { + // LocalFS can return a typed nil for an unavailable discriminator target. + var missing *LocalFile + rolo := NewRolodex(CreateOpenAPIIndexConfig()) + file, errs := rolo.asLocalFile(missing, "missing.yaml") + require.Nil(t, file) + require.Empty(t, errs) +} + +type controlledIndexedFile struct { + *LocalFile + result *SpecIndex + err error + calls int +} + +func (f *controlledIndexedFile) Index(*SpecIndexConfig) (*SpecIndex, error) { + f.calls++ + if f.result != nil { + f.index.Store(f.result) + } + return f.result, f.err +} + +func standaloneIndexedFile(t *testing.T, content string) *LocalFile { + t.Helper() + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte("type: object\n"), &root)) + file := &LocalFile{data: []byte(content), name: "schema.yaml", filename: "schema.yaml", fullPath: filepath.Join(t.TempDir(), "schema.yaml"), extension: YAML} + file.index.Store(NewSpecIndex(&root)) + return file +} + +func TestRolodexOwnsStandaloneAndLateIndexes(t *testing.T) { + for _, late := range []bool{false, true} { + t.Run(map[bool]string{false: "cached", true: "returned-by-index"}[late], func(t *testing.T) { + original := standaloneIndexedFile(t, "type: object\nproperties:\n id: {type: string}\n") + file := &controlledIndexedFile{LocalFile: original, result: original.GetIndex()} + if late { + file.LocalFile = &LocalFile{data: original.data, name: original.name, filename: original.filename, fullPath: original.fullPath, extension: YAML} + } + files := &sharedIndexedFS{files: map[string]RolodexFile{"schema.yaml": file}} + cfg := CreateOpenAPIIndexConfig() + cfg.BasePath = filepath.Dir(original.fullPath) + rolo := NewRolodex(cfg) + rolo.AddLocalFS(cfg.BasePath, files) + require.NoError(t, rolo.IndexTheRolodex(context.Background())) + owned, err := rolo.Open(original.fullPath) + require.NoError(t, err) + require.Same(t, rolo, owned.GetIndex().GetRolodex()) + require.NotSame(t, original.GetIndex(), owned.GetIndex()) + if late { + require.Equal(t, 1, file.calls) + } else { + require.Zero(t, file.calls, "do not ask a cached foreign file to index under another owner") + } + }) + } +} + +func TestRolodexReportsIndexAndOwnedParseErrors(t *testing.T) { + for _, failure := range []string{"index-error", "nil-index", "empty-content"} { + t.Run(failure, func(t *testing.T) { + file := &controlledIndexedFile{LocalFile: &LocalFile{name: "schema.yaml", fullPath: filepath.Join(t.TempDir(), "schema.yaml"), data: []byte("type: string\n")}} + switch failure { + case "index-error": + file.err = errors.New("custom index failed") + case "empty-content": + file.LocalFile = standaloneIndexedFile(t, "") + } + cfg := CreateOpenAPIIndexConfig() + cfg.BasePath = filepath.Dir(file.fullPath) + rolo := NewRolodex(cfg) + rolo.AddLocalFS(cfg.BasePath, &sharedIndexedFS{files: map[string]RolodexFile{"schema.yaml": file}}) + require.Error(t, rolo.IndexTheRolodex(context.Background())) + if failure == "empty-content" { + opened, err := rolo.Open(file.fullPath) + require.Error(t, err) + require.Nil(t, opened) + } + }) + } +} + +type synchronizedContentFile struct { + *LocalFile + reads atomic.Int32 +} + +func (f *synchronizedContentFile) GetContent() string { + f.reads.Add(1) + return f.LocalFile.GetContent() +} + +func TestRolodexConcurrentForeignFileCopiesShareOneIndex(t *testing.T) { + const callers = 8 + var workers sync.WaitGroup + source := &synchronizedContentFile{LocalFile: standaloneIndexedFile(t, "type: object\nproperties:\n id: {type: string}\n")} + cfg := CreateOpenAPIIndexConfig() + cfg.IgnoreArrayCircularReferences = true + cfg.IgnorePolymorphicCircularReferences = true + rolo := NewRolodex(cfg) + results := make([]*LocalFile, callers) + errs := make([]error, callers) + start := make(chan struct{}) + workers.Add(callers) + for i := range callers { + go func() { defer workers.Done(); <-start; results[i], errs[i] = rolo.ownIndexedFile(source) }() + } + close(start) + workers.Wait() + require.Equal(t, int32(1), source.reads.Load(), "concurrent opens must parse the source only once") + for i := range callers { + require.NoError(t, errs[i]) + results[i].WaitForIndexing() + require.Same(t, results[0], results[i]) + } + require.Len(t, rolo.GetIndexes(), 1) + idx := results[0].GetIndex() + require.Same(t, rolo, idx.GetRolodex()) + require.True(t, idx.GetResolver().IgnoreArray) + require.True(t, idx.GetResolver().IgnorePoly) + again, err := rolo.ownIndexedFile(source) + require.NoError(t, err) + require.Same(t, results[0], again) +} + +func TestRolodexOwnsCachedRemoteFile(t *testing.T) { + source := standaloneIndexedFile(t, "type: string\n") + remote := &RemoteFile{data: source.data, filename: "schema.yaml", name: "schema.yaml", fullPath: "https://example.com/schema.yaml", extension: YAML} + remote.index.Store(source.GetIndex()) + rolo := NewRolodex(CreateOpenAPIIndexConfig()) + owned, err := rolo.wrapRemoteRolodexFile(remote) + require.NoError(t, err) + require.Same(t, rolo, owned.GetIndex().GetRolodex()) + require.NotSame(t, remote.GetIndex().GetRootNode(), owned.GetIndex().GetRootNode()) + require.Equal(t, remote.GetFullPath(), owned.GetFullPath()) +} + +func (s *sharedIndexedFS) Open(name string) (fs.File, error) { + if file := s.files[filepath.Base(name)]; file != nil { + return file.(fs.File), nil + } + return nil, fs.ErrNotExist +} + +func (s *sharedIndexedFS) GetFiles() map[string]RolodexFile { + if s.lazy { + return nil + } + return s.files +} + +// customIndexedFile preserves the same cached index as LocalFile through a +// custom RolodexFile implementation, as Vacuum's lazy filesystem does. +type customIndexedFile struct{ *LocalFile } + +type blockedContentFile struct { + RolodexFile + fs.File + entered chan struct{} + resume chan struct{} +} + +func (f *blockedContentFile) GetContent() string { + close(f.entered) + <-f.resume + return f.RolodexFile.GetContent() +} + +func TestRolodexPendingOwnedFileRetainsNodeIdentity(t *testing.T) { + files, dir, root := sharedCircularFiles(t, true) + indexSharedFiles(t, files, dir, root) + blocked := &blockedContentFile{RolodexFile: files.files["b.yaml"], File: files.files["b.yaml"].(fs.File), entered: make(chan struct{}), resume: make(chan struct{})} + files.files["b.yaml"] = blocked + files.lazy = true + cfg := CreateOpenAPIIndexConfig() + cfg.BasePath = dir + cfg.SpecAbsolutePath = filepath.Join(dir, "root.yaml") + rolo := NewRolodex(cfg) + rolo.AddLocalFS(dir, files) + var callerRoot yaml.Node + require.NoError(t, yaml.Unmarshal([]byte("components: {}\n"), &callerRoot)) + caller := NewSpecIndexWithConfig(&callerRoot, cfg) + finished := make(chan error, 1) + go func() { _, err := rolo.Open(filepath.Join(dir, "a.yaml")); finished <- err }() + resume := sync.OnceFunc(func() { close(blocked.resume) }) + defer resume() + select { + case <-blocked.entered: + case <-time.After(time.Second): + t.Fatal("foreign indexing did not reach the second file") + } + // The recursive lookup must expose the owned parsed tree without waiting + // for the A -> B -> A index construction to finish. + ref := caller.lookupRolodex(context.Background(), []string{filepath.Join(dir, "a.yaml"), "A"}) + require.NotNil(t, ref) + require.Nil(t, ref.Index) + resume() + select { + case err := <-finished: + require.NoError(t, err) + case <-time.After(time.Second): + t.Fatal("cross-file ownership indexing deadlocked") + } + file, err := rolo.Open(filepath.Join(dir, "a.yaml")) + require.NoError(t, err) + ready := file.GetIndex().FindComponent(context.Background(), "#/A") + require.NotNil(t, ready) + require.Same(t, ref.Node, ready.Node, "pending and completed lookups must use the same YAML nodes") +} + +func sharedCircularFiles(t *testing.T, custom bool) (*sharedIndexedFS, string, []byte) { + t.Helper() + dir, err := filepath.Abs("../test_specs/circular_cross_file/orig") + require.NoError(t, err) + files := &sharedIndexedFS{files: make(map[string]RolodexFile)} + for _, name := range []string{"a.yaml", "b.yaml"} { + data, err := os.ReadFile(filepath.Join(dir, name)) + require.NoError(t, err) + file := &LocalFile{data: data, name: name, filename: name, fullPath: filepath.Join(dir, name), extension: YAML} + if custom { + files.files[name] = &customIndexedFile{file} + } else { + files.files[name] = file + } + } + root, err := os.ReadFile(filepath.Join(dir, "root.yaml")) + require.NoError(t, err) + return files, dir, root +} + +func indexSharedFiles(t *testing.T, files *sharedIndexedFS, dir string, root []byte) *Rolodex { + t.Helper() + cfg := CreateOpenAPIIndexConfig() + cfg.BasePath = dir + cfg.SpecFilePath = "root.yaml" + r := NewRolodex(cfg) + r.AddLocalFS(dir, files) + var node yaml.Node + require.NoError(t, yaml.Unmarshal(root, &node)) + r.SetRootNode(&node) + require.NoError(t, r.IndexTheRolodex(context.Background())) + return r +} + +func TestRolodexSharedIndexedFilesKeepDocumentOwnership(t *testing.T) { + for _, custom := range []bool{false, true} { + for _, lazy := range []bool{false, true} { + name := "local" + if custom { + name = "custom" + } + if lazy { + name += "-lazy" + } + t.Run(name, func(t *testing.T) { + files, dir, root := sharedCircularFiles(t, custom) + first := indexSharedFiles(t, files, dir, root) + firstA := files.files["a.yaml"].GetIndex() + firstB := files.files["b.yaml"].GetIndex() + firstResolver := firstA.GetResolver() + files.lazy = lazy + second := indexSharedFiles(t, files, dir, root) + require.Same(t, firstA, files.files["a.yaml"].GetIndex()) + require.Same(t, firstResolver, firstA.GetResolver(), "building another document must not reset the original resolver") + for _, name := range []string{"a.yaml", "b.yaml"} { + file, err := second.Open(filepath.Join(dir, name)) + require.NoError(t, err) + file.WaitForIndexing() + idx := file.GetIndex() + require.NotSame(t, files.files[name].GetIndex(), idx) + require.Same(t, second, idx.GetRolodex()) + node, err := file.GetContentAsYAMLNode() + require.NoError(t, err) + require.Same(t, idx.GetRootNode(), node) + require.NotSame(t, files.files[name].GetIndex().GetRootNode(), node) + } + require.Len(t, second.GetIndexes(), 2, "lazy indexes must survive batch registration") + var wg sync.WaitGroup + wg.Add(1) + go func() { defer wg.Done(); first.Resolve() }() + for range 100 { + second.GetRootIndex().GetCircularReferences() + second.GetSafeCircularReferences() + second.GetIgnoredCircularReferences() + } + wg.Wait() + require.Same(t, first, firstB.GetRolodex()) + second.Release() + require.Nil(t, second.ownedFiles) + }) + } + } +} diff --git a/index/rolodex_test.go b/index/rolodex_test.go index 8eb3eb3f..fc4ccbc0 100644 --- a/index/rolodex_test.go +++ b/index/rolodex_test.go @@ -2067,6 +2067,9 @@ func (trf *testRolodexFile) GetContentAsYAMLNode() (*yaml.Node, error) { return &yaml.Node{}, nil } func (trf *testRolodexFile) GetIndex() *SpecIndex { + if trf.errorYaml { + return nil // a file that failed parsing has no cached index + } return &SpecIndex{ specAbsolutePath: "/test/path/spec.yaml", } @@ -3049,3 +3052,17 @@ func TestRolodex_Open_HttpPrefixedLocationWithoutScheme(t *testing.T) { assert.Nil(t, rf) assert.EqualError(t, err, "remote URL 'httpdocs/pet.yaml' has no scheme, unable to fetch it") } + +func TestRolodex_HasCheckedForCircularReferences(t *testing.T) { + var rootNode yaml.Node + _ = yaml.Unmarshal([]byte("openapi: 3.1.0"), &rootNode) + config := CreateOpenAPIIndexConfig() + config.AvoidCircularReferenceCheck = true + rolo := NewRolodex(config) + rolo.SetRootNode(&rootNode) + assert.NoError(t, rolo.IndexTheRolodex(context.Background())) + assert.False(t, rolo.HasCheckedForCircularReferences()) + + rolo.CheckForCircularReferences() + assert.True(t, rolo.HasCheckedForCircularReferences()) +} diff --git a/index/schema_id_resolve.go b/index/schema_id_resolve.go index 505806a7..3ae0cdca 100644 --- a/index/schema_id_resolve.go +++ b/index/schema_id_resolve.go @@ -284,6 +284,10 @@ func navigateToFragment(root *yaml.Node, fragment string) *yaml.Node { segment = strings.ReplaceAll(segment, "~1", "/") segment = strings.ReplaceAll(segment, "~0", "~") + current = utils.ResolveMergeNodes(current) + if current == nil { + return nil + } found := false if current.Kind == yaml.MappingNode { for i := 0; i < len(current.Content)-1; i += 2 { @@ -310,5 +314,5 @@ func navigateToFragment(root *yaml.Node, fragment string) *yaml.Node { } } - return current + return utils.ResolveMergeNodes(current) } diff --git a/index/spec_index_counts.go b/index/spec_index_counts.go index 0793299b..2d8ca803 100644 --- a/index/spec_index_counts.go +++ b/index/spec_index_counts.go @@ -337,14 +337,15 @@ func (index *SpecIndex) GetComponentSchemaCount() int { return index.schemaCount } - for i, n := range index.root.Content[0].Content { + root := utils.ResolveMergeNodes(index.root.Content[0]) + for i, n := range root.Content { if i%2 != 0 { continue } if n.Value == "servers" { - index.rootServersNode = index.root.Content[0].Content[i+1] - if i+1 < len(index.root.Content[0].Content) { - serverDefinitions := index.root.Content[0].Content[i+1] + index.rootServersNode = root.Content[i+1] + if i+1 < len(root.Content) { + serverDefinitions := root.Content[i+1] for x, def := range serverDefinitions.Content { index.serversRefs = append(index.serversRefs, &Reference{ Definition: "servers", @@ -357,9 +358,9 @@ func (index *SpecIndex) GetComponentSchemaCount() int { } } if n.Value == "security" { - index.rootSecurityNode = index.root.Content[0].Content[i+1] - if i+1 < len(index.root.Content[0].Content) { - securityDefinitions := index.root.Content[0].Content[i+1] + index.rootSecurityNode = root.Content[i+1] + if i+1 < len(root.Content) { + securityDefinitions := root.Content[i+1] for x, def := range securityDefinitions.Content { if len(def.Content) > 0 { name := def.Content[0] @@ -374,16 +375,16 @@ func (index *SpecIndex) GetComponentSchemaCount() int { } } if n.Value == "components" { - _, schemasNode := utils.FindKeyNode("schemas", index.root.Content[0].Content[i+1].Content) - _, parametersNode := utils.FindKeyNode("parameters", index.root.Content[0].Content[i+1].Content) - _, requestBodiesNode := utils.FindKeyNode("requestBodies", index.root.Content[0].Content[i+1].Content) - _, responsesNode := utils.FindKeyNode("responses", index.root.Content[0].Content[i+1].Content) - _, securitySchemesNode := utils.FindKeyNode("securitySchemes", index.root.Content[0].Content[i+1].Content) - _, headersNode := utils.FindKeyNode("headers", index.root.Content[0].Content[i+1].Content) - _, examplesNode := utils.FindKeyNode("examples", index.root.Content[0].Content[i+1].Content) - _, linksNode := utils.FindKeyNode("links", index.root.Content[0].Content[i+1].Content) - _, callbacksNode := utils.FindKeyNode("callbacks", index.root.Content[0].Content[i+1].Content) - _, pathItemsNode := utils.FindKeyNode("pathItems", index.root.Content[0].Content[i+1].Content) + schemasNode := effectiveMappingValue(root.Content[i+1], "schemas") + parametersNode := effectiveMappingValue(root.Content[i+1], "parameters") + requestBodiesNode := effectiveMappingValue(root.Content[i+1], "requestBodies") + responsesNode := effectiveMappingValue(root.Content[i+1], "responses") + securitySchemesNode := effectiveMappingValue(root.Content[i+1], "securitySchemes") + headersNode := effectiveMappingValue(root.Content[i+1], "headers") + examplesNode := effectiveMappingValue(root.Content[i+1], "examples") + linksNode := effectiveMappingValue(root.Content[i+1], "links") + callbacksNode := effectiveMappingValue(root.Content[i+1], "callbacks") + pathItemsNode := effectiveMappingValue(root.Content[i+1], "pathItems") if schemasNode != nil { index.extractDefinitionsAndSchemas(schemasNode, "#/components/schemas/") @@ -430,7 +431,7 @@ func (index *SpecIndex) GetComponentSchemaCount() int { } } if n.Value == "definitions" { - schemasNode := index.root.Content[0].Content[i+1] + schemasNode := utils.ResolveMergeNodes(root.Content[i+1]) if schemasNode != nil { index.extractDefinitionsAndSchemas(schemasNode, "#/definitions/") index.schemasNode = schemasNode @@ -438,7 +439,7 @@ func (index *SpecIndex) GetComponentSchemaCount() int { } } if n.Value == "parameters" { - parametersNode := index.root.Content[0].Content[i+1] + parametersNode := utils.ResolveMergeNodes(root.Content[i+1]) if parametersNode != nil { index.extractComponentParameters(parametersNode, "#/parameters/") index.componentLock.Lock() @@ -447,14 +448,14 @@ func (index *SpecIndex) GetComponentSchemaCount() int { } } if n.Value == "responses" { - responsesNode := index.root.Content[0].Content[i+1] + responsesNode := utils.ResolveMergeNodes(root.Content[i+1]) if responsesNode != nil { index.extractComponentResponses(responsesNode, "#/responses/") index.responsesNode = responsesNode } } if n.Value == "securityDefinitions" { - securityDefinitionsNode := index.root.Content[0].Content[i+1] + securityDefinitionsNode := utils.ResolveMergeNodes(root.Content[i+1]) if securityDefinitionsNode != nil { index.extractComponentSecuritySchemes(securityDefinitionsNode, "#/securityDefinitions/") index.securitySchemesNode = securityDefinitionsNode @@ -472,10 +473,11 @@ func (index *SpecIndex) GetComponentParameterCount() int { if index.componentParamCount > 0 { return index.componentParamCount } - for i, n := range index.root.Content[0].Content { + root := utils.ResolveMergeNodes(index.root.Content[0]) + for i, n := range root.Content { if i%2 == 0 { if n.Value == "components" { - _, parametersNode := utils.FindKeyNode("parameters", index.root.Content[0].Content[i+1].Content) + parametersNode := effectiveMappingValue(root.Content[i+1], "parameters") if parametersNode != nil { index.componentLock.Lock() index.parametersNode = parametersNode @@ -484,7 +486,7 @@ func (index *SpecIndex) GetComponentParameterCount() int { } } if n.Value == "parameters" { - parametersNode := index.root.Content[0].Content[i+1] + parametersNode := utils.ResolveMergeNodes(root.Content[i+1]) if parametersNode != nil { index.componentLock.Lock() index.parametersNode = parametersNode @@ -720,3 +722,15 @@ func (index *SpecIndex) GetAllDescriptionsCount() int { return len(index.allDesc // GetAllSummariesCount returns the total number of summary nodes found during indexing. func (index *SpecIndex) GetAllSummariesCount() int { return len(index.allSummaries) } + +// effectiveMappingValue reads a named field and resolves its collection view. +// Both the containing mapping and the field may independently use YAML merges +// or aliases; callers enumerate the same effective entries as model builders. +func effectiveMappingValue(node *yaml.Node, key string) *yaml.Node { + node = utils.ResolveMergeNodes(node) + if node == nil { + return nil + } + _, value := utils.FindKeyNodeTop(key, node.Content) + return utils.ResolveMergeNodes(value) +} diff --git a/index/utility_methods.go b/index/utility_methods.go index 19360ebf..a68799cf 100644 --- a/index/utility_methods.go +++ b/index/utility_methods.go @@ -53,11 +53,9 @@ func extractDefinitionRequiredRefProperties(schemaNode *yaml.Node, reqRefProps m } // If the node we're looking at is a direct ref to another model without any properties, mark it as required, but still continue to look for required properties - isRef, _, defPath := utils.IsNodeRefValue(schemaNode) + isRef, _, _ := utils.IsNodeRefValue(schemaNode) if isRef { - if _, ok := reqRefProps[defPath]; !ok { - reqRefProps[defPath] = []string{} - } + reqRefProps = extractRequiredReferenceProperties(fulldef, schemaNode, "", reqRefProps) } // Check for a required parameters list, and return if none exists, as any properties will be optional @@ -125,7 +123,10 @@ func extractRequiredReferenceProperties(fulldef string, requiredPropDefNode *yam defPath := fulldef - if strings.HasPrefix(refName, "http") || filepath.IsAbs(refName) { + if strings.HasPrefix(refName, "#") { + base, _ := SplitRefFragment(fulldef) + defPath = base + refName + } else if strings.HasPrefix(refName, "http") || filepath.IsAbs(refName) { defPath = refName } else { exp := strings.Split(fulldef, "#/") @@ -135,13 +136,8 @@ func extractRequiredReferenceProperties(fulldef string, requiredPropDefNode *yam u, _ := url.Parse(exp[0]) r := strings.Split(refName, "#/") if len(r) == 2 { - var abs string - if r[0] == "" { - abs = u.Path - } else { - abs, _ = filepath.Abs(utils.CheckPathOverlap(filepath.Dir(u.Path), r[0], - string(os.PathSeparator))) - } + abs, _ := filepath.Abs(utils.CheckPathOverlap(filepath.Dir(u.Path), r[0], + string(os.PathSeparator))) u.Path = utils.ReplaceWindowsDriveWithLinuxPath(abs) u.Fragment = "" @@ -155,16 +151,8 @@ func extractRequiredReferenceProperties(fulldef string, requiredPropDefNode *yam } else { r := strings.Split(refName, "#/") if len(r) == 2 { - var abs string - if r[0] == "" { - abs, _ = filepath.Abs(exp[0]) - } else { - abs, _ = filepath.Abs(utils.CheckPathOverlap(filepath.Dir(exp[0]), r[0], - string(os.PathSeparator))) - - // abs, _ = filepath.Abs(filepath.Join(filepath.Dir(exp[0]), r[0], - // string('J'))) - } + abs, _ := filepath.Abs(utils.CheckPathOverlap(filepath.Dir(exp[0]), r[0], + string(os.PathSeparator))) defPath = fmt.Sprintf("%s#/%s", abs, r[1]) } else { @@ -199,7 +187,9 @@ func extractRequiredReferenceProperties(fulldef string, requiredPropDefNode *yam if _, ok := reqRefProps[defPath]; !ok { reqRefProps[defPath] = []string{} } - reqRefProps[defPath] = append(reqRefProps[defPath], propName) + if propName != "" { + reqRefProps[defPath] = append(reqRefProps[defPath], propName) + } return reqRefProps } @@ -405,13 +395,38 @@ func (index *SpecIndex) countUniqueInlineDuplicates() int { } func seekRefEnd(ctx context.Context, index *SpecIndex, refName string) *Reference { - ref, _, nCtx := index.SearchIndexForReferenceWithContext(ctx, refName) - if ref != nil { - if ok, _, v := utils.IsNodeRefValue(ref.Node); ok { - return seekRefEnd(nCtx, ref.Index, v) - } - } - return ref + owner := index + type target struct { + index *SpecIndex + node *yaml.Node + } + var seen map[target]struct{} + for index != nil { + ref, nextIndex, nextContext := index.SearchIndexForReferenceWithContext(ctx, refName) + if ref == nil { + return nil + } + ok, _, next := utils.IsNodeRefValue(ref.Node) + if !ok { + return ref + } + key := target{nextIndex, ref.Node} + if _, repeated := seen[key]; repeated { + owner.errorLock.Lock() + owner.refErrors = append(owner.refErrors, &ResolvingError{ + ErrorRef: fmt.Errorf("%w while resolving '%s'", ErrReferenceChainCycle, refName), + Node: ref.Node, Path: ref.Path, + }) + owner.errorLock.Unlock() + return nil + } + if seen == nil { + seen = make(map[target]struct{}) + } + seen[key] = struct{}{} + ctx, index, refName = nextContext, nextIndex, next + } + return nil } // formatParameterPath creates a consistent JSON path for parameter error messages diff --git a/index/utility_methods_test.go b/index/utility_methods_test.go index dbf47a5b..052bb063 100644 --- a/index/utility_methods_test.go +++ b/index/utility_methods_test.go @@ -383,3 +383,15 @@ func Test_WriteIntToHash(t *testing.T) { func Test_Empty_HashNode(t *testing.T) { assert.Equal(t, emptyNodeHash, HashNode(nil)) } + +func TestSeekRefEndCycleAndMissingTarget(t *testing.T) { + var root yaml.Node + assert.NoError(t, yaml.Unmarshal([]byte("a: {$ref: '#/b'}\nb: {$ref: '#/a'}"), &root)) + config := CreateClosedAPIIndexConfig() + config.AvoidCircularReferenceCheck = true + idx := NewSpecIndexWithConfig(&root, config) + assert.Nil(t, seekRefEnd(context.Background(), idx, "#/a")) + assert.ErrorContains(t, idx.GetReferenceIndexErrors()[0], "circular reference chain") + assert.Nil(t, seekRefEnd(context.Background(), idx, "#/missing")) + assert.Nil(t, seekRefEnd(context.Background(), nil, "#/a")) +} diff --git a/utils/merge_view_test.go b/utils/merge_view_test.go index 80fe343d..cbb366a6 100644 --- a/utils/merge_view_test.go +++ b/utils/merge_view_test.go @@ -64,3 +64,64 @@ func TestResolvedMergeNodeMappingCycleTerminates(t *testing.T) { assert.Equal(t, []*yaml.Node{key, value}, view.Content) assert.Len(t, node.Content, 4) } + +func TestResolvedMergeNodeSequenceCycleTerminates(t *testing.T) { + const source = `base: &base {inherited: first} +sequence: &sequence [*sequence, *base] +merged: {<<: *sequence, local: retained} +` + var document yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(source), &document)) + _, original := FindKeyNodeTop("merged", document.Content[0].Content) + view := ResolveMergeNodes(original) + _, inherited := FindKeyNodeTop("inherited", view.Content) + require.NotNil(t, inherited) + assert.Equal(t, "first", inherited.Value) + _, local := FindKeyNodeTop("local", view.Content) + require.NotNil(t, local) + assert.Equal(t, "retained", local.Value) + assert.True(t, hasMergeKeys(original.Content)) +} + +func TestResolvedMergeNodeSharedSourcesKeepPrecedence(t *testing.T) { + const source = `base: &base {inherited: base, local: base} +first: &first {<<: *base, inherited: first} +second: &second {<<: *base, inherited: second, extra: retained} +shared: &shared [*first, *second] +repeated: &repeated [*shared, *shared] +merged: {<<: *repeated, local: chosen} +` + var document yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(source), &document)) + before, err := yaml.Marshal(&document) + require.NoError(t, err) + _, original := FindKeyNodeTop("merged", document.Content[0].Content) + visited := make(map[*yaml.Node]struct{}) + content := expandMergeContent(original, visited) + for key, expected := range map[string]string{ + "inherited": "first", "local": "chosen", "extra": "retained", + } { + _, value := FindKeyNodeTop(key, content) + require.NotNil(t, value) + require.Equal(t, expected, value.Value) + } + require.Len(t, content, 6) + // Reaching a completed source again in this expansion does no further work. + require.Empty(t, expandMergeContent(original, visited)) + _, repeated := FindKeyNodeTop("repeated", document.Content[0].Content) + require.Empty(t, appendExpandedMergeContent(nil, make(map[string]struct{}), repeated, visited)) + // A new expansion still gives the same effective mapping. + require.Equal(t, content, ResolveMergeNodes(original).Content) + after, err := yaml.Marshal(&document) + require.NoError(t, err) + require.Equal(t, before, after) +} + +func TestResolveMergeNodesPreservesSequence(t *testing.T) { + var root yaml.Node + require.NoError(t, yaml.Unmarshal([]byte("[one, two]"), &root)) + node := root.Content[0] + require.Same(t, node, ResolveMergeNodes(node)) + require.Equal(t, "one", node.Content[0].Value) + require.Equal(t, "two", node.Content[1].Value) +} diff --git a/utils/utils.go b/utils/utils.go index aa0a5759..8c83cd3c 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -364,7 +364,8 @@ func expandMergeContent(node *yaml.Node, visited map[*yaml.Node]struct{}) []*yam return nil } visited[node] = struct{}{} - defer delete(visited, node) + // Earlier merge sources win. Revisiting this mapping cannot add a key + // that was not already supplied or overridden by its first expansion. if !hasMergeKeys(node.Content) { return node.Content @@ -411,6 +412,10 @@ func appendExpandedMergeContent( } if mergeValue.Kind == yaml.SequenceNode { + if _, seen := visited[mergeValue]; seen { + return target + } + visited[mergeValue] = struct{}{} for _, item := range mergeValue.Content { target = appendExpandedMergeContent(target, seenKeys, item, visited) } @@ -705,6 +710,7 @@ func NodeMerge(nodes []*yaml.Node) *yaml.Node { // precedence over later sources. Aliases are resolved before expansion. // A merged mapping is shallow-copied: source nodes are not modified, and original // key/value nodes and their location/comment metadata are retained. Mappings +// place explicit key/value pairs first, followed by inherited pairs. Mappings // without merges retain their identity. Callers must not mutate shared children // while readers or model builders use them. func ResolveMergeNodes(node *yaml.Node) *yaml.Node { @@ -712,6 +718,9 @@ func ResolveMergeNodes(node *yaml.Node) *yaml.Node { if node == nil { return nil } + if node.Kind != yaml.MappingNode && node.Tag != "!!map" { + return node + } if !hasMergeKeys(node.Content) { return node } @@ -934,36 +943,37 @@ var ( bracketNameExp = regexp.MustCompile(`^(\w+)\['?([\w/]+)'?]$`) ) +var jsonPathDotChars = func() [256]bool { + var allowed [256]bool + for _, c := range []byte("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_\\") { + allowed[c] = true + } + return allowed +}() + // isPathChar checks if a string is valid for JSONPath dot notation. // returns true only if the string contains only alphanumeric, underscore, or backslash characters // and does not start with a digit (unless it's a pure integer, which is handled separately). // jsonPath requires bracket notation for property names starting with digits like "403_permission_denied". // this is an optimized replacement for the pathCharExp regex. + func isPathChar(s string) bool { if len(s) == 0 { return false } - - firstChar := s[0] - startsWithDigit := firstChar >= '0' && firstChar <= '9' - allDigits := startsWithDigit - - // single pass: validate characters and track if all are digits - for _, r := range s { - if !((r >= 'A' && r <= 'Z') || (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '_' || r == '\\') { - return false - } - if allDigits && (r < '0' || r > '9') { - allDigits = false + if s[0] >= '0' && s[0] <= '9' { + for i := 1; i < len(s); i++ { + if s[i] < '0' || s[i] > '9' { + return false + } } + return true } - - // if starts with digit but not pure integer, requires bracket notation - // property names like "403_permission_denied" must use bracket notation - if startsWithDigit && !allDigits { - return false + for i := range len(s) { + if !jsonPathDotChars[s[i]] { + return false + } } - return true } diff --git a/utils/utils_test.go b/utils/utils_test.go index fbaf8378..efb14ca7 100644 --- a/utils/utils_test.go +++ b/utils/utils_test.go @@ -2164,6 +2164,9 @@ func TestConvertComponentIdIntoFriendlyPathSearch_DigitStartingSegments(t *testi // Test isPathChar function directly for comprehensive coverage func TestIsPathChar(t *testing.T) { + for _, name := range []string{"naïve", "東京", string([]byte{'a', 0xff})} { + assert.False(t, isPathChar(name), "non-ASCII names require bracket notation") + } // Valid path characters (letters, numbers not at start, underscore, backslash) assert.True(t, isPathChar("validName")) assert.True(t, isPathChar("Valid123")) diff --git a/what-changed/model/schema.go b/what-changed/model/schema.go index 5ff42aa6..b87bb0b3 100644 --- a/what-changed/model/schema.go +++ b/what-changed/model/schema.go @@ -9,13 +9,13 @@ import ( "slices" "sort" "strings" - "sync" "github.com/pb33f/go-yaml" "github.com/pb33f/libopenapi/datamodel/low" "github.com/pb33f/libopenapi/datamodel/low/base" v3 "github.com/pb33f/libopenapi/datamodel/low/v3" "github.com/pb33f/libopenapi/orderedmap" + "github.com/pb33f/libopenapi/utils" ) // SchemaChanges represent all changes to a base.Schema OpenAPI object. These changes are represented @@ -307,11 +307,6 @@ func (s *SchemaChanges) TotalBreakingChanges() int { t += s.AllOfChanges[n].TotalBreakingChanges() } } - if len(s.AllOfChanges) > 0 { - for n := range s.AllOfChanges { - t += s.AllOfChanges[n].TotalBreakingChanges() - } - } if len(s.AnyOfChanges) > 0 { for n := range s.AnyOfChanges { t += s.AnyOfChanges[n].TotalBreakingChanges() @@ -396,9 +391,32 @@ func (s *SchemaChanges) TotalBreakingChanges() int { return t } +// canCompareBehindRefChange reports whether the schemas either side of a changed $ref can be compared like any +// other two schemas. Both must resolve, and neither may be on a reference loop, which can only be known once the +// document has been checked for circular references. A 3.1 $ref with sibling keywords is held in an internal allOf +// form that doesn't line up with the referenced schema field by field, so it stops at the $ref change too. +func canCompareBehindRefChange(l, r *base.SchemaProxy) bool { + for _, proxy := range []*base.SchemaProxy{l, r} { + if proxy.Schema() == nil || proxy.IsTransformedRefWithSiblings() { + return false + } + idx := proxy.GetIndex() + if idx == nil || idx.GetRolodex() == nil || !idx.GetRolodex().HasCheckedForCircularReferences() || + base.CheckSchemaProxyForCircularRefs(proxy) { + return false + } + } + return true +} + // CompareSchemas accepts a left and right SchemaProxy and checks for changes. If anything is found, returns -// a pointer to SchemaChanges, otherwise returns nil +// a pointer to SchemaChanges, otherwise returns nil. Within one call, a retargeted reference's schema pair +// is compared once; later occurrences retain their reference change. Separate calls produce independent reports. func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { + return new(schemaComparison).compareSchemas(l, r) +} + +func (c *schemaComparison) compareSchemas(l, r *base.SchemaProxy) *SchemaChanges { sc := new(SchemaChanges) var changes []*Change @@ -439,18 +457,15 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { // continue on because the external references are the same and we need to check things going forward. } else { - // references are different, that's all we care to know. + // the reference points somewhere else: record that, then compare the schema it now points to. CreateChange(&changes, Modified, v3.RefLabel, l.GetValueNode().Content[1], r.GetValueNode().Content[1], BreakingModified(CompSchema, PropRef), l.GetReference(), r.GetReference()) - sc.PropertyChanges = NewPropertyChanges(changes) - // check if this is a circular ref. - if base.CheckSchemaProxyForCircularRefs(l) || base.CheckSchemaProxyForCircularRefs(r) { - // if we have a circular reference, we can't do any more work here. - return nil + if !canCompareBehindRefChange(l, r) || low.AreEqual(l.Schema(), r.Schema()) { + sc.PropertyChanges = NewPropertyChanges(changes) + return sc } - return sc } } @@ -463,14 +478,11 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { if lHash != rHash { CreateChange(&changes, Modified, v3.RefLabel, l.GetValueNode(), r.GetValueNode().Content[1], BreakingModified(CompSchema, PropRef), l, r.GetReference()) - sc.PropertyChanges = NewPropertyChanges(changes) - // check if this is a circular ref. - if base.CheckSchemaProxyForCircularRefs(r) { - // if we have a circular reference, we can't do any more work here. - return nil + if !canCompareBehindRefChange(l, r) { + sc.PropertyChanges = NewPropertyChanges(changes) + return sc } - return sc } } @@ -483,17 +495,19 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { if lHash != rHash { CreateChange(&changes, Modified, v3.RefLabel, l.GetValueNode().Content[1], r.GetValueNode(), BreakingModified(CompSchema, PropRef), l.GetReference(), r) - sc.PropertyChanges = NewPropertyChanges(changes) - // check if this is a circular ref. - if base.CheckSchemaProxyForCircularRefs(l) { - // if we have a circular reference, we can't do any more work here. - return nil + if !canCompareBehindRefChange(l, r) { + sc.PropertyChanges = NewPropertyChanges(changes) + return sc } - return sc } } + if len(changes) > 0 && !c.visit(l, r) { + sc.PropertyChanges = NewPropertyChanges(changes) + return sc + } + lSchema := l.Schema() rSchema := r.Schema() preserved := preservedObjectBranch(l, r, lSchema, rSchema) @@ -518,7 +532,7 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { skipSimpleScalarUnionDiff := schemasUseEquivalentSimpleScalarUnion(l, r) // check schema core properties for changes. - checkSchemaPropertyChanges(comparisonLSchema, comparisonRSchema, l, r, &changes, sc, + c.checkSchemaPropertyChanges(comparisonLSchema, comparisonRSchema, l, r, &changes, sc, skipSimpleScalarUnionDiff || preserved != nil) // now for the confusing part, there is also a schema's 'properties' property to parse. @@ -552,10 +566,10 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { ranyOf = nil } - props := checkMappedSchemaOfASchema(lProperties, rProperties, &changes) + props := c.checkMappedSchemaOfASchema(lProperties, rProperties, &changes) sc.SchemaPropertyChanges = props - deps := checkMappedSchemaOfASchema(lDepSchemas, rDepSchemas, &changes) + deps := c.checkMappedSchemaOfASchema(lDepSchemas, rDepSchemas, &changes) sc.DependentSchemasChanges = deps // Check dependent required changes @@ -572,32 +586,28 @@ func CompareSchemas(l, r *base.SchemaProxy) *SchemaChanges { sc.DependentRequiredChanges = depRequiredChanges } - patterns := checkMappedSchemaOfASchema(lPattProp, rPattProp, &changes) + patterns := c.checkMappedSchemaOfASchema(lPattProp, rPattProp, &changes) sc.PatternPropertiesChanges = patterns - var wg sync.WaitGroup - wg.Add(4) - go func() { - extractSchemaChanges(loneOf, roneOf, v3.OneOfLabel, - &sc.OneOfChanges, &changes) - wg.Done() - }() - go func() { - extractSchemaChanges(lallOf, rallOf, v3.AllOfLabel, - &sc.AllOfChanges, &changes) - wg.Done() - }() - go func() { - extractSchemaChanges(lanyOf, ranyOf, v3.AnyOfLabel, - &sc.AnyOfChanges, &changes) - wg.Done() - }() - go func() { - extractSchemaChanges(lprefix, rprefix, v3.PrefixItemsLabel, - &sc.PrefixItemsChanges, &changes) - wg.Done() - }() - wg.Wait() + compositionCount := 0 + for _, size := range [4]int{len(loneOf) + len(roneOf), len(lallOf) + len(rallOf), len(lanyOf) + len(ranyOf), len(lprefix) + len(rprefix)} { + if size > 0 { + compositionCount++ + } + } + if c.serial || compositionCount < 2 { + c.extractSchemaChanges(loneOf, roneOf, v3.OneOfLabel, &sc.OneOfChanges, &changes) + c.extractSchemaChanges(lallOf, rallOf, v3.AllOfLabel, &sc.AllOfChanges, &changes) + c.extractSchemaChanges(lanyOf, ranyOf, v3.AnyOfLabel, &sc.AnyOfChanges, &changes) + c.extractSchemaChanges(lprefix, rprefix, v3.PrefixItemsLabel, &sc.PrefixItemsChanges, &changes) + } else { + c.compareCompositions([]schemaCompositionTask{ + {left: loneOf, right: roneOf, label: v3.OneOfLabel, result: &sc.OneOfChanges}, + {left: lallOf, right: rallOf, label: v3.AllOfLabel, result: &sc.AllOfChanges}, + {left: lanyOf, right: ranyOf, label: v3.AnyOfLabel, result: &sc.AnyOfChanges}, + {left: lprefix, right: rprefix, label: v3.PrefixItemsLabel, result: &sc.PrefixItemsChanges}, + }, &changes) + } } // done @@ -635,12 +645,15 @@ func checkSchemaXML(lSchema *base.Schema, rSchema *base.Schema, changes *[]*Chan } } -func checkMappedSchemaOfASchema( +func (c *schemaComparison) checkMappedSchemaOfASchema( lSchema, rSchema *orderedmap.Map[low.KeyReference[string], low.ValueReference[*base.SchemaProxy]], changes *[]*Change, ) map[string]*SchemaChanges { - var syncPropChanges sync.Map // concurrent-safe map + if orderedmap.Len(lSchema) == 0 && orderedmap.Len(rSchema) == 0 { + return nil + } + propChanges := make(map[string]*SchemaChanges) var lProps []string lEntities := make(map[string]*base.SchemaProxy) lKeyNodes := make(map[string]*yaml.Node) @@ -660,28 +673,33 @@ func checkMappedSchemaOfASchema( } sort.Strings(lProps) sort.Strings(rProps) - buildProperty(lProps, rProps, lEntities, rEntities, &syncPropChanges, changes, rKeyNodes, lKeyNodes) + c.buildProperty(lProps, rProps, lEntities, rEntities, propChanges, changes, rKeyNodes, lKeyNodes) - // Convert the sync.Map into a regular map[string]*SchemaChanges. - propChanges := make(map[string]*SchemaChanges) - syncPropChanges.Range(func(key, value interface{}) bool { - propChanges[key.(string)] = value.(*SchemaChanges) - return true - }) return propChanges } -func buildProperty(lProps, rProps []string, lEntities, rEntities map[string]*base.SchemaProxy, - propChanges *sync.Map, changes *[]*Change, rKeyNodes, lKeyNodes map[string]*yaml.Node, +func (c *schemaComparison) buildProperty(lProps, rProps []string, lEntities, rEntities map[string]*base.SchemaProxy, + propChanges map[string]*SchemaChanges, changes *[]*Change, rKeyNodes, lKeyNodes map[string]*yaml.Node, ) { - var wg sync.WaitGroup + type propertyTask struct { + key string + left, right *base.SchemaProxy + result *SchemaChanges + } + var tasks []propertyTask checkProperty := func(key string, lp, rp *base.SchemaProxy) { - defer wg.Done() + if _, compared := propChanges[key]; compared { + return + } if low.AreEqual(lp, rp) { return } - s := CompareSchemas(lp, rp) - propChanges.Store(key, s) + if c.serial || min(len(lProps), len(rProps)) < 2 { + propChanges[key] = c.compareSchemas(lp, rp) + return + } + propChanges[key] = nil + tasks = append(tasks, propertyTask{key: key, left: lp, right: rp}) } // left and right equal. @@ -690,8 +708,7 @@ func buildProperty(lProps, rProps []string, lEntities, rEntities map[string]*bas lp := lEntities[lProps[w]] rp := rEntities[rProps[w]] if lProps[w] == rProps[w] && lp != nil && rp != nil { - wg.Add(1) - go checkProperty(lProps[w], lp, rp) + checkProperty(lProps[w], lp, rp) } // Handle keys that do not match. if lProps[w] != rProps[w] { @@ -708,8 +725,7 @@ func buildProperty(lProps, rProps []string, lEntities, rEntities map[string]*bas h := slices.Index(lProps, rProps[w]) lp = lEntities[lProps[h]] rp = rEntities[rProps[w]] - wg.Add(1) - go checkProperty(lProps[h], lp, rp) + checkProperty(lProps[h], lp, rp) } } } @@ -719,8 +735,7 @@ func buildProperty(lProps, rProps []string, lEntities, rEntities map[string]*bas if len(lProps) > len(rProps) { for w := range lProps { if rEntities[lProps[w]] != nil { - wg.Add(1) - go checkProperty(lProps[w], lEntities[lProps[w]], rEntities[lProps[w]]) + checkProperty(lProps[w], lEntities[lProps[w]], rEntities[lProps[w]]) } else { CreateChange(changes, ObjectRemoved, v3.PropertiesLabel, lKeyNodes[lProps[w]], nil, BreakingRemoved(CompSchema, PropProperties), lEntities[lProps[w]], nil) @@ -728,8 +743,7 @@ func buildProperty(lProps, rProps []string, lEntities, rEntities map[string]*bas } for w := range rProps { if lEntities[rProps[w]] != nil { - wg.Add(1) - go checkProperty(rProps[w], lEntities[rProps[w]], rEntities[rProps[w]]) + checkProperty(rProps[w], lEntities[rProps[w]], rEntities[rProps[w]]) } else { CreateChange(changes, ObjectAdded, v3.PropertiesLabel, nil, rKeyNodes[rProps[w]], BreakingAdded(CompSchema, PropProperties), nil, rEntities[rProps[w]]) @@ -741,8 +755,7 @@ func buildProperty(lProps, rProps []string, lEntities, rEntities map[string]*bas if len(rProps) > len(lProps) { for _, propName := range rProps { if lEntities[propName] != nil { - wg.Add(1) - go checkProperty(propName, lEntities[propName], rEntities[propName]) + checkProperty(propName, lEntities[propName], rEntities[propName]) } else { CreateChange(changes, ObjectAdded, v3.PropertiesLabel, nil, rKeyNodes[propName], BreakingAdded(CompSchema, PropProperties), nil, rEntities[propName]) @@ -750,20 +763,22 @@ func buildProperty(lProps, rProps []string, lEntities, rEntities map[string]*bas } for _, propName := range lProps { if rEntities[propName] != nil { - wg.Add(1) - go checkProperty(propName, lEntities[propName], rEntities[propName]) + checkProperty(propName, lEntities[propName], rEntities[propName]) } else { CreateChange(changes, ObjectRemoved, v3.PropertiesLabel, nil, lKeyNodes[propName], BreakingRemoved(CompSchema, PropProperties), lEntities[propName], nil) } } } - - // Wait for all property comparisons to finish. - wg.Wait() + c.branch(len(tasks), func(i int, branch *schemaComparison) { + tasks[i].result = branch.compareSchemas(tasks[i].left, tasks[i].right) + }) + for _, task := range tasks { + propChanges[task.key] = task.result + } } -func checkSchemaPropertyChanges( +func (c *schemaComparison) checkSchemaPropertyChanges( lSchema *base.Schema, rSchema *base.Schema, lProxy *base.SchemaProxy, @@ -771,6 +786,13 @@ func checkSchemaPropertyChanges( changes *[]*Change, sc *SchemaChanges, skipSimpleScalarUnionDiff bool, ) { var props []*PropertyCheck + addCheck := func(left, right *yaml.Node, label, property string) { + if left == nil && right == nil { + return + } + addPropertyCheck(&props, left, right, lSchema, rSchema, changes, label, + BreakingModified(CompSchema, property), CompSchema, property) + } // $schema (breaking change) var lnv, rnv *yaml.Node @@ -781,17 +803,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.SchemaTypeRef.ValueNode } - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.SchemaDialectLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropSchemaDialect), - Component: CompSchema, - Property: PropSchemaDialect, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.SchemaDialectLabel, PropSchemaDialect) lnv = nil rnv = nil @@ -802,17 +814,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.ExclusiveMaximum.ValueNode } // ExclusiveMaximum - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.ExclusiveMaximumLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropExclusiveMaximum), - Component: CompSchema, - Property: PropExclusiveMaximum, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.ExclusiveMaximumLabel, PropExclusiveMaximum) lnv = nil rnv = nil @@ -824,17 +826,7 @@ func checkSchemaPropertyChanges( } // ExclusiveMinimum - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.ExclusiveMinimumLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropExclusiveMinimum), - Component: CompSchema, - Property: PropExclusiveMinimum, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.ExclusiveMinimumLabel, PropExclusiveMinimum) lnv = nil rnv = nil @@ -846,17 +838,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.Type.ValueNode } // Type - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.TypeLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropType), - Component: CompSchema, - Property: PropType, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.TypeLabel, PropType) lnv = nil rnv = nil } @@ -868,17 +850,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.Title.ValueNode } // Title - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.TitleLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropTitle), - Component: CompSchema, - Property: PropTitle, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.TitleLabel, PropTitle) lnv = nil rnv = nil @@ -890,17 +862,7 @@ func checkSchemaPropertyChanges( } // MultipleOf - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.MultipleOfLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropMultipleOf), - Component: CompSchema, - Property: PropMultipleOf, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.MultipleOfLabel, PropMultipleOf) lnv = nil rnv = nil @@ -911,17 +873,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.Maximum.ValueNode } // Maximum - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.MaximumLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropMaximum), - Component: CompSchema, - Property: PropMaximum, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.MaximumLabel, PropMaximum) lnv = nil rnv = nil @@ -932,17 +884,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.Minimum.ValueNode } // Minimum - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.MinimumLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropMinimum), - Component: CompSchema, - Property: PropMinimum, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.MinimumLabel, PropMinimum) lnv = nil rnv = nil @@ -953,17 +895,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.MaxLength.ValueNode } // MaxLength - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.MaxLengthLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropMaxLength), - Component: CompSchema, - Property: PropMaxLength, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.MaxLengthLabel, PropMaxLength) lnv = nil rnv = nil @@ -974,17 +906,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.MinLength.ValueNode } // MinLength - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.MinLengthLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropMinLength), - Component: CompSchema, - Property: PropMinLength, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.MinLengthLabel, PropMinLength) lnv = nil rnv = nil @@ -995,17 +917,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.Pattern.ValueNode } // Pattern - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.PatternLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropPattern), - Component: CompSchema, - Property: PropPattern, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.PatternLabel, PropPattern) lnv = nil rnv = nil @@ -1016,17 +928,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.Format.ValueNode } // Format - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.FormatLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropFormat), - Component: CompSchema, - Property: PropFormat, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.FormatLabel, PropFormat) lnv = nil rnv = nil @@ -1037,17 +939,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.MaxItems.ValueNode } // MaxItems - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.MaxItemsLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropMaxItems), - Component: CompSchema, - Property: PropMaxItems, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.MaxItemsLabel, PropMaxItems) lnv = nil rnv = nil @@ -1058,17 +950,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.MinItems.ValueNode } // MinItems - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.MinItemsLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropMinItems), - Component: CompSchema, - Property: PropMinItems, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.MinItemsLabel, PropMinItems) lnv = nil rnv = nil @@ -1079,17 +961,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.MaxProperties.ValueNode } // MaxProperties - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.MaxPropertiesLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropMaxProperties), - Component: CompSchema, - Property: PropMaxProperties, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.MaxPropertiesLabel, PropMaxProperties) lnv = nil rnv = nil @@ -1101,17 +973,7 @@ func checkSchemaPropertyChanges( } // MinProperties - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.MinPropertiesLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropMinProperties), - Component: CompSchema, - Property: PropMinProperties, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.MinPropertiesLabel, PropMinProperties) lnv = nil rnv = nil @@ -1122,17 +984,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.UniqueItems.ValueNode } // UniqueItems - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.UniqueItemsLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropUniqueItems), - Component: CompSchema, - Property: PropUniqueItems, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.UniqueItemsLabel, PropUniqueItems) lnv = nil rnv = nil @@ -1141,7 +993,7 @@ func checkSchemaPropertyChanges( if lSchema != nil && lSchema.AdditionalProperties.Value != nil && rSchema != nil && rSchema.AdditionalProperties.Value != nil { if lSchema.AdditionalProperties.Value.IsA() && rSchema.AdditionalProperties.Value.IsA() { if !low.AreEqual(lSchema.AdditionalProperties.Value.A, rSchema.AdditionalProperties.Value.A) { - sc.AdditionalPropertiesChanges = CompareSchemas(lSchema.AdditionalProperties.Value.A, rSchema.AdditionalProperties.Value.A) + sc.AdditionalPropertiesChanges = c.compareSchemas(lSchema.AdditionalProperties.Value.A, rSchema.AdditionalProperties.Value.A) } } else { if lSchema.AdditionalProperties.Value.IsB() && rSchema.AdditionalProperties.Value.IsB() { @@ -1176,17 +1028,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.Description.ValueNode } // Description - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.DescriptionLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropDescription), - Component: CompSchema, - Property: PropDescription, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.DescriptionLabel, PropDescription) lnv = nil rnv = nil @@ -1197,17 +1039,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.ContentEncoding.ValueNode } // ContentEncoding - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.ContentEncodingLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropContentEncoding), - Component: CompSchema, - Property: PropContentEncoding, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.ContentEncodingLabel, PropContentEncoding) lnv = nil rnv = nil @@ -1218,17 +1050,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.ContentMediaType.ValueNode } // ContentMediaType - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.ContentMediaType, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropContentMediaType), - Component: CompSchema, - Property: PropContentMediaType, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.ContentMediaType, PropContentMediaType) lnv = nil rnv = nil @@ -1239,17 +1061,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.Default.ValueNode } // Default - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.DefaultLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropDefault), - Component: CompSchema, - Property: PropDefault, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.DefaultLabel, PropDefault) lnv = nil rnv = nil @@ -1260,17 +1072,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.Const.ValueNode } // Const - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.ConstLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropConst), - Component: CompSchema, - Property: PropConst, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.ConstLabel, PropConst) lnv = nil rnv = nil @@ -1281,17 +1083,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.Nullable.ValueNode } // Nullable - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.NullableLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropNullable), - Component: CompSchema, - Property: PropNullable, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.NullableLabel, PropNullable) lnv = nil rnv = nil @@ -1302,17 +1094,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.ReadOnly.ValueNode } // ReadOnly - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.ReadOnlyLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropReadOnly), - Component: CompSchema, - Property: PropReadOnly, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.ReadOnlyLabel, PropReadOnly) lnv = nil rnv = nil @@ -1323,17 +1105,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.WriteOnly.ValueNode } // WriteOnly - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.WriteOnlyLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropWriteOnly), - Component: CompSchema, - Property: PropWriteOnly, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.WriteOnlyLabel, PropWriteOnly) lnv = nil rnv = nil @@ -1358,17 +1130,7 @@ func checkSchemaPropertyChanges( rnv = rSchema.Deprecated.ValueNode } // Deprecated - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.DeprecatedLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropDeprecated), - Component: CompSchema, - Property: PropDeprecated, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.DeprecatedLabel, PropDeprecated) // Required j := make(map[string]int) @@ -1466,7 +1228,7 @@ func checkSchemaPropertyChanges( // If if (lSchema != nil && lSchema.If.Value != nil) && (rSchema != nil && rSchema.If.Value != nil) { if !low.AreEqual(lSchema.If.Value, rSchema.If.Value) { - sc.IfChanges = CompareSchemas(lSchema.If.Value, rSchema.If.Value) + sc.IfChanges = c.compareSchemas(lSchema.If.Value, rSchema.If.Value) } } // added If @@ -1482,7 +1244,7 @@ func checkSchemaPropertyChanges( // Else if (lSchema != nil && lSchema.Else.Value != nil) && (rSchema == nil || rSchema.Else.Value != nil) { if !low.AreEqual(lSchema.Else.Value, rSchema.Else.Value) { - sc.ElseChanges = CompareSchemas(lSchema.Else.Value, rSchema.Else.Value) + sc.ElseChanges = c.compareSchemas(lSchema.Else.Value, rSchema.Else.Value) } } // added Else @@ -1498,7 +1260,7 @@ func checkSchemaPropertyChanges( // Then if (lSchema != nil && lSchema.Then.Value != nil) && (rSchema != nil && rSchema.Then.Value != nil) { if !low.AreEqual(lSchema.Then.Value, rSchema.Then.Value) { - sc.ThenChanges = CompareSchemas(lSchema.Then.Value, rSchema.Then.Value) + sc.ThenChanges = c.compareSchemas(lSchema.Then.Value, rSchema.Then.Value) } } // added Then @@ -1514,7 +1276,7 @@ func checkSchemaPropertyChanges( // PropertyNames if (lSchema != nil && lSchema.PropertyNames.Value != nil) && (rSchema != nil && rSchema.PropertyNames.Value != nil) { if !low.AreEqual(lSchema.PropertyNames.Value, rSchema.PropertyNames.Value) { - sc.PropertyNamesChanges = CompareSchemas(lSchema.PropertyNames.Value, rSchema.PropertyNames.Value) + sc.PropertyNamesChanges = c.compareSchemas(lSchema.PropertyNames.Value, rSchema.PropertyNames.Value) } } // added PropertyNames @@ -1530,7 +1292,7 @@ func checkSchemaPropertyChanges( // Contains if (lSchema != nil && lSchema.Contains.Value != nil) && (rSchema != nil && rSchema.Contains.Value != nil) { if !low.AreEqual(lSchema.Contains.Value, rSchema.Contains.Value) { - sc.ContainsChanges = CompareSchemas(lSchema.Contains.Value, rSchema.Contains.Value) + sc.ContainsChanges = c.compareSchemas(lSchema.Contains.Value, rSchema.Contains.Value) } } // added Contains @@ -1546,7 +1308,7 @@ func checkSchemaPropertyChanges( // UnevaluatedItems if (lSchema != nil && lSchema.UnevaluatedItems.Value != nil) && (rSchema != nil && rSchema.UnevaluatedItems.Value != nil) { if !low.AreEqual(lSchema.UnevaluatedItems.Value, rSchema.UnevaluatedItems.Value) { - sc.UnevaluatedItemsChanges = CompareSchemas(lSchema.UnevaluatedItems.Value, rSchema.UnevaluatedItems.Value) + sc.UnevaluatedItemsChanges = c.compareSchemas(lSchema.UnevaluatedItems.Value, rSchema.UnevaluatedItems.Value) } } // added UnevaluatedItems @@ -1564,7 +1326,7 @@ func checkSchemaPropertyChanges( if (lSchema != nil && lSchema.UnevaluatedProperties.Value != nil) && (rSchema != nil && rSchema.UnevaluatedProperties.Value != nil) { if lSchema.UnevaluatedProperties.Value.IsA() && rSchema.UnevaluatedProperties.Value.IsA() { if !low.AreEqual(lSchema.UnevaluatedProperties.Value.A, rSchema.UnevaluatedProperties.Value.A) { - sc.UnevaluatedPropertiesChanges = CompareSchemas(lSchema.UnevaluatedProperties.Value.A, rSchema.UnevaluatedProperties.Value.A) + sc.UnevaluatedPropertiesChanges = c.compareSchemas(lSchema.UnevaluatedProperties.Value.A, rSchema.UnevaluatedProperties.Value.A) } } else { if lSchema.UnevaluatedProperties.Value.IsB() && rSchema.UnevaluatedProperties.Value.IsB() { @@ -1595,7 +1357,7 @@ func checkSchemaPropertyChanges( // Not if (lSchema != nil && lSchema.Not.Value != nil) && (rSchema != nil && rSchema.Not.Value != nil) { if !low.AreEqual(lSchema.Not.Value, rSchema.Not.Value) { - sc.NotChanges = CompareSchemas(lSchema.Not.Value, rSchema.Not.Value) + sc.NotChanges = c.compareSchemas(lSchema.Not.Value, rSchema.Not.Value) } } // added Not @@ -1613,7 +1375,7 @@ func checkSchemaPropertyChanges( if (lSchema != nil && lSchema.Items.Value != nil) && (rSchema != nil && rSchema.Items.Value != nil) { if lSchema.Items.Value.IsA() && rSchema.Items.Value.IsA() { if !low.AreEqual(lSchema.Items.Value.A, rSchema.Items.Value.A) { - sc.ItemsChanges = CompareSchemas(lSchema.Items.Value.A, rSchema.Items.Value.A) + sc.ItemsChanges = c.compareSchemas(lSchema.Items.Value.A, rSchema.Items.Value.A) } } else { CreateChange(changes, Modified, v3.ItemsLabel, @@ -1640,17 +1402,7 @@ func checkSchemaPropertyChanges( if rSchema != nil && rSchema.DynamicAnchor.ValueNode != nil { rnv = rSchema.DynamicAnchor.ValueNode } - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.DynamicAnchorLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropDynamicAnchor), - Component: CompSchema, - Property: PropDynamicAnchor, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.DynamicAnchorLabel, PropDynamicAnchor) lnv = nil rnv = nil @@ -1661,17 +1413,7 @@ func checkSchemaPropertyChanges( if rSchema != nil && rSchema.DynamicRef.ValueNode != nil { rnv = rSchema.DynamicRef.ValueNode } - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: v3.DynamicRefLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropDynamicRef), - Component: CompSchema, - Property: PropDynamicRef, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, v3.DynamicRefLabel, PropDynamicRef) lnv = nil rnv = nil @@ -1682,17 +1424,7 @@ func checkSchemaPropertyChanges( if rSchema != nil && rSchema.Id.ValueNode != nil { rnv = rSchema.Id.ValueNode } - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: base.IdLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropId), - Component: CompSchema, - Property: PropId, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, base.IdLabel, PropId) lnv = nil rnv = nil @@ -1703,23 +1435,13 @@ func checkSchemaPropertyChanges( if rSchema != nil && rSchema.Comment.ValueNode != nil { rnv = rSchema.Comment.ValueNode } - props = append(props, &PropertyCheck{ - LeftNode: lnv, - RightNode: rnv, - Label: base.CommentLabel, - Changes: changes, - Breaking: BreakingModified(CompSchema, PropComment), - Component: CompSchema, - Property: PropComment, - Original: lSchema, - New: rSchema, - }) + addCheck(lnv, rnv, base.CommentLabel, PropComment) lnv = nil rnv = nil // contentSchema (JSON Schema 2020-12) - recursive schema comparison if lSchema != nil && !lSchema.ContentSchema.IsEmpty() && rSchema != nil && !rSchema.ContentSchema.IsEmpty() { - sc.ContentSchemaChanges = CompareSchemas(lSchema.ContentSchema.Value, rSchema.ContentSchema.Value) + sc.ContentSchemaChanges = c.compareSchemas(lSchema.ContentSchema.Value, rSchema.ContentSchema.Value) } if lSchema != nil && !lSchema.ContentSchema.IsEmpty() && (rSchema == nil || rSchema.ContentSchema.IsEmpty()) { CreateChange(changes, PropertyRemoved, base.ContentSchemaLabel, @@ -1804,7 +1526,7 @@ func schemaComparisonViewForSimpleAllOfObject(proxy *base.SchemaProxy, schema *b } func isSimpleAllOfObjectSchema(proxy *base.SchemaProxy, schema *base.Schema) bool { - if proxy == nil || schema == nil || proxy.IsReference() || len(schema.AllOf.Value) == 0 { + if proxy == nil || schema == nil || len(schema.AllOf.Value) == 0 { return false } if len(schema.OneOf.Value) > 0 || len(schema.AnyOf.Value) > 0 || len(schema.PrefixItems.Value) > 0 || schema.Not.Value != nil { @@ -2049,21 +1771,37 @@ func schemaPairUsesEquivalentSimpleScalarUnion(typeProxy, anyOfProxy *base.Schem } func isPureTypeArraySchema(proxy *base.SchemaProxy) bool { - if proxy == nil || proxy.IsReference() || !schemaNodeHasSingleKey(proxy.GetValueNode(), v3.TypeLabel) { + node := schemaDefinitionNode(proxy) + if !schemaNodeHasSingleKey(node, v3.TypeLabel) { return false } - typeNode := proxy.GetValueNode().Content[1] + typeNode := node.Content[1] return typeNode.Kind == yaml.SequenceNode && len(typeNode.Content) > 0 } func isPureAnyOfUnionSchema(proxy *base.SchemaProxy) bool { - if proxy == nil || proxy.IsReference() || !schemaNodeHasSingleKey(proxy.GetValueNode(), v3.AnyOfLabel) { + if !schemaNodeHasSingleKey(schemaDefinitionNode(proxy), v3.AnyOfLabel) { return false } schema := proxy.Schema() return schema != nil && len(schema.AnyOf.Value) > 0 } +// schemaDefinitionNode returns the node a schema is defined by, following a reference to the schema it points to. +func schemaDefinitionNode(proxy *base.SchemaProxy) *yaml.Node { + if proxy == nil { + return nil + } + node := proxy.GetValueNode() + if isRef, _, _ := utils.IsNodeRefValue(node); isRef { + if proxy.GetIndex() == nil { + return nil + } + node, _, _, _ = low.LocateRefNodeWithContext(proxy.GetContext(), node, proxy.GetIndex()) + } + return node +} + func schemaNodeHasSingleKey(node *yaml.Node, key string) bool { return node != nil && node.Kind == yaml.MappingNode && len(node.Content) == 2 && node.Content[0].Value == key } @@ -2081,10 +1819,11 @@ func schemaNodeHasOnlyAllowedKeys(node *yaml.Node, allowed map[string]struct{}) } func extractTypeArraySet(proxy *base.SchemaProxy) (map[string]struct{}, bool) { - if proxy == nil || proxy.IsReference() || !schemaNodeHasSingleKey(proxy.GetValueNode(), v3.TypeLabel) { + node := schemaDefinitionNode(proxy) + if !schemaNodeHasSingleKey(node, v3.TypeLabel) { return nil, false } - typeNode := proxy.GetValueNode().Content[1] + typeNode := node.Content[1] if typeNode.Kind != yaml.SequenceNode || len(typeNode.Content) == 0 { return nil, false } @@ -2204,7 +1943,7 @@ func checkExamples(lSchema *base.Schema, rSchema *base.Schema, changes *[]*Chang } } -func extractSchemaChanges( +func (c *schemaComparison) extractSchemaChanges( lSchema []low.ValueReference[*base.SchemaProxy], rSchema []low.ValueReference[*base.SchemaProxy], label string, @@ -2217,7 +1956,7 @@ func extractSchemaChanges( } if isOrderInsensitiveSchemaCompositionLabel(label) { - extractOrderInsensitiveSchemaChanges(lSchema, rSchema, label, sc, changes) + c.extractOrderInsensitiveSchemaChanges(lSchema, rSchema, label, sc, changes) return } @@ -2246,7 +1985,7 @@ func extractSchemaChanges( for w := range lKeys { // keys are different, which means there are changes. if lKeys[w] != rKeys[w] { - *sc = append(*sc, CompareSchemas(lEntities[lKeys[w]], rEntities[rKeys[w]])) + *sc = append(*sc, c.compareSchemas(lEntities[lKeys[w]], rEntities[rKeys[w]])) } } } @@ -2255,7 +1994,7 @@ func extractSchemaChanges( if len(lKeys) > len(rKeys) { for w := range lKeys { if w < len(rKeys) && lKeys[w] != rKeys[w] { - *sc = append(*sc, CompareSchemas(lEntities[lKeys[w]], rEntities[rKeys[w]])) + *sc = append(*sc, c.compareSchemas(lEntities[lKeys[w]], rEntities[rKeys[w]])) } if w >= len(rKeys) { breaking := schemaCompositionChangeBreaking(label, ObjectRemoved) @@ -2269,7 +2008,7 @@ func extractSchemaChanges( if len(rKeys) > len(lKeys) { for w := range rKeys { if w < len(lKeys) && rKeys[w] != lKeys[w] { - *sc = append(*sc, CompareSchemas(lEntities[lKeys[w]], rEntities[rKeys[w]])) + *sc = append(*sc, c.compareSchemas(lEntities[lKeys[w]], rEntities[rKeys[w]])) } if w >= len(lKeys) { breaking := schemaCompositionChangeBreaking(label, ObjectAdded) @@ -2288,15 +2027,13 @@ type schemaCompositionEntry struct { } type schemaCompositionPair struct { - left schemaCompositionEntry - right schemaCompositionEntry - changes *SchemaChanges + left schemaCompositionEntry + right schemaCompositionEntry } type schemaCompositionPairCandidate struct { left schemaCompositionEntry right schemaCompositionEntry - changes *SchemaChanges totalChanges int breakingChanges int stableKeyMatch bool @@ -2415,7 +2152,7 @@ func pairExactSchemaCompositionEntries( return pairs, leftUnmatched, rightUnmatched } -func buildSchemaCompositionPairCandidateMatrix( +func (c *schemaComparison) buildSchemaCompositionPairCandidateMatrix( rowEntries, columnEntries []schemaCompositionEntry, swapPairDirection bool, ) [][]schemaCompositionPairCandidate { @@ -2429,19 +2166,12 @@ func buildSchemaCompositionPairCandidateMatrix( leftEntry = columnEntries[j] rightEntry = rowEntries[i] } - changes := CompareSchemas(leftEntry.proxy, rightEntry.proxy) - totalChanges := 0 - breakingChanges := 0 - if changes != nil { - totalChanges = changes.TotalChanges() - breakingChanges = changes.TotalBreakingChanges() - } + score := c.score(leftEntry.proxy, rightEntry.proxy) row[j] = schemaCompositionPairCandidate{ left: leftEntry, right: rightEntry, - changes: changes, - totalChanges: totalChanges, - breakingChanges: breakingChanges, + totalChanges: score.total, + breakingChanges: score.breaking, stableKeyMatch: leftEntry.stableKey == rightEntry.stableKey, } } @@ -2549,25 +2279,26 @@ func selectBestSchemaCompositionPairCandidates( return best } -func pairUnmatchedSchemaCompositionEntries( +func (c *schemaComparison) pairUnmatchedSchemaCompositionEntries( leftEntries, rightEntries []schemaCompositionEntry, ) []schemaCompositionPair { if len(leftEntries) == 0 || len(rightEntries) == 0 { return nil } - matrix := buildSchemaCompositionPairCandidateMatrix(leftEntries, rightEntries, false) + var matrix [][]schemaCompositionPairCandidate if len(leftEntries) > len(rightEntries) { - matrix = buildSchemaCompositionPairCandidateMatrix(rightEntries, leftEntries, true) + matrix = c.buildSchemaCompositionPairCandidateMatrix(rightEntries, leftEntries, true) + } else { + matrix = c.buildSchemaCompositionPairCandidateMatrix(leftEntries, rightEntries, false) } selected := selectBestSchemaCompositionPairCandidates(matrix) pairs := make([]schemaCompositionPair, 0, len(selected)) for i := range selected { pairs = append(pairs, schemaCompositionPair{ - left: selected[i].left, - right: selected[i].right, - changes: selected[i].changes, + left: selected[i].left, + right: selected[i].right, }) } @@ -2607,7 +2338,7 @@ func schemaCompositionChangeBreaking(label string, changeType int) bool { } } -func extractOrderInsensitiveSchemaChanges( +func (c *schemaComparison) extractOrderInsensitiveSchemaChanges( lSchema []low.ValueReference[*base.SchemaProxy], rSchema []low.ValueReference[*base.SchemaProxy], label string, @@ -2618,16 +2349,17 @@ func extractOrderInsensitiveSchemaChanges( rightEntries := buildSchemaCompositionEntries(rSchema) exactPairs, leftUnmatched, rightUnmatched := pairExactSchemaCompositionEntries(leftEntries, rightEntries) - unmatchedPairs := pairUnmatchedSchemaCompositionEntries(leftUnmatched, rightUnmatched) + unmatchedPairs := c.pairUnmatchedSchemaCompositionEntries(leftUnmatched, rightUnmatched) for i := range exactPairs { - if changes := CompareSchemas(exactPairs[i].left.proxy, exactPairs[i].right.proxy); changes != nil { + if changes := c.compareSchemas(exactPairs[i].left.proxy, exactPairs[i].right.proxy); changes != nil { *sc = append(*sc, changes) } } for i := range unmatchedPairs { - if unmatchedPairs[i].changes != nil { - *sc = append(*sc, unmatchedPairs[i].changes) + pair := unmatchedPairs[i] + if change := c.compareSchemas(pair.left.proxy, pair.right.proxy); change != nil { + *sc = append(*sc, change) } } diff --git a/what-changed/model/schema_breaking_count_test.go b/what-changed/model/schema_breaking_count_test.go new file mode 100644 index 00000000..b7a81053 --- /dev/null +++ b/what-changed/model/schema_breaking_count_test.go @@ -0,0 +1,25 @@ +package model + +import ( + "strings" + "testing" + + "github.com/pb33f/testify/require" +) + +func TestCompareSchemas_AllOfBreakingChangesCountedOnce(t *testing.T) { + left := `openapi: 3.0.3 +components: + schemas: + Root: + allOf: + - type: string +` + right := strings.Replace(left, "type: string", "type: integer", 1) + l, r := test_BuildDoc(left, right) + changes := CompareSchemas(l.Components.Value.FindSchema("Root").Value, r.Components.Value.FindSchema("Root").Value) + require.Equal(t, 1, changes.TotalChanges()) + require.Equal(t, 1, changes.TotalBreakingChanges()) + require.Len(t, changes.GetAllChanges(), 1) + require.True(t, changes.GetAllChanges()[0].Breaking) +} diff --git a/what-changed/model/schema_comparison.go b/what-changed/model/schema_comparison.go new file mode 100644 index 00000000..5e5cb8cc --- /dev/null +++ b/what-changed/model/schema_comparison.go @@ -0,0 +1,208 @@ +package model + +import ( + "github.com/pb33f/go-yaml" + "github.com/pb33f/libopenapi/datamodel/low" + "github.com/pb33f/libopenapi/datamodel/low/base" + "github.com/pb33f/libopenapi/index" + "runtime" + "sync" +) + +// Each report owns its visited pairs. Candidate scores are shared within one +// public comparison, but scoring never marks a pair visited in the report. +type schemaComparison struct { + visited map[schemaComparisonPair]struct{} + inherited map[schemaComparisonPair]struct{} + previous map[schemaComparisonPair]struct{} + scores *schemaScores + scoring *schemaScoring + serial bool +} + +type schemaScores struct { + sync.RWMutex + values map[schemaScorePair]schemaScore +} + +// branch compares independent siblings with bounded workers. Forks read the +// parent's fixed visited set and keep private deltas. Reconcile in report order; +// replay an overlapping fork serially to retain one stable location per pair. +// Each worker remembers its earlier siblings so shared subtrees are expanded +// at most once per worker, rather than once per sibling. +func (c *schemaComparison) branch(count int, task func(int, *schemaComparison)) { + if c.serial || count < 2 || runtime.GOMAXPROCS(0) == 1 { + for i := range count { + task(i, c) + } + return + } + if c.scores == nil { + c.scores = new(schemaScores) + } + deltas := make([]map[schemaComparisonPair]struct{}, count) + workers := min(count, runtime.GOMAXPROCS(0), 4) + var wg sync.WaitGroup + for worker := range workers { + wg.Add(1) + go func() { + defer wg.Done() + fork := schemaComparison{inherited: c.visited, scores: c.scores, serial: true} + for i := worker; i < count; i += workers { + fork.visited = nil + task(i, &fork) + deltas[i] = fork.visited + if len(fork.visited) > 0 { + if fork.previous == nil { + fork.previous = make(map[schemaComparisonPair]struct{}) + } + for pair := range fork.visited { + fork.previous[pair] = struct{}{} + } + } + } + }() + } + wg.Wait() + for i, delta := range deltas { + overlap := false + for pair := range delta { + if _, exists := c.visited[pair]; exists { + overlap = true + break + } + } + if overlap { + c.serial = true + task(i, c) + c.serial = false + continue + } + if len(delta) > 0 { + if c.visited == nil { + c.visited = delta + } else { + for pair := range delta { + c.visited[pair] = struct{}{} + } + } + } + } +} + +// Active scores belong to one traversal. A recursion cut must not publish a +// context-dependent partial score for another candidate to reuse. +type schemaScoring struct { + active map[schemaScorePair]struct{} + cycles uint64 +} + +type schemaComparisonPair struct { + left, right *yaml.Node +} + +type schemaScoreProxy struct { + node *yaml.Node + index *index.SpecIndex + reference string + isRef bool +} + +type schemaScorePair struct { + left, right schemaScoreProxy +} + +type schemaScore struct { + total, breaking int +} + +type schemaCompositionTask struct { + left, right []low.ValueReference[*base.SchemaProxy] + label string + result *[]*SchemaChanges + changes []*Change +} + +func (c *schemaComparison) compareCompositions(tasks []schemaCompositionTask, changes *[]*Change) { + active := tasks[:0] + for _, task := range tasks { + if len(task.left) > 0 || len(task.right) > 0 { + active = append(active, task) + } + } + c.branch(len(active), func(i int, branch *schemaComparison) { + task := &active[i] + *task.result = nil + task.changes = nil + branch.extractSchemaChanges(task.left, task.right, task.label, task.result, &task.changes) + }) + for _, task := range active { + *changes = append(*changes, task.changes...) + } +} + +func schemaScoreIdentity(proxy *base.SchemaProxy) schemaScoreProxy { + if proxy == nil { + return schemaScoreProxy{} + } + return schemaScoreProxy{schemaDefinitionNode(proxy), proxy.GetIndex(), proxy.GetReference(), proxy.IsReference()} +} + +func (c *schemaComparison) score(left, right *base.SchemaProxy) schemaScore { + pair := schemaScorePair{schemaScoreIdentity(left), schemaScoreIdentity(right)} + if c.scores != nil { + c.scores.RLock() + score, exists := c.scores.values[pair] + c.scores.RUnlock() + if exists { + return score + } + } + if c.scoring == nil { + c.scoring = &schemaScoring{active: make(map[schemaScorePair]struct{})} + } + if _, active := c.scoring.active[pair]; active { + c.scoring.cycles++ + return schemaScore{} + } + c.scoring.active[pair] = struct{}{} + defer delete(c.scoring.active, pair) + cycles := c.scoring.cycles + if c.scores == nil { + c.scores = new(schemaScores) + } + // Each candidate must see a complete comparison, independent of earlier + // candidates and report placement. Keep only its counts, not its report nodes. + candidate := schemaComparison{scores: c.scores, scoring: c.scoring, serial: true} + var score schemaScore + if changes := candidate.compareSchemas(left, right); changes != nil { + score = schemaScore{changes.TotalChanges(), changes.TotalBreakingChanges()} + } + if c.scoring.cycles == cycles { + c.scores.Lock() + if c.scores.values == nil { + c.scores.values = make(map[schemaScorePair]schemaScore) + } + c.scores.values[pair] = score + c.scores.Unlock() + } + return score +} + +func (c *schemaComparison) visit(left, right *base.SchemaProxy) bool { + pair := schemaComparisonPair{schemaDefinitionNode(left), schemaDefinitionNode(right)} + if _, exists := c.visited[pair]; exists { + return false + } + if _, exists := c.inherited[pair]; exists { + return false + } + if _, exists := c.previous[pair]; exists { + return false + } + if c.visited == nil { + c.visited = make(map[schemaComparisonPair]struct{}) + } + c.visited[pair] = struct{}{} + return true +} diff --git a/what-changed/model/schema_comparison_test.go b/what-changed/model/schema_comparison_test.go new file mode 100644 index 00000000..0af3f7cd --- /dev/null +++ b/what-changed/model/schema_comparison_test.go @@ -0,0 +1,371 @@ +package model + +import ( + "encoding/json" + "fmt" + "runtime" + "strings" + "sync" + "sync/atomic" + "testing" + + "github.com/pb33f/libopenapi/datamodel/low/base" + "github.com/pb33f/testify/require" +) + +func TestCompareSchemas_RecursiveScoresWithoutCircularMetadata(t *testing.T) { + for _, union := range []string{"anyOf", "oneOf"} { + t.Run(union, func(t *testing.T) { + makeSpec := func(name, kind string) string { + spec := fmt.Sprintf(`openapi: 3.1.1 +info: {title: Recursive scores, version: '1'} +paths: {} +components: + schemas: + Holder: + properties: + p: {$ref: '#/components/schemas/%s'} + %s: + %s: + - {$ref: '#/components/schemas/example/properties/next%s'} + - {type: %s} + example: + type: object + properties: + nextA: + %s: + - {$ref: '#/components/schemas/example/properties/nextB'} + - {type: %s} + nextB: + %s: + - {$ref: '#/components/schemas/example/properties/nextA'} + - {type: %s} +`, name, name, union, name[len(name)-1:], kind, union, kind, union, kind) + return strings.Replace(spec, " p:", fmt.Sprintf(" q: {$ref: '#/components/schemas/%s'}\n p:", name), 1) + } + left, right := test_BuildDoc(makeSpec("exampleA", "string"), makeSpec("exampleB", "integer")) + l := left.Components.Value.FindSchema("Holder").Value + r := right.Components.Value.FindSchema("Holder").Value + for _, proxy := range []*base.SchemaProxy{l, r} { + idx := proxy.GetIndex() + // Model mutation or incomplete metadata must not make scoring fatal. + idx.SetCircularReferences(nil) + idx.GetRolodex().SetSafeCircularReferences(nil) + } + serial, err := json.Marshal((&schemaComparison{serial: true}).compareSchemas(l, r)) + require.NoError(t, err) + var previous []byte + for range 3 { + comparison := new(schemaComparison) + changes := comparison.compareSchemas(l, r) + require.NotNil(t, changes) + require.Greater(t, changes.TotalChanges(), 0) + comparison.score(l, r) + require.NotNil(t, comparison.scoring) + require.Greater(t, comparison.scoring.cycles, uint64(0)) + require.Empty(t, comparison.scoring.active) + _, cached := comparison.scores.values[schemaScorePair{schemaScoreIdentity(l), schemaScoreIdentity(r)}] + require.False(t, cached, "a score cut short by recursion must not enter the completed-score cache") + encoded, err := json.Marshal(changes) + require.NoError(t, err) + require.Equal(t, string(serial), string(encoded)) + if previous != nil { + require.Equal(t, string(previous), string(encoded)) + } + previous = encoded + } + }) + } +} + +func diamondComparisonSpec(prefix string, depth int) string { + var spec strings.Builder + spec.WriteString("openapi: 3.0.3\ninfo: {title: test, version: '1'}\npaths: {}\ncomponents:\n schemas:\n Root:\n type: object\n properties:\n entry:\n $ref: '#/components/schemas/" + prefix + "0'\n") + for i := 0; i < depth; i++ { + fmt.Fprintf(&spec, " %s%d:\n type: object\n properties:\n a: {$ref: '#/components/schemas/%s%d'}\n b: {$ref: '#/components/schemas/%s%d'}\n", prefix, i, prefix, i+1, prefix, i+1) + } + fmt.Fprintf(&spec, " %s%d: {type: string}\n", prefix, depth) + return spec.String() +} + +func TestCompareSchemas_NestedUnionScoresAreShared(t *testing.T) { + for _, union := range []string{"anyOf", "oneOf"} { + for _, depth := range []int{0, 1, 2, 3, 12} { + t.Run(fmt.Sprintf("%s/%d", union, depth), func(t *testing.T) { + makeSpec := func(prefix, kind string) string { + var spec strings.Builder + spec.WriteString("openapi: 3.0.3\ninfo: {title: Union, version: '1'}\npaths: {}\ncomponents:\n schemas:\n") + fmt.Fprintf(&spec, " Root: {$ref: '#/components/schemas/%s0'}\n", prefix) + for i := 0; i < depth; i++ { + fmt.Fprintf(&spec, " %s%d:\n %s:\n - {$ref: '#/components/schemas/%s%d'}\n - {$ref: '#/components/schemas/%s%d'}\n", prefix, i, union, prefix, i+1, prefix, i+1) + } + fmt.Fprintf(&spec, " %s%d: {type: %s}\n", prefix, depth, kind) + return spec.String() + } + l, r := test_BuildDoc(makeSpec("L", "string"), makeSpec("R", "integer")) + left, right := l.Components.Value.FindSchema("Root").Value, r.Components.Value.FindSchema("Root").Value + comparison := new(schemaComparison) + changes := comparison.compareSchemas(left, right) + require.Equal(t, 2*depth+2, changes.TotalChanges()) + // Four candidates at each level share one complete score. Nested + // candidates reuse the same cache instead of starting four more trees. + if depth == 0 { + require.Nil(t, comparison.scores) + } else { + require.Len(t, comparison.scores.values, depth) + } + require.Equal(t, changes.TotalChanges(), CompareSchemas(left, right).TotalChanges()) + if depth == 12 { + var wg sync.WaitGroup + counts := make([]int, 4) + for i := range counts { + wg.Add(1) + go func() { defer wg.Done(); counts[i] = CompareSchemas(left, right).TotalChanges() }() + } + wg.Wait() + for _, count := range counts { + require.Equal(t, 2*depth+2, count) + } + } + }) + } + } +} + +func TestCompareSchemas_CandidateScoresPreserveReferenceIdentity(t *testing.T) { + makeSpec := func(kind string) string { + return fmt.Sprintf(`openapi: 3.0.3 +components: + schemas: + Target: {type: %s} + Alias: {$ref: '#/components/schemas/Target'} + Ref: {$ref: '#/components/schemas/Target'} + AliasRef: {$ref: '#/components/schemas/Alias'} +`, kind) + } + l, r := test_BuildDoc(makeSpec("string"), makeSpec("integer")) + left := l.Components.Value.FindSchema("Ref").Value + right := r.Components.Value.FindSchema("Ref").Value + alias := r.Components.Value.FindSchema("AliasRef").Value + inlineLeft := l.Components.Value.FindSchema("Target").Value + inlineRight := r.Components.Value.FindSchema("Target").Value + require.Same(t, schemaDefinitionNode(right), schemaDefinitionNode(alias)) + require.Same(t, schemaDefinitionNode(right), schemaDefinitionNode(inlineRight)) + comparison := new(schemaComparison) + for _, pair := range [][2]*base.SchemaProxy{{left, right}, {left, alias}, {inlineLeft, right}, {left, inlineRight}} { + var expected schemaScore + if changes := CompareSchemas(pair[0], pair[1]); changes != nil { + expected = schemaScore{changes.TotalChanges(), changes.TotalBreakingChanges()} + } + require.Equal(t, expected, comparison.score(pair[0], pair[1])) + require.Equal(t, expected, comparison.score(pair[0], pair[1])) + } + require.Len(t, comparison.scores.values, 4) +} + +func TestCompareSchemas_CircularUnionCandidatesStopAtReferences(t *testing.T) { + for _, union := range []string{"anyOf", "oneOf"} { + for _, mutual := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/mutual=%t", union, mutual), func(t *testing.T) { + makeSpec := func(prefix string) string { + next := prefix + "A" + if mutual { + next = prefix + "B" + } + return fmt.Sprintf(`openapi: 3.0.3 +components: + schemas: + Root: + %s: + - {$ref: '#/components/schemas/%sA'} + - {$ref: '#/components/schemas/%sA'} + %sA: + %s: + - {$ref: '#/components/schemas/%s'} + - {type: string} + %sB: + %s: + - {$ref: '#/components/schemas/%sA'} + - {type: integer} +`, union, prefix, prefix, prefix, union, next, prefix, union, prefix) + } + l, r := test_BuildDoc(makeSpec("L"), makeSpec("R")) + left, right := l.Components.Value.FindSchema("Root").Value, r.Components.Value.FindSchema("Root").Value + comparison := new(schemaComparison) + changes := comparison.compareSchemas(left, right) + require.Equal(t, 2, changes.TotalChanges()) + require.Len(t, comparison.scores.values, 1) + for _, score := range comparison.scores.values { + require.Equal(t, 1, score.total) + } + }) + } + } +} + +func TestCompareSchemas_RetargetedDiamondIsBounded(t *testing.T) { + for _, depth := range []int{8, 16, 24} { + t.Run(fmt.Sprint(depth), func(t *testing.T) { + left, right := test_BuildDoc(diamondComparisonSpec("L", depth), diamondComparisonSpec("R", depth)) + l, r := left.Components.Value.FindSchema("Root").Value, right.Components.Value.FindSchema("Root").Value + for i := 0; i < 3; i++ { + changes := CompareSchemas(l, r) + require.Equal(t, 2*depth+1, changes.TotalChanges()) + current := changes.SchemaPropertyChanges["entry"] + for level := 0; level < depth; level++ { + require.Equal(t, 1, current.SchemaPropertyChanges["b"].TotalChanges()) + current = current.SchemaPropertyChanges["a"] + } + } + var wg sync.WaitGroup + counts := make([]int, 4) + for i := range counts { + wg.Add(1) + go func() { defer wg.Done(); counts[i] = CompareSchemas(l, r).TotalChanges() }() + } + wg.Wait() + for _, count := range counts { + require.Equal(t, 2*depth+1, count) + } + }) + } +} + +func TestCompareSchemas_RetargetedDiamondWithPropertyAddRemove(t *testing.T) { + const depth = 8 + left := diamondComparisonSpec("L", depth) + right := strings.Replace(diamondComparisonSpec("R", depth), " entry:", " extra: {type: string}\n entry:", 1) + l, r := test_BuildDoc(left, right) + for _, reverse := range []bool{false, true} { + lp, rp := l.Components.Value.FindSchema("Root").Value, r.Components.Value.FindSchema("Root").Value + if reverse { + lp, rp = rp, lp + } + changes := CompareSchemas(lp, rp) + require.Equal(t, 2*depth+2, changes.TotalChanges()) + require.Equal(t, 2*depth+1, changes.SchemaPropertyChanges["entry"].TotalChanges()) + } +} + +func TestCompareSchemas_CandidateScoringHasIndependentPairs(t *testing.T) { + left := `openapi: 3.0.3 +components: + schemas: + Root: + type: object + properties: + first: + anyOf: + - {$ref: '#/components/schemas/A'} + - {$ref: '#/components/schemas/B'} + - {$ref: '#/components/schemas/X'} + second: + anyOf: + - {$ref: '#/components/schemas/A'} + - {$ref: '#/components/schemas/B'} + - {$ref: '#/components/schemas/X'} + A: {type: object, properties: {a: {type: string}}} + B: {type: object, properties: {b: {type: integer}}} + X: {type: object, properties: {x: {type: boolean}}} +` + right := `openapi: 3.0.3 +components: + schemas: + Root: + type: object + properties: + first: + anyOf: + - {$ref: '#/components/schemas/B2'} + - {$ref: '#/components/schemas/A2'} + second: + anyOf: + - {$ref: '#/components/schemas/B2'} + - {$ref: '#/components/schemas/A2'} + A2: {type: object, properties: {a: {type: integer}}} + B2: {type: object, properties: {b: {type: string}}} +` + l, r := test_BuildDoc(left, right) + for i := 0; i < 3; i++ { + changes := CompareSchemas(l.Components.Value.FindSchema("Root").Value, r.Components.Value.FindSchema("Root").Value) + require.Equal(t, 8, changes.TotalChanges()) + for _, name := range []string{"first", "second"} { + union := changes.SchemaPropertyChanges[name] + require.Len(t, union.AnyOfChanges, 2) + for _, branch := range union.AnyOfChanges { + for _, property := range branch.SchemaPropertyChanges { + require.Len(t, property.Changes, 1) + require.Equal(t, "type", property.Changes[0].Property) + } + } + } + require.Equal(t, 5, changes.SchemaPropertyChanges["first"].TotalChanges()) + require.Equal(t, 3, changes.SchemaPropertyChanges["second"].TotalChanges()) + } +} + +func TestCompareSchemas_ParallelPlacementMatchesSerial(t *testing.T) { + for _, composition := range []bool{false, true} { + t.Run(fmt.Sprintf("composition=%t", composition), func(t *testing.T) { + makeSpec := func(prefix, kind string) string { + var spec strings.Builder + spec.WriteString("openapi: 3.1.0\ninfo: {title: Parallel comparison, version: '1'}\npaths: {}\ncomponents:\n schemas:\n Root:\n") + if composition { + fmt.Fprintf(&spec, " oneOf: [{$ref: '#/components/schemas/%s'}]\n anyOf: [{$ref: '#/components/schemas/%s'}]\n", prefix, prefix) + } else { + spec.WriteString(" properties:\n") + for i := range 8 { + fmt.Fprintf(&spec, " p%d: {$ref: '#/components/schemas/%s'}\n", i, prefix) + } + } + fmt.Fprintf(&spec, " %s:\n type: object\n properties:\n value: {type: %s}\n", prefix, kind) + return spec.String() + } + l, r := test_BuildDoc(makeSpec("Before", "string"), makeSpec("After", "integer")) + left, right := l.Components.Value.FindSchema("Root").Value, r.Components.Value.FindSchema("Root").Value + serial := (&schemaComparison{serial: true}).compareSchemas(left, right) + require.NotNil(t, serial) + expected, err := json.Marshal(serial) + require.NoError(t, err) + for range 20 { + parallel := new(schemaComparison).compareSchemas(left, right) + actual, err := json.Marshal(parallel) + require.NoError(t, err) + require.Equal(t, string(expected), string(actual)) + if composition { + require.Greater(t, parallel.OneOfChanges[0].TotalChanges(), 1) + require.Equal(t, 1, parallel.AnyOfChanges[0].TotalChanges()) + } else { + require.Greater(t, parallel.SchemaPropertyChanges["p0"].TotalChanges(), 1) + require.Equal(t, 1, parallel.SchemaPropertyChanges["p7"].TotalChanges()) + } + } + }) + } +} + +func TestSchemaComparisonBranchBoundsSharedWork(t *testing.T) { + l, r := test_BuildDoc(diamondComparisonSpec("L", 12), diamondComparisonSpec("R", 12)) + left, right := l.Components.Value.FindSchema("Root").Value, r.Components.Value.FindSchema("Root").Value + comparison := new(schemaComparison) + var visits atomic.Int32 + results := make([]bool, 40) + comparison.branch(len(results), func(i int, branch *schemaComparison) { + results[i] = branch.visit(left, right) + if results[i] { + visits.Add(1) + } + }) + require.LessOrEqual(t, visits.Load(), int32(min(runtime.GOMAXPROCS(0), 4))) + require.True(t, results[0]) + for _, result := range results[1:] { + require.False(t, result) + } + require.Len(t, comparison.visited, 1) + comparison.branch(len(results), func(i int, branch *schemaComparison) { + results[i] = branch.visit(left, right) + }) + for _, result := range results { + require.False(t, result, "forks inherit already reported pairs") + } +} diff --git a/what-changed/model/schema_composition_wrap.go b/what-changed/model/schema_composition_wrap.go index 00d6e7b5..b847c194 100644 --- a/what-changed/model/schema_composition_wrap.go +++ b/what-changed/model/schema_composition_wrap.go @@ -10,9 +10,10 @@ import ( ) // preservedObjectBranch returns a comparison view for an object moved into a -// composition. Uncertain wrappers retain the normal structural comparison. +// composition. Uncertain wrappers retain the normal structural comparison. The +// old object may be a reference, which is compared as the schema it resolves to. func preservedObjectBranch(l, r *base.SchemaProxy, old, wrapper *base.Schema) *base.Schema { - if l.IsReference() || r.IsReference() || old == nil || wrapper == nil || + if r.IsReference() || old == nil || wrapper == nil || old.Properties.Value == nil || old.Properties.Value.Len() == 0 || hasObjectComposition(old) { return nil } diff --git a/what-changed/model/schema_composition_wrap_test.go b/what-changed/model/schema_composition_wrap_test.go index ac08df46..2dd3639c 100644 --- a/what-changed/model/schema_composition_wrap_test.go +++ b/what-changed/model/schema_composition_wrap_test.go @@ -10,6 +10,7 @@ import ( "github.com/pb33f/go-yaml" "github.com/pb33f/libopenapi/datamodel" "github.com/pb33f/libopenapi/datamodel/low" + "github.com/pb33f/libopenapi/datamodel/low/base" "github.com/pb33f/libopenapi/datamodel/low/v3" "github.com/pb33f/testify/assert" "github.com/pb33f/testify/require" @@ -498,3 +499,61 @@ components: CompareSchemas(lSchemaProxy, rSchemaProxy) }) } + +// https://github.com/pb33f/libopenapi/issues/663 +// A referenced object moved into a nullable composition is reported like the same inline refactor, plus the +// non-breaking $ref change. +func TestCompareSchemas_ReferencedObjectWrappedMatchesInline(t *testing.T) { + left := `openapi: 3.1.0 +components: + schemas: + Thing: + type: object + required: [id] + properties: + id: + type: string + Entry: + $ref: '#/components/schemas/Thing' + Inline: + type: object + required: [id] + properties: + id: + type: string` + + right := `openapi: 3.1.0 +components: + schemas: + Thing: + type: object + required: [id] + properties: + id: + type: string + Entry: + anyOf: + - $ref: '#/components/schemas/Thing' + - type: 'null' + Inline: + anyOf: + - type: object + required: [id] + properties: + id: + type: string + - type: 'null'` + + leftDoc, rightDoc := test_BuildDoc(left, right) + find := func(doc *v3.Document, name string) *base.SchemaProxy { + return doc.Components.Value.FindSchema(name).Value + } + + changes := CompareSchemas(find(leftDoc, "Entry"), find(rightDoc, "Entry")) + inline := CompareSchemas(find(leftDoc, "Inline"), find(rightDoc, "Inline")) + require.NotNil(t, changes) + require.NotNil(t, inline) + assert.Zero(t, changes.TotalBreakingChanges()) + assert.Equal(t, inline.TotalChanges()+1, changes.TotalChanges()) + assert.Equal(t, changesOtherThanRef(inline), changesOtherThanRef(changes)) +} diff --git a/what-changed/model/schema_test.go b/what-changed/model/schema_test.go index f61968f9..19f4b88c 100644 --- a/what-changed/model/schema_test.go +++ b/what-changed/model/schema_test.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "reflect" + "sort" "strings" "sync" "sync/atomic" @@ -24,6 +25,7 @@ import ( "github.com/pb33f/libopenapi/index" "github.com/pb33f/libopenapi/orderedmap" "github.com/pb33f/testify/assert" + "github.com/pb33f/testify/require" ) // These tests require full documents to be tested properly. schemas are perhaps the most complex @@ -336,10 +338,12 @@ components: changes := CompareSchemas(lSchemaProxy, rSchemaProxy) assert.NotNil(t, changes) - assert.Len(t, changes.Changes, 1) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.Changes, 2) + assert.Len(t, changes.GetAllChanges(), 2) assert.Equal(t, Modified, changes.Changes[0].ChangeType) assert.Equal(t, "#/components/schemas/Yo", changes.Changes[0].New) + assert.Equal(t, v3.TypeLabel, changes.Changes[1].Property) + assert.True(t, changes.Changes[1].Breaking) // the schema behind the reference is compared too. } func TestCompareSchemas_RefToInline(t *testing.T) { @@ -369,11 +373,13 @@ components: changes := CompareSchemas(lSchemaProxy, rSchemaProxy) assert.NotNil(t, changes) - assert.Len(t, changes.Changes, 1) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.Changes, 2) + assert.Len(t, changes.GetAllChanges(), 2) assert.Equal(t, Modified, changes.Changes[0].ChangeType) assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) assert.Equal(t, "#/components/schemas/Yo", changes.Changes[0].Original) + assert.Equal(t, v3.TypeLabel, changes.Changes[1].Property) + assert.True(t, changes.Changes[1].Breaking) // the schema behind the reference is compared too. } func TestCompareSchemas_InlineToRef(t *testing.T) { @@ -403,11 +409,13 @@ components: changes := CompareSchemas(rSchemaProxy, lSchemaProxy) assert.NotNil(t, changes) - assert.Len(t, changes.Changes, 1) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.Changes, 2) + assert.Len(t, changes.GetAllChanges(), 2) assert.Equal(t, Modified, changes.Changes[0].ChangeType) assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) assert.Equal(t, "#/components/schemas/Yo", changes.Changes[0].New) + assert.Equal(t, v3.TypeLabel, changes.Changes[1].Property) + assert.True(t, changes.Changes[1].Breaking) // the schema behind the reference is compared too. } func TestCompareSchemas_Identical(t *testing.T) { @@ -2110,7 +2118,7 @@ components: assert.NotNil(t, changes) assert.Equal(t, 2, changes.TotalChanges()) assert.Len(t, changes.GetAllChanges(), 2) - assert.Equal(t, 2, changes.TotalBreakingChanges()) + assert.Equal(t, 1, changes.TotalBreakingChanges()) assert.Equal(t, ObjectAdded, changes.Changes[0].ChangeType) assert.Equal(t, v3.AllOfLabel, changes.Changes[0].Property) assert.Equal(t, Modified, changes.AllOfChanges[0].Changes[0].ChangeType) @@ -3452,7 +3460,8 @@ components: changes := CompareSchemas(lSchemaProxy, rSchemaProxy) assert.NotNil(t, changes) - assert.Equal(t, 1, changes.TotalChanges()) + assert.Equal(t, 2, changes.TotalChanges()) // the $ref change, and int becoming string behind it. + assert.Equal(t, 1, changes.TotalBreakingChanges()) } // https://github.com/pb33f/openapi-changes/issues/104 @@ -4195,7 +4204,7 @@ func TestExtractOrderInsensitiveSchemaChanges_ExactRefPairsCanProduceChanges(t * var schemaChanges []*SchemaChanges var changes []*Change - extractOrderInsensitiveSchemaChanges( + new(schemaComparison).extractOrderInsensitiveSchemaChanges( []low.ValueReference[*base.SchemaProxy]{{Value: leftProxy}}, []low.ValueReference[*base.SchemaProxy]{{Value: rightProxy}}, v3.OneOfLabel, @@ -4406,9 +4415,9 @@ func TestSchemaCompositionPairingHelpers(t *testing.T) { []schemaCompositionPairCandidate{{left: schemaCompositionEntry{position: 0}, right: schemaCompositionEntry{position: 0}, totalChanges: 1, stableKeyMatch: true, breakingChanges: 0}}, )) - assert.Nil(t, pairUnmatchedSchemaCompositionEntries(nil, []schemaCompositionEntry{{position: 0}})) - assert.Nil(t, pairUnmatchedSchemaCompositionEntries([]schemaCompositionEntry{{position: 0}}, nil)) - pairs := pairUnmatchedSchemaCompositionEntries( + assert.Nil(t, new(schemaComparison).pairUnmatchedSchemaCompositionEntries(nil, []schemaCompositionEntry{{position: 0}})) + assert.Nil(t, new(schemaComparison).pairUnmatchedSchemaCompositionEntries([]schemaCompositionEntry{{position: 0}}, nil)) + pairs := new(schemaComparison).pairUnmatchedSchemaCompositionEntries( []schemaCompositionEntry{{position: 0}, {position: 0}}, []schemaCompositionEntry{{position: 1}, {position: 0}}, ) @@ -4952,7 +4961,7 @@ func TestExtractSchemaChanges_PrefixItemsRemovedAndModified(t *testing.T) { var schemaChanges []*SchemaChanges var changes []*Change - extractSchemaChanges(left, right, v3.PrefixItemsLabel, &schemaChanges, &changes) + new(schemaComparison).extractSchemaChanges(left, right, v3.PrefixItemsLabel, &schemaChanges, &changes) assert.Len(t, schemaChanges, 1) assert.Len(t, changes, 1) @@ -4974,7 +4983,7 @@ func TestExtractSchemaChanges_PrefixItemsAddedAndModified(t *testing.T) { var schemaChanges []*SchemaChanges var changes []*Change - extractSchemaChanges(left, right, v3.PrefixItemsLabel, &schemaChanges, &changes) + new(schemaComparison).extractSchemaChanges(left, right, v3.PrefixItemsLabel, &schemaChanges, &changes) assert.Len(t, schemaChanges, 1) assert.Len(t, changes, 1) @@ -5054,7 +5063,7 @@ func TestCompareSchemas_fireNilCheck(t *testing.T) { // Clear hash cache to ensure deterministic results in concurrent test environments low.ClearHashCache() checkSchemaXML(nil, nil, nil, nil) - checkSchemaPropertyChanges(nil, nil, nil, nil, nil, nil, false) + new(schemaComparison).checkSchemaPropertyChanges(nil, nil, nil, nil, nil, nil, false) checkExamples(nil, nil, nil) } @@ -5405,7 +5414,9 @@ components: rSchemaProxy := rightDoc.Components.Value.FindSchema("OK").Value changes := CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Nil(t, changes) + // the reference is circular, so it isn't followed, but the $ref change itself is kept. + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) } func TestCompareSchemas_CheckRefChangeCircular_HackIndex(t *testing.T) { @@ -5472,15 +5483,15 @@ components: //rSchemaProxy.GetIndex().SetAbsolutePath("/something/else.yaml") changes := CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.GetAllChanges(), 2) rSchemaProxy.GetIndex().SetAbsolutePath("/something/else.yaml") changes = CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.GetAllChanges(), 2) rSchemaProxy.GetIndex().SetAbsolutePath("") changes = CompareSchemas(rSchemaProxy, lSchemaProxy) // flip them back - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.GetAllChanges(), 2) } @@ -5512,7 +5523,9 @@ components: //rSchemaProxy.GetIndex().SetAbsolutePath("/something/else.yaml") changes := CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Nil(t, changes) + // the reference is circular, so it isn't followed, but the $ref change itself is kept. + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) } func TestCompareSchemas_CheckRefChange_HackIndex_RightToLeft(t *testing.T) { @@ -5543,11 +5556,11 @@ components: //rSchemaProxy.GetIndex().SetAbsolutePath("/something/else.yaml") changes := CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.GetAllChanges(), 2) rSchemaProxy.GetIndex().SetAbsolutePath("/something/else.yaml") changes = CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Len(t, changes.GetAllChanges(), 1) + assert.Len(t, changes.GetAllChanges(), 2) } func TestCompareSchemas_CheckRefChangeCircular_HackIndex_RightToLeft(t *testing.T) { @@ -5578,7 +5591,9 @@ components: //rSchemaProxy.GetIndex().SetAbsolutePath("/something/else.yaml") changes := CompareSchemas(lSchemaProxy, rSchemaProxy) - assert.Nil(t, changes) + // the reference is circular, so it isn't followed, but the $ref change itself is kept. + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) } @@ -7423,6 +7438,384 @@ components: assert.Equal(t, 1, changes.TotalBreakingChanges()) } +// changesOtherThanRef summarises every change except the $ref change, for comparing a change made behind a +// reference with the same change made inline. +func changesOtherThanRef(changes *SchemaChanges) []string { + var summary []string + for _, c := range changes.GetAllChanges() { + if c.Property != v3.RefLabel { + summary = append(summary, fmt.Sprintf("%s %d %v", c.Property, c.ChangeType, c.Breaking)) + } + } + sort.Strings(summary) + return summary +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefToInline_RequiredAdded(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + OK: + $ref: '#/components/schemas/Pet' + Inline: + type: object + properties: + name: + type: string` + + right := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + OK: + type: object + properties: + name: + type: string + required: [name] + Inline: + type: object + properties: + name: + type: string + required: [name]` + + leftDoc, rightDoc := test_BuildDoc(left, right) + find := func(doc *v3.Document, name string) *base.SchemaProxy { + return doc.Components.Value.FindSchema(name).Value + } + + changes := CompareSchemas(find(leftDoc, "OK"), find(rightDoc, "OK")) + assert.NotNil(t, changes) + assert.Equal(t, 2, changes.TotalChanges()) + assert.Equal(t, 1, changes.TotalBreakingChanges()) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) + assert.False(t, changes.Changes[0].Breaking) + + // reported the way the same change is reported between two inline schemas. + inline := CompareSchemas(find(leftDoc, "Inline"), find(rightDoc, "Inline")) + assert.Equal(t, changesOtherThanRef(inline), changesOtherThanRef(changes)) + + // and in the other direction. + changes = CompareSchemas(find(rightDoc, "OK"), find(leftDoc, "OK")) + inline = CompareSchemas(find(rightDoc, "Inline"), find(leftDoc, "Inline")) + assert.Equal(t, 2, changes.TotalChanges()) + assert.Equal(t, changesOtherThanRef(inline), changesOtherThanRef(changes)) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefRetargeted_RequiredAdded(t *testing.T) { + low.ClearHashCache() + spec := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + NewPet: + type: object + properties: + name: + type: string + required: [name] + OK: + $ref: '#/components/schemas/Pet'` + + leftDoc, rightDoc := test_BuildDoc(spec, strings.Replace(spec, "$ref: '#/components/schemas/Pet'", "$ref: '#/components/schemas/NewPet'", 1)) + + changes := CompareSchemas(leftDoc.Components.Value.FindSchema("OK").Value, rightDoc.Components.Value.FindSchema("OK").Value) + assert.NotNil(t, changes) + assert.Equal(t, 2, changes.TotalChanges()) + assert.Equal(t, 1, changes.TotalBreakingChanges()) + assert.Equal(t, "#/components/schemas/NewPet", changes.Changes[0].New) + assert.Equal(t, v3.RequiredLabel, changes.Changes[1].Property) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefToInline_IdenticalCopy(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + OK: + $ref: '#/components/schemas/Pet'` + + right := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + OK: + type: object + properties: + name: + type: string` + + leftDoc, rightDoc := test_BuildDoc(left, right) + assert.Nil(t, CompareSchemas(leftDoc.Components.Value.FindSchema("OK").Value, rightDoc.Components.Value.FindSchema("OK").Value)) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefToInline_SimpleAllOf(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Pet: + allOf: + - type: object + properties: + name: + type: string + OK: + $ref: '#/components/schemas/Pet' + Inline: + allOf: + - type: object + properties: + name: + type: string` + + right := `openapi: 3.1.0 +components: + schemas: + OK: + allOf: + - type: object + properties: + name: + type: string + required: [name] + Inline: + allOf: + - type: object + properties: + name: + type: string + required: [name]` + + leftDoc, rightDoc := test_BuildDoc(left, right) + find := func(doc *v3.Document, name string) *base.SchemaProxy { + return doc.Components.Value.FindSchema(name).Value + } + + // the referenced allOf is compared through the same merged view as an inline one. + changes := CompareSchemas(find(leftDoc, "OK"), find(rightDoc, "OK")) + inline := CompareSchemas(find(leftDoc, "Inline"), find(rightDoc, "Inline")) + assert.NotNil(t, changes) + assert.NotEmpty(t, changesOtherThanRef(changes)) + assert.Equal(t, changesOtherThanRef(inline), changesOtherThanRef(changes)) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefWithSiblings_StopsAtRef(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + OK: + $ref: '#/components/schemas/Pet'` + + right := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + properties: + name: + type: string + OK: + $ref: '#/components/schemas/Pet' + required: [name]` + + leftDoc, rightDoc := test_BuildDoc(left, right) + l := leftDoc.Components.Value.FindSchema("OK").Value + r := rightDoc.Components.Value.FindSchema("OK").Value + assert.True(t, r.IsTransformedRefWithSiblings()) + + // a $ref with siblings is held as an allOf, which doesn't line up with the referenced schema field by field. + changes := CompareSchemas(l, r) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) + + changes = CompareSchemas(r, l) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefToInline_CircularStopsAtRef(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + Node: + type: object + properties: + next: + $ref: '#/components/schemas/Node' + OK: + $ref: '#/components/schemas/Node'` + + right := `openapi: 3.1.0 +components: + schemas: + Node: + type: object + properties: + next: + $ref: '#/components/schemas/Node' + OK: + type: object + required: [next]` + + leftDoc, rightDoc := test_BuildDoc(left, right) + l := leftDoc.Components.Value.FindSchema("OK").Value + r := rightDoc.Components.Value.FindSchema("OK").Value + + changes := CompareSchemas(l, r) + require.NotNil(t, changes) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) + + changes = CompareSchemas(r, l) + require.NotNil(t, changes) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefRetargeted_Unresolved(t *testing.T) { + low.ClearHashCache() + spec := `openapi: 3.1.0 +components: + schemas: + Pet: + type: object + required: [name] + properties: + name: + type: string + OK: + $ref: '#/components/schemas/Pet'` + + cases := map[string]struct { + ref string + config *datamodel.DocumentConfiguration + }{ + "missing": {"#/components/schemas/Missing", datamodel.NewDocumentConfiguration()}, + "external, not resolved": {"other.yaml#/Pet", &datamodel.DocumentConfiguration{SkipExternalRefResolution: true}}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + leftInfo, _ := datamodel.ExtractSpecInfo([]byte(spec)) + rightInfo, _ := datamodel.ExtractSpecInfo([]byte(strings.Replace(spec, "#/components/schemas/Pet'", tc.ref+"'", 1))) + leftDoc, _ := v3.CreateDocumentFromConfig(leftInfo, tc.config) + rightDoc, _ := v3.CreateDocumentFromConfig(rightInfo, tc.config) + + // there's nothing to compare the old schema with, so only the $ref change is reported. + changes := CompareSchemas(leftDoc.Components.Value.FindSchema("OK").Value, rightDoc.Components.Value.FindSchema("OK").Value) + require.NotNil(t, changes) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) + }) + } +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefRetargeted_CircularCheckSkipped(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + A: + type: object + properties: + b: + $ref: '#/components/schemas/B' + B: + type: object + properties: + a: + $ref: '#/components/schemas/A' + OK: + $ref: '#/components/schemas/A'` + right := strings.NewReplacer("schemas/A", "schemas/X", "schemas/B", "schemas/Y", " A:", " X:", " B:", " Y:").Replace(left) + + config := datamodel.NewDocumentConfiguration() + config.SkipCircularReferenceCheck = true + leftInfo, _ := datamodel.ExtractSpecInfo([]byte(left)) + rightInfo, _ := datamodel.ExtractSpecInfo([]byte(strings.Replace(right, "type: object\n properties:\n b:", "type: object\n description: changed\n properties:\n b:", 1))) + leftDoc, _ := v3.CreateDocumentFromConfig(leftInfo, config) + rightDoc, _ := v3.CreateDocumentFromConfig(rightInfo, config) + + // without the circular check a loop can't be recognised, so the reference is not followed. + changes := CompareSchemas(leftDoc.Components.Value.FindSchema("OK").Value, rightDoc.Components.Value.FindSchema("OK").Value) + require.NotNil(t, changes) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) +} + +// https://github.com/pb33f/libopenapi/issues/663 +func TestCompareSchemas_RefToInline_EquivalentScalarUnion(t *testing.T) { + low.ClearHashCache() + left := `openapi: 3.1.0 +components: + schemas: + NullableString: + type: [string, 'null'] + OK: + $ref: '#/components/schemas/NullableString'` + + right := `openapi: 3.1.0 +components: + schemas: + OK: + anyOf: + - type: string + - type: 'null'` + + leftDoc, rightDoc := test_BuildDoc(left, right) + l := leftDoc.Components.Value.FindSchema("OK").Value + r := rightDoc.Components.Value.FindSchema("OK").Value + + // a type array and the equivalent anyOf are the same schema, whichever side is a reference. + changes := CompareSchemas(l, r) + require.NotNil(t, changes) + assert.Len(t, changes.GetAllChanges(), 1) + assert.Equal(t, v3.RefLabel, changes.Changes[0].Property) + + changes = CompareSchemas(r, l) + require.NotNil(t, changes) + assert.Len(t, changes.GetAllChanges(), 1) +} + // https://github.com/pb33f/libopenapi/issues/662 func TestCompareSchemas_RefToInline_UnrelatedCircularReference(t *testing.T) { low.ClearHashCache() diff --git a/what-changed/what_changed_test.go b/what-changed/what_changed_test.go index 2654c678..da5cfb2c 100644 --- a/what-changed/what_changed_test.go +++ b/what-changed/what_changed_test.go @@ -279,13 +279,18 @@ func TestCheckExplodedFileCheck(t *testing.T) { modDoc, _ := v3.CreateDocumentFromConfig(infoMod, config) changes := CompareOpenAPIDocuments(origDoc, modDoc) - assert.Equal(t, 1, changes.TotalChanges()) + assert.Equal(t, 3, changes.TotalChanges()) assert.Equal(t, 0, changes.TotalBreakingChanges()) + // both references changed, and the files they now point to are compared: c-alt.yaml has a new description. allChanges := changes.GetAllChanges() - assert.Len(t, allChanges, 1) + assert.Len(t, allChanges, 3) assert.Equal(t, "b.yaml#/components/schemas/SchemaB", allChanges[0].OriginalObject) assert.Equal(t, "b-alt.yaml#/components/schemas/SchemaB", allChanges[0].NewObject) + assert.Equal(t, "c.yaml", allChanges[1].OriginalObject) + assert.Equal(t, "c-alt.yaml", allChanges[1].NewObject) + assert.Equal(t, "description", allChanges[2].Property) + assert.Equal(t, "object C, but this has a changed description", allChanges[2].New) }