Specialize Unit conversion for JSONSchema

By always inlining Unit as an empty object rather than creating a pointer to a Unit schema, models will not need to rename either the built-in Unit or Units of their own in order to perform JSONSchema conversion.
smithy-lang · Jan 6, 2022 · 7b8eea1 · 7b8eea1
1 parent f00cdf7
commit 7b8eea1
Show file tree

Hide file tree

Showing 13 changed files with 295 additions and 22 deletions.
diff --git a/smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/DeconflictingStrategy.java b/smithy-jsonschema/src/main/java/software/amazon/smithy/jsonschema/DeconflictingStrategy.java
@@ -17,6 +17,7 @@
 
 import java.util.HashMap;
 import java.util.Map;
+import java.util.function.Predicate;
 import java.util.logging.Logger;
 import java.util.regex.Pattern;
 import software.amazon.smithy.model.Model;
@@ -49,12 +50,12 @@ final class DeconflictingStrategy implements RefStrategy {
     private final Map<ShapeId, String> pointers = new HashMap<>();
     private final Map<String, ShapeId> reversePointers = new HashMap<>();
 
-    DeconflictingStrategy(Model model, RefStrategy delegate) {
+    DeconflictingStrategy(Model model, RefStrategy delegate, Predicate<Shape> shapePredicate) {
         this.delegate = delegate;
 
         // Pre-compute a map of all converted shape refs. Sort the shapes
         // to make the result deterministic.
-        model.shapes().filter(FunctionalUtils.not(this::isIgnoredShape)).sorted().forEach(shape -> {
+        model.shapes().filter(shapePredicate.and(FunctionalUtils.not(this::isIgnoredShape))).sorted().forEach(shape -> {
             String pointer = delegate.toPointer(shape.getId());
             if (!reversePointers.containsKey(pointer)) {
                 pointers.put(shape.getId(), pointer);

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
@@ -33,6 +33,8 @@
 import software.amazon.smithy.model.shapes.Shape;
 import software.amazon.smithy.model.shapes.ShapeId;
 import software.amazon.smithy.model.shapes.ToShapeId;
+import software.amazon.smithy.model.shapes.UnionShape;
+import software.amazon.smithy.model.traits.UnitTypeTrait;
 import software.amazon.smithy.model.transform.ModelTransformer;
 import software.amazon.smithy.utils.FunctionalUtils;
 import software.amazon.smithy.utils.Pair;
@@ -63,6 +65,9 @@ public final class JsonSchemaConverter implements ToSmithyBuilder<JsonSchemaConv
     private final String rootDefinitionPointer;
     private final int rootDefinitionSegments;
 
+    /** A workaround for including definitions for Unit; it's only included in the schema if a union targets it. */
+    private final boolean unitTargetedByUnion;
+
     private JsonSchemaConverter(Builder builder) {
         mappers.addAll(builder.mappers);
         config = SmithyBuilder.requiredState("config", builder.config);
@@ -83,7 +88,12 @@ private JsonSchemaConverter(Builder builder) {
         LOGGER.fine("Creating JSON ref strategy");
         Model refModel = config.isEnableOutOfServiceReferences()
                 ? this.model : scopeModelToService(model, config.getService());
-        refStrategy = RefStrategy.createDefaultStrategy(refModel, config, propertyNamingStrategy);
+
+        unitTargetedByUnion = refModel.shapes(UnionShape.class)
+                .anyMatch(u -> u.members().stream().anyMatch(m -> m.getTarget().equals(UnitTypeTrait.UNIT)));
+
+        refStrategy = RefStrategy.createDefaultStrategy(refModel, config, propertyNamingStrategy,
+                new FilterPreludeUnit(unitTargetedByUnion));
 
         // Combine custom mappers with the discovered mappers and sort them.
         realizedMappers = new ArrayList<>(mappers);
@@ -250,7 +260,7 @@ public SchemaDocument convert() {
                 // Don't convert unsupported shapes.
                 .filter(FunctionalUtils.not(this::isUnsupportedShapeType))
                 // Ignore prelude shapes.
-                .filter(FunctionalUtils.not(Prelude::isPreludeShape))
+                .filter(s -> s.getId().equals(UnitTypeTrait.UNIT) && unitTargetedByUnion || !Prelude.isPreludeShape(s))
                 // Do not generate inlined shapes in the definitions map.
                 .filter(FunctionalUtils.not(refStrategy::isInlined))
                 // Create a pair of pointer and shape.
@@ -411,4 +421,17 @@ public Builder mappers(List<JsonSchemaMapper> jsonSchemaMappers) {
             return this;
         }
     }
+
+    static final class FilterPreludeUnit implements Predicate<Shape> {
+        private final boolean includePreludeUnit;
+
+        FilterPreludeUnit(boolean includePreludeUnit) {
+            this.includePreludeUnit = includePreludeUnit;
+        }
+
+        @Override
+        public boolean test(Shape shape) {
+            return includePreludeUnit || !shape.getId().equals(UnitTypeTrait.UNIT);
+        }
+    }
 }
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
@@ -15,6 +15,7 @@
 
 package software.amazon.smithy.jsonschema;
 
+import java.util.function.Predicate;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.shapes.Shape;
 import software.amazon.smithy.model.shapes.ShapeId;
@@ -70,14 +71,16 @@ interface RefStrategy {
      * @param model Model being converted.
      * @param config Conversion configuration.
      * @param propertyNamingStrategy Property naming strategy.
+     * @param shapePredicate a predicate to use to filter shapes in model when determining conflicts.
      * @return Returns the created strategy.
      */
     static RefStrategy createDefaultStrategy(
             Model model,
             JsonSchemaConfig config,
-            PropertyNamingStrategy propertyNamingStrategy
+            PropertyNamingStrategy propertyNamingStrategy,
+            Predicate<Shape> shapePredicate
     ) {
         RefStrategy delegate = new DefaultRefStrategy(model, config, propertyNamingStrategy);
-        return new DeconflictingStrategy(model, delegate);
+        return new DeconflictingStrategy(model, delegate, shapePredicate);
     }
 }
diff --git a/...jsonschema/src/test/java/software/amazon/smithy/jsonschema/DeconflictingStrategyTest.java b/...jsonschema/src/test/java/software/amazon/smithy/jsonschema/DeconflictingStrategyTest.java
@@ -2,6 +2,7 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
+import static software.amazon.smithy.utils.FunctionalUtils.alwaysTrue;
 
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
@@ -26,7 +27,7 @@ public void canDeconflictNamesWhereListsAreActuallyDifferent() {
 
         PropertyNamingStrategy propertyNamingStrategy = PropertyNamingStrategy.createDefaultStrategy();
         RefStrategy strategy = RefStrategy
-                .createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy);
+                .createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy, alwaysTrue());
         assertThat(strategy.toPointer(a.getId()), equalTo("#/definitions/Page"));
         assertThat(strategy.toPointer(b.getId()), equalTo("#/definitions/PageComFoo"));
     }
@@ -39,7 +40,7 @@ public void detectsUnsupportedConflicts() {
         PropertyNamingStrategy propertyNamingStrategy = PropertyNamingStrategy.createDefaultStrategy();
 
         Assertions.assertThrows(ConflictingShapeNameException.class, () -> {
-            RefStrategy.createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy);
+            RefStrategy.createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy, alwaysTrue());
         });
     }
 
@@ -48,8 +49,42 @@ public void deconflictingStrategyPassesThroughToDelegate() {
         Model model = Model.builder().build();
         PropertyNamingStrategy propertyNamingStrategy = PropertyNamingStrategy.createDefaultStrategy();
         RefStrategy strategy = RefStrategy
-                .createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy);
+                .createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy, alwaysTrue());
 
         assertThat(strategy.toPointer(ShapeId.from("com.foo#Nope")), equalTo("#/definitions/Nope"));
     }
+
+    @Test
+    public void detectsUnitConflictsWhenPreludeUnitIsNotFiltered() {
+        StructureShape a = StructureShape.builder().id("com.foo#Unit").build();
+        Model model = Model.assembler().addShapes(a).assemble().unwrap();
+        PropertyNamingStrategy propertyNamingStrategy = PropertyNamingStrategy.createDefaultStrategy();
+
+        Assertions.assertThrows(ConflictingShapeNameException.class, () -> {
+            RefStrategy.createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy, alwaysTrue());
+        });
+    }
+
+    @Test
+    public void doesNotDetectUnitConflictsWhenPreludeUnitIsFiltered() {
+        StructureShape a = StructureShape.builder().id("com.foo#Unit").build();
+        Model model = Model.assembler().addShapes(a).assemble().unwrap();
+        PropertyNamingStrategy propertyNamingStrategy = PropertyNamingStrategy.createDefaultStrategy();
+
+        RefStrategy.createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy,
+                new JsonSchemaConverter.FilterPreludeUnit(false));
+    }
+
+    @Test
+    public void detectsUnitConflictsWithNonPreludeUnitsNoMatterWhat() {
+        StructureShape a = StructureShape.builder().id("com.foo#Unit").build();
+        StructureShape b = StructureShape.builder().id("com.bar#Unit").build();
+        Model model = Model.assembler().addShapes(a, b).assemble().unwrap();
+        PropertyNamingStrategy propertyNamingStrategy = PropertyNamingStrategy.createDefaultStrategy();
+
+        Assertions.assertThrows(ConflictingShapeNameException.class, () -> {
+            RefStrategy.createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy,
+                    new JsonSchemaConverter.FilterPreludeUnit(false));
+        });
+    }
 }
