From 6f6f0068d19270a591c9f1caf27e6fbc39e57267 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Sun, 28 Feb 2021 15:40:56 -0800 Subject: [PATCH] Improve Selector usability This commit improves the usability of Selectors by adding new methods to the Selector interface and by deprecating its runner functionality. The new methods include methods to stream matching shapes, stream matches, and consume matches. Streaming matches and shapes requires a minimal amount of buffering, and they provide the ability to more easily perform checks like determine if any shapes in a model match a selector. If all matching shapes are necessary, calling select is preferred. If consuming matches can be done in a way that is push-based (e.g., writing to CLI output), then using consumeMatches is preferred to matches. The previous runner() method of Selector is now deprecated and just delegates calls back to the Selector. runner() was initially intended to allow for the customization of the environment in which a selector is executed, but this design turned out to be unnecessary, and as such, the need to create a runner and use a kind of builder interface to use Selectors made them awkward to work with for no benefit. A final change is made to implement an IdentitySelector to optimize matching all shapes in a model. --- .../smithy/cli/commands/SelectCommand.java | 6 +- .../model/loader/ValidatorDefinition.java | 6 +- .../amazon/smithy/model/selector/Context.java | 12 +- .../model/selector/IdentitySelector.java | 60 +++++++ .../smithy/model/selector/Selector.java | 151 +++++++++++------- .../model/selector/WrappedSelector.java | 92 +++++++++-- .../linters/EmitEachSelectorValidator.java | 17 +- .../model/selector/IdentitySelectorTest.java | 56 +++++++ .../smithy/model/selector/SelectorTest.java | 65 ++++++-- .../model/selector/TopDownSelectorTest.java | 6 +- 10 files changed, 358 insertions(+), 113 deletions(-) create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/selector/IdentitySelector.java create mode 100644 smithy-model/src/test/java/software/amazon/smithy/model/selector/IdentitySelectorTest.java diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SelectCommand.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SelectCommand.java index 939c72b6165..9bdc7ddc68b 100644 --- a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SelectCommand.java +++ b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/SelectCommand.java @@ -87,10 +87,10 @@ public void execute(Arguments arguments, ClassLoader classLoader) { } else { // Show the JSON output for writing with --vars. List result = new ArrayList<>(); - selector.runner().model(model).selectMatches((shape, vars) -> { + selector.consumeMatches(model, match -> { result.add(Node.objectNodeBuilder() - .withMember("shape", Node.from(shape.getId().toString())) - .withMember("vars", collectVars(vars)) + .withMember("shape", Node.from(match.getShape().getId().toString())) + .withMember("vars", collectVars(match)) .build()); }); Cli.stdout(Node.prettyPrintJson(new ArrayNode(result, SourceLocation.NONE))); diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ValidatorDefinition.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ValidatorDefinition.java index 052b7e598ba..f653942efaa 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ValidatorDefinition.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ValidatorDefinition.java @@ -54,10 +54,8 @@ List map(Model model, List events) { // If there's a selector, create a list of candidate shape IDs that can be emitted. if (selector != null) { - candidates = selector.runner() - .model(model) - .selectShapes() - .stream() + candidates = selector + .shapes(model) .map(Shape::getId) .collect(Collectors.toSet()); } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/Context.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/Context.java index ad0391d59af..c1eddd8abb1 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/Context.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/Context.java @@ -20,7 +20,6 @@ import java.util.Set; import software.amazon.smithy.model.knowledge.NeighborProviderIndex; import software.amazon.smithy.model.shapes.Shape; -import software.amazon.smithy.utils.MapUtils; /** * Selector evaluation context object. @@ -28,7 +27,7 @@ final class Context { NeighborProviderIndex neighborIndex; - private Map> variables; + private final Map> variables; Context(NeighborProviderIndex neighborIndex) { this.neighborIndex = neighborIndex; @@ -45,15 +44,6 @@ Context clearVars() { return this; } - /** - * Copies the current set of variables to an immutable map. - * - * @return Returns a copy of the variables. - */ - Map> copyVars() { - return MapUtils.copyOf(variables); - } - /** * Gets the currently set variables. * diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/IdentitySelector.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/IdentitySelector.java new file mode 100644 index 00000000000..0fe9c5245d7 --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/IdentitySelector.java @@ -0,0 +1,60 @@ +/* + * 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; + +import java.util.Collections; +import java.util.Set; +import java.util.stream.Stream; +import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.shapes.Shape; + +/** + * An optimized Selector implementation that uses the provided Model directly + * rather than needing to send each shape through the Selector machinery. + * + * @see Selector#IDENTITY + */ +final class IdentitySelector implements Selector { + @Override + public Set select(Model model) { + return model.toSet(); + } + + @Override + public Stream shapes(Model model) { + return model.shapes(); + } + + @Override + public Stream matches(Model model) { + return model.shapes().map(shape -> new ShapeMatch(shape, Collections.emptyMap())); + } + + @Override + public String toString() { + return "*"; + } + + @Override + public boolean equals(Object other) { + return other instanceof Selector && toString().equals(other.toString()); + } + + @Override + public int hashCode() { + return toString().hashCode(); + } +} diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/Selector.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/Selector.java index 784af088c43..6a9008b6bb9 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/Selector.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/Selector.java @@ -1,5 +1,5 @@ /* - * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * 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. @@ -15,17 +15,18 @@ package software.amazon.smithy.model.selector; -import java.util.HashSet; +import java.util.HashMap; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.function.BiConsumer; +import java.util.function.Consumer; +import java.util.stream.Collectors; +import java.util.stream.Stream; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.SourceException; -import software.amazon.smithy.model.knowledge.NeighborProviderIndex; import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.shapes.Shape; -import software.amazon.smithy.utils.ListUtils; -import software.amazon.smithy.utils.SmithyBuilder; /** * Matches a set of shapes using a selector expression. @@ -33,7 +34,7 @@ public interface Selector { /** A selector that always returns all provided values. */ - Selector IDENTITY = new WrappedSelector("*", ListUtils.of(InternalSelector.IDENTITY)); + Selector IDENTITY = new IdentitySelector(); /** * Parses a selector expression. @@ -42,7 +43,11 @@ public interface Selector { * @return Returns the parsed {@link Selector}. */ static Selector parse(String expression) { - return SelectorParser.parse(expression); + if (expression.equals("*")) { + return IDENTITY; + } else { + return SelectorParser.parse(expression); + } } /** @@ -67,7 +72,65 @@ static Selector fromNode(Node node) { * @return Returns the matching shapes. */ default Set select(Model model) { - return runner().model(model).selectShapes(); + return shapes(model).collect(Collectors.toSet()); + } + + /** + * Matches a selector to a set of shapes and receives each matched shape + * with the variables that were set when the shape was matched. + * + * @param model Model to select shapes from. + * @param shapeMatchConsumer Receives each matched shape and the vars available when the shape was matched. + */ + default void consumeMatches(Model model, Consumer shapeMatchConsumer) { + matches(model).forEach(shapeMatchConsumer); + } + + /** + * Returns a stream of shapes in a model that match the selector. + * + * @param model Model to match the selector against. + * @return Returns a stream of matching shapes. + */ + Stream shapes(Model model); + + /** + * Returns a stream of {@link ShapeMatch} objects for each match found in + * a model. + * + * @param model Model to match the selector against. + * @return Returns a stream of {@code ShapeMatch} objects. + */ + Stream matches(Model model); + + /** + * Represents a selector match found in the model. + * + *

The {@code getShape} method is used to get the shape that matched, + * and all of the contextual variables that were set when the match + * occurred can be accessed using typical {@link Map} methods like + * {@code get}, {@code contains}, etc. + */ + final class ShapeMatch extends HashMap> { + private final Shape shape; + + /** + * @param shape Shape that matched. + * @param variables Variables that matched. This map is copied into ShapeMatch. + */ + public ShapeMatch(Shape shape, Map> variables) { + super(variables); + this.shape = shape; + } + + /** + * Gets the matching shape. + * + * @return Returns the matching shape. + */ + public Shape getShape() { + return shape; + } } /** @@ -76,79 +139,43 @@ default Set select(Model model) { * * @return Returns the created runner. */ - Runner runner(); + @Deprecated + default Runner runner() { + return new Runner(this); + } /** * Builds the execution environment for a selector and executes selectors. + * + * @deprecated This class is no longer necessary. It was originally intended + * to allow more customization to how selectors are executed against a model, + * but this has proven unnecessary. */ + @Deprecated final class Runner { - private final InternalSelector delegate; - private final Class startingShapeType; + private final Selector selector; private Model model; - Runner(InternalSelector delegate, Class startingShapeType) { - this.delegate = delegate; - this.startingShapeType = startingShapeType; + Runner(Selector selector) { + this.selector = selector; } - /** - * Sets the required model to use to select shapes with. - * - * @param model Model used in the selector evaluation. - * @return Returns the Runner. - */ + @Deprecated public Runner model(Model model) { this.model = model; return this; } - /** - * Runs the selector and returns the set of matching shapes. - * - * @return Returns the set of matching shapes. - * @throws IllegalStateException if a {@code model} has not been set. - */ + @Deprecated public Set selectShapes() { - Set result = new HashSet<>(); - pushShapes((ctx, s) -> { - result.add(s); - return true; - }); - return result; - } - - private Context createContext() { - SmithyBuilder.requiredState("model", model); - return new Context(NeighborProviderIndex.of(model)); + return selector.select(Objects.requireNonNull(model, "model not set")); } - /** - * Matches a selector to a set of shapes and receives each matched shape - * with the variables that were set when the shape was matched. - * - * @param matchConsumer Receives each matched shape and the vars available when the shape was matched. - * @throws IllegalStateException if a {@code model} has not been set. - */ + @Deprecated public void selectMatches(BiConsumer>> matchConsumer) { - pushShapes((ctx, s) -> { - matchConsumer.accept(s, ctx.copyVars()); - return true; - }); - } - - private void pushShapes(InternalSelector.Receiver acceptor) { - Context context = createContext(); - - if (startingShapeType != null) { - model.shapes(startingShapeType).forEach(shape -> { - delegate.push(context.clearVars(), shape, acceptor); - }); - } else { - for (Shape shape : model.toSet()) { - delegate.push(context.clearVars(), shape, acceptor); - } - } + selector.consumeMatches(Objects.requireNonNull(model, "model not set"), + m -> matchConsumer.accept(m.getShape(), m)); } } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/selector/WrappedSelector.java b/smithy-model/src/main/java/software/amazon/smithy/model/selector/WrappedSelector.java index 6645551f794..ce74d2015a6 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/selector/WrappedSelector.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/selector/WrappedSelector.java @@ -1,5 +1,5 @@ /* - * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * 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. @@ -15,14 +15,18 @@ package software.amazon.smithy.model.selector; +import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; +import java.util.function.Consumer; +import java.util.stream.Stream; +import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.knowledge.NeighborProviderIndex; import software.amazon.smithy.model.shapes.Shape; /** * Provides a toString method that prints the expression. - * - *

This is the only concrete implementation of Selector that is - * exported from the package. */ final class WrappedSelector implements Selector { private final String expression; @@ -47,11 +51,6 @@ final class WrappedSelector implements Selector { } } - @Override - public Runner runner() { - return new Runner(delegate, startingShapeType); - } - @Override public String toString() { return expression; @@ -59,11 +58,84 @@ public String toString() { @Override public boolean equals(Object other) { - return other instanceof WrappedSelector && expression.equals(((WrappedSelector) other).expression); + return other instanceof Selector && toString().equals(other.toString()); } @Override public int hashCode() { return expression.hashCode(); } + + @Override + public Set select(Model model) { + Set result = new HashSet<>(); + // This is more optimized than using shapes() and collecting to a Set + // because it avoids creating streams and buffering the result of + // pushing each shape into internal selectors. + pushShapes(model, (ctx, s) -> { + result.add(s); + return true; + }); + return result; + } + + @Override + public void consumeMatches(Model model, Consumer shapeMatchConsumer) { + // This is more optimized than using matches() and collecting to a Set + // because it avoids creating streams and buffering the result of + // pushing each shape into internal selectors. + pushShapes(model, (ctx, s) -> { + shapeMatchConsumer.accept(new ShapeMatch(s, ctx.getVars())); + return true; + }); + } + + @Override + public Stream shapes(Model model) { + Context context = createContext(model); + return streamStartingShape(model).flatMap(shape -> { + List result = new ArrayList<>(); + delegate.push(context.clearVars(), shape, (ctx, s) -> { + result.add(s); + return true; + }); + return result.stream(); + }); + } + + @Override + public Stream matches(Model model) { + Context context = createContext(model); + return streamStartingShape(model).flatMap(shape -> { + List result = new ArrayList<>(); + delegate.push(context.clearVars(), shape, (ctx, s) -> { + result.add(new ShapeMatch(s, ctx.getVars())); + return true; + }); + return result.stream(); + }); + } + + private Context createContext(Model model) { + return new Context(NeighborProviderIndex.of(model)); + } + + private void pushShapes(Model model, InternalSelector.Receiver acceptor) { + Context context = createContext(model); + + if (startingShapeType != null) { + model.shapes(startingShapeType).forEach(shape -> { + delegate.push(context.clearVars(), shape, acceptor); + }); + } else { + for (Shape shape : model.toSet()) { + delegate.push(context.clearVars(), shape, acceptor); + } + } + } + + private Stream streamStartingShape(Model model) { + // Optimization for selectors that start with a type. + return startingShapeType != null ? model.shapes(startingShapeType) : model.shapes(); + } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/linters/EmitEachSelectorValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/linters/EmitEachSelectorValidator.java index 87eb6318bbb..003c4ab1932 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/linters/EmitEachSelectorValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/linters/EmitEachSelectorValidator.java @@ -18,10 +18,8 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; -import java.util.Map; import java.util.Objects; import java.util.Optional; -import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; import software.amazon.smithy.model.FromSourceLocation; @@ -163,15 +161,14 @@ private FromSourceLocation determineEventLocation(Shape shape) { // into a BiConsumer and building up a mutated List of events. private List validateWithTemplate(Model model) { List events = new ArrayList<>(); - config.getSelector() - .runner() - .model(model) - .selectMatches((shape, vars) -> createTemplatedEvent(shape, vars).ifPresent(events::add)); + config.getSelector().consumeMatches(model, match -> { + createTemplatedEvent(match).ifPresent(events::add); + }); return events; } - private Optional createTemplatedEvent(Shape shape, Map> vars) { - FromSourceLocation location = determineEventLocation(shape); + private Optional createTemplatedEvent(Selector.ShapeMatch match) { + FromSourceLocation location = determineEventLocation(match.getShape()); // Only create a validation event if the bound trait (if any) is present on the shape. if (location == null) { return Optional.empty(); @@ -179,8 +176,8 @@ private Optional createTemplatedEvent(Shape shape, Map operation)\n" + + ":test(${operations}[trait|http])\n" + + "${operations}\n" + + ":not([trait|http])"; + private static Model modelJson; private static Model traitModel; + private static Model httpModel; @BeforeAll public static void before() { @@ -65,6 +74,11 @@ public static void before() { .addImport(SelectorTest.class.getResource("nested-traits.smithy")) .assemble() .unwrap(); + httpModel = Model.assembler() + .addImport(SelectorTest.class.getResource("http-model.smithy")) + .assemble() + .getResult() // ignore built-in errors + .get(); } @Test @@ -75,6 +89,23 @@ public void selectorEquality() { assertThat(a.hashCode(), equalTo(a.hashCode())); } + @Test + public void defaultSelectorInterfaceSelectCollectsStreamToSet() { + Selector test = new Selector() { + @Override + public Stream shapes(Model model) { + return model.shapes(); + } + + @Override + public Stream matches(Model model) { + return Stream.empty(); + } + }; + + assertThat(test.select(modelJson), equalTo(modelJson.toSet())); + } + @Test public void supportsDeprecatedEachFunction() { Set result1 = Selector.parse(":each(collection)").select(modelJson); @@ -968,17 +999,31 @@ public void detectsInvalidVariableAccess() { @Test public void findsOperationsMissingHttpBindings() { - Model model = Model.assembler() - .addImport(getClass().getResource("http-model.smithy")) - .assemble() - .getResult() // ignore built-in errors - .get(); - Set ids = exampleIds(model, "service\n" - + "$operations(~> operation)\n" - + ":test(${operations}[trait|http])\n" - + "${operations}\n" - + ":not([trait|http])"); + Set ids = exampleIds(httpModel, OPERATIONS_MISSING_BINDINGS); assertThat(ids, contains("smithy.example#NoHttp")); } + + @Test + public void returnsStreamOfShapes() { + Selector selector = Selector.parse(":test(string, map)"); + Set shapes = selector + .shapes(modelJson) + .collect(Collectors.toSet()); + + assertThat(shapes, equalTo(selector.select(modelJson))); + } + + @Test + public void returnsStreamOfMatches() { + Selector selector = Selector.parse(OPERATIONS_MISSING_BINDINGS); + Set matches = selector + .matches(httpModel) + .collect(Collectors.toSet()); + + Set matches2 = new HashSet<>(); + selector.consumeMatches(httpModel, matches2::add); + + assertThat(matches, equalTo(matches2)); + } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/selector/TopDownSelectorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/selector/TopDownSelectorTest.java index 06751598351..025167feb22 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/selector/TopDownSelectorTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/selector/TopDownSelectorTest.java @@ -79,9 +79,9 @@ public void topDownWithNoDisqualifiers() { public void topDownWithNoDisqualifiersWithServiceVariableFollowedByFilter() { Map matches = new HashMap<>(); Selector.parse("service $service(*) :topdown([trait|smithy.example#a]) resource") - .runner() - .model(model2) - .selectMatches((s, vars) -> matches.put(s.getId(), vars.get("service").iterator().next().getId())); + .consumeMatches(model2, match -> { + matches.put(match.getShape().getId(), match.get("service").iterator().next().getId()); + }); assertThat(matches, hasKey(ShapeId.from("smithy.example#R1"))); assertThat(matches.get(ShapeId.from("smithy.example#R1")), equalTo(ShapeId.from("smithy.example#Service1")));