Repository: smithy-lang/smithy
Pull request #1964 (2023-09-01 20:49:49)

Add transform that can remove invalid defaults
If a default trait is incompatible with the range trait of the member or the member target, then the shape is essentially in a state where the member is invalid: omit a value for the member at it is automatically incompatible with the range trait of the member.

This transformer finds such cases and removes invalid default values. It is likely that services with such a model did not intend for the members to actually have a default value, and this typically only happens with members that have a default value of `0`.

*Issue #, if available:*

*Description of changes:*


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.



Patch:
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/transform/ModelTransformer.java b/smithy-model/src/main/java/software/amazon/smithy/model/transform/ModelTransformer.java
index bc8bf0217..e822cb38a 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/transform/ModelTransformer.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/transform/ModelTransformer.java
@@ -656,4 +656,14 @@ public final class ModelTransformer {
     public Model downgradeToV1(Model model) {
         return new DowngradeToV1().transform(this, model);
     }
+
+    /**
+     * Remove default traits if the default conflicts with the range trait of the shape.
+     *
+     * @param model Model to transform.
+     * @return Returns the transformed model.
+     */
+    public Model removeInvalidDefaults(Model model) {
+        return new RemoveInvalidDefaults().transform(this, model);
+    }
 }
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/transform/RemoveInvalidDefaults.java b/smithy-model/src/main/java/software/amazon/smithy/model/transform/RemoveInvalidDefaults.java
new file mode 100644
index 000000000..11077ec95
--- /dev/null
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/transform/RemoveInvalidDefaults.java
@@ -0,0 +1,75 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package software.amazon.smithy.model.transform;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.logging.Logger;
+import software.amazon.smithy.model.Model;
+import software.amazon.smithy.model.node.Node;
+import software.amazon.smithy.model.shapes.MemberShape;
+import software.amazon.smithy.model.shapes.Shape;
+import software.amazon.smithy.model.traits.DefaultTrait;
+import software.amazon.smithy.model.traits.RangeTrait;
+
+/**
+ * Removes default values from shapes where the default value is incompatible with
+ * the constraint traits of the shape.
+ */
+final class RemoveInvalidDefaults {
+
+    private static final Logger LOGGER = Logger.getLogger(RemoveInvalidDefaults.class.getName());
+
+    Model transform(ModelTransformer transformer, Model model) {
+        Set<Shape> invalidDefaults = new HashSet<>();
+        Set<Shape> updates = new HashSet<>();
+
+        // First collect invalid shapes. Members with invalid defaults either need to remove the default or
+        // set the default to null if their target shape's default remains intact but the member is invalid.
+        for (Shape shape : model.getShapesWithTrait(DefaultTrait.class)) {
+            shape.getMemberTrait(model, RangeTrait.class).ifPresent(rangeTrait -> {
+                DefaultTrait defaultTrait = shape.expectTrait(DefaultTrait.class);
+                if (defaultTrait.toNode().isNumberNode()) {
+                    defaultTrait.toNode().expectNumberNode().asBigDecimal().ifPresent(value -> {
+                        if (rangeTrait.getMin().filter(min -> value.compareTo(min) < 0).isPresent()
+                                || rangeTrait.getMin().filter(max -> value.compareTo(max) > 0).isPresent()) {
+                            invalidDefaults.add(shape);
+                        }
+                    });
+                }
+            });
+        }
+
+        for (Shape shape : invalidDefaults) {
+            updates.add(modify(shape, model, invalidDefaults));
+        }
+
+        return transformer.replaceShapes(model, updates);
+    }
+
+    private Shape modify(Shape shape, Model model, Set<Shape> otherShapes) {
+        // To show up here, the shape has to have a range trait, or the target has to have one.
+        RangeTrait rangeTrait = shape.getMemberTrait(model, RangeTrait.class).get();
+        LOGGER.info(() -> "Removing default trait from " + shape.getId()
+                          + " because of an incompatible range trait: "
+                          + Node.printJson(rangeTrait.toNode()));
+
+        // Members that target a shape with a default value need to set their default to null to override it.
+        // Other members and other shapes can simply remove the default trait.
+        if (shape.isMemberShape()) {
+            MemberShape member = shape.asMemberShape().get();
+            boolean targetHasDefault = model.getShape(member.getTarget())
+                    // Treat target shapes that will have their default removed as if it doesn't have a default.
+                    .filter(target -> !otherShapes.contains(target) && target.hasTrait(DefaultTrait.class))
+                    .isPresent();
+            if (targetHasDefault) {
+                return member.toBuilder().addTrait(new DefaultTrait(Node.nullNode())).build();
+            }
+        }
+
+        return Shape.shapeToBuilder(shape).removeTrait(DefaultTrait.ID).build();
+    }
+}
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/RangeTraitPlugin.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/RangeTraitPlugin.java
index 0dbfc5db9..5cf2e6386 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/RangeTraitPlugin.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/node/RangeTraitPlugin.java
@@ -15,7 +15,6 @@
 
 package software.amazon.smithy.model.validation.node;
 
-import java.math.BigDecimal;
 import software.amazon.smithy.model.node.Node;
 import software.amazon.smithy.model.node.Node.NonNumericFloat;
 import software.amazon.smithy.model.node.NumberNode;
