Skip to content

Commit

Permalink
Allow constraint trait errors on error example inputs (smithy-lang#1949)
Browse files Browse the repository at this point in the history
  • Loading branch information
srchase authored and alextwoods committed Sep 15, 2023
1 parent aff5f7a commit af3c3e9
Show file tree
Hide file tree
Showing 15 changed files with 319 additions and 47 deletions.
2 changes: 1 addition & 1 deletion docs/source-2.0/guides/building-models/build-config.rst
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ The configuration file accepts the following properties:
projection. Plugins are a mapping of :ref:`plugin IDs <plugin-id>` to
plugin-specific configuration objects.
* - ignoreMissingPlugins
- ``bool``
- ``boolean``
- If a plugin can't be found, Smithy will by default fail the build. This
setting can be set to ``true`` to allow the build to progress even if
a plugin can't be found on the classpath.
Expand Down
12 changes: 6 additions & 6 deletions docs/source-2.0/spec/documentation-traits.rst
Original file line number Diff line number Diff line change
Expand Up @@ -139,11 +139,10 @@ Each ``example`` trait value is a structure with the following members:
- :ref:`examples-ErrorExample-structure`
- Provides an error shape ID and example error parameters for the
operation.

The values provided for the ``input`` and ``output`` members MUST be
compatible with the shapes and constraints of the corresponding structure.
These values use the same semantics and format as
:ref:`custom trait values <trait-node-values>`.
* - allowConstraintErrors
- ``boolean``
- Set to true to lower input constraint trait validations to warnings.
This can only be set when ``error`` is provided.

A value for ``output`` or ``error`` SHOULD be provided. However, both
MUST NOT be defined for the same example.
Expand Down Expand Up @@ -186,7 +185,8 @@ MUST NOT be defined for the same example.
content: {
message: "Invalid 'foo'. Special character not allowed."
}
}
},
allowConstraintErrors: true
}
])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.Objects;
import java.util.Optional;
import software.amazon.smithy.model.node.ArrayNode;
import software.amazon.smithy.model.node.BooleanNode;
import software.amazon.smithy.model.node.Node;
import software.amazon.smithy.model.node.ObjectNode;
import software.amazon.smithy.model.node.ToNode;
Expand Down Expand Up @@ -55,6 +56,24 @@ protected Node createNode() {
return examples.stream().map(Example::toNode).collect(ArrayNode.collect(getSourceLocation()));
}

@Override
public boolean equals(Object other) {
if (!(other instanceof ExamplesTrait)) {
return false;
} else if (other == this) {
return true;
} else {
ExamplesTrait trait = (ExamplesTrait) other;
return this.examples.size() == trait.examples.size() && this.examples.containsAll(trait.examples);
}
}

@Override
public int hashCode() {
return Objects.hash(toShapeId(), examples);
}


