Make adoption of Unit easier for existing models

docs/source/1.0/spec/core/model.rst

@@ -1366,6 +1366,16 @@ closure of a service. This constraint allows services to discern between
 operations and resources using only their shape name rather than a
 fully-qualified path from the service to the shape.
 
+.. rubric:: Undeclared operation inputs and outputs are not a
+            part of the service closure
+
+:ref:`smithy.api#Unit <unit-type>` is the shape that is implicitly targeted by
+operation inputs and outputs when they are not explicitly declared. This does
+not, however, add ``smithy.api#Unit`` to the service's closure, and does not
+require renaming to avoid conflicts with other shapes named ``Unit``. Unions in
+the service closure with members targeting ``smithy.api#Unit``, however, will
+cause ``smithy.api#Unit`` to be a part of the service closure.
+
 
 ..  _operation:
 

docs/source/1.0/spec/core/selectors.rst

@@ -1209,9 +1209,21 @@ The table below lists the labeled directed relationships from each shape.
     * - operation
       - input
       - The input structure of the operation (if present).
+
+        .. note::
+
+            :ref:`smithy.api#Unit <unit-type>` is considered "not present"
+            for this relationship, and will not be yielded.
+
     * - operation
       - output
       - The output structure of the operation (if present).
+
+        .. note::
+
+            :ref:`smithy.api#Unit <unit-type>` is considered "not present"
+            for this relationship, and will not be yielded.
+
     * - operation
       - error
       - Each error structure referenced by the operation (if present).

smithy-codegen-core/src/test/java/software/amazon/smithy/codegen/core/TopologicalIndexTest.java

@@ -19,7 +19,7 @@
 import software.amazon.smithy.model.loader.Prelude;
 import software.amazon.smithy.model.shapes.Shape;
 import software.amazon.smithy.model.shapes.ShapeId;
-import software.amazon.smithy.model.traits.UnitTypeTrait;
+import software.amazon.smithy.utils.FunctionalUtils;
 
 public class TopologicalIndexTest {
 
@@ -101,9 +101,9 @@ public void getsRecursiveClosureByShape() {
         TopologicalIndex index = TopologicalIndex.of(model);
 
         assertThat(index.getRecursiveClosure(model.expectShape(ShapeId.from("smithy.example#MyString"))),
-                   empty());
+                empty());
         assertThat(index.getRecursiveClosure(model.expectShape(ShapeId.from("smithy.example#Recursive$b"))),
-                   not(empty()));
+                not(empty()));
     }
 
     @Test
@@ -114,10 +114,9 @@ public void handlesMoreRecursion() {
                 .unwrap();
         TopologicalIndex index = TopologicalIndex.of(recursive);
 
-        // The topological index must capture all shapes in the index not in the prelude,
-        // but include the Unit shape.
+        // The topological index must capture all shapes in the index not in the prelude.
         Set<Shape> nonPrelude = recursive.shapes()
-                .filter(shape -> shape.getId().equals(UnitTypeTrait.UNIT) || !Prelude.isPreludeShape(shape))
+                .filter(FunctionalUtils.not(Prelude::isPreludeShape))
                 .collect(Collectors.toSet());
         Set<Shape> topologicalShapes = new HashSet<>(index.getOrderedShapes());
         topologicalShapes.addAll(index.getRecursiveShapes());
@@ -129,7 +128,6 @@ public void handlesMoreRecursion() {
                 .map(ShapeId::toString)
                 .collect(Collectors.toList());
         assertThat(orderedIds, contains(
-                "smithy.api#Unit",
                 "smithy.example#MyString",
                 "smithy.example#NonRecursive$foo",
                 "smithy.example#NonRecursive",

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);

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);
+        }
+    }
 }

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);
     }
 }

smithy-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));
+        });
+    }
 }

smithy-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"));

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"
                 }
             }
         }