diff --git a/...hy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/DefaultRefStrategyTest.java b/...hy-jsonschema/src/test/java/software/amazon/smithy/jsonschema/DefaultRefStrategyTest.java
@@ -2,6 +2,7 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
+import static software.amazon.smithy.utils.FunctionalUtils.alwaysTrue;
 
 import org.junit.jupiter.api.Test;
 import software.amazon.smithy.model.Model;
@@ -19,7 +20,7 @@ public class DefaultRefStrategyTest {
     @Test
     public void usesDefaultPointer() {
         RefStrategy ref = RefStrategy.createDefaultStrategy(
-                Model.builder().build(), new JsonSchemaConfig(), propertyNamingStrategy);
+                Model.builder().build(), new JsonSchemaConfig(), propertyNamingStrategy, alwaysTrue());
         String pointer = ref.toPointer(ShapeId.from("smithy.example#Foo"));
 
         assertThat(pointer, equalTo("#/definitions/Foo"));
@@ -30,7 +31,7 @@ public void usesCustomPointerAndAppendsSlashWhenNecessary() {
         JsonSchemaConfig config = new JsonSchemaConfig();
         config.setDefinitionPointer("#/components/schemas");
         RefStrategy ref = RefStrategy
-                .createDefaultStrategy(Model.builder().build(), config, propertyNamingStrategy);
+                .createDefaultStrategy(Model.builder().build(), config, propertyNamingStrategy, alwaysTrue());
         String pointer = ref.toPointer(ShapeId.from("smithy.example#Foo"));
 
         assertThat(pointer, equalTo("#/components/schemas/Foo"));
@@ -41,7 +42,7 @@ public void usesCustomPointerAndOmitsSlashWhenNecessary() {
         JsonSchemaConfig config = new JsonSchemaConfig();
         config.setDefinitionPointer("#/components/schemas");
         RefStrategy ref = RefStrategy
-                .createDefaultStrategy(Model.builder().build(), config, propertyNamingStrategy);
+                .createDefaultStrategy(Model.builder().build(), config, propertyNamingStrategy, alwaysTrue());
         String pointer = ref.toPointer(ShapeId.from("smithy.example#Foo"));
 
         assertThat(pointer, equalTo("#/components/schemas/Foo"));
@@ -52,7 +53,7 @@ public void stripsNonAlphanumericCharactersWhenRequested() {
         JsonSchemaConfig config = new JsonSchemaConfig();
         config.setAlphanumericOnlyRefs(true);
         RefStrategy ref = RefStrategy
-                .createDefaultStrategy(Model.builder().build(), config, propertyNamingStrategy);
+                .createDefaultStrategy(Model.builder().build(), config, propertyNamingStrategy, alwaysTrue());
         String pointer = ref.toPointer(ShapeId.from("smithy.example#Foo_Bar"));
 
         assertThat(pointer, equalTo("#/definitions/FooBar"));
@@ -71,7 +72,7 @@ public void addsListAndSetMembers() {
                 .build();
         Model model = Model.builder().addShapes(string, list, member).build();
         RefStrategy ref = RefStrategy
-                .createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy);
+                .createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy, alwaysTrue());
         String pointer = ref.toPointer(member.getId());
 
         assertThat(pointer, equalTo("#/definitions/Scripts/items"));
@@ -95,7 +96,7 @@ public void addsMapMembers() {
                 .build();
         Model model = Model.builder().addShapes(string, map, key, value).build();
         RefStrategy ref = RefStrategy
-                .createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy);
+                .createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy, alwaysTrue());
 
         assertThat(ref.toPointer(key.getId()), equalTo("#/definitions/Scripts/propertyNames"));
         assertThat(ref.toPointer(value.getId()), equalTo("#/definitions/Scripts/additionalProperties"));
@@ -114,7 +115,7 @@ public void addsStructureMembers() {
                 .build();
         Model model = Model.builder().addShapes(string, struct, member).build();
         RefStrategy ref = RefStrategy
-                .createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy);
+                .createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy, alwaysTrue());
 
         assertThat(ref.toPointer(struct.getId()), equalTo("#/definitions/Scripts"));
         assertThat(ref.toPointer(member.getId()), equalTo("#/definitions/Scripts/properties/pages"));
@@ -131,7 +132,7 @@ public void usesRefForStructureMembers() {
                 .build();
         Model model = Model.builder().addShapes(baz, bam).build();
         RefStrategy ref = RefStrategy
-                .createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy);
+                .createDefaultStrategy(model, new JsonSchemaConfig(), propertyNamingStrategy, alwaysTrue());
 
         assertThat(ref.toPointer(baz.getMember("bam").get().getId()), equalTo("#/definitions/Bam"));
     }
