From f1bbdc6e73358e7fee038d874a2611dfedbc4c50 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Tue, 19 Oct 2021 20:09:22 -0700 Subject: [PATCH] Make selector attributes stricter 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). --- docs/source/1.0/spec/core/selectors.rst | 46 ++++++++++++------- .../model/selector/AttributeValueImpl.java | 7 ++- .../model/selector/SelectorException.java | 29 ++++++++++++ .../selector/SelectorSyntaxException.java | 2 +- .../smithy/model/selector/SelectorTest.java | 12 ++--- .../linters/emit-each-selector-validator.json | 11 ++++- 6 files changed, 75 insertions(+), 32 deletions(-) create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/selector/SelectorException.java diff --git a/docs/source/1.0/spec/core/selectors.rst b/docs/source/1.0/spec/core/selectors.rst index 8942f00e99c..cf4683e7bee 100644 --- a/docs/source/1.0/spec/core/selectors.rst +++ b/docs/source/1.0/spec/core/selectors.rst @@ -370,19 +370,6 @@ above selector: [trait | range | min = 1 ] -Accessing an attribute or nested attribute property that does not exist -returns an *empty value*. An empty value does not satisfy existence checks, -returns an empty string when used with string comparators, and returns an -empty value when attempting to access any properties. - -The following selector attempts to descend into non-existent properties of -the :ref:`documentation-trait`. This example MUST NOT cause an error and -MUST NOT match any shapes: - -.. code-block:: none - - [trait|documentation|invalid|child = Hi] - .. _id-attribute: @@ -544,10 +531,11 @@ to a shape. The ``trait`` attribute supports the following properties: [trait|(length) > 10] ``*`` - Other values are treated as shape IDs, where a relative shape ID is + Any other value is treated as a shape ID, where a relative shape ID is resolved to the ``smithy.api`` namespace. If a matching trait with the given shape ID is attached to the shape, it's :ref:`node value ` - is returned. An empty value is returned if the trait does not exist. + is returned. An :ref:`empty value ` is returned if the + trait does not exist. The following selector matches shapes that have the :ref:`deprecated-trait`: @@ -661,6 +649,30 @@ values return empty strings when used by string comparators. [trait|externalDocumentation|'Reference Docs'] + Attempting to access a nested property that does not exist or + attempting to descend into nested values of a scalar type returns + an :ref:`empty value `. + + +.. _empty-attributes: + +Empty attribute +--------------- + +Attempting to access a trait that does not exist, a variable that does +not exist, or attempting to descend into node attribute values that do not +exist returns an *empty value*. An empty value does not satisfy existence +checks, returns an empty string when used with string comparators, and +returns an empty value when attempting to access any properties. + +The following selector attempts to descend into non-existent properties of +the :ref:`documentation-trait`. This example MUST NOT cause an error and +MUST NOT match any shapes: + +.. code-block:: none + + [trait|documentation|invalid|child = Hi] + .. _projection-attribute: @@ -1491,8 +1503,8 @@ A *var attribute* is an object accessible from a shape that provides access to the named :ref:`variables ` currently in scope. Variables are accessed by providing the variable name after ``var``. The values returned from ``var`` are :ref:`projections ` -that contain the set of shapes that were bound to the variable, or an empty -value if the variable does not exist. +that contain the set of shapes that were bound to the variable, or an +:ref:`empty value ` if the variable does not exist. The following selector finds all operations in the closure of a service where the operation has an :ref:`auth-trait` that is not a subset of the diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/AttributeValueImpl.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/AttributeValueImpl.java index 0b51d6f3e86..dbff9c6803f 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/AttributeValueImpl.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/AttributeValueImpl.java @@ -313,7 +313,7 @@ public AttributeValue getProperty(String key) { } else if (key.equals("id")) { return AttributeValue.id(service.getId()); } else { - return EMPTY; + throw new SelectorException("Invalid nested 'service' selector attribute property: " + key); } } } @@ -357,7 +357,7 @@ public AttributeValue getProperty(String property) { // Length returns the length of the shape ID. return AttributeValue.literal(id.toString().length()); default: - return EMPTY; + throw new SelectorException("Invalid nested 'id' selector attribute property: " + property); } } } @@ -445,8 +445,7 @@ public AttributeValue getProperty(String property) { case "var": return new VariableValue(vars); default: - LOGGER.warning("Unsupported shape selector attribute: " + property); - return EMPTY; + throw new SelectorException("Invalid shape selector attribute: " + property); } } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/SelectorException.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/SelectorException.java new file mode 100644 index 00000000000..16b8afb974e --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/SelectorException.java @@ -0,0 +1,29 @@ +/* + * Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.smithy.model.selector; + +/** + * Exception thrown when a selector evaluation is invalid. + */ +public class SelectorException extends RuntimeException { + public SelectorException(String message) { + super(message); + } + + public SelectorException(String message, Throwable previous) { + super(message, previous); + } +} diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/SelectorSyntaxException.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/SelectorSyntaxException.java index 6599b15182f..1bb7263e433 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/SelectorSyntaxException.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/SelectorSyntaxException.java @@ -18,7 +18,7 @@ /** * Exception thrown when a selector expression is invalid. */ -public final class SelectorSyntaxException extends RuntimeException { +public final class SelectorSyntaxException extends SelectorException { SelectorSyntaxException(String message, String expression, int pos, int line, int column) { super(createMessage(message, expression, pos, line, column)); } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/selector/SelectorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/selector/SelectorTest.java index c0e2f6d0530..3de9537555f 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/selector/SelectorTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/selector/SelectorTest.java @@ -593,8 +593,8 @@ public void invalidNumbersFailsGracefully() { } @Test - public void toleratesGettingNullPropertyFromString() { - assertThat(ids(traitModel, "[id|no|no=100]"), empty()); + public void doesNotToleratesInvalidIdAccess() { + Assertions.assertThrows(SelectorException.class, () -> ids(traitModel, "[id|no|no=100]")); } @Test @@ -633,12 +633,8 @@ public void selectsServiceVersions() { } @Test - public void toleratesUnknownServicePaths() { - Set services1 = ids(traitModel, "[service|foo|baz='bam']"); - Set services2 = ids(traitModel, "[service|foo|baz]"); - - assertThat(services1, empty()); - assertThat(services2, empty()); + public void doesNotTolerateUnknownServicePaths() { + Assertions.assertThrows(SelectorException.class, () -> ids(traitModel, "[service|foo|baz='bam']")); } @Test diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/linters/emit-each-selector-validator.json b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/linters/emit-each-selector-validator.json index 42daab9d390..d311b4ad836 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/linters/emit-each-selector-validator.json +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/linters/emit-each-selector-validator.json @@ -400,14 +400,21 @@ "id": "ignored", "name": "EmitEachSelector", "configuration": { - "selector": "[foo]" + "selector": "[trait|notRealTrait|notFound]" } }, { "id": "ignored", "name": "EmitEachSelector", "configuration": { - "selector": "[foo=baz]" + "selector": "[var|notRealVar|notFound]" + } + }, + { + "id": "ignored", + "name": "EmitEachSelector", + "configuration": { + "selector": "[trait|error|nesting|into|things|is|tolerated]" } }, {