Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow constraint trait errors on error example inputs #1949

Merged
merged 5 commits into from
Aug 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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``
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated for consistency, since we use boolean elsewhere in the docs.

- 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