Add new shapeExamples trait that communicates and enforces allowed and disallowed values by brandondahler · Pull Request #2851 · smithy-lang/smithy

brandondahler · 2025-11-13T03:49:58Z

Background

What do these changes do?
- Adds a new @shapeExamples trait which defines the set of allowed and disallowed values for a given shape.
- Updates the NodeValidationVisitor to be more accurate in some less-commonly used cases
- Adds a new ShapeExamplesTraitValidator implementation which verifies that:
  1. Allowed values are accepted by the shape based on the configured constraints
  2. Disallowed values are not accepted by the shape based on the configured constraints
Why are they important?
- Configuring and modifying a shape's constraints is tedious work with potentially large consequences if mistakes are made.
- This is especially true for @pattern-based constrained values
- This feature offers a mechanism to both express your intent as a shape author about what you meant to configure while also enforcing that the intent is a reality during the model validation phase.

Testing

How did you test these changes?
- Significant effort was put into building out an ErrorFilesTest-based suite of tests to ensure that the validation works across as much of the gamut of constraint and shape type combinations that exist.
- Documentation updates were best effort -- I do not have an environment setup that is readily capable of building and inspecting the output.

Links

Links to additional context, if necessary
- Implements Feature request: Support for defining and verifying positive and negative test vectors for constrained shapes and members #2821

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…d disallowed values

.changes/next-release/feature-0dad6fe920f7dc053e14bed2d43b2e553a2f142a.json

…3a2f142a.json Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

brandondahler · 2025-11-13T03:53:20Z

...y-model/src/main/java/software/amazon/smithy/model/validation/testrunner/SmithyTestCase.java

        }
    }
