Skip to content

Commit

Permalink
Add transform that can remove invalid defaults
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
mtdowling committed Sep 5, 2023
1 parent 6e7cc38 commit 454a3d3
Show file tree
Hide file tree
Showing 6 changed files with 214 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
});
});
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 454a3d3

Please sign in to comment.