@Override
public Builder toBuilder() {
Builder builder = new Builder().sourceLocation(getSourceLocation());
Expand All @@ -70,7 +89,7 @@ public static Builder builder() {
}

/**
* Builds and examples trait.
* Builds an examples trait.
*/
public static final class Builder extends AbstractTraitBuilder<ExamplesTrait, Builder> {
private final List<Example> examples = new ArrayList<>();
Expand Down Expand Up @@ -100,13 +119,15 @@ public static final class Example implements ToNode, ToSmithyBuilder<Example> {
private final ObjectNode input;
private final ObjectNode output;
private final ErrorExample error;
private final boolean allowConstraintErrors;

private Example(Builder builder) {
this.title = Objects.requireNonNull(builder.title, "Example title must not be null");
this.documentation = builder.documentation;
this.input = builder.input;
this.output = builder.output;
this.error = builder.error;
this.allowConstraintErrors = builder.allowConstraintErrors;
}

/**
Expand Down Expand Up @@ -144,12 +165,21 @@ public Optional<ErrorExample> getError() {
return Optional.ofNullable(error);
}

/**
* @return Returns true if input constraints errors are allowed.
*/
public boolean getAllowConstraintErrors() {
return allowConstraintErrors;
}

@Override
public Node toNode() {
ObjectNode.Builder builder = Node.objectNodeBuilder()
.withMember("title", Node.from(title))
.withOptionalMember("documentation", getDocumentation().map(Node::from))
.withOptionalMember("error", getError().map(ErrorExample::toNode));
.withOptionalMember("error", getError().map(ErrorExample::toNode))
.withOptionalMember("allowConstraintErrors", BooleanNode.from(allowConstraintErrors)
.asBooleanNode());

if (!input.isEmpty()) {
builder.withMember("input", input);
Expand All @@ -163,7 +193,8 @@ public Node toNode() {

@Override
public Builder toBuilder() {
return new Builder().documentation(documentation).title(title).input(input).output(output).error(error);
return new Builder().documentation(documentation).title(title).input(input).output(output).error(error)
.allowConstraintErrors(allowConstraintErrors);
}

public static Builder builder() {
Expand All @@ -179,6 +210,7 @@ public static final class Builder implements SmithyBuilder<Example> {
private ObjectNode input = Node.objectNode();
private ObjectNode output;
private ErrorExample error;
private boolean allowConstraintErrors;

@Override
public Example build() {
Expand Down Expand Up @@ -209,6 +241,11 @@ public Builder error(ErrorExample error) {
this.error = error;
return this;
}

public Builder allowConstraintErrors(Boolean allowConstraintErrors) {
this.allowConstraintErrors = allowConstraintErrors;
return this;
}
}
}

Expand Down Expand Up @@ -302,7 +339,8 @@ private static Example exampleFromNode(ObjectNode node) {
.getStringMember("documentation", builder::documentation)
.getObjectMember("input", builder::input)
.getObjectMember("output", builder::output)
.getMember("error", ErrorExample::fromNode, builder::error);
.getMember("error", ErrorExample::fromNode, builder::error)
.getBooleanMember("allowConstraintErrors", builder::allowConstraintErrors);
return builder.build();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,21 @@ public enum Feature {
* Emit a warning when a range trait is incompatible with a default value of 0.
*
* <p>This was a common pattern in Smithy 1.0 and earlier. It implies that the value is effectively
* required. However, chaning the type of the value by un-boxing it or adjusting the range trait would
* be a lossy tranformation when migrating a model from 1.0 to 2.0.
* required. However, changing the type of the value by un-boxing it or adjusting the range trait would
* be a lossy transformation when migrating a model from 1.0 to 2.0.
*/
RANGE_TRAIT_ZERO_VALUE_WARNING
RANGE_TRAIT_ZERO_VALUE_WARNING,

// Lowers severity of constraint trait validations to WARNING.
ALLOW_CONSTRAINT_ERRORS;

public static Feature fromNode(Node node) {
return Feature.valueOf(node.expectStringNode().getValue());
}

public static Node toNode(Feature feature) {
return StringNode.from(feature.toString());
}
}

public static Builder builder() {
Expand Down Expand Up @@ -317,9 +328,11 @@ public List<ValidationEvent> structureShape(StructureShape shape) {

for (MemberShape member : members.values()) {
if (member.isRequired() && !object.getMember(member.getMemberName()).isPresent()) {
Severity severity = this.validationContext.hasFeature(Feature.ALLOW_CONSTRAINT_ERRORS)
? Severity.WARNING : Severity.ERROR;
events.add(event(String.format(
"Missing required structure member `%s` for `%s`",
member.getMemberName(), shape.getId())));
member.getMemberName(), shape.getId()), severity));
}
}
return events;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import software.amazon.smithy.model.shapes.BlobShape;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.traits.LengthTrait;
import software.amazon.smithy.model.validation.NodeValidationVisitor;
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.utils.SmithyInternalApi;

/**
Expand All @@ -39,16 +41,23 @@ protected void check(Shape shape, LengthTrait trait, StringNode node, Context co

trait.getMin().ifPresent(min -> {
if (size < min) {
emitter.accept(node, "Value provided for `" + shape.getId() + "` must have at least "
+ min + " bytes, but the provided value only has " + size + " bytes");
emitter.accept(node, getSeverity(context), "Value provided for `" + shape.getId()
+ "` must have at least " + min + " bytes, but the provided value only has " + size
+ " bytes");
}
});

trait.getMax().ifPresent(max -> {
if (value.getBytes(StandardCharsets.UTF_8).length > max) {
emitter.accept(node, "Value provided for `" + shape.getId() + "` must have no more than "
+ max + " bytes, but the provided value has " + size + " bytes");
emitter.accept(node, getSeverity(context), "Value provided for `" + shape.getId()
+ "` must have no more than " + max + " bytes, but the provided value has " + size
+ " bytes");
}
});
}

private Severity getSeverity(Context context) {
return context.hasFeature(NodeValidationVisitor.Feature.ALLOW_CONSTRAINT_ERRORS)
? Severity.WARNING : Severity.ERROR;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import software.amazon.smithy.model.shapes.MapShape;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.traits.LengthTrait;
import software.amazon.smithy.model.validation.NodeValidationVisitor;
import software.amazon.smithy.model.validation.Severity;
import software.amazon.smithy.utils.SmithyInternalApi;

/**
Expand All @@ -35,18 +37,23 @@ final class MapLengthPlugin extends MemberAndShapeTraitPlugin<MapShape, ObjectNo
protected void check(Shape shape, LengthTrait trait, ObjectNode node, Context context, Emitter emitter) {
trait.getMin().ifPresent(min -> {
if (node.size() < min) {
emitter.accept(node, String.format(
emitter.accept(node, getSeverity(context), String.format(
"Value provided for `%s` must have at least %d entries, but the provided value only "
+ "has %d entries", shape.getId(), min, node.size()));
}
});

trait.getMax().ifPresent(max -> {
if (node.size() > max) {
emitter.accept(node, String.format(
emitter.accept(node, getSeverity(context), String.format(
"Value provided for `%s` must have no more than %d entries, but the provided value "
+ "has %d entries", shape.getId(), max, node.size()));
}
});
}

private Severity getSeverity(Context context) {
return context.hasFeature(NodeValidationVisitor.Feature.ALLOW_CONSTRAINT_ERRORS)
? Severity.WARNING : Severity.ERROR;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.StringShape;
import software.amazon.smithy.model.traits.PatternTrait;
import software.amazon.smithy.model.validation.NodeValidationVisitor;
import software.amazon.smithy.model.validation.Severity;

/**
* Validates the pattern trait on string shapes or members that target them.
Expand All @@ -32,9 +34,15 @@ final class PatternTraitPlugin extends MemberAndShapeTraitPlugin<StringShape, St
@Override
protected void check(Shape shape, PatternTrait trait, StringNode node, Context context, Emitter emitter) {
if (!trait.getPattern().matcher(node.getValue()).find()) {
emitter.accept(node, String.format(
emitter.accept(node, getSeverity(context), String.format(
"String value provided for `%s` must match regular expression: %s",
shape.getId(), trait.getPattern().pattern()));
}
}


private Severity getSeverity(Context context) {
return context.hasFeature(NodeValidationVisitor.Feature.ALLOW_CONSTRAINT_ERRORS)
? Severity.WARNING : Severity.ERROR;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,49 +36,48 @@ class RangeTraitPlugin implements NodeValidatorPlugin {
public final void apply(Shape shape, Node value, Context context, Emitter emitter) {
if (shape.hasTrait(RangeTrait.class)) {
if (value.isNumberNode()) {
boolean zeroValueWarning = context
.hasFeature(NodeValidationVisitor.Feature.RANGE_TRAIT_ZERO_VALUE_WARNING);
check(shape, zeroValueWarning, shape.expectTrait(RangeTrait.class), value.expectNumberNode(), emitter);
check(shape, context, shape.expectTrait(RangeTrait.class), value.expectNumberNode(), emitter);
} else if (value.isStringNode()) {
checkNonNumeric(shape, shape.expectTrait(RangeTrait.class), value.expectStringNode(), emitter);
checkNonNumeric(shape, shape.expectTrait(RangeTrait.class), value.expectStringNode(), emitter,
context);
}
}
}

private void checkNonNumeric(Shape shape, RangeTrait trait, StringNode node, Emitter emitter) {
private void checkNonNumeric(Shape shape, RangeTrait trait, StringNode node, Emitter emitter, Context context) {
NonNumericFloat.fromStringRepresentation(node.getValue()).ifPresent(value -> {
if (value.equals(NonNumericFloat.NAN)) {
emitter.accept(node, Severity.ERROR, String.format(
emitter.accept(node, getSeverity(context), String.format(
"Value provided for `%s` must be a number because the `smithy.api#range` trait is applied, "
+ "but found \"%s\"",
shape.getId(), node.getValue()));
}

if (trait.getMin().isPresent() && value.equals(NonNumericFloat.NEGATIVE_INFINITY)) {
emitter.accept(node, Severity.ERROR, String.format(
emitter.accept(node, getSeverity(context), String.format(
"Value provided for `%s` must be greater than or equal to %s, but found \"%s\"",
shape.getId(), trait.getMin().get(), node.getValue()),
shape.isMemberShape() ? MEMBER : TARGET);
}

if (trait.getMax().isPresent() && value.equals(NonNumericFloat.POSITIVE_INFINITY)) {
emitter.accept(node, Severity.ERROR, String.format(
emitter.accept(node, getSeverity(context), String.format(
"Value provided for `%s` must be less than or equal to %s, but found \"%s\"",
shape.getId(), trait.getMax().get(), node.getValue()),
shape.isMemberShape() ? MEMBER : TARGET);
}
});
}

protected void check(Shape shape, boolean zeroValueWarning, RangeTrait trait, NumberNode node, Emitter emitter) {
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, zeroValueWarning), String.format(
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);
Expand All @@ -87,15 +86,23 @@ protected void check(Shape shape, boolean zeroValueWarning, RangeTrait trait, Nu

trait.getMax().ifPresent(max -> {
if (decimal.compareTo(new BigDecimal(max.toString())) > 0) {
emitter.accept(node, getSeverity(node, zeroValueWarning), String.format(
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);
}
});
}

private Severity getSeverity(NumberNode node, boolean zeroValueWarning) {
return zeroValueWarning && node.isZero() ? Severity.WARNING : Severity.ERROR;
private Severity getSeverity(NumberNode node, Context context) {
boolean zeroValueWarning = context
.hasFeature(NodeValidationVisitor.Feature.RANGE_TRAIT_ZERO_VALUE_WARNING);
boolean rangeTraitWarning = context.hasFeature(NodeValidationVisitor.Feature.ALLOW_CONSTRAINT_ERRORS);
return (zeroValueWarning && node.isZero()) || rangeTraitWarning ? Severity.WARNING : Severity.ERROR;
}

private Severity getSeverity(Context context) {
return context.hasFeature(NodeValidationVisitor.Feature.ALLOW_CONSTRAINT_ERRORS)
? Severity.WARNING : Severity.ERROR;
}
}
Loading

0 comments on commit af3c3e9

Please sign in to comment.