Fix 3 bugs in @length-constrained collection and map shapes
#2085
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
The 3 bugs are related, hence why a single commit to address them all. 1. #2028 added support for the `@length` constraint trait on collection shapes, but the code enforcing the trait was not being exercised upon receiving a request, specifically when converting the input unconstrained list into the constrained one. Note that #2028 did not result in the removal of the relevant protocol tests from `ServerProtocolTestGenerator`'s list of known failing tests. 2. Fixes code generation of `@length`-constrained list shapes whose members are not constrained: the converter being generated only worked for the case where the member was (transitively) constrained. The `constraints.smithy` model has been expanded to cover this case. 3. Fixes bug in code generation, when the `codegenConfig.publicConstrainedTypes` setting is set to `false`, of `@length`-constrained map shapes and collection shapes whose values or members (respectively) are constrained, but result in Rust types that would have been public regardless of the setting's value. This is the case only when they are modeled as structure shapes or union shapes. In these cases, two converters from the constrained type to the inner type were being generated, resulting in two `From` trait implementations. The `constraints.smithy` model has been expanded to cover this case.
LukeMathWalker
previously approved these changes
Dec 9, 2022
crisidev
approved these changes
Dec 9, 2022
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
LukeMathWalker
approved these changes
Dec 9, 2022
…lection-and-map-shapes
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
LukeMathWalker
deleted the
davidpz/fix-3-bugs-in-length-constrained-collection-and-map-shapes
branch
david-perez
added a commit
that referenced
this pull request
Dec 14, 2022
…strained The generated code these should have emitted was fixed in #2085 (it's bug number 2), but code generation is still crashing because the call to calculate the inner constraint violation symbol is performed _before_ checking that the collection's member can reach a constrained shape. The test that #2085 added in `constraints.smithy`: ```smithy @Length(max: 69) list LengthList { member: ConB } ``` was not exercising what it should have, since `ConB`, is its name hints at, is a constrained structure shape.
This was referenced Dec 14, 2022
jjant
added a commit
that referenced
this pull request
Dec 15, 2022
…strained (#2103) * Fix `@length`-constrained collection shapes whose members are not constrained The generated code these should have emitted was fixed in #2085 (it's bug number 2), but code generation is still crashing because the call to calculate the inner constraint violation symbol is performed _before_ checking that the collection's member can reach a constrained shape. The test that #2085 added in `constraints.smithy`: ```smithy @Length(max: 69) list LengthList { member: ConB } ``` was not exercising what it should have, since `ConB`, is its name hints at, is a constrained structure shape. * Add changelog entry Co-authored-by: Julian Antonielli <[email protected]>
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.
The 3 bugs are related, hence why a single commit to address them all.
@lengthon collections #2028 added support for the@lengthconstraint trait on collectionshapes, but the code enforcing the trait was not being exercised upon
receiving a request, specifically when converting the input
unconstrained list into the constrained one. Note that Implement
@lengthon collections #2028 did notresult in the removal of the relevant protocol tests from
ServerProtocolTestGenerator's list of known failing tests.@length-constrained list shapes whosemembers are not constrained: the converter being generated only
worked for the case where the member was (transitively) constrained.
The
constraints.smithymodel has been expanded to cover this case.codegenConfig.publicConstrainedTypessetting is set tofalse, of@length-constrained map shapes and collection shapes whose valuesor members (respectively) are constrained, but result in Rust types
that would have been public regardless of the setting's value. This
is the case only when they are modeled as structure shapes or union
shapes. In these cases, two converters from the constrained type to
the inner type were being generated, resulting in two
Fromtraitimplementations. The
constraints.smithymodel has been expanded tocover this case.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.