+
+    private static final class ErrorsFileValidationEventFormatter implements ValidationEventFormatter {


Updated implementation here to make it easier to iterate on error files -- the previous implementation would output the extra file location information and hints if they exist, which are explicitly omitted from the errors file normally.

You can now copy+paste directly from the failure message in the test result into the errors file.

brandondahler · 2025-11-13T03:57:43Z

smithy-model/src/main/java/software/amazon/smithy/model/validation/NodeValidationVisitor.java

-        if (value.isNullNode()) {
+
+        if (!value.isNullNode()) {
+            model.getShape(shape.getTarget()).ifPresent(target -> {


Originally traversing into the shape when the value was null pretty much guaranteed that an error would be reported -- generally of the form "<string, number, object> was expected but value was null" (not exact wording).

Since is the memberShape method, this only affects aggregate types which have an explicit null value as opposed to the member being omitted.

robin-aws · 2025-12-08T17:54:53Z

smithy-model/src/main/resources/software/amazon/smithy/model/loader/prelude.smithy

+  allowed: ShapeExampleList
+  disallowed: ShapeExampleList


nit:

Suggested change

allowed: ShapeExampleList

disallowed: ShapeExampleList

valid: ShapeExampleList

invalid: ShapeExampleList

AFAICT "valid" appears a whole lot more in the spec/documentation than "allowed"

JordonPhillips

I talked to the SDK teams and got some general consensus on bringing this in. There's a few changes I'd like to make, notably adding some optional documentation to each example to explain why something does or doesn't pass.

JordonPhillips · 2026-02-20T10:49:07Z

smithy-model/src/main/resources/software/amazon/smithy/model/loader/prelude.smithy

+@trait(
+    selector: ":test(number, string, blob, structure, list, map, member)"
+)


This should be able to target unions. Also, gotta make sure to restrict the target for member shapes.

Suggested change

@trait(

selector: ":test(number, string, blob, structure, list, map, member)"

)

@trait(

selector: """

:test(

number, string, blob, structure, union, list, map,

member > :test(number, string, blob, structure, union, list, map)

)"""

)

Member targets were not restricted because all members can have constraining traits applied to them regardless of whether the underlying value can be constrained. For example, a boolean shape does not have any valid constraint traits available; however, a member pointing at the boolean shape (i.e. Boolean) can have @required applied to it which causes null to become invalid.

For unions - I'll have to look closer but I suspect my thought was that union types wouldn't make sense to have constraints. I can see potential value in defining the examples at the union level instead of having to go deeper to the union's target shapes or higher to the member usage of the union -- I just need to check that the implementation actually works.

FWIW @conditions is allowed on unions, although only because it's well-formed and not because I could think any valuable uses for it. :)

JordonPhillips · 2026-02-20T10:53:29Z

smithy-model/src/main/resources/software/amazon/smithy/model/loader/prelude.smithy

+@private
+@length(min: 1)
+@sparse
+list ShapeExampleList {
+  member: Document
+}


I'd like to open this up a bit by making the list value a structure. That'll let us add additional properties aside from the value itself. At the moment I've added a documentation property to describe why a given value does or does not validate. There was also some desire to add a specific error that invalid values trigger. I'm not so sure about that. But opening it up like this makes that possible too.

Suggested change

@private

@length(min: 1)

@sparse

list ShapeExampleList {

member: Document

}

/// A list of values to be mapped to a shape.

@private

@length(min: 1)

@sparse

list ShapeExampleList {

member: ShapeExample

}

/// A value that may be mapped to a shape.

@private

structure ShapeExample {

/// The actual value that may be mapped to the shape. This must match the

/// expected shape type, regardless of whether this example represents a

/// valid or invalid value.

@required

value: Document

/// Optional documentation in CommonMark format describing why the example

/// does or does not match the shape's constraints.

documentation: String

}

+1. As the person with the desire mentioned :), I think it would be great to also have a validationEventId member stating precisely what event is expected, using the same matching rules as suppressions. That would enable the validation of this trait much more precise.

I'd also suggest adding tags, so that there is a way to mark examples that should not be included in documentation. This would support using this trait to thoroughly test individual @pattern or @conditions applied traits without necessarily including far too many "examples" in docs.

I think it would be great to also have a validationEventId member stating precisely what event is expected,

Is the idea that this would:

Be specifically for invalid ShapeExample instances to say what failure(s) are expected,

Define the segment in the validation event id if this example fails, or

Something else?

Yes to 1. It would be a way to express more precisely WHY examples are invalid, and for validation to fail if the example raises other unexpected events. For example:

e.g.

@conditions({ MutexAB: { documentation: "Only provide A or B, not both" expression: "!(A != null && B != null)" } }) @shapeExamples({ invalid: [ { value: { A: "Hi", B: "There" }, validationEventId: "MutexAB" ] }) structure FooInput { A: String B: String }

(Not 100% sure if there needs to be a prefix on the event ID for that to be right. The main point is that the @conditions validation adds the ID of the condition that failed to the event id.)

Validation of the @shapeExamples trait would fail if you got some other validation failures, such as a type mismatch on { A: "Hi", B: 42 }.

Also probably the most general to support a list of validationEventIds in case it's difficult to trigger only one in some complicated cases.

JordonPhillips · 2026-02-20T11:00:19Z

...ain/java/software/amazon/smithy/model/validation/validators/ShapeExamplesTraitValidator.java

+
+            events.addAll(nonErrorValidationEvents);
+
+            if (validationEvents.size() == nonErrorValidationEvents.size()) {


I don't think it's useful to allow examples that don't match the expected type, even for the invalid list. Perhaps we should forward that along.

If you take my suggestion (https://github.com/smithy-lang/smithy/pull/2851/changes#r2834425764), this could be more precise and check that you only get events that match the expected id.

I don't follow either of your two comments -- can you explain further?

This if statement is saying that if all validation events are non-error events then it should log an error event because this disallowed case should have resulted in an error.

@robin-aws I think what you're alluding to matches my interpretation of your idea being "Be specifically for invalid ShapeExample instances to say what failure(s) are expected"?

For example, this would not be useful:

@shapeExamples(invalid: [{value: 2}]) string Foo

It's a type mismatch. That could never be valid, so there's no confusion to clear up and no reason to have such a test case for validation purposes.

Yeah, see #2851 (comment) for more details on my suggestion.

This logic here would just assert that all validationEvents match the validationEventId prefix, because anything else is unexpected, error or not.

I see what each of you are saying now, makes sense

JordonPhillips · 2026-02-20T11:06:44Z

smithy-model/src/main/java/software/amazon/smithy/model/traits/ShapeExamplesTrait.java

+        private List<Node> allowed;
+        private List<Node> disallowed;
+
+        public ShapeExamplesTrait.Builder allowed(List<Node> allowed) {
+            this.allowed = allowed;
+            return this;
+        }
+
+        public ShapeExamplesTrait.Builder disallowed(List<Node> disallowed) {
+            this.disallowed = disallowed;


Using BuilderRef prevents leaking mutations and reduces copies.

Suggested change

private List<Node> allowed;

private List<Node> disallowed;

public ShapeExamplesTrait.Builder allowed(List<Node> allowed) {

this.allowed = allowed;

return this;

}

public ShapeExamplesTrait.Builder disallowed(List<Node> disallowed) {

this.disallowed = disallowed;

private BuilderRef<List<Node>> allowed = BuilderRef.forList();

private BuilderRef<List<Node>> disallowed = BuilderRef.forList();

public ShapeExamplesTrait.Builder allowed(List<Node> allowed) {

this.allowed.clear();

this.allowed.get().addAll(allowed);

return this;

}

public ShapeExamplesTrait.Builder disallowed(List<Node> disallowed) {

this.disallowed.clear();

this.disallowed.addAll(disallowed);

JordonPhillips · 2026-02-20T11:10:19Z

smithy-model/src/main/java/software/amazon/smithy/model/traits/ShapeExamplesTrait.java

+        this.allowed = builder.allowed;
+        this.disallowed = builder.disallowed;
+        if (allowed == null && disallowed == null) {
+            throw new SourceException("One of 'allowed' or 'disallowed' must be provided.", getSourceLocation());
+        }
+        if (allowed != null && allowed.isEmpty()) {
+            throw new SourceException("'allowed' must be non-empty when provided.", getSourceLocation());
+        }
+        if (disallowed != null && disallowed.isEmpty()) {
+            throw new SourceException("'disallowed' must be non-empty when provided.", getSourceLocation());
+        }


Empty lists are perfectly fine, so long as at least one is populated.

Suggested change

this.allowed = builder.allowed;

this.disallowed = builder.disallowed;

if (allowed == null && disallowed == null) {

throw new SourceException("One of 'allowed' or 'disallowed' must be provided.", getSourceLocation());

}

if (allowed != null && allowed.isEmpty()) {

throw new SourceException("'allowed' must be non-empty when provided.", getSourceLocation());

}

if (disallowed != null && disallowed.isEmpty()) {

throw new SourceException("'disallowed' must be non-empty when provided.", getSourceLocation());

}

this.allowed = builder.allowed.copy();

this.disallowed = builder.disallowed.copy();

if (allowed.isEmpty() && disallowed.isEmpty()) {

throw new SourceException("One of 'allowed' or 'disallowed' must be non-empty.", getSourceLocation());

}

JordonPhillips · 2026-02-20T11:11:31Z

smithy-model/src/main/java/software/amazon/smithy/model/traits/ShapeExamplesTrait.java

+    public Optional<List<Node>> getAllowed() {
+        return Optional.ofNullable(allowed);
+    }
+
+    /**
+     * Gets the disallowed values.
+     *
+     * @return returns the optional disallowed values.
+     */
+    public Optional<List<Node>> getDisallowed() {
+        return Optional.ofNullable(disallowed);
+    }


Suggested change

public Optional<List<Node>> getAllowed() {

return Optional.ofNullable(allowed);

}

/**

* Gets the disallowed values.

*

* @return returns the optional disallowed values.

*/

public Optional<List<Node>> getDisallowed() {

return Optional.ofNullable(disallowed);

}

public List<Node> getAllowed() {

return allowed;

}

/**

* Gets the disallowed values.

*

* @return returns the optional disallowed values.

*/

public List<Node> getDisallowed() {

return disallowed;

}

brandondahler · 2026-03-06T05:32:19Z

I've worked through a majority of the feedback, have a working update and will push a new revision in the next few days -- just need to tweak the docs update as well.

robin-aws

Just submitting an actual review so I get the chance to review before it's merged. My approval doesn't technically matter here. :)

Add new shapeExamples trait that communicates and enforces allowed an…

902d6fe

…d disallowed values

brandondahler requested a review from a team as a code owner November 13, 2025 03:49

brandondahler requested a review from yefrig November 13, 2025 03:49

github-actions bot reviewed Nov 13, 2025

View reviewed changes

.changes/next-release/feature-0dad6fe920f7dc053e14bed2d43b2e553a2f142a.json Show resolved Hide resolved

Update .changes/next-release/feature-0dad6fe920f7dc053e14bed2d43b2e55…

69c5090

…3a2f142a.json Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

brandondahler commented Nov 13, 2025

View reviewed changes

Fix issue with length-restricted blob members with invalid trait values

d83ccca

robin-aws mentioned this pull request Dec 5, 2025

JMESPath evaluation #2878

Merged

robin-aws reviewed Dec 8, 2025

View reviewed changes

JordonPhillips requested changes Feb 20, 2026

View reviewed changes

robin-aws suggested changes Mar 7, 2026

View reviewed changes

-@trait(
-    selector: ":test(number, string, blob, structure, list, map, member)"
-)
+@trait(
+    selector: """
+        :test(
+            number, string, blob, structure, union, list, map,
+            member > :test(number, string, blob, structure, union, list, map)
+        )"""
+)

-@private
-@length(min: 1)
-@sparse
-list ShapeExampleList {
-  member: Document
-}
+/// A list of values to be mapped to a shape.
+@private
+@length(min: 1)
+@sparse
+list ShapeExampleList {
+    member: ShapeExample
+}
+/// A value that may be mapped to a shape.
+@private
+structure ShapeExample {
+    /// The actual value that may be mapped to the shape. This must match the
+    /// expected shape type, regardless of whether this example represents a
+    /// valid or invalid value.
+    @required
+    value: Document
+    /// Optional documentation in CommonMark format describing why the example
+    /// does or does not match the shape's constraints.
+    documentation: String
+}


		events.addAll(nonErrorValidationEvents);

		if (validationEvents.size() == nonErrorValidationEvents.size()) {

Conversation

brandondahler commented Nov 13, 2025

Background

Testing

Links

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandondahler Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JordonPhillips left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robin-aws Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandondahler commented Mar 6, 2026

Uh oh!

robin-aws left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brandondahler Nov 13, 2025 •

edited

Loading

robin-aws Mar 7, 2026 •

edited

Loading