Use structure member parsing for lists and maps #1782
Conversation
| }, | ||
| "com.test#WrongMemberList": { | ||
| "type": "list", | ||
| "foo": { |
There was a problem hiding this comment.
Does this not cause an error because we first check for the required members before checking for extraneous members?
There was a problem hiding this comment.
No I think we check for extra members first: https://github.com/awslabs/smithy/blob/7342b2c5168dc868c49241e22632a18ca63438bb/smithy-model/src/main/java/software/amazon/smithy/model/loader/AstModelLoader.java#L237, but It just causes a warning: https://github.com/awslabs/smithy/blob/7342b2c5168dc868c49241e22632a18ca63438bb/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderUtils.java#L44.
There was a problem hiding this comment.
Ah I didn't see an error about the extra members in the test. Maybe it's not working as intended.
There was a problem hiding this comment.
I stepped through the code and I think its this: https://github.com/awslabs/smithy/blob/7342b2c5168dc868c49241e22632a18ca63438bb/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java#L575
Since there were errors loading, the assembler doesn't return the warnings.
...urces/software/amazon/smithy/model/errorfiles/loader/idl-invalid-list-and-map-members.errors
Outdated
Show resolved
Hide resolved
| ShapeId expectedKeyId = getId().withMember("key"); | ||
| if (!key.getId().equals(expectedKeyId)) { | ||
| throw new SourceException(String.format( | ||
| "Expected member of `%s` to have an ID of `%s` but found `%s`", |
There was a problem hiding this comment.
Would it be possible to move this validation here and below into the builder, like what you did with CollectionShape?
There was a problem hiding this comment.
I think this is different validation, and CollectionShape has it too: https://github.com/awslabs/smithy/blob/5636d15d067a63613cb9a1a069585fc81312df6b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/CollectionShape.java#L39-L44 I couldn't find any tests for it, so I didn't want to mess with it. But the validation I added to MapShape is in the builder, same as CollectionShape.
There was a problem hiding this comment.
I confused myself with this one. I did add these checks in the MapShape constructor, not the one in CollectionShape. They aren't necessary though because the constructor calls validateMemberShapeIds which does the same thing. I'll push an update to remove them.
smithy-model/src/main/java/software/amazon/smithy/model/loader/IdlModelLoader.java
Outdated
Show resolved
Hide resolved
| @@ -108,39 +109,69 @@ protected void validateMixins(Map<ShapeId, Shape> mixins, Map<ShapeId, Trait> in | |||
| } | |||
| } | |||
|
|
|||
| protected MemberShape getRequiredMixinMember( | |||
| protected Map<String, MemberShape> getRequiredMembers( | |||
There was a problem hiding this comment.
I think this could be a MemberShape[] since we don't really need a dynamically sized List, nor do we need a Map since the caller of this methods knows the order of the provided member names.
smithy-model/src/main/java/software/amazon/smithy/model/shapes/Shape.java
Outdated
Show resolved
Hide resolved
| return members; | ||
| } | ||
|
|
||
| protected Optional<MemberShape> getRequiredMixinMember( |
There was a problem hiding this comment.
Since this is all internal stuff, can it just have a nullable return value?
milesziemer
force-pushed
the
merge-parsing-of-list-map-and-structure-members
branch
from
a5efba9 to
4d71721
Compare
This commit moves the validation of list and map members out of the
parser. List and map members have special validation, because they
can only have specific members, and those members can't have default
values. Prior to this commit, we did this validation in the parser,
throwing a parse error if default value syntax sugar was used, or
incorrect member names were used. This meant we needed special
parsing for list and map shape members. It also lead to situations
like the following:
```
list Foo {
member: String = "foo"
}
```
fails to parse, saying it expected a "}" but got a "=", while
```
list Foo {
@default("foo")
member: String
}
```
fails model validation, explaining you can't assign a default to
a list member. Removing special handling of list and map members
consolidates this logic and simplifies the parser.
Because we previously threw parse errors when encountering unexpected
members, this PR had to update CollectionShape and MapShape to throw
SourceExceptions, which become validation events, when the incorrect
members were added to their respective builders. Since both shapes
constructors require the builder as an argument, you can't construct
the shapes with the wrong members.
For testing, cases were added for invalid map and list members, and
for default value syntax on list and map members. Some old test cases
that expected the parse error were removed or adjusted.
Previously, an error was only emitted for the first member that was found to be missing. So if you had an empty map shape, it would just say you're missing the key. This commit makes the error message say that both key and value are missing. To do this, Shape was updated, adding a getRequiredMembers method which stores a list of missing members, throwing an error after all required members have been checked.
Previously, we didn't have to give dedicated error messages when you try to mixin a list/map with missing or incorrect members, because parsing would fail. So we didn't have test cases verifying this behavior.
milesziemer
force-pushed
the
merge-parsing-of-list-map-and-structure-members
branch
3 times, most recently
from
cabdb36 to
e23fa78
Compare
milesziemer
force-pushed
the
merge-parsing-of-list-map-and-structure-members
branch
from
e23fa78 to
9777699
Compare
* Use structure member parsing for lists and maps
This commit moves the validation of list and map members out of the
parser. List and map members have special validation, because they
can only have specific members, and those members can't have default
values. Prior to this commit, we did this validation in the parser,
throwing a parse error if default value syntax sugar was used, or
incorrect member names were used. This meant we needed special
parsing for list and map shape members. It also lead to situations
like the following:
```
list Foo {
member: String = "foo"
}
```
fails to parse, saying it expected a "}" but got a "=", while
```
list Foo {
@default("foo")
member: String
}
```
fails model validation, explaining you can't assign a default to
a list member. Removing special handling of list and map members
consolidates this logic and simplifies the parser.
Because we previously threw parse errors when encountering unexpected
members, this PR had to update CollectionShape and MapShape to throw
SourceExceptions, which become validation events, when the incorrect
members were added to their respective builders. Since both shapes
constructors require the builder as an argument, you can't construct
the shapes with the wrong members.
For testing, cases were added for invalid map and list members, and
for default value syntax on list and map members. Some old test cases
that expected the parse error were removed or adjusted.
This also merges the grammar for list, union, and map shapes.
* Emit both wrong and missing member errors
This change makes the loader emit error messages for both
incorrect members and missing members in lists and maps.
Previously, a single error would stop the loader from continuing
to load the shape.
* Remove extra shape id checks in list/map ctor
Removes extraneous checks in the list/map constructor that
were validating the shape id for list/map members. This validation
is now performed when the members are collected, and errors are
emitted when the members are missing.
* Emit error for all missing members
Previously, an error was only emitted for the first member that
was found to be missing. So if you had an empty map shape, it would
just say you're missing the key. This commit makes the error message
say that both key and value are missing. To do this, Shape was
updated, adding a getRequiredMembers method which stores a list
of missing members, throwing an error after all required members
have been checked.
* Add test cases for invalid list/map mixins
Previously, we didn't have to give dedicated error messages when
you try to mixin a list/map with missing or incorrect members,
because parsing would fail. So we didn't have test cases verifying
this behavior.
This commit moves the validation of list and map members out of the parser. List and map members have special validation, because they can only have specific members, and those members can't have default values. Prior to this commit, we did this validation in the parser, throwing a parse error if default value syntax sugar was used, or incorrect member names were used. This meant we needed special parsing for list and map shape members. It also lead to situations like the following:
fails to parse, saying it expected a "}" but got a "=", while
fails model validation, explaining you can't assign a default to a list member. Removing special handling of list and map members consolidates this logic and simplifies the parser.
Because we previously threw parse errors when encountering unexpected members, this PR had to update CollectionShape and MapShape to throw SourceExceptions, which become validation events, when the incorrect members were added to their respective builders. Since both shapes constructors require the builder as an argument, you can't construct the shapes with the wrong members.
For testing, cases were added for invalid map and list members, and for default value syntax on list and map members. Some old test cases that expected the parse error were removed or adjusted. The AST didn't previously throw errors when extra members were encountered, so this PR maintains that behavior.
The grammar was also updated for these changes.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.