Merge pull request #590 from joeljons/ref-name-conflicts

frantuma · web-flow · commit 6be4df3b625e · 2017-11-22T18:33:07.000+01:00
$ref name conflicts for Swagger 2
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
@@ -29,6 +29,11 @@ public ExternalRefProcessor(ResolverCache cache, Swagger swagger) {
     }
 
     public String processRefToExternalDefinition(String $ref, RefFormat refFormat) {
+        String renamedRef = cache.getRenamedRef($ref);
+        if(renamedRef != null) {
+            return renamedRef;
+        }
+
         final Model model = cache.loadRef($ref, refFormat, Model.class);
 
         if(model == null) {
@@ -45,7 +50,7 @@ public String processRefToExternalDefinition(String $ref, RefFormat refFormat) {
             definitions = new LinkedHashMap<>();
         }
 
-        final String possiblyConflictingDefinitionName = computeDefinitionName($ref);
+        final String possiblyConflictingDefinitionName = computeDefinitionName($ref, definitions.keySet());
 
         Model existingModel = definitions.get(possiblyConflictingDefinitionName);
 
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
@@ -9,10 +9,11 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.List;
+import java.util.Set;
 
 public class RefUtils {
 
-    public static String computeDefinitionName(String ref) {
+    public static String computeDefinitionName(String ref, Set<String> reserved) {
 
         final String[] refParts = ref.split("#/");
 
@@ -36,8 +37,13 @@ public static String computeDefinitionName(String ref) {
             final String[] split = plausibleName.split("\\.");
             plausibleName = split[0];
         }
+        String tryName = plausibleName;
 
-        return plausibleName;
+        for (int i = 2; reserved.contains(tryName); i++) {
+            tryName = plausibleName + "_" + i;
+        }
+
+        return tryName;
     }
 
     public static boolean isAnExternalRefFormat(RefFormat refFormat) {
diff --git a/modules/swagger-parser/src/test/java/io/swagger/parser/JsonToYamlFileDuplicator.java b/modules/swagger-parser/src/test/java/io/swagger/parser/JsonToYamlFileDuplicator.java
@@ -42,8 +42,8 @@ private static void processFile(File next, Path inputDirectory, Path outputDirec
 
             final JsonNode jsonNode = DeserializationUtils.deserializeIntoTree(fileContents, next.toString());
 
-            final String yamlOutput = Yaml.mapper().writerWithDefaultPrettyPrinter().writeValueAsString(jsonNode);
-
+            final String yamlOutput = Yaml.mapper().writerWithDefaultPrettyPrinter().writeValueAsString(jsonNode)
+                    .replaceAll("\\n", System.getProperty("line.separator"));
 
             final String relativePath = "./" + next.toString().replace(inputDirectory.toString(), "").replace(".json", ".yaml");
 
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
@@ -220,7 +220,9 @@ public void testLoadExternalNestedDefinitions() throws Exception {
         assertTrue(definitions.containsKey("x"));
         assertTrue(definitions.containsKey("y"));
         assertTrue(definitions.containsKey("z"));
-        assertEquals(((RefModel) definitions.get("i")).get$ref(), "#/definitions/k");
+        assertEquals("#/definitions/k_2", ((RefModel) definitions.get("i")).get$ref());
+        assertEquals("k-definition", definitions.get("k").getTitle());
+        assertEquals("k-definition", definitions.get("k_2").getTitle());
     }
 
     @Test
@@ -613,7 +615,7 @@ private Swagger doRelativeFileTest(String location) {
 
         assertEquals(composedCat.getInterfaces().size(), 2);
         assertEquals(composedCat.getInterfaces().get(0).get$ref(), "#/definitions/pet");
-        assertEquals(composedCat.getInterfaces().get(1).get$ref(), "#/definitions/foo");
+        assertEquals(composedCat.getInterfaces().get(1).get$ref(), "#/definitions/foo_2");
 
         return swagger;
     }
@@ -1013,4 +1015,15 @@ public void readingSpecNodeShouldNotOverQuotingStringExample() throws Exception
         Map<String, Model> definitions = swagger.getDefinitions();
         assertEquals("NoQuotePlease", definitions.get("CustomerType").getExample());
     }
+
+    @Test
+    public void testRefNameConflicts() throws Exception {
+        Swagger swagger = new SwaggerParser().read("./refs-name-conflict/a.yaml");
+
+        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());
+        assertEquals("local", swagger.getDefinitions().get("PersonObj").getProperties().get("location").getExample());
+        assertEquals("referred", swagger.getDefinitions().get("PersonObj_2").getProperties().get("location").getExample());
+    }
 }
diff --git a/modules/swagger-parser/src/test/java/io/swagger/parser/processors/ExternalRefProcessorTest.java b/modules/swagger-parser/src/test/java/io/swagger/parser/processors/ExternalRefProcessorTest.java
@@ -36,6 +36,10 @@ public void testProcessRefToExternalDefinition_NoNameConflict(
         final RefFormat refFormat = RefFormat.URL;
 
         new StrictExpectations() {{
+        	cache.getRenamedRef(ref);
+        	times = 1;
+        	result = null;
+
             cache.loadRef(ref, refFormat, Model.class);
             times = 1;
             result = mockedModel;
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
@@ -16,10 +16,14 @@
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 
+import static java.util.Arrays.asList;
+import static java.util.Collections.singleton;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertFalse;
 import static org.testng.Assert.assertTrue;
@@ -61,10 +65,13 @@ public void testComputeDefinitionName() throws Exception {
         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));
+        assertEquals(expectedDefinitionName, RefUtils.computeDefinitionName(ref, Collections.<String>emptySet()));
     }
 
     private Map<String, Model> createMap(String... keys) {
diff --git a/modules/swagger-parser/src/test/resources/nested-references/b.yaml b/modules/swagger-parser/src/test/resources/nested-references/b.yaml
@@ -11,6 +11,7 @@ definitions:
     $ref: "./a.yaml#/definitions/j"
   k:
     type: object
+    title: k-definition
     properties:
       name:
         type: string
diff --git a/modules/swagger-parser/src/test/resources/refs-name-conflict/a.yaml b/modules/swagger-parser/src/test/resources/refs-name-conflict/a.yaml
@@ -0,0 +1,43 @@
+swagger: '2.0'
+info:
+  version: 1.0.0
+  title: Person
+  description: Maintain Person data
+paths:
+ /newPerson:
+  post:
+    summary: Create new person
+    description: Create new person
+    parameters: []
+    responses:
+      200:
+       description: OK
+       schema:
+         $ref: '#/definitions/PersonObj'
+ /oldPerson:
+  post:
+    summary: Create old person
+    description: Create old person
+    parameters: []
+    responses:
+      200:
+       description: OK
+       schema:
+         $ref: "./refs-name-conflict/b.yaml#/definitions/PersonObj"
+ /yetAnotherPerson:
+  post:
+    summary: Create yet another person
+    description: Create yet another person
+    parameters: []
+    responses:
+      200:
+       description: OK
+       schema:
+         $ref: "./refs-name-conflict/b.yaml#/definitions/PersonObj"
+
+definitions:
+ PersonObj:
+  properties:
+    location:
+      type: string
+      example: local
diff --git a/modules/swagger-parser/src/test/resources/refs-name-conflict/b.yaml b/modules/swagger-parser/src/test/resources/refs-name-conflict/b.yaml
@@ -0,0 +1,7 @@
+---
+definitions:
+ PersonObj:
+  properties:
+    location:
+      type: string
+      example: referred