@@ -144,7 +145,7 @@ public void usesServiceRenames() {
                 .unwrap();
         JsonSchemaConfig config = new JsonSchemaConfig();
         config.setService(ShapeId.from("smithy.example#MyService"));
-        RefStrategy ref = RefStrategy.createDefaultStrategy(model, config, propertyNamingStrategy);
+        RefStrategy ref = RefStrategy.createDefaultStrategy(model, config, propertyNamingStrategy, alwaysTrue());
 
         assertThat(ref.toPointer(ShapeId.from("smithy.example#Widget")), equalTo("#/definitions/Widget"));
         assertThat(ref.toPointer(ShapeId.from("foo.example#Widget")), equalTo("#/definitions/FooWidget"));

diff --git a/smithy-jsonschema/src/test/resources/software/amazon/smithy/jsonschema/test-service.json b/smithy-jsonschema/src/test/resources/software/amazon/smithy/jsonschema/test-service.json
@@ -237,6 +237,9 @@
                 },
                 "b": {
                     "target": "smithy.api#String"
+                },
+                "c": {
+                    "target": "smithy.api#Unit"
                 }
             }
         }

diff --git a/...nschema/src/test/resources/software/amazon/smithy/jsonschema/test-service.jsonschema.json b/...nschema/src/test/resources/software/amazon/smithy/jsonschema/test-service.jsonschema.json
@@ -114,29 +114,44 @@
       "oneOf": [
         {
           "type": "object",
-          "title": "a",
           "required": [
             "a"
           ],
           "properties": {
             "a": {
               "type": "string"
             }
-          }
+          },
+          "title": "a"
         },
         {
           "type": "object",
-          "title": "b",
           "required": [
             "b"
           ],
           "properties": {
             "b": {
               "type": "string"
             }
-          }
+          },
+          "title": "b"
+        },
+        {
+          "type": "object",
+          "required": [
+            "c"
+          ],
+          "properties": {
+            "c": {
+              "$ref": "#/definitions/Unit"
+            }
+          },
+          "title": "c"
         }
       ]