@@ -70,27 +69,26 @@ class RangeTraitPlugin implements NodeValidatorPlugin {
     }
 
     protected void check(Shape shape, Context context, RangeTrait trait, NumberNode node, Emitter emitter) {
-        Number number = node.getValue();
-        BigDecimal decimal = number instanceof BigDecimal
-                ? (BigDecimal) number
-                : new BigDecimal(number.toString());
-
         trait.getMin().ifPresent(min -> {
-            if (decimal.compareTo(new BigDecimal(min.toString())) < 0) {
-                emitter.accept(node, getSeverity(node, context), String.format(
-                        "Value provided for `%s` must be greater than or equal to %s, but found %s",
-                        shape.getId(), min, number),
-                        shape.isMemberShape() ? MEMBER : TARGET);
-            }
+            node.asBigDecimal().ifPresent(decimal -> {
+                if (decimal.compareTo(min) < 0) {
+                    emitter.accept(node, getSeverity(node, context), String.format(
+                                           "Value provided for `%s` must be greater than or equal to %s, but found %s",
+                                           shape.getId(), min, decimal),
+                                   shape.isMemberShape() ? MEMBER : TARGET);
+                }
+            });
         });
 
         trait.getMax().ifPresent(max -> {
-            if (decimal.compareTo(new BigDecimal(max.toString())) > 0) {
-                emitter.accept(node, getSeverity(node, context), String.format(
-                        "Value provided for `%s` must be less than or equal to %s, but found %s",
-                        shape.getId(), max, number),
-                        shape.isMemberShape() ? MEMBER : TARGET);
-            }
+            node.asBigDecimal().ifPresent(decimal -> {
+                if (decimal.compareTo(max) > 0) {
+                    emitter.accept(node, getSeverity(node, context), String.format(
+                                           "Value provided for `%s` must be less than or equal to %s, but found %s",
+                                           shape.getId(), max, decimal),
+                                   shape.isMemberShape() ? MEMBER : TARGET);
+                }
+            });
         });
     }
 


Test patch:
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/transform/RemoveInvalidDefaultsTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/transform/RemoveInvalidDefaultsTest.java
new file mode 100644
index 000000000..77f5f539f
--- /dev/null
+++ b/smithy-model/src/test/java/software/amazon/smithy/model/transform/RemoveInvalidDefaultsTest.java
@@ -0,0 +1,33 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package software.amazon.smithy.model.transform;
+
+import org.junit.jupiter.api.Test;
+import software.amazon.smithy.model.Model;
+import software.amazon.smithy.model.node.Node;
+import software.amazon.smithy.model.shapes.ModelSerializer;
+
+public class RemoveInvalidDefaultsTest {
+    @Test
+    public void removeInvalidDefaultsBasedOnRangeTrait() {
+        Model input = Model.assembler()
+                .addImport(getClass().getResource("bad-defaults-range-trait.smithy"))
+                .assemble()
+                .unwrap();
+        Model output = Model.assembler()
+                .addImport(getClass().getResource("bad-defaults-range-trait.fixed.smithy"))
+                .assemble()
+                .unwrap();
+
+        ModelTransformer transformer = ModelTransformer.create();
+
+        Model result = transformer.removeInvalidDefaults(input);
+
+        Node actual = ModelSerializer.builder().build().serialize(result);
+        Node expected = ModelSerializer.builder().build().serialize(output);
+        Node.assertEquals(actual, expected);
+    }
+}
diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/transform/bad-defaults-range-trait.fixed.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/transform/bad-defaults-range-trait.fixed.smithy
new file mode 100644
index 000000000..1dc374158
--- /dev/null
+++ b/smithy-model/src/test/resources/software/amazon/smithy/model/transform/bad-defaults-range-trait.fixed.smithy
@@ -0,0 +1,39 @@
+$version: "2.0"
+
+namespace smithy.example
+
+structure Foo {
+    // Default was removed.
+    @range(min: 1)
+    invalid1: Integer
+
+    // Default was removed.
+    invalid2: ValueGreaterThanZero
+
+    // The default of the target shape was removed, so it can be removed here too.
+    invalid3: ValueGreaterThanZeroWithDefault
+
+    // Cancel out the root level default.
+    @range(min: 1)
+    invalid4: PrimitiveInteger = null
+
+    valid1: ValueGreaterThanZero = 1
+
+    valid2: ValueGreaterThanZero
+
+    valid3: Integer
+
+    valid4: Integer = 0
+
+    @range(min: 1)
+    valid5: Integer
+
+    @range(min: 1)
+    valid6: Integer = 1
+}
+
+@range(min: 1)
+integer ValueGreaterThanZero
+
+@range(min: 1)
+integer ValueGreaterThanZeroWithDefault // default was removed
diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/transform/bad-defaults-range-trait.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/transform/bad-defaults-range-trait.smithy
new file mode 100644
index 000000000..6f1ce2f19
--- /dev/null
+++ b/smithy-model/src/test/resources/software/amazon/smithy/model/transform/bad-defaults-range-trait.smithy
@@ -0,0 +1,41 @@
+$version: "2.0"
+
+namespace smithy.example
+
+structure Foo {
+    // The member itself creates an invalid combination of the range trait and default value.
+    @range(min: 1)
+    invalid1: Integer = 0
+
+    // The member adds a default value that is incompatible with the target shape.
+    invalid2: ValueGreaterThanZero = 0
+
+    // The member targets a shape where the range trait is incompatible with the default of the member.
+    invalid3: ValueGreaterThanZeroWithDefault = 0
+
+    // The range trait here makes the default value invalid. This member targets a root shape with a default, so the
+    // default has to be set to null to cancel out the root level default.
+    @range(min: 1)
+    invalid4: PrimitiveInteger = 0
+
+    valid1: ValueGreaterThanZero = 1
+
+    valid2: ValueGreaterThanZero
+
+    valid3: Integer
+
+    valid4: Integer = 0
+
+    @range(min: 1)
+    valid5: Integer
+
+    @range(min: 1)
+    valid6: Integer = 1
+}
+
+@range(min: 1)
+integer ValueGreaterThanZero
+
+@range(min: 1)
+@default(0) // bad default and range trait combination.
+integer ValueGreaterThanZeroWithDefault