Fix an issue where JSON schema member names collided

smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/JsonSchemaConverter.java

@@ -37,13 +37,16 @@
  */
 public final class JsonSchemaConverter {
 
+    private static final RefStrategy DEFAULT_REF_STRATEGY = RefStrategy.createDefaultStrategy();
+
     /** All converters use the built-in mappers. */
     private final List<JsonSchemaMapper> mappers = new ArrayList<>();
 
     private PropertyNamingStrategy propertyNamingStrategy;
     private ObjectNode config = Node.objectNode();
     private RefStrategy refStrategy;
     private Predicate<Shape> shapePredicate = shape -> true;
+    private boolean softRefStrategy = false;
 
     private JsonSchemaConverter() {
         mappers.add(new DisableMapper());
@@ -142,6 +145,7 @@ public PropertyNamingStrategy getPropertyNamingStrategy() {
      * @return Returns the converter.
      */
     public JsonSchemaConverter refStrategy(RefStrategy refStrategy) {
+        softRefStrategy = false;
         this.refStrategy = refStrategy;
         return this;
     }
@@ -152,10 +156,7 @@ public JsonSchemaConverter refStrategy(RefStrategy refStrategy) {
      * @return Reference naming strategy to use.
      */
     public RefStrategy getRefStrategy() {
-        if (refStrategy == null) {
-            refStrategy = RefStrategy.createDefaultStrategy();
-        }
-        return refStrategy;
+        return refStrategy != null ? refStrategy : DEFAULT_REF_STRATEGY;
     }
 
     /**
@@ -194,6 +195,45 @@ public SchemaDocument convert(ShapeIndex shapeIndex, Shape shape) {
     }
 
     private SchemaDocument doConversion(ShapeIndex shapeIndex, Shape rootShape) {
+        // TODO: break this API to make this work more reliably.
+        //
+        // Temporarily set a de-conflicting ref strategy. This is the
+        // minimal change needed to fix a reported bug, but it is a hack
+        // and we should break this API before GA.
+        //
+        // We want to do the right thing by default. However, the default
+        // that was previously chosen didn't account for the possibility
+        // of shape name conflicts when converting members to JSON schema
+        // pointers. For example, consider the following shapes:
+        //
+        // - A member of a list, foo.baz#PageScripts$member
+        // - A member of a structure, foo.baz#Page$scripts
+        //
+        // If we only rely on the RefStrategy#createDefaultStrategy, then
+        // we would actually generate the same JSON schema shape name for
+        // both of the above member shapes: FooBazPageScriptsMember. To
+        // avoid this, we need to know the shape index being converted and
+        // automatically handle conflicts. However, because this class is
+        // mutable, we have to do some funky stuff with state to "do the
+        // right thing" by lazily creating a RefStrategy#createDefaultDeconflictingStrategy
+        // in this method once a ShapeIndex is available
+        // (given as an argument).
+        //
+        // A better API would use a builder that builds a JsonSchemaConverter
+        // that has a fixed shape index, ref strategy, etc. This would allow
+        // ref strategies to do more up-front computations, and allow them to
+        // even become implementation details of JsonSchemaConverter by exposing
+        // a similar API that delegates from a converter into the strategy.
+        //
+        // There's also quite a bit of awkward code in the OpenAPI conversion code
+        // base that tries to deal with merging configuration values and deriving
+        // a default JsonSchemaConverter if one isn't set. A better API there would
+        // be to not even allow the injection of a custom JsonSchemaConverter at all.
+        if (softRefStrategy || refStrategy == null) {
+            softRefStrategy = true;
+            refStrategy = RefStrategy.createDefaultDeconflictingStrategy(shapeIndex, getConfig());
+        }
+
         // Combine custom mappers with the discovered mappers and sort them.
         mappers.sort(Comparator.comparing(JsonSchemaMapper::getOrder));
 

smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/RefStrategy.java

@@ -15,9 +15,12 @@
 
 package software.amazon.smithy.jsonschema;
 
+import java.util.HashMap;
 import java.util.Locale;
+import java.util.Map;
 import software.amazon.smithy.model.node.ObjectNode;
 import software.amazon.smithy.model.shapes.ShapeId;
+import software.amazon.smithy.model.shapes.ShapeIndex;
 import software.amazon.smithy.utils.StringUtils;
 
 /**
@@ -38,6 +41,21 @@ public interface RefStrategy {
      */
     String toPointer(ShapeId id, ObjectNode config);
 
+    /**
+     * Creates a default ref strategy that calls a delegate and deconflicts
+     * pointers by appending an incrementing number to conflicting
+     * IDs.
+     *
+     * @param index Shape index to use to deconflict shapes.
+     * @param config JSON schema configuration to use.
+     * @return Returns the created strategy.
+     * @see #createDefaultStrategy()
+     * @see #createDeconflictingStrategy(ShapeIndex, ObjectNode, RefStrategy)
+     */
+    static RefStrategy createDefaultDeconflictingStrategy(ShapeIndex index, ObjectNode config) {
+        return createDeconflictingStrategy(index, config, createDefaultStrategy());
+    }
+
     /**
      * Creates a default strategy for converting shape IDs to $refs.
      *
@@ -92,4 +110,43 @@ static RefStrategy createDefaultStrategy() {
             return builder.toString();
         };
     }
+
+    /**
+     * Creates a ref strategy that calls a delegate and de-conflicts
+     * pointers by appending an incrementing number to conflicting
+     * IDs.
+     *
+     * <p>To make the generated IDs deterministic, shapes are sorted by shape
+     * ID when performing comparisons.
+     *
+     * @param index Shape index to use to de-conflict shapes.
+     * @param config JSON schema configuration to use.
+     * @param delegate Ref strategy to call to and then de-conflict.
+     * @return Returns the created strategy.
+     */
+    static RefStrategy createDeconflictingStrategy(ShapeIndex index, ObjectNode config, RefStrategy delegate) {
+        Map<ShapeId, String> pointers = new HashMap<>();
+        Map<String, ShapeId> reversePointers = new HashMap<>();
+
+        index.shapes().sorted().forEach(shape -> {
+            String pointer = delegate.toPointer(shape.getId(), config);
+
+            if (!reversePointers.containsKey(pointer)) {
+                pointers.put(shape.getId(), pointer);
+                reversePointers.put(pointer, shape.getId());
+                return;
+            }
+
+            for (int i = 2; ; i++) {
+                String incrementedPointer = pointer + i;
+                if (!reversePointers.containsKey(incrementedPointer)) {
+                    pointers.put(shape.getId(), incrementedPointer);
+                    reversePointers.put(incrementedPointer, shape.getId());
+                    return;
+                }
+            }
+        });
+
+        return (id, cfg) -> pointers.computeIfAbsent(id, i -> delegate.toPointer(i, cfg));
+    }
 }

smithy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/JsonSchemaConverterTest.java

@@ -20,6 +20,7 @@
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.not;
 import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -535,4 +536,43 @@ public void throwsForUnsupportUnionSetting() {
                     .convert(index, union);
         });
     }
+
+    @Test
+    public void dealsWithConflictsWithoutPollutingState() {
+        ShapeIndex index1 = Model.assembler()
+                .addImport(getClass().getResource("recursive.json"))
+                .assemble()
+                .unwrap()
+                .getShapeIndex();
+
+        StringShape stringShape = StringShape.builder().id("com.foo#String").build();
+        MemberShape pageScriptsListMember = MemberShape.builder()
+                .id("com.foo#PageScripts$member")
+                .target(stringShape)
+                .build();
+        ListShape pageScripts = ListShape.builder()
+                .id("com.foo#PageScripts")
+                .member(pageScriptsListMember)
+                .build();
+        MemberShape pageScriptsMember = MemberShape.builder()
+                .id("com.foo#Page$scripts")
+                .target(stringShape)
+                .build();
+        StructureShape page = StructureShape.builder()
+                .id("com.foo#Page")
+                .addMember(pageScriptsMember)
+                .build();
+        ShapeIndex index2 = ShapeIndex.builder()
+                .addShapes(page, pageScriptsMember, pageScripts, pageScriptsListMember, stringShape)
+                .build();
+
+        JsonSchemaConverter converter = JsonSchemaConverter.create();
+        SchemaDocument document1 = converter.convert(index1);
+        assertThat(document1.getDefinitions().keySet(), not(empty()));
+
+        SchemaDocument document2 = converter.convert(index2);
+        assertThat(document2.getDefinitions().keySet(), not(empty()));
+        assertThat(document2.getDefinitions().keySet(), hasItem("#/definitions/ComFooPageScriptsMember"));
+        assertThat(document2.getDefinitions().keySet(), hasItem("#/definitions/ComFooPageScriptsMember2"));
+    }
 }

