From 23699ce3858d49194966f43cb91925c85b85fef4 Mon Sep 17 00:00:00 2001 From: dlwlrma <965810157@qq.com> Date: Sun, 27 Sep 2026 11:46:04 +0800 Subject: [PATCH] fix: preserve discriminator aliases in oneOf comparisons --- .../ComposedSchemaDiffResult.java | 49 +++-- .../model/deferred/DeferredSchemaCache.java | 12 +- .../openapidiff/core/OneOfDiffTest.java | 196 ++++++++++++++++++ .../oneOf_discriminator-aliases_1.yaml | 42 ++++ .../oneOf_discriminator-aliases_2.yaml | 42 ++++ 5 files changed, 324 insertions(+), 17 deletions(-) create mode 100644 core/src/test/resources/oneOf_discriminator-aliases_1.yaml create mode 100644 core/src/test/resources/oneOf_discriminator-aliases_2.yaml diff --git a/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/ComposedSchemaDiffResult.java b/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/ComposedSchemaDiffResult.java index 0c9ff762..99798518 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/ComposedSchemaDiffResult.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/compare/schemadiffresult/ComposedSchemaDiffResult.java @@ -4,12 +4,14 @@ import io.swagger.v3.oas.models.media.ComposedSchema; import io.swagger.v3.oas.models.media.Discriminator; import io.swagger.v3.oas.models.media.Schema; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.stream.Collectors; +import java.util.Set; import org.apache.commons.collections4.CollectionUtils; +import org.openapitools.openapidiff.core.compare.CacheKey; import org.openapitools.openapidiff.core.compare.MapKeyDiff; import org.openapitools.openapidiff.core.compare.OpenApiDiff; import org.openapitools.openapidiff.core.compare.SchemaDiff; @@ -73,16 +75,30 @@ public , X> DeferredChanged diff( getSchema(leftComponents, leftMapping, leftComposedSchema), getSchema(rightComponents, rightMapping, rightComposedSchema)); Map changedMapping = new LinkedHashMap<>(); + // Guard against ancestors, not other aliases scheduled in this same oneOf. + RecursiveSchemaSet mappingAncestors = new RecursiveSchemaSet(); + mappingAncestors.getLeftKeys().addAll(refSet.getLeftKeys()); + mappingAncestors.getRightKeys().addAll(refSet.getRightKeys()); + DiffContext mappingContext = context.copyWithRequired(true); for (String key : mappingDiff.getSharedKey()) { Schema leftSchema = new Schema<>(); leftSchema.set$ref(leftMapping.get(key)); Schema rightSchema = new Schema<>(); rightSchema.set$ref(rightMapping.get(key)); - discriminatorChangedBuilder - .with( - openApiDiff + CacheKey schemaKey = + new CacheKey(leftSchema.get$ref(), rightSchema.get$ref(), mappingContext); + // Keep the shared visited set for deferred traversal, including mutually recursive + // schemas. + DeferredChanged schemaDiff = + leftSchema.get$ref() != null && rightSchema.get$ref() != null + ? openApiDiff + .getDeferredSchemaCache() + .getOrAddSchema(refSet, mappingAncestors, schemaKey, leftSchema, rightSchema) + : openApiDiff .getSchemaDiff() - .diff(refSet, leftSchema, rightSchema, context.copyWithRequired(true))) + .diff(refSet, leftSchema, rightSchema, mappingContext); + discriminatorChangedBuilder + .with(schemaDiff) .ifPresent(schema -> changedMapping.put(key, schema)); } @@ -130,7 +146,13 @@ private Map getSchema( } private Map getMapping(ComposedSchema composedSchema) { - Map reverseMapping = new LinkedHashMap<>(); + Map mapping = new LinkedHashMap<>(); + if (composedSchema.getDiscriminator() != null + && composedSchema.getDiscriminator().getMapping() != null) { + // Keep every alias: multiple discriminator values can refer to the same schema. + mapping.putAll(composedSchema.getDiscriminator().getMapping()); + } + Set explicitlyMappedRefs = new HashSet<>(mapping.values()); if (composedSchema.getOneOf() != null) { for (Schema schema : composedSchema.getOneOf()) { String ref = schema.get$ref(); @@ -141,19 +163,14 @@ private Map getMapping(ComposedSchema composedSchema) { if (schemaName == null) { throw new IllegalArgumentException("invalid schema: " + ref); } - reverseMapping.put(ref, schemaName); - } - } - - if (composedSchema.getDiscriminator() != null - && composedSchema.getDiscriminator().getMapping() != null) { - for (String ref : composedSchema.getDiscriminator().getMapping().keySet()) { - reverseMapping.put(composedSchema.getDiscriminator().getMapping().get(ref), ref); + // Use the implicit schema name only when no explicit alias maps to this reference. + if (!explicitlyMappedRefs.contains(ref)) { + mapping.putIfAbsent(schemaName, ref); + } } } - return reverseMapping.entrySet().stream() - .collect(Collectors.toMap(Map.Entry::getValue, Map.Entry::getKey)); + return mapping; } private Map getUnnamedSchemas(List schemas, String name) { diff --git a/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredSchemaCache.java b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredSchemaCache.java index d1100a0b..cd27f2b2 100644 --- a/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredSchemaCache.java +++ b/core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredSchemaCache.java @@ -42,8 +42,18 @@ public SchemaDiffOperation addSchema( public DeferredChanged getOrAddSchema( RecursiveSchemaSet refSet, CacheKey key, Schema left, Schema right) { + return getOrAddSchema(refSet, refSet, key, left, right); + } + + /** Use a separate recursion guard when scheduling siblings that share references. */ + public DeferredChanged getOrAddSchema( + RecursiveSchemaSet refSet, + RecursiveSchemaSet recursionGuard, + CacheKey key, + Schema left, + Schema right) { // don't allow recursive references to schemas - if (refSet.contains(key)) { + if (recursionGuard.contains(key)) { log.debug("getOrAddSchema recursive call aborted {} ", key); return DeferredChanged.empty(); } diff --git a/core/src/test/java/org/openapitools/openapidiff/core/OneOfDiffTest.java b/core/src/test/java/org/openapitools/openapidiff/core/OneOfDiffTest.java index bc7b316e..0c63e7db 100644 --- a/core/src/test/java/org/openapitools/openapidiff/core/OneOfDiffTest.java +++ b/core/src/test/java/org/openapitools/openapidiff/core/OneOfDiffTest.java @@ -1,10 +1,28 @@ package org.openapitools.openapidiff.core; +import static io.swagger.v3.oas.models.PathItem.HttpMethod.POST; +import static org.assertj.core.api.Assertions.assertThat; +import static org.openapitools.openapidiff.core.ChangesResolver.getRequestBodyChangedSchema; +import static org.openapitools.openapidiff.core.ChangesResolver.getResponseBodyChangedSchema; import static org.openapitools.openapidiff.core.TestUtils.assertOpenApiAreEquals; import static org.openapitools.openapidiff.core.TestUtils.assertOpenApiBackwardIncompatible; import static org.openapitools.openapidiff.core.TestUtils.assertOpenApiChangedEndpoints; +import io.swagger.parser.OpenAPIParser; +import io.swagger.v3.oas.models.OpenAPI; +import io.swagger.v3.oas.models.Operation; +import io.swagger.v3.oas.models.media.IntegerSchema; +import io.swagger.v3.oas.models.media.Schema; +import io.swagger.v3.parser.core.models.ParseOptions; +import java.util.Map; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; +import org.openapitools.openapidiff.core.model.ChangedOpenApi; +import org.openapitools.openapidiff.core.model.ChangedSchema; +import org.openapitools.openapidiff.core.model.DiffResult; +import org.openapitools.openapidiff.core.model.schema.ChangedOneOfSchema; public class OneOfDiffTest { @@ -20,6 +38,184 @@ public class OneOfDiffTest { private final String OPENAPI_DOC10 = "unnamed_oneof_schema_1.yaml"; private final String OPENAPI_DOC11 = "oneOf_to_anyOf_1.yaml"; private final String OPENAPI_DOC12 = "oneOf_to_anyOf_2.yaml"; + private static final String MULTIPLE_ALIASES = "oneOf_discriminator-aliases_1.yaml"; + private static final String REORDERED_ALIASES = "oneOf_discriminator-aliases_2.yaml"; + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + public void testOneOfDiscriminatorMappingOrder(boolean response) { + OpenAPI oldSpec = readAliasSpec(MULTIPLE_ALIASES); + OpenAPI newSpec = readAliasSpec(REORDERED_ALIASES); + assertThat(compareAliases(oldSpec, newSpec, response).isChanged()) + .isEqualTo(DiffResult.NO_CHANGES); + assertThat(OpenApiCompare.fromSpecifications(newSpec, oldSpec).isChanged()) + .isEqualTo(DiffResult.NO_CHANGES); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + public void testOneOfDiscriminatorAliasAdded(boolean response) { + OpenAPI oldSpec = readAliasSpec(MULTIPLE_ALIASES); + OpenAPI newSpec = readAliasSpec(MULTIPLE_ALIASES); + getAliasMapping(oldSpec).remove("z-type"); + + ChangedOpenApi diff = compareAliases(oldSpec, newSpec, response); + assertThat(diff.isChanged()) + .isEqualTo(response ? DiffResult.INCOMPATIBLE : DiffResult.COMPATIBLE); + ChangedOneOfSchema oneOf = getOneOfChange(diff, response); + assertThat(oneOf.getIncreased()).containsOnlyKeys("z-type"); + assertThat(oneOf.getMissing()).isEmpty(); + assertThat(oneOf.getChanged()).isEmpty(); + assertThat(oneOf.getNewMapping()).containsOnlyKeys("z-type", "a-type", "b-type"); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + public void testOneOfDiscriminatorAliasRemoved(boolean response) { + OpenAPI oldSpec = readAliasSpec(MULTIPLE_ALIASES); + OpenAPI newSpec = readAliasSpec(MULTIPLE_ALIASES); + getAliasMapping(newSpec).remove("z-type"); + + ChangedOpenApi diff = compareAliases(oldSpec, newSpec, response); + assertThat(diff.isChanged()) + .isEqualTo(response ? DiffResult.COMPATIBLE : DiffResult.INCOMPATIBLE); + ChangedOneOfSchema oneOf = getOneOfChange(diff, response); + assertThat(oneOf.getMissing()).containsOnlyKeys("z-type"); + assertThat(oneOf.getIncreased()).isEmpty(); + assertThat(oneOf.getChanged()).isEmpty(); + assertThat(oneOf.getOldMapping()).containsOnlyKeys("z-type", "a-type", "b-type"); + } + + @ParameterizedTest + @CsvSource({"false, false", "false, true", "true, false", "true, true"}) + public void testOneOfDiscriminatorAliasTargetChanged(boolean response, boolean reordered) { + String location = reordered ? REORDERED_ALIASES : MULTIPLE_ALIASES; + OpenAPI oldSpec = readAliasSpec(location); + OpenAPI newSpec = readAliasSpec(location); + getAliasMapping(newSpec).put("z-type", "#/components/schemas/B"); + + ChangedOpenApi diff = compareAliases(oldSpec, newSpec, response); + assertThat(diff.isChanged()).isEqualTo(DiffResult.INCOMPATIBLE); + ChangedOneOfSchema oneOf = getOneOfChange(diff, response); + assertThat(oneOf.getChanged()).containsOnlyKeys("z-type"); + assertThat(oneOf.getIncreased()).isEmpty(); + assertThat(oneOf.getMissing()).isEmpty(); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + public void testOneOfSharedSchemaChangeReportedForEveryAlias(boolean response) { + OpenAPI oldSpec = readAliasSpec(MULTIPLE_ALIASES); + OpenAPI newSpec = readAliasSpec(MULTIPLE_ALIASES); + newSpec.getComponents().getSchemas().get("A").addProperty("value", new IntegerSchema()); + + ChangedOpenApi diff = compareAliases(oldSpec, newSpec, response); + assertThat(diff.isChanged()).isEqualTo(DiffResult.INCOMPATIBLE); + ChangedOneOfSchema oneOf = getOneOfChange(diff, response); + assertThat(oneOf.getChanged()).containsOnlyKeys("z-type", "a-type"); + assertThat(oneOf.getIncreased()).isEmpty(); + assertThat(oneOf.getMissing()).isEmpty(); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + public void testOneOfMultipleAliasesWithImplicitMapping(boolean response) { + OpenAPI oldSpec = readAliasSpec(MULTIPLE_ALIASES); + OpenAPI newSpec = readAliasSpec(REORDERED_ALIASES); + getAliasMapping(oldSpec).remove("b-type"); + getAliasMapping(newSpec).remove("b-type"); + getAliasMapping(newSpec).put("B", "#/components/schemas/B"); + + assertThat(compareAliases(oldSpec, newSpec, response).isChanged()) + .isEqualTo(DiffResult.NO_CHANGES); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + public void testOneOfRecursiveSchemaWithMultipleAliases(boolean response) { + OpenAPI oldSpec = readAliasSpec(MULTIPLE_ALIASES); + OpenAPI newSpec = readAliasSpec(REORDERED_ALIASES); + oldSpec + .getComponents() + .getSchemas() + .get("A") + .addProperty("child", new Schema<>().$ref("#/components/schemas/A")); + newSpec + .getComponents() + .getSchemas() + .get("A") + .addProperty("child", new Schema<>().$ref("#/components/schemas/A")); + newSpec.getComponents().getSchemas().get("A").addProperty("value", new IntegerSchema()); + + ChangedOpenApi diff = compareAliases(oldSpec, newSpec, response); + assertThat(diff.isChanged()).isEqualTo(DiffResult.INCOMPATIBLE); + assertThat(getOneOfChange(diff, response).getChanged()).containsOnlyKeys("z-type", "a-type"); + } + + private OpenAPI readAliasSpec(String location) { + return new OpenAPIParser().readLocation(location, null, new ParseOptions()).getOpenAPI(); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + public void testOneOfMutuallyRecursiveSchemasWithMultipleAliases(boolean response) { + OpenAPI oldSpec = readAliasSpec(MULTIPLE_ALIASES); + OpenAPI newSpec = readAliasSpec(REORDERED_ALIASES); + addMutualReferences(oldSpec); + addMutualReferences(newSpec); + newSpec.getComponents().getSchemas().get("A").addProperty("value", new IntegerSchema()); + + ChangedOpenApi diff = compareAliases(oldSpec, newSpec, response); + assertThat(diff.isChanged()).isEqualTo(DiffResult.INCOMPATIBLE); + assertThat(getOneOfChange(diff, response).getChanged()).containsKeys("z-type", "a-type"); + } + + private void addMutualReferences(OpenAPI spec) { + Schema schemaA = spec.getComponents().getSchemas().get("A"); + Schema schemaB = spec.getComponents().getSchemas().get("B"); + schemaA.setNullable(true); + schemaB.setNullable(true); + schemaA.addProperty("peer", new Schema<>().$ref("#/components/schemas/B")); + schemaB.addProperty("peer", new Schema<>().$ref("#/components/schemas/A")); + schemaA.addRequiredItem("peer"); + schemaB.addRequiredItem("peer"); + } + + private Map getAliasMapping(OpenAPI spec) { + return spec.getPaths() + .get("/state") + .getPost() + .getRequestBody() + .getContent() + .get("application/json") + .getSchema() + .getDiscriminator() + .getMapping(); + } + + private ChangedOpenApi compareAliases(OpenAPI oldSpec, OpenAPI newSpec, boolean response) { + if (response) { + moveRequestBodyToResponse(oldSpec); + moveRequestBodyToResponse(newSpec); + } + return OpenApiCompare.fromSpecifications(oldSpec, newSpec); + } + + private void moveRequestBodyToResponse(OpenAPI spec) { + Operation operation = spec.getPaths().get("/state").getPost(); + operation.getResponses().get("201").setContent(operation.getRequestBody().getContent()); + operation.setRequestBody(null); + } + + private ChangedOneOfSchema getOneOfChange(ChangedOpenApi diff, boolean response) { + ChangedSchema schema = + response + ? getResponseBodyChangedSchema(diff, POST, "/state", "201", "application/json") + : getRequestBodyChangedSchema(diff, POST, "/state", "application/json"); + assertThat(schema).isNotNull(); + assertThat(schema.getOneOfSchema()).isNotNull(); + return schema.getOneOfSchema(); + } @Test public void testDiffSame() { diff --git a/core/src/test/resources/oneOf_discriminator-aliases_1.yaml b/core/src/test/resources/oneOf_discriminator-aliases_1.yaml new file mode 100644 index 00000000..505381fa --- /dev/null +++ b/core/src/test/resources/oneOf_discriminator-aliases_1.yaml @@ -0,0 +1,42 @@ +openapi: 3.0.1 +info: + title: Multiple discriminator aliases + version: '1.0' +paths: + /state: + post: + requestBody: + required: true + content: + application/json: + schema: + oneOf: + - $ref: '#/components/schemas/A' + - $ref: '#/components/schemas/B' + discriminator: + propertyName: realtype + mapping: + z-type: '#/components/schemas/A' + a-type: '#/components/schemas/A' + b-type: '#/components/schemas/B' + responses: + '201': + description: OK +components: + schemas: + A: + type: object + required: [realtype, value] + properties: + realtype: + type: string + value: + type: string + B: + type: object + required: [realtype, value] + properties: + realtype: + type: string + value: + type: integer diff --git a/core/src/test/resources/oneOf_discriminator-aliases_2.yaml b/core/src/test/resources/oneOf_discriminator-aliases_2.yaml new file mode 100644 index 00000000..6e2f661e --- /dev/null +++ b/core/src/test/resources/oneOf_discriminator-aliases_2.yaml @@ -0,0 +1,42 @@ +openapi: 3.0.1 +info: + title: Multiple discriminator aliases + version: '1.0' +paths: + /state: + post: + requestBody: + required: true + content: + application/json: + schema: + oneOf: + - $ref: '#/components/schemas/A' + - $ref: '#/components/schemas/B' + discriminator: + propertyName: realtype + mapping: + a-type: '#/components/schemas/A' + z-type: '#/components/schemas/A' + b-type: '#/components/schemas/B' + responses: + '201': + description: OK +components: + schemas: + A: + type: object + required: [realtype, value] + properties: + realtype: + type: string + value: + type: string + B: + type: object + required: [realtype, value] + properties: + realtype: + type: string + value: + type: integer