Make selector attributes stricter #946
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
Selector attributes are curretly *too* permissive, and this makes it harr to know when a selector is invalid. The original intent of making selector attributes permissive was to make it possible to add new attributes and nested attribute values without needing to version the Smithy specification, but this looseness ended up making it really hard to understand if you were using the current version of the specification correctly. For example, consider the following selector that previously worked: ``` [id|com.foo#Baz] ``` Smithy previoulsy would let this through, and it didn't emit any warnings. This should have been: ``` [id=com.foo#Baz] ``` It's in our users' best interest for us to be stricer about attributes so that they know when they use them incorrectly. Even as the designer of selectors, I got this wrong, and struggled for about 20 minutes trying to figure out why. This change no longer allows wildcard access to top level shapes, the id attribute, or the service attribute. While this _is_ a breaking change, it's a desirable breaking change that will only serve to reveal bugs in selectors. The following selecors are examples that will no longer work: * `[id|com.foo#Baz]` (invalid wildcard access of id) * `[foo]` (invalid top-level attribute) * `[id|foo]` (invalid wildcard access of id) * `[service|foo]` (invalid wildcard access of service) * `[service|id|foo]` (invalid wildcard access of id) Attempting to access traits that do not exist, variables that do not exist, values of traits or variables that do not exist, or to descend into literal scalar values will continue to work and will continue to return an empty value: * `[id|name|foo]` (descending into scalars is still ok. This might be a legitimate use case when using heterogenous projections, for example). * `[vars|doesNotExist|nested]` (accessing unknown variables is ok) * `[service|id|name|foo]` (descending into scalars is ok, even nested) * `[trait|doesNotExist]` (accessing wildcard traits, even unknown traits is ok).
JordonPhillips
approved these changes
Oct 20, 2021
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.
Selector attributes are curretly too permissive, and this makes it
harr to know when a selector is invalid. The original intent of making
selector attributes permissive was to make it possible to add new
attributes and nested attribute values without needing to version the
Smithy specification, but this looseness ended up making it really hard
to understand if you were using the current version of the specification
correctly.
For example, consider the following selector that previously worked:
Smithy previoulsy would let this through, and it didn't emit any
warnings. This should have been:
It's in our users' best interest for us to be stricer about attributes
so that they know when they use them incorrectly. Even as the designer
of selectors, I got this wrong, and struggled for about 20 minutes
trying to figure out why.
This change no longer allows wildcard access to top level shapes, the id
attribute, or the service attribute. While this is a breaking change,
it's a desirable breaking change that will only serve to reveal bugs in
selectors.
The following selecors are examples that will no longer work:
[id|com.foo#Baz](invalid wildcard access of id)[foo](invalid top-level attribute)[id|foo](invalid wildcard access of id)[service|foo](invalid wildcard access of service)[service|id|foo](invalid wildcard access of id)Attempting to access traits that do not exist, variables that do not
exist, values of traits or variables that do not exist, or to descend
into literal scalar values will continue to work and will continue to
return an empty value:
[id|name|foo](descending into scalars is still ok. This might be alegitimate use case when using heterogenous projections, for example).
[vars|doesNotExist|nested](accessing unknown variables is ok)[service|id|name|foo](descending into scalars is ok, even nested)[trait|doesNotExist](accessing wildcard traits, even unknown traitsis ok).
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.