smithy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/RefStrategyTest.java

@@ -21,7 +21,12 @@
 import org.junit.jupiter.api.Test;
 import software.amazon.smithy.model.node.Node;
 import software.amazon.smithy.model.node.ObjectNode;
+import software.amazon.smithy.model.shapes.ListShape;
+import software.amazon.smithy.model.shapes.MemberShape;
 import software.amazon.smithy.model.shapes.ShapeId;
+import software.amazon.smithy.model.shapes.ShapeIndex;
+import software.amazon.smithy.model.shapes.StringShape;
+import software.amazon.smithy.model.shapes.StructureShape;
 
 public class RefStrategyTest {
     @Test
@@ -72,4 +77,46 @@ public void defaultImplAddsRefWhenMember() {
 
         assertThat(pointer, equalTo("#/definitions/SmithyExampleFooBarMember"));
     }
+
+    @Test
+    public void canDeconflictNames() {
+        ObjectNode config = Node.objectNode();
+
+        StringShape stringShape = StringShape.builder().id("com.foo#String").build();
+        MemberShape pageScriptsListMember = MemberShape.builder()
+                .id("com.foo#PageScripts$member")
+                .target(stringShape)
+                .build();
+        ListShape pageScripts = ListShape.builder()
+                .id("com.foo#PageScripts")
+                .member(pageScriptsListMember)
+                .build();
+        MemberShape pageScriptsMember = MemberShape.builder()
+                .id("com.foo#Page$scripts")
+                .target(stringShape)
+                .build();
+        StructureShape page = StructureShape.builder()
+                .id("com.foo#Page")
+                .addMember(pageScriptsMember)
+                .build();
+
+        ShapeIndex index = ShapeIndex.builder()
+                .addShapes(page, pageScriptsMember, pageScripts, pageScriptsListMember, stringShape)
+                .build();
+
+        RefStrategy strategy = RefStrategy.createDefaultDeconflictingStrategy(index, config);
+        assertThat(strategy.toPointer(pageScriptsMember.getId(), config),
+                   equalTo("#/definitions/ComFooPageScriptsMember"));
+        assertThat(strategy.toPointer(pageScriptsListMember.getId(), config),
+                   equalTo("#/definitions/ComFooPageScriptsMember2"));
+    }
+
+    @Test
+    public void deconflictingStrategyPassesThroughToDelegate() {
+        ObjectNode config = Node.objectNode();
+        ShapeIndex index = ShapeIndex.builder().build();
+        RefStrategy strategy = RefStrategy.createDefaultDeconflictingStrategy(index, config);
+
+        assertThat(strategy.toPointer(ShapeId.from("com.foo#Nope"), config), equalTo("#/definitions/ComFooNope"));
+    }
 }