Fix issue swagger-api#1501: External schema name is correctly constru…

…cted based on all existing models
BabisK · Dec 3, 2020 · ccff482 · ccff482
1 parent f5fdccb
commit ccff482
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 30 deletions.
diff --git a/...swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ExternalRefProcessor.java b/...swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ExternalRefProcessor.java
@@ -49,6 +49,23 @@ public ExternalRefProcessor(ResolverCache cache, OpenAPI openAPI) {
         this.openAPI = openAPI;
     }
 
+    private String finalNameRec(Map<String, Schema> schemas, String possiblyConflictingDefinitionName, Schema newScema,
+        int iteration) {
+        String tryName =
+            iteration == 0 ? possiblyConflictingDefinitionName : possiblyConflictingDefinitionName + "_" + iteration;
+        Schema existingModel = schemas.get(tryName);
+        if (existingModel != null) {
+            if (existingModel.get$ref() != null) {
+                // use the new model
+                existingModel = null;
+            } else if (!newScema.equals(existingModel)) {
+                LOGGER.debug("A model for " + existingModel + " already exists");
+                return finalNameRec(schemas, possiblyConflictingDefinitionName, newScema, ++iteration);
+            }
+        }
+        return tryName;
+    }
+
     public String processRefToExternalSchema(String $ref, RefFormat refFormat) {
         String renamedRef = cache.getRenamedRef($ref);
         if(renamedRef != null) {
@@ -75,34 +92,13 @@ public String processRefToExternalSchema(String $ref, RefFormat refFormat) {
         }
 
         final String possiblyConflictingDefinitionName = computeDefinitionName($ref);
-        String tryName = null;
-        Schema existingModel = schemas.get(possiblyConflictingDefinitionName);
-
-        if (existingModel != null) {
-            LOGGER.warn("A model for " + existingModel + " already exists");
-            if(existingModel.get$ref() != null) {
-                // use the new model
-                existingModel = null;
-            }else{
-                if (!schema.equals(existingModel)){
-                    //We add a number at the end of the definition name
-                    int i = 2;
-                    for (String name : schemas.keySet()) {
-                        if (name.equals(possiblyConflictingDefinitionName)) {
-                            tryName = possiblyConflictingDefinitionName + "_" + i;
-                            existingModel = schemas.get(tryName);
-                            i++;
-                        }
-                    }
-                }
-            }
-        }
-        if (StringUtils.isNotBlank(tryName)){
-            newRef = tryName;
-        }else{
-            newRef = possiblyConflictingDefinitionName;
-        }
+        newRef = finalNameRec(schemas, possiblyConflictingDefinitionName, schema, 0);
         cache.putRenamedRef($ref, newRef);
+        Schema existingModel = schemas.get(newRef);
+        if(existingModel != null && existingModel.get$ref() != null) {
+            // use the new model
+            existingModel = null;
+        }
 
         if(existingModel == null) {
             // don't overwrite existing model reference

diff --git a/modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/test/OpenAPIResolverTest.java b/modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/test/OpenAPIResolverTest.java
@@ -777,7 +777,8 @@ public void testRefNameConflicts() throws Exception {
         assertEquals("referred", ((Schema)openAPI.getPaths().get("/oldPerson").getPost().getResponses().get("200").getContent().get("*/*").getSchema().getProperties().get("location")).getExample());
         assertEquals("referred", ((Schema)openAPI.getPaths().get("/yetAnotherPerson").getPost().getResponses().get("200").getContent().get("*/*").getSchema().getProperties().get("location")).getExample());
         assertEquals("local", ((Schema) openAPI.getComponents().getSchemas().get("PersonObj").getProperties().get("location")).getExample());
-        assertEquals("referred", ((Schema) openAPI.getComponents().getSchemas().get("PersonObj_2").getProperties().get("location")).getExample());
+        assertEquals("referred", ((Schema) openAPI.getComponents().getSchemas().get("PersonObj_1").getProperties().get("location")).getExample());
+        assertEquals("referred-again", ((Schema) openAPI.getComponents().getSchemas().get("PersonObj_2").getProperties().get("location")).getExample());
     }
 
 

diff --git a/modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/test/OpenAPIV3ParserTest.java b/modules/swagger-parser-v3/src/test/java/io/swagger/v3/parser/test/OpenAPIV3ParserTest.java
@@ -1082,7 +1082,7 @@ public void testOneOfExternalRefConflictName() throws Exception {
         Schema pet = openAPI.getComponents().getSchemas().get("Pet");
         Assert.assertNotNull(pet);
         Assert.assertTrue(pet.getDiscriminator().getMapping().containsKey("Cat"));
-        Assert.assertTrue(pet.getDiscriminator().getMapping().get("Cat").equals("#/components/schemas/Cat_2"));
+        Assert.assertTrue(pet.getDiscriminator().getMapping().get("Cat").equals("#/components/schemas/Cat_1"));
     }
 
     @Test
@@ -1687,7 +1687,7 @@ private OpenAPI doRelativeFileTest(String location) {
 
         assertEquals(composedCat.getAllOf().size(), 3);
         assertEquals(composedCat.getAllOf().get(0).get$ref(), "#/components/schemas/pet");
-        assertEquals(composedCat.getAllOf().get(1).get$ref(), "#/components/schemas/foo_2");
+        assertEquals(composedCat.getAllOf().get(1).get$ref(), "#/components/schemas/foo_1");
 
         return openAPI;
     }

diff --git a/modules/swagger-parser-v3/src/test/resources/refs-name-conflict/a.yaml b/modules/swagger-parser-v3/src/test/resources/refs-name-conflict/a.yaml
@@ -38,6 +38,17 @@ paths:
             '*/*':
               schema:
                 $ref: './refs-name-conflict/b.yaml#/components/schemas/PersonObj'
+  /thisAintAnotherPerson:
+    post:
+      summary: Create no more persons
+      description: Create no more persons
+      responses:
+        '200':
+          description: OK
+          content:
+            '*/*':
+              schema:
+                $ref: './refs-name-conflict/c.yaml#/components/schemas/PersonObj'
 components:
   schemas:
     PersonObj:

diff --git a/modules/swagger-parser-v3/src/test/resources/refs-name-conflict/c.yaml b/modules/swagger-parser-v3/src/test/resources/refs-name-conflict/c.yaml
@@ -0,0 +1,13 @@
+openapi: 3.0.1
+servers: []
+info:
+  version: ''
+  title: ''
+paths: {}
+components:
+  schemas:
+    PersonObj:
+      properties:
+        location:
+          type: string
+          example: referred-again