Merge pull request #646 from swagger-api/fix/issue_643_master

Avoid duplicating definitions when they have the same name
swagger-api · Feb 14, 2018 · e8cb13e · e8cb13e
2 parents c2a3fcf + 5f19070
commit e8cb13e
Show file tree

Hide file tree

Showing 7 changed files with 85 additions and 20 deletions.
diff --git a/modules/swagger-parser/src/main/java/io/swagger/parser/processors/ExternalRefProcessor.java b/modules/swagger-parser/src/main/java/io/swagger/parser/processors/ExternalRefProcessor.java
@@ -7,13 +7,15 @@
 import io.swagger.models.properties.RefProperty;
 import io.swagger.models.refs.RefFormat;
 import io.swagger.parser.ResolverCache;
+import org.apache.commons.lang3.StringUtils;
 import org.slf4j.LoggerFactory;
 
 import java.net.URI;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 
+
 import static io.swagger.parser.util.RefUtils.computeDefinitionName;
 import static io.swagger.parser.util.RefUtils.isAnExternalRefFormat;
 
@@ -30,6 +32,7 @@ public ExternalRefProcessor(ResolverCache cache, Swagger swagger) {
 
     public String processRefToExternalDefinition(String $ref, RefFormat refFormat) {
         String renamedRef = cache.getRenamedRef($ref);
+
         if(renamedRef != null) {
             return renamedRef;
         }
@@ -50,20 +53,34 @@ public String processRefToExternalDefinition(String $ref, RefFormat refFormat) {
             definitions = new LinkedHashMap<>();
         }
 
-        final String possiblyConflictingDefinitionName = computeDefinitionName($ref, definitions.keySet());
+        final String possiblyConflictingDefinitionName = computeDefinitionName($ref);
 
+        String tryName = null;
         Model existingModel = definitions.get(possiblyConflictingDefinitionName);
 
         if (existingModel != null) {
             LOGGER.debug("A model for " + existingModel + " already exists");
             if(existingModel instanceof RefModel) {
                 // use the new model
                 existingModel = null;
+            }else{
+                //We add a number at the end of the definition name
+                int i = 2;
+                for (String name : definitions.keySet()) {
+                    if (name.equals(possiblyConflictingDefinitionName)) {
+                        tryName = possiblyConflictingDefinitionName + "_" + i;
+                        existingModel = definitions.get(tryName);
+                        i++;
+                    }
+                }
             }
         }
-        newRef = possiblyConflictingDefinitionName;
+        if (StringUtils.isNotBlank(tryName)){
+            newRef = tryName;
+        }else{
+            newRef = possiblyConflictingDefinitionName;
+        }
         cache.putRenamedRef($ref, newRef);
-
         if(existingModel == null) {
             // don't overwrite existing model reference
             swagger.addDefinition(newRef, model);

diff --git a/modules/swagger-parser/src/main/java/io/swagger/parser/util/RefUtils.java b/modules/swagger-parser/src/main/java/io/swagger/parser/util/RefUtils.java
@@ -13,7 +13,8 @@
 
 public class RefUtils {
 
-    public static String computeDefinitionName(String ref, Set<String> reserved) {
+    public static String computeDefinitionName(String ref) {
+
 
         final String[] refParts = ref.split("#/");
 
@@ -37,15 +38,9 @@ public static String computeDefinitionName(String ref, Set<String> reserved) {
             final String[] split = plausibleName.split("\\.");
             plausibleName = split[0];
         }
-        String tryName = plausibleName;
-
-        for (int i = 2; reserved.contains(tryName); i++) {
-            tryName = plausibleName + "_" + i;
-        }
 
-        return tryName;
+        return plausibleName;
     }
-
     public static boolean isAnExternalRefFormat(RefFormat refFormat) {
         return refFormat == RefFormat.URL || refFormat == RefFormat.RELATIVE;
     }

diff --git a/modules/swagger-parser/src/test/java/io/swagger/parser/SwaggerParserTest.java b/modules/swagger-parser/src/test/java/io/swagger/parser/SwaggerParserTest.java
@@ -1085,6 +1085,10 @@ public void readingSpecNodeShouldNotOverQuotingStringExample() throws Exception
     public void testRefNameConflicts() throws Exception {
         Swagger swagger = new SwaggerParser().read("./refs-name-conflict/a.yaml");
 
+        Yaml.prettyPrint(swagger);
+
+        assertTrue(swagger.getDefinitions().size() == 2);
+
         assertEquals("#/definitions/PersonObj", ((RefProperty) swagger.getPath("/newPerson").getPost().getResponses().get("200").getSchema()).get$ref());
         assertEquals("#/definitions/PersonObj_2", ((RefProperty) swagger.getPath("/oldPerson").getPost().getResponses().get("200").getSchema()).get$ref());
         assertEquals("#/definitions/PersonObj_2", ((RefProperty) swagger.getPath("/yetAnotherPerson").getPost().getResponses().get("200").getSchema()).get$ref());
@@ -1112,13 +1116,23 @@ public void testRefEnum() throws Exception {
         Assert.assertNotNull(swagger);
 
         Assert.assertTrue(swagger.getDefinitions().size() == 5);
-        Yaml.prettyPrint(swagger);
 
         Assert.assertNotNull(swagger.getDefinitions().get("PrintInfo"));
         Assert.assertNotNull(swagger.getDefinitions().get("SomeEnum"));
         Assert.assertNotNull(swagger.getDefinitions().get("ShippingInfo"));
     }
 
+    @Test
+    public void testIssue643() throws Exception {
+        Swagger swagger = new SwaggerParser().read("src/test/resources/issue_643.yaml");
+
+        Assert.assertNotNull(swagger);
+
+        Assert.assertTrue(swagger.getDefinitions().size() == 1);
+
+        Assert.assertNotNull(swagger.getDefinitions().get("XYZResponse"));
+
+    }
 
 
 }
diff --git a/...s/swagger-parser/src/test/java/io/swagger/parser/processors/ExternalRefProcessorTest.java b/...s/swagger-parser/src/test/java/io/swagger/parser/processors/ExternalRefProcessorTest.java
@@ -36,9 +36,9 @@ public void testProcessRefToExternalDefinition_NoNameConflict(
         final RefFormat refFormat = RefFormat.URL;
 
         new StrictExpectations() {{
-        	cache.getRenamedRef(ref);
-        	times = 1;
-        	result = null;
+			cache.getRenamedRef(ref);
+			times = 1;
+			result = null;
 
             cache.loadRef(ref, refFormat, Model.class);
             times = 1;
@@ -60,6 +60,8 @@ public void testProcessRefToExternalDefinition_NoNameConflict(
         assertEquals(newRef, "bar");
     }
 
+
+
     @Test
     public void testNestedExternalRefs(@Injectable final Model mockedModel){
     	final RefFormat refFormat = RefFormat.URL;

diff --git a/modules/swagger-parser/src/test/java/io/swagger/parser/util/RefUtilsTest.java b/modules/swagger-parser/src/test/java/io/swagger/parser/util/RefUtilsTest.java
@@ -64,14 +64,10 @@ public void testComputeDefinitionName() throws Exception {
         doComputeDefinitionNameTestCase("./path/to/file#/foo", "foo");
         doComputeDefinitionNameTestCase("./path/to/file#/foo/bar", "bar");
         doComputeDefinitionNameTestCase("./path/to/file#/foo/bar/hello", "hello");
-
-        // Name conflicts resolved by adding _number
-        assertEquals("file_2", RefUtils.computeDefinitionName("http://my.company.com/path/to/file.json", singleton("file")));
-        assertEquals("file_3", RefUtils.computeDefinitionName("http://my.company.com/path/to/file.json", new HashSet<>(asList("file", "file_2"))));
     }
 
     private void doComputeDefinitionNameTestCase(String ref, String expectedDefinitionName) {
-        assertEquals(expectedDefinitionName, RefUtils.computeDefinitionName(ref, Collections.<String>emptySet()));
+        assertEquals(expectedDefinitionName, RefUtils.computeDefinitionName(ref));
     }
 
     private Map<String, Model> createMap(String... keys) {

diff --git a/modules/swagger-parser/src/test/resources/issue_643.yaml b/modules/swagger-parser/src/test/resources/issue_643.yaml
@@ -0,0 +1,25 @@
+---
+swagger: '2.0'
+info:
+  version: 1.0.0
+  title: Sample Issue
+  description: API
+  termsOfService: TBD
+  contact:
+    name: API Team
+  license:
+    name: MIT
+host: xyz.com
+basePath: ''
+schemes:
+- https
+consumes:
+- application/json
+produces:
+- application/json
+paths:
+  "/xyz":
+    "$ref": "./xyz.yaml#/do_xyz"
+definitions:
+  XYZResponse:
+    "$ref": "./xyz.yaml#/definitions/XYZResponse"
diff --git a/modules/swagger-parser/src/test/resources/xyz.yaml b/modules/swagger-parser/src/test/resources/xyz.yaml
@@ -0,0 +1,16 @@
+---
+do_xyz:
+  get:
+    description: do xyz stuff
+    operationId: doXyz
+    produces:
+    - application/json
+    parameters: []
+    responses:
+      '200': {}
+      default: {}
+definitions:
+  XYZResponse:
+    properties:
+      foo:
+        type: string