diff --git a/smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/JsonSchemaConverter.java b/smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/JsonSchemaConverter.java index e4320bd8594..137ae69c1d9 100644 --- a/smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/JsonSchemaConverter.java +++ b/smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/JsonSchemaConverter.java @@ -37,6 +37,8 @@ */ public final class JsonSchemaConverter { + private static final RefStrategy DEFAULT_REF_STRATEGY = RefStrategy.createDefaultStrategy(); + /** All converters use the built-in mappers. */ private final List mappers = new ArrayList<>(); @@ -44,6 +46,7 @@ public final class JsonSchemaConverter { private ObjectNode config = Node.objectNode(); private RefStrategy refStrategy; private Predicate 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)); diff --git a/smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/RefStrategy.java b/smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/RefStrategy.java index 0c40e7f5f58..b427bb2f95f 100644 --- a/smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/RefStrategy.java +++ b/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. + * + *

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 pointers = new HashMap<>(); + Map 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)); + } } diff --git a/smithy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/JsonSchemaConverterTest.java b/smithy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/JsonSchemaConverterTest.java index cd799e2b53e..755ff302730 100644 --- a/smithy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/JsonSchemaConverterTest.java +++ b/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")); + } } diff --git a/smithy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/RefStrategyTest.java b/smithy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/RefStrategyTest.java index bcb2be90cce..5462825f7c3 100644 --- a/smithy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/RefStrategyTest.java +++ b/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")); + } }