-
Notifications
You must be signed in to change notification settings - Fork 218
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
Improve IDL 1 and 2 interoperability #1394
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 9, 2022
mtdowling
force-pushed
the
model-evolution-updates
branch
from
September 9, 2022 17:46
f9fc6ae
to
5f1daf0
Compare
mtdowling
commented
Sep 9, 2022
smithy-model/src/main/java/software/amazon/smithy/model/traits/TagsTrait.java
Show resolved
Hide resolved
mtdowling
force-pushed
the
model-evolution-updates
branch
3 times, most recently
from
September 9, 2022 21:48
304eb3b
to
4abd94c
Compare
This commit makes several significant updates to Smithy IDL 2 that improve lossless interoperability with Smithy IDL 1: * Default traits can now coexist with required. This indicates that a member should be serialized, but it is a protocol-specific decision if and how this is enforced. This was a pattern that occurred in Smithy 1.0 models when a member was required and targeted a shape with a zero value. * Default traits can be added to root-level shapes. Any structure member that targets a shape marked with the default trait must repeat the default on the member. This removes the action at a distance problem observed in Smithy IDL 1 where a root level shape implicitly introduced a default zero value, and to know if that's the case for any member, you had to look through from the member to the target shape. This change allows us to know if a root level shape was boxed in IDL 1.0 too -- if it is a shape that had a zero value in IDL 1 and either doesn't have a default or the default is not the zero value, then it was boxed in v1. * When loading v2 models, we will now patch synthetic box traits onto shapes that would have been considered boxed by existing smithy-model consumers. This improves further interop with tooling that has not yet adopted Smithy IDL 2 or that hasn't yet migrated to use the NullableIndex abstraction. * Added an optional nullability report to smithy-build that shows the computed nullability semantics of each member in a model. This can be used to better understand nullability semantics. * Updated smithy-diff to not emit events when diffing a 1.0 model against the 2.0 serialized version of the model. This means that changes to the box trait are ignored unless the change impacts the nullability of the shape. Special handling was added to detect breaking changes with the default trait too (you can't change a default value of a root-level shape for example, you can't change a default value of a shape to or from the zero value of a type as this might break code generators, etc). * Add new NullableIndex modes for testing if a member is nullable based on the supported features of the generator. For example, some generators only make members non-optional when the member is set to the zero value of a type, so there is a NullableIndex check mode for that and other use cases. * Added the @addedDefault trait which is used to indicate that a `@default` trait was added to a member after it was initially released. This can be used by tooling to make an appropriate determination if generating a non-nullable type for the member is a backward compatible change. For example, if a generator only uses default zero values to generate non-nullable types, then the removal of the required trait and addition of a default trait would be a breaking change for them, so they can use addedDefault to ignore the default trait. * When loading 1.0 models, rather than dropping the default trait from a member when the range trait of a shape is invalid for its zero value, we now instead emit only a warning for this specific case. This prevents changing the type and also doesn't lose the range constraint in case implementations are relying on this validation. * Un-deprecated Primitive* shapes in the prelude and add the `@default` trait to them. Any member that targets one of these primitive prelude shapes must now also repeat the zero value of the target shape. * Fixed an issue where synthetic box traits were being dropped when the same model is loaded multiple times.
mtdowling
force-pushed
the
model-evolution-updates
branch
from
September 10, 2022 22:53
2391870
to
72e4b5a
Compare
gosar
reviewed
Sep 11, 2022
smithy-build/src/test/resources/software/amazon/smithy/build/plugins/nullability/example.smithy
Show resolved
Hide resolved
smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedDefault.java
Outdated
Show resolved
Hide resolved
.../src/main/java/software/amazon/smithy/model/validation/validators/DefaultTraitValidator.java
Outdated
Show resolved
Hide resolved
smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedDefaultTest.java
Show resolved
Hide resolved
smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedDefaultTest.java
Show resolved
Hide resolved
smithy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/box.v1.smithy
Outdated
Show resolved
Hide resolved
smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelUpgrader.java
Outdated
Show resolved
Hide resolved
smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelUpgrader.java
Outdated
Show resolved
Hide resolved
.../src/main/java/software/amazon/smithy/model/validation/validators/DefaultTraitValidator.java
Outdated
Show resolved
Hide resolved
smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java
Show resolved
Hide resolved
gosar
approved these changes
Sep 11, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All my comments are non-blocking, so marking Approved.
mtdowling
force-pushed
the
model-evolution-updates
branch
from
September 11, 2022 16:06
798e8c4
to
ac28bd2
Compare
gosar
reviewed
Sep 11, 2022
...cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/primitive.v2.smithy
Show resolved
Hide resolved
events.add(error(change.getNewShape(), | ||
"@default trait was removed. This will break previously generated code.")); | ||
} else if (oldTrait == null) { | ||
if (change.getNewShape().getType() != ShapeType.MEMBER) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can also be changed to !change.getNewShape().isMemberShape()
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit makes several significant updates to Smithy IDL 2 that
improve lossless interoperability with Smithy IDL 1:
a member should be serialized, but it is a protocol-specific
decision if and how this is enforced. This was a pattern that occurred
in Smithy 1.0 models when a member was required and targeted a shape
with a zero value.
that targets a shape marked with the default trait must repeat the
default on the member. This removes the action at a distance problem
observed in Smithy IDL 1 where a root level shape implicitly introduced
a default zero value, and to know if that's the case for any member, you
had to look through from the member to the target shape. This change
allows us to know if a root level shape was boxed in IDL 1.0 too -- if
it is a shape that had a zero value in IDL 1 and either doesn't have
a default or the default is not the zero value, then it was boxed in v1.
that would have been considered boxed by existing smithy-model consumers.
This improves further interop with tooling that has not yet adopted
Smithy IDL 2 or that hasn't yet migrated to use the NullableIndex
abstraction.
computed nullability semantics of each member in a model. This can be
used to better understand nullability semantics.
the 2.0 serialized version of the model. This means that changes to the
box trait are ignored unless the change impacts the nullability of the
shape. Special handling was added to detect breaking changes with the
default trait too (you can't change a default value of a root-level shape
for example, you can't change a default value of a shape to or from the
zero value of a type as this might break code generators, etc).
the supported features of the generator. For example, some generators only
make members non-optional when the member is set to the zero value of a
type, so there is a NullableIndex check mode for that and other use cases.
@default
trait was added to a member after it was initially released. This can be
used by tooling to make an appropriate determination if generating a
non-nullable type for the member is a backward compatible change. For
example, if a generator only uses default zero values to generate
non-nullable types, then the removal of the required trait and addition
of a default trait would be a breaking change for them, so they can use
addedDefault to ignore the default trait.
member when the range trait of a shape is invalid for its zero value, we
now instead emit only a warning for this specific case. This prevents
changing the type and also doesn't lose the range constraint in case
implementations are relying on this validation.
@default
trait to them. Any member that targets one of these primitive prelude
shapes must now also repeat the zero value of the target shape.
same model is loaded multiple times.