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

Validations with ERROR severity are suppressible when documented as insuppressible #2128

Closed
Xtansia opened this issue Feb 7, 2024 · 1 comment · Fixed by #2130
Closed

Comments

@Xtansia
Copy link
Contributor

Xtansia commented Feb 7, 2024

The documentation states that validations with a severity level of ERROR are unable to be suppressed: docs/source-2.0/spec/model-validation.rst#severity.

However in version 1.36.0 with PR #1890 the call to Severity.canSuppress() was removed: d8ee25e#diff-5088b9a4c34e5de8b7a61ddd131c7279111079cfd92d464b02f97bb794f738a5L328-L331.

It would be great if we could get this new behaviour documented and Severity.canSuppress fully removed if it's intentional, or if the old behaviour is to be reinstated have a validation or warning when an ERROR severity is attempted to be suppressed.

I personally lean towards keeping the current behaviour as I have a use-case for suppressing HttpUriConflict as my team and I are writing a specification for an existing API/server and are generating OpenAPI from Smithy which allows these apparent conflicts. But if this behaviour is to be reverted we do not want to rely on the current behaviour.

@mtdowling
Copy link
Member

Oh that wasn't an intentional regression. We don't want to be able to suppress ERROR since those typically indicate that a model violates some kind of MUST from the spec that people are likely reasonably relying on in things like code generators or runtime libraries.

mtdowling added a commit that referenced this issue Feb 7, 2024
There was a regression in #1890 that allowed ERROR events to be
suppressed. This was not intentional and we do not want to remove this
restriction. ERROR events indicate that the model is in an unusable
state because it violates a MUST in the spec or some other necessary
requirement to make it usable (like referring only to shapes that
exist).

New test cases have been added to guard against this regression in
the future.

Closes #2128
mtdowling added a commit that referenced this issue Feb 7, 2024
There was a regression in #1890 that allowed ERROR events to be
suppressed. This was not intentional and we do not want to remove this
restriction. ERROR events indicate that the model is in an unusable
state because it violates a MUST in the spec or some other necessary
requirement to make it usable (like referring only to shapes that
exist).

New test cases have been added to guard against this regression in
the future.

Closes #2128
mtdowling added a commit that referenced this issue Feb 7, 2024
There was a regression in #1890 that allowed ERROR events to be
suppressed. This was not intentional and we do not want to remove this
restriction. ERROR events indicate that the model is in an unusable
state because it violates a MUST in the spec or some other necessary
requirement to make it usable (like referring only to shapes that
exist).

New test cases have been added to guard against this regression in
the future.

Closes #2128
hpmellema pushed a commit to hpmellema/smithy that referenced this issue Feb 22, 2024
There was a regression in smithy-lang#1890 that allowed ERROR events to be
suppressed. This was not intentional and we do not want to remove this
restriction. ERROR events indicate that the model is in an unusable
state because it violates a MUST in the spec or some other necessary
requirement to make it usable (like referring only to shapes that
exist).

New test cases have been added to guard against this regression in
the future.

Closes smithy-lang#2128
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 a pull request may close this issue.

2 participants