Add new shapeExamples trait that communicates and enforces allowed and disallowed values

[brandondahler]

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.

[brandondahler]

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]

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]

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]

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)
	)"""
	)

[JordonPhillips]

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
	}

[robin-aws]

+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.

[JordonPhillips]

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.

[robin-aws]

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.

[JordonPhillips]

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]

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]

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;
	}