-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
fix(openapi-yaml): localize nested multi-file component $refs #24540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -19,16 +19,22 @@ | |||||
|
|
||||||
| import com.google.common.collect.ImmutableMap; | ||||||
| import com.samskivert.mustache.Mustache.Lambda; | ||||||
| import io.swagger.v3.oas.models.Components; | ||||||
| import io.swagger.v3.oas.models.OpenAPI; | ||||||
| import io.swagger.v3.oas.models.Operation; | ||||||
| import io.swagger.v3.oas.models.media.Schema; | ||||||
| import org.apache.commons.lang3.StringUtils; | ||||||
| import org.openapitools.codegen.*; | ||||||
| import org.openapitools.codegen.meta.features.*; | ||||||
| import org.openapitools.codegen.serializer.SerializerUtils; | ||||||
| import org.openapitools.codegen.templating.mustache.OnChangeLambda; | ||||||
| import org.openapitools.codegen.utils.ModelUtils; | ||||||
| import org.openapitools.codegen.utils.OpenAPISorter; | ||||||
| import org.slf4j.Logger; | ||||||
| import org.slf4j.LoggerFactory; | ||||||
|
|
||||||
| import java.net.URLDecoder; | ||||||
| import java.nio.charset.StandardCharsets; | ||||||
| import java.util.ArrayList; | ||||||
| import java.util.EnumSet; | ||||||
| import java.util.List; | ||||||
|
|
@@ -98,13 +104,150 @@ public void processOpts() { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public void preprocessOpenAPI(OpenAPI openAPI) { | ||||||
| // swagger-parser can leave nested multi-file $refs as relative paths into the root | ||||||
| // document (e.g. ./swagger.yml#/components/schemas/ComplexType). Rewrite those to | ||||||
| // internal refs when the target already exists in local components so the bundled | ||||||
| // openapi.yaml is self-contained and valid. See #24528. | ||||||
| // Run in preprocessOpenAPI (before models / info processing) rather than processOpenAPI. | ||||||
| localizeExternalComponentRefs(openAPI); | ||||||
| super.preprocessOpenAPI(openAPI); | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public void processOpenAPI(OpenAPI openAPI) { | ||||||
| if (sortOutput) { | ||||||
| OpenAPISorter.sort(openAPI); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Rewrite external-looking component $refs that still point at a local component to | ||||||
| * internal {@code #/components/...} form. | ||||||
| * | ||||||
| * @param openAPI OpenAPI document being processed | ||||||
| */ | ||||||
| void localizeExternalComponentRefs(OpenAPI openAPI) { | ||||||
| if (openAPI == null || openAPI.getComponents() == null | ||||||
| || openAPI.getComponents().getSchemas() == null) { | ||||||
| return; | ||||||
| } | ||||||
| // Walk component schemas directly so we do not call getSimpleRef (and its warnings) | ||||||
| // on still-external nested refs before they are rewritten. | ||||||
| for (Schema<?> schema : openAPI.getComponents().getSchemas().values()) { | ||||||
| walkAndRewriteSchemaRefs(schema, openAPI); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| private void walkAndRewriteSchemaRefs(Schema<?> schema, OpenAPI openAPI) { | ||||||
| if (schema == null) { | ||||||
| return; | ||||||
| } | ||||||
| rewriteSchemaRefIfLocal(schema, openAPI); | ||||||
| if (schema.getProperties() != null) { | ||||||
| for (Object property : schema.getProperties().values()) { | ||||||
| walkAndRewriteSchemaRefs((Schema<?>) property, openAPI); | ||||||
| } | ||||||
| } | ||||||
| if (ModelUtils.isArraySchema(schema) && schema.getItems() != null) { | ||||||
| walkAndRewriteSchemaRefs(schema.getItems(), openAPI); | ||||||
| } | ||||||
| if (schema.getAdditionalProperties() instanceof Schema) { | ||||||
| walkAndRewriteSchemaRefs((Schema<?>) schema.getAdditionalProperties(), openAPI); | ||||||
| } | ||||||
| if (schema.getAllOf() != null) { | ||||||
| for (Object s : schema.getAllOf()) { | ||||||
| walkAndRewriteSchemaRefs((Schema<?>) s, openAPI); | ||||||
| } | ||||||
| } | ||||||
| if (schema.getAnyOf() != null) { | ||||||
| for (Object s : schema.getAnyOf()) { | ||||||
| walkAndRewriteSchemaRefs((Schema<?>) s, openAPI); | ||||||
| } | ||||||
| } | ||||||
| if (schema.getOneOf() != null) { | ||||||
| for (Object s : schema.getOneOf()) { | ||||||
| walkAndRewriteSchemaRefs((Schema<?>) s, openAPI); | ||||||
| } | ||||||
| } | ||||||
| if (schema.getNot() != null) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Refs nested in several schema keywords are still omitted from localization. The recursive walker handles properties, items, additionalProperties, allOf/anyOf/oneOf, and not, but not Prompt for AI agents |
||||||
| walkAndRewriteSchemaRefs(schema.getNot(), openAPI); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| private void rewriteSchemaRefIfLocal(Schema<?> schema, OpenAPI openAPI) { | ||||||
| if (schema == null || StringUtils.isEmpty(schema.get$ref())) { | ||||||
| return; | ||||||
| } | ||||||
| String localRef = toLocalComponentRef(schema.get$ref(), openAPI); | ||||||
| if (localRef != null) { | ||||||
| schema.set$ref(localRef); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * If {@code ref} is an external (file-relative or absolute) reference into a local | ||||||
| * component, return the internal {@code #/components/...} form; otherwise return null. | ||||||
| * | ||||||
| * @param ref original $ref value | ||||||
| * @param openAPI OpenAPI document used to verify the target exists locally | ||||||
| * @return internal ref, or null when no rewrite should be applied | ||||||
| */ | ||||||
| String toLocalComponentRef(String ref, OpenAPI openAPI) { | ||||||
| if (ref == null || ref.startsWith("#/")) { | ||||||
| return null; | ||||||
| } | ||||||
| int fragmentIndex = ref.indexOf("#/components/"); | ||||||
| if (fragmentIndex < 0) { | ||||||
| return null; | ||||||
| } | ||||||
| String fragment = ref.substring(fragmentIndex); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: A reference to a genuinely external document can be silently rebound to a same-named root component. This code keeps only the Prompt for AI agents |
||||||
| // fragment is "#/components/{section}/{name}[/...]" — strip "#/" before splitting | ||||||
| String path = fragment.startsWith("#/") ? fragment.substring(2) : fragment.substring(1); | ||||||
| String[] parts = path.split("/"); | ||||||
| // parts[0]=components, parts[1]=section, parts[2]=name | ||||||
| if (parts.length < 3 || !"components".equals(parts[0])) { | ||||||
| return null; | ||||||
| } | ||||||
| String section = parts[1]; | ||||||
| String name = URLDecoder.decode(parts[2], StandardCharsets.UTF_8) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Component names containing a literal Prompt for AI agents
Suggested change
|
||||||
| .replace("~1", "/") | ||||||
| .replace("~0", "~"); | ||||||
| if (!componentExists(openAPI.getComponents(), section, name)) { | ||||||
| return null; | ||||||
| } | ||||||
| return fragment; | ||||||
| } | ||||||
|
|
||||||
| private static boolean componentExists(Components components, String section, String name) { | ||||||
| if (components == null || StringUtils.isEmpty(name)) { | ||||||
| return false; | ||||||
| } | ||||||
| switch (section) { | ||||||
| case "schemas": | ||||||
| return components.getSchemas() != null && components.getSchemas().containsKey(name); | ||||||
| case "parameters": | ||||||
| return components.getParameters() != null && components.getParameters().containsKey(name); | ||||||
| case "responses": | ||||||
| return components.getResponses() != null && components.getResponses().containsKey(name); | ||||||
| case "requestBodies": | ||||||
| return components.getRequestBodies() != null && components.getRequestBodies().containsKey(name); | ||||||
| case "headers": | ||||||
| return components.getHeaders() != null && components.getHeaders().containsKey(name); | ||||||
| case "examples": | ||||||
| return components.getExamples() != null && components.getExamples().containsKey(name); | ||||||
| case "links": | ||||||
| return components.getLinks() != null && components.getLinks().containsKey(name); | ||||||
| case "callbacks": | ||||||
| return components.getCallbacks() != null && components.getCallbacks().containsKey(name); | ||||||
| case "securitySchemes": | ||||||
| return components.getSecuritySchemes() != null && components.getSecuritySchemes().containsKey(name); | ||||||
| default: | ||||||
| return false; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| protected ImmutableMap.Builder<String, Lambda> addMustacheLambdas() { | ||||||
| return super.addMustacheLambdas() | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -243,4 +243,41 @@ public void testSortOutput() throws Exception { | |||||
| Assert.assertTrue(mammalsGet != -1 && mammalsDelete != -1, "Expected HTTP methods must be present within /mammals"); | ||||||
| Assert.assertTrue(mammalsGet < mammalsDelete, "GET must appear before DELETE within /mammals"); | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| public void testIssue24528NestedExternalRefsLocalized() throws Exception { | ||||||
| Map<String, Object> properties = new HashMap<>(); | ||||||
| properties.put(OpenAPIYamlGenerator.OUTPUT_NAME, "issue_24528.yaml"); | ||||||
|
|
||||||
| File output = Files.createTempDirectory("issue_24528").toFile(); | ||||||
| output.deleteOnExit(); | ||||||
|
|
||||||
| final CodegenConfigurator configurator = new CodegenConfigurator() | ||||||
| .setGeneratorName("openapi-yaml") | ||||||
| .setAdditionalProperties(properties) | ||||||
| .setInputSpec("src/test/resources/3_0/issue_24528/swagger.yml") | ||||||
| .setOutputDir(output.getAbsolutePath().replace("\\", "/")); | ||||||
|
|
||||||
| new DefaultGenerator().opts(configurator.toClientOptInput()).generate(); | ||||||
|
|
||||||
| Path generated = Path.of(output.getAbsolutePath(), "issue_24528.yaml"); | ||||||
| String generatedYaml = new String(Files.readAllBytes(generated), StandardCharsets.UTF_8); | ||||||
|
|
||||||
| // Nested multi-file $refs must be rewritten to internal component refs. | ||||||
| Assert.assertFalse(generatedYaml.contains("swagger.yml#/components/schemas/ComplexType"), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The regression test can pass while other nested refs remain external. The Prompt for AI agents
Suggested change
|
||||||
| "Nested ComplexType $refs must not keep an external file path. Output was:\n" + generatedYaml); | ||||||
| Assert.assertTrue(generatedYaml.contains("#/components/schemas/ComplexType"), | ||||||
| "Nested ComplexType $refs must be localized. Output was:\n" + generatedYaml); | ||||||
|
|
||||||
| OpenAPI actual = TestUtils.parseSpec(generated.toString()); | ||||||
| Schema<?> arrayOfComplex = actual.getComponents().getSchemas().get("ArrayOfComplexTypes"); | ||||||
| Schema<?> complexArrayItems = (Schema<?>) arrayOfComplex.getProperties().get("complexArray"); | ||||||
| Assert.assertEquals(complexArrayItems.getItems().get$ref(), "#/components/schemas/ComplexType"); | ||||||
|
|
||||||
| Schema<?> nullableComplex = actual.getComponents().getSchemas().get("NullableComplexType"); | ||||||
| Schema<?> complexOrNull = (Schema<?>) nullableComplex.getProperties().get("complexOrNull"); | ||||||
| // nullable + anyOf is normalized to allOf + nullable | ||||||
| Assert.assertNotNull(complexOrNull.getAllOf()); | ||||||
| Assert.assertEquals(complexOrNull.getAllOf().get(0).get$ref(), "#/components/schemas/ComplexType"); | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| type: object | ||
| properties: | ||
| complexArray: | ||
| type: array | ||
| items: | ||
| $ref: '../swagger.yml#/components/schemas/ComplexType' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| type: object | ||
| properties: | ||
| prop1: | ||
| $ref: '../swagger.yml#/components/schemas/SimpleType' | ||
| prop2: | ||
| type: array | ||
| items: | ||
| $ref: '../swagger.yml#/components/schemas/SimpleType' | ||
| prop3: | ||
| nullable: true | ||
| anyOf: | ||
| - $ref: '../swagger.yml#/components/schemas/SimpleType' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| type: object | ||
| properties: | ||
| complexOrNull: | ||
| nullable: true | ||
| anyOf: | ||
| - $ref: '../swagger.yml#/components/schemas/ComplexType' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| type: object | ||
| properties: | ||
| value: | ||
| type: string |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| openapi: 3.0.4 | ||
| info: | ||
| version: "1.0" | ||
| title: Open API Ref Test | ||
| paths: | ||
| '/simple': | ||
| get: | ||
| responses: | ||
| '200': | ||
| description: Simple return | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/SimpleType' | ||
| '/arr': | ||
| get: | ||
| responses: | ||
| '200': | ||
| description: description | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ArrayOfComplexTypes' | ||
| '/null': | ||
| get: | ||
| responses: | ||
| '200': | ||
| description: description | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/NullableComplexType' | ||
|
|
||
| components: | ||
| schemas: | ||
| ComplexType: | ||
| $ref: "./schemas/ComplexType.yml" | ||
| SimpleType: | ||
| $ref: "./schemas/SimpleType.yml" | ||
| ArrayOfComplexTypes: | ||
| $ref: "./schemas/ArrayOfComplexTypes.yml" | ||
| NullableComplexType: | ||
| $ref: "./schemas/NullableComplexType.yml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: External component refs outside
components.schemasare never visited, so the generated single-file document can still contain the exact invalid-looking refs this change is intended to remove. The walker is started only for schema components; it does not inspect schemas in path operations or in component parameters, responses, request bodies, headers, callbacks, or path items. The existingInlineModelResolverhas dedicated traversal for those containers, which confirms they can carry schema refs. The localization pass should traverse all schema-bearing locations in the OpenAPI document, not just the top-level schema map.Prompt for AI agents