Resolve same-named schemas from different files as distinct components (fixes #2333)

modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/ResolverCache.java

@@ -38,6 +38,7 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -422,6 +423,10 @@ public void putRenamedRef(String originalRef, String newRef) {
         canonicalRenameCache.put(canonicalize(originalRef), newRef);
     }
 
+    public boolean refsAreEquivalent(String first, String second) {
+        return Objects.equals(canonicalize(first), canonicalize(second));
+    }
+
     private static String canonicalize(String ref) {
         if (ref == null || ref.isEmpty()) {
             return ref;

modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ExternalRefProcessor.java

@@ -55,31 +55,33 @@ public ExternalRefProcessor(ResolverCache cache, OpenAPI openAPI) {
     }
 
     private String finalNameRec(Map<String, Schema> schemas, String possiblyConflictingDefinitionName, Schema newSchema,
-        int iteration) {
+        int iteration, String incomingRef) {
         String tryName =
             iteration == 0 ? possiblyConflictingDefinitionName : possiblyConflictingDefinitionName + "_" + iteration;
         Schema existingModel = schemas.get(tryName);
+        if (existingModel == null) {
+            for (String name : schemas.keySet()) {
+                if (name.equalsIgnoreCase(tryName)) {
+                    existingModel = schemas.get(name);
+                    tryName = name;
+                    break;
+                }
+            }
+        }
         if (existingModel != null) {
             if (existingModel.get$ref() != null) {
+                if (incomingRef != null && !cache.refsAreEquivalent(existingModel.get$ref(), incomingRef)) {
+                    LOGGER.debug("A different external $ref already claims the name " + tryName);
+                    return finalNameRec(schemas, possiblyConflictingDefinitionName, newSchema, ++iteration, incomingRef);
+                }
                 // use the new model
                 existingModel = null;
             } else if (!newSchema.equals(existingModel)) {
                 if (cache.getRenamedRef(newSchema.get$ref()) != null) {
                     return tryName;
                 }
                 LOGGER.debug("A model for " + existingModel + " already exists");
-                return finalNameRec(schemas, possiblyConflictingDefinitionName, newSchema, ++iteration);
-            }
-        }else{
-            // validate the name
-            if(existingModel == null){
-                for(String name: schemas.keySet()){
-                    if(name.toLowerCase().equals(tryName.toLowerCase())){
-                        existingModel = schemas.get(name);
-                        tryName = name;
-                        break;
-                    }
-                }
+                return finalNameRec(schemas, possiblyConflictingDefinitionName, newSchema, ++iteration, incomingRef);
             }
         }
         return tryName;
@@ -110,7 +112,7 @@ public String processRefToExternalSchema(String $ref, RefFormat refFormat) {
         }
 
         final String possiblyConflictingDefinitionName = computeDefinitionName($ref);
-        newRef = finalNameRec(schemas, possiblyConflictingDefinitionName, schema, 0);
+        newRef = finalNameRec(schemas, possiblyConflictingDefinitionName, schema, 0, $ref);
         cache.putRenamedRef($ref, newRef);
         Schema existingModel = schemas.get(newRef);
        if(existingModel != null && existingModel.get$ref() != null) {

modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/test/Issue2333Test.java

@@ -0,0 +1,119 @@
+package io.swagger.v3.parser.test;
+
+import io.swagger.v3.oas.models.media.Schema;
+import io.swagger.v3.parser.OpenAPIV3Parser;
+import io.swagger.v3.parser.core.models.ParseOptions;
+import io.swagger.v3.parser.core.models.SwaggerParseResult;
+import org.testng.annotations.Test;
+
+import java.util.Arrays;
+import java.util.Map;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertSame;
+import static org.testng.Assert.assertTrue;
+
+public class Issue2333Test {
+
+    /**
+     * Two schemas defined in different files but sharing the same name (both {@code Pet}) must be resolved as
+     * distinct components. Previously the resolver reused the first model for both references, so a schema that
+     * was still an unresolved external {@code $ref} placeholder was silently overwritten by an unrelated one.
+     */
+    @Test
+    public void referencedModelsWithSameNameFromDifferentFilesAreNotMerged() {
+        Map<String, Schema> schemas = parse("main.yaml");
+
+        assertEquals(schemas.size(), 3);
+        assertTrue(schemas.keySet().containsAll(Arrays.asList("SomeItem", "Pet", "Pet_1")));
+        assertSchemaHasOnlyProperty(resolve(schemas, schemas.get("Pet")), "id");
+        assertSchemaHasOnlyProperty(schemas.get("Pet_1"), "name");
+        assertSame(schemas.get("SomeItem"), schemas.get("Pet_1"));
+        assertSchemaHasOnlyProperty(resolve(schemas, schemas.get("SomeItem")), "name");
+    }
+
+    @Test
+    public void identicalModelsWithSameNameFromDifferentFilesReuseSingleComponent() {
+        Map<String, Schema> schemas = parse("identical-models-different-files.yaml");
+
+        assertEquals(schemas.size(), 3);
+        assertTrue(schemas.keySet().containsAll(Arrays.asList("FirstPet", "SecondPet", "Pet")));
+        assertFalse(schemas.containsKey("Pet_1"));
+        assertSame(schemas.get("FirstPet"), schemas.get("Pet"));
+        assertSame(schemas.get("SecondPet"), schemas.get("Pet"));
+        assertSchemaHasOnlyProperty(resolve(schemas, schemas.get("FirstPet")), "id");
+        assertSchemaHasOnlyProperty(resolve(schemas, schemas.get("SecondPet")), "id");
+    }
+
+    @Test
+    public void equivalentRefsWithDifferentSpellingReuseThePlaceholderName() {
+        Map<String, Schema> schemas = parse("equivalent-refs.yaml");
+
+        assertEquals(schemas.size(), 2);
+        assertTrue(schemas.containsKey("Thing"));
+        assertFalse(schemas.containsKey("Thing_1"));
+        assertSame(schemas.get("ThingAlias"), schemas.get("Thing"));
+        assertSchemaHasOnlyProperty(resolve(schemas, schemas.get("ThingAlias")), "value");
+    }
+
+    @Test
+    public void reversedTraversalPreservesDistinctTargetsAndCollapsesEquivalentRefs() {
+        Map<String, Schema> schemas = parse("reversed.yaml");
+
+        assertEquals(schemas.size(), 4);
+        assertTrue(schemas.keySet().containsAll(Arrays.asList("Pet", "SomeItem", "PetAlias", "Pet_1")));
+        assertFalse(schemas.containsKey("Pet_2"));
+        assertSchemaHasOnlyProperty(resolve(schemas, schemas.get("Pet")), "id");
+        assertSchemaHasOnlyProperty(schemas.get("Pet_1"), "name");
+        assertSame(schemas.get("SomeItem"), schemas.get("Pet_1"));
+        assertSame(schemas.get("PetAlias"), schemas.get("Pet"));
+        assertSchemaHasOnlyProperty(resolve(schemas, schemas.get("SomeItem")), "name");
+        assertSchemaHasOnlyProperty(resolve(schemas, schemas.get("PetAlias")), "id");
+    }
+
+    @Test
+    public void caseInsensitiveNameCollisionPreservesDistinctTargets() {
+        Map<String, Schema> schemas = parse("case-insensitive.yaml");
+
+        assertEquals(schemas.size(), 3);
+        assertTrue(schemas.keySet().containsAll(Arrays.asList("SomeItem", "pet", "Pet_1")));
+        assertFalse(schemas.containsKey("Pet"));
+        assertSchemaHasOnlyProperty(resolve(schemas, schemas.get("pet")), "id");
+        assertSchemaHasOnlyProperty(schemas.get("Pet_1"), "name");
+        assertSame(schemas.get("SomeItem"), schemas.get("Pet_1"));
+        assertSchemaHasOnlyProperty(resolve(schemas, schemas.get("SomeItem")), "name");
+    }
+
+    private Map<String, Schema> parse(String fixture) {
+        ParseOptions options = new ParseOptions();
+        options.setResolve(true);
+        options.setResolveResponses(true);
+
+        SwaggerParseResult result = new OpenAPIV3Parser()
+                .readLocation("./src/test/resources/issue-2333/" + fixture, null, options);
+
+        assertNotNull(result.getOpenAPI());
+        assertTrue(result.getMessages().isEmpty(), "Unexpected parser messages: " + result.getMessages());
+        assertNotNull(result.getOpenAPI().getComponents());
+        assertNotNull(result.getOpenAPI().getComponents().getSchemas());
+        return result.getOpenAPI().getComponents().getSchemas();
+    }
+
+    private void assertSchemaHasOnlyProperty(Schema schema, String property) {
+        assertNotNull(schema);
+        assertNotNull(schema.getProperties());
+        assertEquals(schema.getProperties().size(), 1);
+        assertTrue(schema.getProperties().containsKey(property),
+                "Expected property " + property + ", but was " + schema.getProperties().keySet());
+    }
+
+    private Schema resolve(Map<String, Schema> schemas, Schema schema) {
+        if (schema != null && schema.get$ref() != null) {
+            String name = schema.get$ref().substring(schema.get$ref().lastIndexOf('/') + 1);
+            return schemas.get(name);
+        }
+        return schema;
+    }
+}

modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/test/ResolverCacheTest.java

@@ -27,9 +27,11 @@
 import java.util.List;
 
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
 import static org.testng.Assert.assertNotNull;
 import static org.testng.Assert.assertNull;
 import static org.testng.Assert.assertSame;
+import static org.testng.Assert.assertTrue;
 
 public class ResolverCacheTest {
 
@@ -347,6 +349,18 @@ public void testIssue2016EquivalentRelativeRefsShareRenameCacheIdentity() {
         assertNull(cache.getRenameCache().get(equivalentRef));
     }
 
+    @Test
+    public void testRefEquivalenceUsesNormalizedCacheIdentity() {
+        ResolverCache cache = new ResolverCache(openAPI, auths, null);
+
+        assertTrue(cache.refsAreEquivalent(
+                "./components/schemas/Thing.yaml#/components/schemas/Thing",
+                "components/foo/../schemas/Thing.yaml#/components/schemas/Thing"));
+        assertFalse(cache.refsAreEquivalent(
+                "inventory.yaml#/components/schemas/Pet",
+                "pets.yaml#/components/schemas/Pet"));
+    }
+
     @Test
     public void testCanonicalRenameCacheKeysPreserveUriParts() {
         ResolverCache cache = new ResolverCache(openAPI, auths, null);

modules/swagger-parser-v3/src/test/resources/issue-2333/case-insensitive.yaml

@@ -0,0 +1,11 @@
+openapi: 3.0.3
+info:
+  title: Case-insensitive collision
+  version: 1.0.0
+paths: {}
+components:
+  schemas:
+    SomeItem:
+      $ref: "inventory.yaml#/components/schemas/Pet"
+    pet:
+      $ref: "pets.yaml#/components/schemas/Pet"

modules/swagger-parser-v3/src/test/resources/issue-2333/components/schemas/Thing.yaml

@@ -0,0 +1,12 @@
+openapi: 3.0.3
+info:
+  title: Thing API
+  version: 1.0.0
+paths: {}
+components:
+  schemas:
+    Thing:
+      type: object
+      properties:
+        value:
+          type: string

modules/swagger-parser-v3/src/test/resources/issue-2333/equivalent-refs.yaml

@@ -0,0 +1,11 @@
+openapi: 3.0.3
+info:
+  title: Equivalent refs
+  version: 1.0.0
+paths: {}
+components:
+  schemas:
+    ThingAlias:
+      $ref: "./components/schemas/Thing.yaml#/components/schemas/Thing"
+    Thing:
+      $ref: "components/schemas/../schemas/Thing.yaml#/components/schemas/Thing"

modules/swagger-parser-v3/src/test/resources/issue-2333/identical-models-different-files.yaml

@@ -0,0 +1,11 @@
+openapi: 3.0.3
+info:
+  title: Identical models in different files
+  version: 1.0.0
+paths: {}
+components:
+  schemas:
+    FirstPet:
+      $ref: "identical-pet-a.yaml#/components/schemas/Pet"
+    SecondPet:
+      $ref: "identical-pet-b.yaml#/components/schemas/Pet"

modules/swagger-parser-v3/src/test/resources/issue-2333/identical-pet-a.yaml

@@ -0,0 +1,12 @@
+openapi: 3.0.3
+info:
+  title: Identical Pet A
+  version: 1.0.0
+paths: {}
+components:
+  schemas:
+    Pet:
+      type: object
+      properties:
+        id:
+          type: integer

modules/swagger-parser-v3/src/test/resources/issue-2333/identical-pet-b.yaml

@@ -0,0 +1,12 @@
+openapi: 3.0.3
+info:
+  title: Identical Pet B
+  version: 1.0.0
+paths: {}
+components:
+  schemas:
+    Pet:
+      type: object
+      properties:
+        id:
+          type: integer

modules/swagger-parser-v3/src/test/resources/issue-2333/inventory.yaml

@@ -0,0 +1,12 @@
+openapi: 3.0.3
+info:
+  title: Inventory API
+  version: 1.0.0
+paths: {}
+components:
+  schemas:
+    Pet:
+      type: object
+      properties:
+        name:
+          type: string

modules/swagger-parser-v3/src/test/resources/issue-2333/main.yaml

@@ -0,0 +1,11 @@
+openapi: 3.0.3
+info:
+  title: Pet Store API
+  version: 1.0.0
+paths: {}
+components:
+  schemas:
+    SomeItem:
+      $ref: "inventory.yaml#/components/schemas/Pet"
+    Pet:
+      $ref: "pets.yaml#/components/schemas/Pet"

modules/swagger-parser-v3/src/test/resources/issue-2333/pets.yaml

@@ -0,0 +1,12 @@
+openapi: 3.0.3
+info:
+  title: Pets API
+  version: 1.0.0
+paths: {}
+components:
+  schemas:
+    Pet:
+      type: object
+      properties:
+        id:
+          type: integer

modules/swagger-parser-v3/src/test/resources/issue-2333/reversed.yaml

@@ -0,0 +1,13 @@
+openapi: 3.0.3
+info:
+  title: Reversed collision order
+  version: 1.0.0
+paths: {}
+components:
+  schemas:
+    Pet:
+      $ref: "pets.yaml#/components/schemas/Pet"
+    SomeItem:
+      $ref: "inventory.yaml#/components/schemas/Pet"
+    PetAlias:
+      $ref: "./components/../pets.yaml#/components/schemas/Pet"