+    },
+    "Unit": {
+      "type": "object"
     }
   }
 }
diff --git a/...openapi/src/test/java/software/amazon/smithy/openapi/fromsmithy/OpenApiConverterTest.java b/...openapi/src/test/java/software/amazon/smithy/openapi/fromsmithy/OpenApiConverterTest.java
@@ -514,4 +514,20 @@ public void generatesOpenApiForSharedErrors() {
 
         Node.assertEquals(result, expectedNode);
     }
+
+    @Test
+    public void convertsUnitsThatDoNotConflict() {
+        Model model = Model.assembler()
+                .addImport(getClass().getResource("nonconflicting-unit.smithy"))
+                .discoverModels()
+                .assemble()
+                .unwrap();
+        OpenApiConfig config = new OpenApiConfig();
+        config.setService(ShapeId.from("example.rest#RestService"));
+        Node result = OpenApiConverter.create().config(config).convertToNode(model);
+        Node expectedNode = Node.parse(IoUtils.toUtf8String(
+                getClass().getResourceAsStream("nonconflicting-unit.openapi.json")));
+
+        Node.assertEquals(result, expectedNode);
+    }
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -237,6 +237,9 @@ @@
                     },
                     "b": {
                         "target": "smithy.api#String"
+                    },
+                    "c": {
+                        "target": "smithy.api#Unit"
                     }
                 }
             }
@@ Expand Down @@