From 356ae82284377399b20ba46f4a21f4653e865835 Mon Sep 17 00:00:00 2001 From: Tristan Cartledge Date: Fri, 12 Dec 2025 01:26:41 +0000 Subject: [PATCH] fix: validate empty component names in schema $ref and use spec-compliant separator --- cmd/openapi/commands/openapi/README.md | 2 +- cmd/openapi/commands/openapi/bundle.go | 2 +- cmd/openapi/commands/openapi/join.go | 2 +- jsonschema/oas3/jsonschema_validate_test.go | 204 ++++++++++++++++++ jsonschema/oas3/schema_validate_test.go | 42 ++++ jsonschema/oas3/validation.go | 16 +- openapi/bundle.go | 10 +- openapi/join.go | 9 +- openapi/testdata/inline/bundled_expected.yaml | 22 +- .../join/joined_filepath_expected.yaml | 8 +- 10 files changed, 283 insertions(+), 34 deletions(-) diff --git a/cmd/openapi/commands/openapi/README.md b/cmd/openapi/commands/openapi/README.md index 625e2e8..ad443f9 100644 --- a/cmd/openapi/commands/openapi/README.md +++ b/cmd/openapi/commands/openapi/README.md @@ -410,7 +410,7 @@ openapi spec bundle --naming filepath ./spec.yaml ./bundled.yaml **Naming Strategies:** -- `filepath` (default): Uses file path-based naming like `external_api_yaml~User` for conflicts +- `filepath` (default): Uses file path-based naming like `external_api_yaml__User` for conflicts - `counter`: Uses counter-based suffixes like `User_1`, `User_2` for conflicts What bundling does: diff --git a/cmd/openapi/commands/openapi/bundle.go b/cmd/openapi/commands/openapi/bundle.go index f21457a..1611d93 100644 --- a/cmd/openapi/commands/openapi/bundle.go +++ b/cmd/openapi/commands/openapi/bundle.go @@ -28,7 +28,7 @@ This operation is useful when you want to: The bundle command supports two naming strategies: • counter: Uses counter-based suffixes like User_1, User_2 for conflicts -• filepath: Uses file path-based naming like external_api_yaml~User +• filepath: Uses file path-based naming like external_api_yaml__User Examples: # Bundle to stdout (pipe-friendly) diff --git a/cmd/openapi/commands/openapi/join.go b/cmd/openapi/commands/openapi/join.go index 549f5af..ab8fa09 100644 --- a/cmd/openapi/commands/openapi/join.go +++ b/cmd/openapi/commands/openapi/join.go @@ -30,7 +30,7 @@ This command merges OpenAPI specifications by: The join operation supports two conflict resolution strategies: • counter: Uses counter-based suffixes like User_1, User_2 for conflicts -• filepath: Uses file path-based naming like second_yaml~User +• filepath: Uses file path-based naming like second_yaml__User Smart conflict handling: • Components: Identical components are merged, conflicts are renamed diff --git a/jsonschema/oas3/jsonschema_validate_test.go b/jsonschema/oas3/jsonschema_validate_test.go index 15b6f97..577ee2a 100644 --- a/jsonschema/oas3/jsonschema_validate_test.go +++ b/jsonschema/oas3/jsonschema_validate_test.go @@ -11,6 +11,210 @@ import ( "github.com/stretchr/testify/require" ) +func TestValidate_TopLevel_Success(t *testing.T) { + t.Parallel() + + t.Run("nil schema returns nil", func(t *testing.T) { + t.Parallel() + + var schema *oas3.JSONSchema[oas3.Referenceable] + errs := oas3.Validate(t.Context(), schema) + require.Nil(t, errs, "nil schema should return nil errors") + }) + + t.Run("bool schema returns nil", func(t *testing.T) { + t.Parallel() + + schema := oas3.NewJSONSchemaFromBool(true) + errs := oas3.Validate(t.Context(), schema) + require.Nil(t, errs, "bool schema should return nil errors") + }) + + t.Run("bool false schema returns nil", func(t *testing.T) { + t.Parallel() + + schema := oas3.NewJSONSchemaFromBool(false) + errs := oas3.Validate(t.Context(), schema) + require.Nil(t, errs, "bool false schema should return nil errors") + }) + + t.Run("valid schema returns nil errors", func(t *testing.T) { + t.Parallel() + + yml := ` +type: string +title: Valid Schema +` + var schema oas3.JSONSchema[oas3.Referenceable] + _, err := marshaller.Unmarshal(t.Context(), bytes.NewBufferString(yml), &schema) + require.NoError(t, err) + + errs := oas3.Validate(t.Context(), &schema) + require.Empty(t, errs, "valid schema should return no errors") + }) +} + +func TestValidate_TopLevel_Error(t *testing.T) { + t.Parallel() + + t.Run("invalid schema returns errors", func(t *testing.T) { + t.Parallel() + + yml := ` +type: invalid_type +` + var schema oas3.JSONSchema[oas3.Referenceable] + _, err := marshaller.Unmarshal(t.Context(), bytes.NewBufferString(yml), &schema) + require.NoError(t, err) + + errs := oas3.Validate(t.Context(), &schema) + require.NotEmpty(t, errs, "invalid schema should return errors") + }) +} + +func TestSchema_Validate_OpenAPIVersions_Success(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + version string + yml string + }{ + { + name: "OpenAPI 3.0 version via context", + version: "3.0.3", + yml: ` +type: string +`, + }, + { + name: "OpenAPI 3.1 version via context", + version: "3.1.0", + yml: ` +type: string +`, + }, + { + name: "OpenAPI 3.2 version via context", + version: "3.2.0", + yml: ` +type: string +`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var schema oas3.Schema + _, err := marshaller.Unmarshal(t.Context(), bytes.NewBufferString(tt.yml), &schema) + require.NoError(t, err) + + dv := &oas3.ParentDocumentVersion{ + OpenAPI: &tt.version, + } + + errs := schema.Validate(t.Context(), validation.WithContextObject(dv)) + require.Empty(t, errs, "valid schema should return no errors for version %s", tt.version) + }) + } +} + +func TestSchema_Validate_SchemaField_Success(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + yml string + }{ + { + name: "explicit 3.0 $schema field", + yml: ` +$schema: "https://spec.openapis.org/oas/3.0/dialect/2024-10-18" +type: string +`, + }, + { + name: "explicit 3.1 $schema field", + yml: ` +$schema: "https://spec.openapis.org/oas/3.1/meta/2024-11-10" +type: string +`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var schema oas3.Schema + _, err := marshaller.Unmarshal(t.Context(), bytes.NewBufferString(tt.yml), &schema) + require.NoError(t, err) + + errs := schema.Validate(t.Context()) + require.Empty(t, errs, "valid schema should return no errors") + }) + } +} + +func TestSchema_Validate_UnsupportedVersion_Defaults(t *testing.T) { + t.Parallel() + + t.Run("unsupported OpenAPI version defaults to 3.1", func(t *testing.T) { + t.Parallel() + + yml := ` +type: string +` + var schema oas3.Schema + _, err := marshaller.Unmarshal(t.Context(), bytes.NewBufferString(yml), &schema) + require.NoError(t, err) + + version := "2.0.0" + dv := &oas3.ParentDocumentVersion{ + OpenAPI: &version, + } + + errs := schema.Validate(t.Context(), validation.WithContextObject(dv)) + require.Empty(t, errs, "unsupported version should default to 3.1 and validate successfully") + }) + + t.Run("Arazzo version is unsupported and defaults to 3.1", func(t *testing.T) { + t.Parallel() + + yml := ` +type: string +` + var schema oas3.Schema + _, err := marshaller.Unmarshal(t.Context(), bytes.NewBufferString(yml), &schema) + require.NoError(t, err) + + version := "1.0.0" + dv := &oas3.ParentDocumentVersion{ + Arazzo: &version, + } + + errs := schema.Validate(t.Context(), validation.WithContextObject(dv)) + require.Empty(t, errs, "Arazzo version should default to 3.1 and validate successfully") + }) + + t.Run("unsupported $schema field defaults to 3.1", func(t *testing.T) { + t.Parallel() + + yml := ` +$schema: "https://json-schema.org/draft/2020-12/schema" +type: string +` + var schema oas3.Schema + _, err := marshaller.Unmarshal(t.Context(), bytes.NewBufferString(yml), &schema) + require.NoError(t, err) + + errs := schema.Validate(t.Context()) + require.Empty(t, errs, "unsupported $schema should default to 3.1") + }) +} + func TestJSONSchema_Validate_Error(t *testing.T) { t.Parallel() diff --git a/jsonschema/oas3/schema_validate_test.go b/jsonschema/oas3/schema_validate_test.go index 4d5a814..644c6c6 100644 --- a/jsonschema/oas3/schema_validate_test.go +++ b/jsonschema/oas3/schema_validate_test.go @@ -508,6 +508,48 @@ required: ["name", "email"] `, wantErrs: []string{"[2:1] schema. additional properties '$ref' not allowed"}, }, + { + name: "empty component name in $ref", + yml: ` +$ref: "#/components/schemas/" +`, + wantErrs: []string{"[2:1] invalid reference: component name cannot be empty"}, + }, + { + name: "missing component name in $ref", + yml: ` +$ref: "#/components/schemas" +`, + wantErrs: []string{"[2:1] invalid reference: component name cannot be empty"}, + }, + { + name: "component name with invalid characters in $ref", + yml: ` +$ref: "#/components/schemas/User@Schema" +`, + wantErrs: []string{`[2:1] invalid reference: component name "User@Schema" must match pattern ^[a-zA-Z0-9.\-_]+$`}, + }, + { + name: "component name with space in $ref", + yml: ` +$ref: "#/components/schemas/User Schema" +`, + wantErrs: []string{`[2:1] invalid reference: component name "User Schema" must match pattern ^[a-zA-Z0-9.\-_]+$`}, + }, + { + name: "invalid JSON pointer - missing leading slash in $ref", + yml: ` +$ref: "#components/schemas/User" +`, + wantErrs: []string{"[2:1] invalid reference JSON pointer: validation error -- jsonpointer must start with /: components/schemas/User"}, + }, + { + name: "empty JSON pointer in $ref", + yml: ` +$ref: "#" +`, + wantErrs: []string{"[2:1] invalid reference JSON pointer: empty"}, + }, } for _, tt := range tests { diff --git a/jsonschema/oas3/validation.go b/jsonschema/oas3/validation.go index b80c350..b27ac26 100644 --- a/jsonschema/oas3/validation.go +++ b/jsonschema/oas3/validation.go @@ -80,6 +80,15 @@ func (js *Schema) Validate(ctx context.Context, opts ...validation.Option) []err dv := validation.GetContextObject[ParentDocumentVersion](o) + var errs []error + + // Validate reference string if present + if js.IsReference() { + if err := js.GetRef().Validate(); err != nil { + errs = append(errs, validation.NewValidationError(err, js.GetCore().Ref.GetKeyNodeOrRoot(js.GetRootNode()))) + } + } + var schema string if js.Schema != nil { switch *js.Schema { @@ -131,16 +140,13 @@ func (js *Schema) Validate(ctx context.Context, opts ...validation.Option) []err } } - var errs []error err = oasSchemaValidator.Validate(jsAny) if err != nil { var validationErr *jsValidator.ValidationError if errors.As(err, &validationErr) { - errs = getRootCauses(validationErr, *core) + errs = append(errs, getRootCauses(validationErr, *core)...) } else { - errs = []error{ - validation.NewValidationError(validation.NewValueValidationError("schema invalid: %s", err.Error()), core.RootNode), - } + errs = append(errs, validation.NewValidationError(validation.NewValueValidationError("schema invalid: %s", err.Error()), core.RootNode)) } } diff --git a/openapi/bundle.go b/openapi/bundle.go index 959f036..84be045 100644 --- a/openapi/bundle.go +++ b/openapi/bundle.go @@ -23,7 +23,7 @@ type BundleNamingStrategy int const ( // BundleNamingCounter uses counter-based suffixes like User_1, User_2 for conflicts BundleNamingCounter BundleNamingStrategy = iota - // BundleNamingFilePath uses file path-based naming like file_path_somefile_yaml~User + // BundleNamingFilePath uses file path-based naming like file_path_somefile_yaml__User BundleNamingFilePath ) @@ -85,7 +85,7 @@ type BundleOptions struct { // "content": { // "application/json": { // "schema": { -// "$ref": "#/components/schemas/external_api_yaml~User" +// "$ref": "#/components/schemas/external_api_yaml__User" // } // } // } @@ -96,7 +96,7 @@ type BundleOptions struct { // }, // "components": { // "schemas": { -// "external_api_yaml~User": { +// "external_api_yaml__User": { // "type": "object", // "properties": { // "id": {"type": "string"}, @@ -843,7 +843,7 @@ func generateFilePathBasedNameWithConflictResolution(ref string, usedNames map[s return generateFilePathBasedName(ref, usedNames, targetLocation) } -// generateFilePathBasedName creates names like "some_path_external_yaml~User" or "some_path_external_yaml" for top-level refs +// generateFilePathBasedName creates names like "some_path_external_yaml__User" or "some_path_external_yaml" for top-level refs func generateFilePathBasedName(ref string, usedNames map[string]bool, targetLocation string) (string, error) { // Parse the reference to extract file path and fragment using references package reference := references.Reference(ref) @@ -885,7 +885,7 @@ func generateFilePathBasedName(ref string, usedNames map[string]bool, targetLoca // Clean up fragment (remove leading slash and convert path separators) cleanFragment := strings.TrimPrefix(fragment, "/") cleanFragment = strings.ReplaceAll(cleanFragment, "/", "_") - componentName = safeFileName + "~" + cleanFragment + componentName = safeFileName + "__" + cleanFragment } // Ensure uniqueness diff --git a/openapi/join.go b/openapi/join.go index 425aced..5c51126 100644 --- a/openapi/join.go +++ b/openapi/join.go @@ -22,7 +22,7 @@ type JoinConflictStrategy int const ( // JoinConflictCounter uses counter-based suffixes like User_1, User_2 for conflicts JoinConflictCounter JoinConflictStrategy = iota - // JoinConflictFilePath uses file path-based naming like file_path_somefile_yaml~User + // JoinConflictFilePath uses file path-based naming like file_path_somefile_yaml__User JoinConflictFilePath ) @@ -188,7 +188,6 @@ func initializeUsedNames(doc *OpenAPI, usedComponentNames map[string]bool, compo usedPathNames[path] = true } } - } // joinSingleDocument joins a single document into the result document @@ -359,7 +358,6 @@ func joinWebhooks(result, src *OpenAPI) { // For webhooks, we append all - no conflict resolution needed as they're named result.Webhooks.Set(name, webhook) } - } // joinComponents joins components from source document into result with conflict resolution @@ -422,7 +420,6 @@ func joinSchemas(resultComponents, srcComponents *Components, srcPath string, st componentHashes[name] = schemaHash } } - } // joinOtherComponents joins non-schema components with conflict resolution @@ -678,7 +675,7 @@ func generateJoinComponentName(originalName, filePath string, strategy JoinConfl } } -// generateJoinFilePathBasedName creates names like "some_path_external_yaml~User" +// generateJoinFilePathBasedName creates names like "some_path_external_yaml__User" func generateJoinFilePathBasedName(originalName, filePath string, usedNames map[string]bool) string { // Convert file path to safe component name cleanPath := filepath.Clean(filePath) @@ -693,7 +690,7 @@ func generateJoinFilePathBasedName(originalName, filePath string, usedNames map[ // Replace path separators and unsafe characters with underscores safeFileName := regexp.MustCompile(`[^a-zA-Z0-9_]`).ReplaceAllString(cleanPath, "_") - componentName := safeFileName + "~" + originalName + componentName := safeFileName + "__" + originalName // Ensure uniqueness originalComponentName := componentName diff --git a/openapi/testdata/inline/bundled_expected.yaml b/openapi/testdata/inline/bundled_expected.yaml index 1924fa6..f5e7615 100644 --- a/openapi/testdata/inline/bundled_expected.yaml +++ b/openapi/testdata/inline/bundled_expected.yaml @@ -82,7 +82,7 @@ paths: schema: type: array items: - $ref: "#/components/schemas/external_conflicting_user_yaml~User" + $ref: "#/components/schemas/external_conflicting_user_yaml__User" /posts: get: tags: @@ -332,13 +332,13 @@ paths: content: application/json: schema: - $ref: "#/components/schemas/testdata_clean_clean_input_yaml~components_schemas_User" + $ref: "#/components/schemas/testdata_clean_clean_input_yaml__components_schemas_User" "400": description: Error from parent directory (conflicts with existing Error) content: application/json: schema: - $ref: "#/components/schemas/testdata_clean_clean_input_yaml~components_schemas_Error" + $ref: "#/components/schemas/testdata_clean_clean_input_yaml__components_schemas_Error" "404": description: Non-conflicting schema from parent directory content: @@ -362,13 +362,13 @@ paths: content: application/json: schema: - $ref: "#/components/schemas/testdata_test_openapi_yaml~components_schemas_Product" + $ref: "#/components/schemas/testdata_test_openapi_yaml__components_schemas_Product" "201": description: Product from intermediate testdata (should conflict and get counter) content: application/json: schema: - $ref: "#/components/schemas/testdata_test_openapi_yaml~components_schemas_Product_1" + $ref: "#/components/schemas/testdata_test_openapi_yaml__components_schemas_Product_1" components: parameters: LimitParam: @@ -564,7 +564,7 @@ components: type: boolean default: true description: Product status - external_conflicting_user_yaml~User: + external_conflicting_user_yaml__User: type: object properties: userId: @@ -592,7 +592,7 @@ components: format: date-time description: When the user was created manager: - $ref: '#/components/schemas/external_conflicting_user_yaml~User' + $ref: '#/components/schemas/external_conflicting_user_yaml__User' description: User's manager (creates circular reference) required: - userId @@ -845,14 +845,14 @@ components: properties: {} title: Abstract Body description: An abstract schema used to define other request and response body model schemas. - testdata_clean_clean_input_yaml~components_schemas_User: + testdata_clean_clean_input_yaml__components_schemas_User: type: object properties: id: type: integer name: type: string - testdata_clean_clean_input_yaml~components_schemas_Error: + testdata_clean_clean_input_yaml__components_schemas_Error: type: object properties: message: @@ -862,7 +862,7 @@ components: UnusedSchema: type: string description: This schema is not referenced anywhere - testdata_test_openapi_yaml~components_schemas_Product: + testdata_test_openapi_yaml__components_schemas_Product: type: object properties: id: @@ -887,7 +887,7 @@ components: - price description: Product information from root testdata x-test: some-value - testdata_test_openapi_yaml~components_schemas_Product_1: + testdata_test_openapi_yaml__components_schemas_Product_1: type: object properties: id: diff --git a/openapi/testdata/join/joined_filepath_expected.yaml b/openapi/testdata/join/joined_filepath_expected.yaml index 18b0c74..0ed37b4 100644 --- a/openapi/testdata/join/joined_filepath_expected.yaml +++ b/openapi/testdata/join/joined_filepath_expected.yaml @@ -32,7 +32,7 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/subdir_second_yaml~User' + $ref: '#/components/schemas/subdir_second_yaml__User' responses: "201": description: Created @@ -131,7 +131,7 @@ components: price: type: number format: float - subdir_second_yaml~User: + subdir_second_yaml__User: type: object properties: id: @@ -172,7 +172,7 @@ components: application/json: schema: $ref: '#/components/schemas/Error' - subdir_second_yaml~NotFound: + subdir_second_yaml__NotFound: description: Resource not found content: application/json: @@ -214,7 +214,7 @@ components: schema: type: integer format: int32 - subdir_second_yaml~limitParam: + subdir_second_yaml__limitParam: name: limit in: query description: maximum number of items to return