Preserve all discriminator aliases when comparing oneOf schemas

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 <T extends Schema<X>, X> DeferredChanged<ChangedSchema> diff(
                 getSchema(leftComponents, leftMapping, leftComposedSchema),
                 getSchema(rightComponents, rightMapping, rightComposedSchema));
         Map<String, ChangedSchema> 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<ChangedSchema> 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<String, Schema> getSchema(
   }
 
   private Map<String, String> getMapping(ComposedSchema composedSchema) {
-    Map<String, String> reverseMapping = new LinkedHashMap<>();
+    Map<String, String> 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<String> 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<String, String> 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<String, Schema> getUnnamedSchemas(List<Schema> schemas, String name) {

core/src/main/java/org/openapitools/openapidiff/core/model/deferred/DeferredSchemaCache.java

@@ -42,8 +42,18 @@ public SchemaDiffOperation addSchema(
 
   public DeferredChanged<ChangedSchema> 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<ChangedSchema> 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();
     }

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<String, String> 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() {

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