From 454a3d3ff17c3be92dcb4316bf5fdf894e9c438e Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Fri, 1 Sep 2023 15:43:32 -0500 Subject: [PATCH] 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`. --- .../model/transform/ModelTransformer.java | 10 +++ .../transform/RemoveInvalidDefaults.java | 75 +++++++++++++++++++ .../validation/node/RangeTraitPlugin.java | 34 ++++----- .../transform/RemoveInvalidDefaultsTest.java | 33 ++++++++ .../bad-defaults-range-trait.fixed.smithy | 39 ++++++++++ .../transform/bad-defaults-range-trait.smithy | 41 ++++++++++ 6 files changed, 214 insertions(+), 18 deletions(-) create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/transform/RemoveInvalidDefaults.java create mode 100644 smithy-model/src/test/java/software/amazon/smithy/model/transform/RemoveInvalidDefaultsTest.java create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/transform/bad-defaults-range-trait.fixed.smithy create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/transform/bad-defaults-range-trait.smithy 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 bc8bf0217c2..e822cb38ae9 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 Model addClientOptional(Model model, boolean applyWhenNoDefaultValue) { 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 00000000000..11077ec95c6 --- /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 invalidDefaults = new HashSet<>(); + Set 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 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 0dbfc5db9c2..5cf2e63860d 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 @@ private void checkNonNumeric(Shape shape, RangeTrait trait, StringNode node, Emi } 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); + } + }); }); } 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 00000000000..77f5f539f71 --- /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 00000000000..1dc37415833 --- /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 00000000000..6f1ce2f190c --- /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