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

Conversation

srchase
Copy link
Contributor

@srchase srchase commented Aug 24, 2023

This PR adds a disableConstraints boolean to the @examples trait, allowing for input constraint trait validations to be lowered from ERROR to WARNING.

By lowering the severity from an ERROR, Smithy model authors can include examples of invalid inputs and their corresponding errors.

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

@srchase srchase requested a review from a team as a code owner August 24, 2023 22:17
Copy link
Member

@mtdowling mtdowling left a comment

Choose a reason for hiding this comment

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

I don't think error definitions using values that don't match the types of for the error are possible to use. If examples let you give exact HTTP messages or something, then sure. But they're structured based on the model. So I think we would want to disable constraints, but not type checking.

@srchase srchase force-pushed the example-input-validation-warnings branch from 6556b90 to a378fdb Compare August 31, 2023 15:42
@srchase
Copy link
Contributor Author

srchase commented Aug 31, 2023

I don't think error definitions using values that don't match the types of for the error are possible to use. If examples let you give exact HTTP messages or something, then sure. But they're structured based on the model. So I think we would want to disable constraints, but not type checking.

Updated this PR to only allow lowering specific trait validations. Type checking validation issues are still emitted as an ERROR.

@srchase srchase requested a review from mtdowling August 31, 2023 15:50
@@ -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.

@srchase srchase force-pushed the example-input-validation-warnings branch from 2f7261d to 05d809a Compare August 31, 2023 17:30
@srchase srchase force-pushed the example-input-validation-warnings branch from 05d809a to 5fe3341 Compare August 31, 2023 17:33
@srchase srchase force-pushed the example-input-validation-warnings branch from 5fe3341 to 2655235 Compare August 31, 2023 17:33
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
* - disableConstraints
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this only be allowed for errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ExamplesTraitsValidator checks that this is only set when errors are also present. Updated the docs to make this clear.

RANGE_TRAIT_ZERO_VALUE_WARNING,

// Lowers severity of constraint trait validations to WARNING.
DISABLE_CONSTRAINTS;
Copy link
Member

Choose a reason for hiding this comment

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

This name seems weird because it doesn't disable validation of constraints but lowers the severity. DISABLE_CONSTRAINTS -> IGNORE_CONSTRAINTS?

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 to allowConstraintError in e5bcfd3

@srchase srchase changed the title Lower input validation severity for error examples Allow constraint trait errors on error example inputs Aug 31, 2023
@srchase srchase merged commit 5944796 into smithy-lang:main Aug 31, 2023
@srchase srchase deleted the example-input-validation-warnings branch August 31, 2023 18:32
alextwoods pushed a commit to alextwoods/smithy that referenced this pull request Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants