diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/transform/plugins/CleanOperationStructures.java b/smithy-model/src/main/java/software/amazon/smithy/model/transform/plugins/CleanOperationStructures.java index ca066dafc4c..b5ee7974591 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/transform/plugins/CleanOperationStructures.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/transform/plugins/CleanOperationStructures.java @@ -7,9 +7,8 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; +import java.util.List; import java.util.Set; -import java.util.stream.Collectors; -import java.util.stream.Stream; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.shapes.OperationShape; import software.amazon.smithy.model.shapes.Shape; @@ -28,42 +27,67 @@ public Model onRemove(ModelTransformer transformer, Collection removed, M } private Collection getModifiedOperations(Model model, Collection removed) { - return model.shapes(OperationShape.class) - .flatMap(operation -> { - OperationShape result = transformErrors(removed, operation); - result = transformInput(removed, result); - result = transformOutput(removed, result); - return result.equals(operation) ? Stream.empty() : Stream.of(result); - }) - .collect(Collectors.toList()); + List modifiedShapes = new ArrayList<>(); + for (OperationShape operation : model.getOperationShapes()) { + OperationShape.Builder builder = transformInput(removed, operation); + builder = transformOutput(removed, operation, builder); + builder = transformErrors(removed, operation, builder); + if (builder != null) { + modifiedShapes.add(builder.build()); + } + } + return modifiedShapes; } - private OperationShape transformInput(Collection removed, OperationShape operation) { + private OperationShape.Builder transformInput( + Collection removed, + OperationShape operation + ) { for (Shape remove : removed) { if (remove.getId().equals(operation.getInputShape())) { - return operation.toBuilder().input(null).build(); + OperationShape.Builder builder = operation.toBuilder(); + builder.input(null); + return builder; } } - return operation; + return null; } - private OperationShape transformOutput(Collection removed, OperationShape operation) { + private OperationShape.Builder transformOutput( + Collection removed, + OperationShape operation, + OperationShape.Builder builder + ) { for (Shape remove : removed) { if (remove.getId().equals(operation.getOutputShape())) { - return operation.toBuilder().output(null).build(); + if (builder == null) { + builder = operation.toBuilder(); + } + builder.output(null); + return builder; } } - return operation; + return builder; } - private OperationShape transformErrors(Collection removed, OperationShape operation) { + private OperationShape.Builder transformErrors( + Collection removed, + OperationShape operation, + OperationShape.Builder builder + ) { Set errors = new HashSet<>(operation.getErrors()); - removed.forEach(shape -> errors.remove(shape.getId())); + for (Shape remove : removed) { + errors.remove(remove.getId()); + } - if (new ArrayList<>(errors).equals(operation.getErrors())) { - return operation; + if (errors.size() != operation.getErrors().size()) { + if (builder == null) { + builder = operation.toBuilder(); + } + builder.errors(errors); + return builder; } - return operation.toBuilder().errors(errors).build(); + return builder; } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/transform/plugins/CleanStructureAndUnionMembers.java b/smithy-model/src/main/java/software/amazon/smithy/model/transform/plugins/CleanStructureAndUnionMembers.java index 344e1a70ab5..1ea393d1786 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/transform/plugins/CleanStructureAndUnionMembers.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/transform/plugins/CleanStructureAndUnionMembers.java @@ -4,18 +4,15 @@ */ package software.amazon.smithy.model.transform.plugins; -import static java.util.stream.Collectors.groupingBy; -import static java.util.stream.Collectors.mapping; - import java.util.ArrayList; import java.util.Collection; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.function.Function; -import java.util.stream.Collectors; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.shapes.EnumShape; import software.amazon.smithy.model.shapes.IntEnumShape; @@ -26,8 +23,6 @@ import software.amazon.smithy.model.shapes.UnionShape; import software.amazon.smithy.model.transform.ModelTransformer; import software.amazon.smithy.model.transform.ModelTransformerPlugin; -import software.amazon.smithy.utils.OptionalUtils; -import software.amazon.smithy.utils.Pair; /** * Cleans up structure, union, enum, and intEnum shapes after shapes are removed. @@ -59,7 +54,9 @@ private Model removeMembersFromContainers(ModelTransformer transformer, Collecti private Collection getEnumReplacements(Model model, Collection removed) { return createUpdatedShapes(model, removed, Shape::asEnumShape, entry -> { EnumShape.Builder builder = entry.getKey().toBuilder(); - entry.getValue().forEach(member -> builder.removeMember(member.getMemberName())); + for (MemberShape member : entry.getValue()) { + builder.removeMember(member.getMemberName()); + } return builder.build(); }); } @@ -67,7 +64,9 @@ private Collection getEnumReplacements(Model model, Collection rem private Collection getIntEnumReplacements(Model model, Collection removed) { return createUpdatedShapes(model, removed, Shape::asIntEnumShape, entry -> { IntEnumShape.Builder builder = entry.getKey().toBuilder(); - entry.getValue().forEach(member -> builder.removeMember(member.getMemberName())); + for (MemberShape member : entry.getValue()) { + builder.removeMember(member.getMemberName()); + } return builder.build(); }); } @@ -75,7 +74,9 @@ private Collection getIntEnumReplacements(Model model, Collection private Collection getStructureReplacements(Model model, Collection removed) { return createUpdatedShapes(model, removed, Shape::asStructureShape, entry -> { StructureShape.Builder builder = entry.getKey().toBuilder(); - entry.getValue().forEach(member -> builder.removeMember(member.getMemberName())); + for (MemberShape member : entry.getValue()) { + builder.removeMember(member.getMemberName()); + } return builder.build(); }); } @@ -83,7 +84,9 @@ private Collection getStructureReplacements(Model model, Collection getUnionReplacements(Model model, Collection removed) { return createUpdatedShapes(model, removed, Shape::asUnionShape, entry -> { UnionShape.Builder builder = entry.getKey().toBuilder(); - entry.getValue().forEach(member -> builder.removeMember(member.getMemberName())); + for (MemberShape member : entry.getValue()) { + builder.removeMember(member.getMemberName()); + } return builder.build(); }); } @@ -115,16 +118,24 @@ private Collection createUpdatedShapes( Function> containerShapeMapper, Function>, S> entryMapperAndFactory ) { - return removed.stream() - .flatMap(shape -> OptionalUtils.stream(shape.asMemberShape())) - .flatMap(member -> OptionalUtils.stream(model.getShape(member.getContainer()) - .flatMap(containerShapeMapper) - .map(container -> Pair.of(container, member)))) - .collect(groupingBy(Pair::getLeft, mapping(Pair::getRight, Collectors.toList()))) - .entrySet() - .stream() - .map(entryMapperAndFactory) - .collect(Collectors.toList()); + Map> containerMemberMap = new HashMap<>(); + for (Shape shape : removed) { + if (!shape.isMemberShape()) { + continue; + } + + MemberShape member = (MemberShape) shape; + Optional container = model.getShape(member.getContainer()).flatMap(containerShapeMapper); + if (container.isPresent()) { + containerMemberMap.computeIfAbsent(container.get(), k -> new ArrayList<>()).add(member); + } + } + + Collection updatedShapes = new ArrayList<>(); + for (Map.Entry> entry : containerMemberMap.entrySet()) { + updatedShapes.add(entryMapperAndFactory.apply(entry)); + } + return updatedShapes; } /** @@ -137,16 +148,26 @@ private Collection createUpdatedShapes( * their target was removed. */ private Collection findMembersThatNeedRemoval(Model model, Collection removed) { - Set removedIds = removed.stream().map(Shape::getId).collect(Collectors.toSet()); + Set removedIds = new HashSet<>(); + for (Shape shape : removed) { + removedIds.add(shape.getId()); + } + Collection removeMembers = new HashSet<>(); - model.shapes(StructureShape.class) - .flatMap(shape -> shape.getAllMembers().values().stream()) - .filter(value -> removedIds.contains(value.getTarget())) - .forEach(removeMembers::add); - model.shapes(UnionShape.class) - .flatMap(shape -> shape.getAllMembers().values().stream()) - .filter(value -> removedIds.contains(value.getTarget())) - .forEach(removeMembers::add); + for (StructureShape structure : model.getStructureShapes()) { + for (MemberShape member : structure.members()) { + if (removedIds.contains(member.getTarget())) { + removeMembers.add(member); + } + } + } + for (UnionShape structure : model.getUnionShapes()) { + for (MemberShape member : structure.members()) { + if (removedIds.contains(member.getTarget())) { + removeMembers.add(member); + } + } + } return removeMembers; } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ResourceIdentifierBindingValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ResourceIdentifierBindingValidator.java index 3bbbfaa427b..7cd279b2d5c 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ResourceIdentifierBindingValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ResourceIdentifierBindingValidator.java @@ -9,15 +9,15 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.function.Function; import java.util.stream.Collectors; -import java.util.stream.Stream; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.knowledge.IdentifierBindingIndex; +import software.amazon.smithy.model.knowledge.IdentifierBindingIndex.BindingType; import software.amazon.smithy.model.shapes.MemberShape; import software.amazon.smithy.model.shapes.OperationShape; import software.amazon.smithy.model.shapes.ResourceShape; @@ -28,9 +28,6 @@ import software.amazon.smithy.model.validation.AbstractValidator; import software.amazon.smithy.model.validation.ValidationEvent; import software.amazon.smithy.model.validation.ValidationUtils; -import software.amazon.smithy.utils.FunctionalUtils; -import software.amazon.smithy.utils.OptionalUtils; -import software.amazon.smithy.utils.Pair; /** * Validates that operations bound to resource shapes have identifier @@ -122,31 +119,26 @@ private void validateResourceIdentifierTraitConflicts(Shape structure, List events) { IdentifierBindingIndex bindingIndex = IdentifierBindingIndex.of(model); - Stream.of( - model.shapes(ResourceShape.class) - .flatMap(resource -> validateResource(model, resource, bindingIndex)), - model.shapes(ResourceShape.class) - .flatMap(resource -> validateCollectionBindings(model, resource, bindingIndex)), - model.shapes(ResourceShape.class) - .flatMap(resource -> validateInstanceBindings(model, resource, bindingIndex))) - .flatMap(Function.identity()) - .forEach(events::add); + for (ResourceShape resource : model.getResourceShapes()) { + validateResource(model, resource, bindingIndex, events); + validateCollectionBindings(model, resource, bindingIndex, events); + validateInstanceBindings(model, resource, bindingIndex, events); + } } - private Stream validateResource( + private void validateResource( Model model, ResourceShape parent, - IdentifierBindingIndex bindingIndex + IdentifierBindingIndex bindingIndex, + List events ) { - return parent.getResources() - .stream() - .flatMap(childId -> OptionalUtils.stream(model.getShape(childId).flatMap(Shape::asResourceShape))) - .flatMap(child -> child.getAllOperations() - .stream() - .flatMap(id -> OptionalUtils.stream(model.getShape(id).flatMap(Shape::asOperationShape))) - .map(operation -> Pair.of(child, operation))) - .flatMap(pair -> OptionalUtils.stream( - validateOperation(parent, pair.getLeft(), pair.getRight(), bindingIndex))); + for (ShapeId childId : parent.getResources()) { + ResourceShape child = model.expectShape(childId, ResourceShape.class); + for (ShapeId operationId : child.getAllOperations()) { + OperationShape operation = model.expectShape(operationId, OperationShape.class); + validateOperation(parent, child, operation, bindingIndex).ifPresent(events::add); + } + } } private Optional validateOperation( @@ -155,18 +147,15 @@ private Optional validateOperation( OperationShape operation, IdentifierBindingIndex bindingIndex ) { - if (bindingIndex.getOperationBindingType(child, operation) != IdentifierBindingIndex.BindingType.NONE) { + if (bindingIndex.getOperationBindingType(child, operation) != BindingType.NONE) { Set bindings = bindingIndex.getOperationInputBindings(child, operation).keySet(); - Set missing = parent.getIdentifiers() - .keySet() - .stream() - .filter(FunctionalUtils.not(bindings::contains)) - .collect(Collectors.toSet()); + Set missing = new LinkedHashSet<>(parent.getIdentifiers().keySet()); + missing.removeAll(bindings); if (!missing.isEmpty()) { return Optional.of(error(operation, String.format( - "This operation is bound to the `%s` resource, which is a child of the `%s` resource, and " - + "it is missing the following resource identifier bindings of `%s`: [%s]", + "This operation is bound to the `%s` resource, which is a child of the `%s` resource, " + + "and it is missing the following resource identifier bindings of `%s`: [%s]", child.getId(), parent.getId(), parent.getId(), @@ -177,54 +166,55 @@ private Optional validateOperation( return Optional.empty(); } - private Stream validateCollectionBindings( + private void validateCollectionBindings( Model model, ResourceShape resource, - IdentifierBindingIndex identifierIndex + IdentifierBindingIndex bindingIndex, + List events ) { - return resource.getAllOperations() - .stream() - // Find all collection operations bound to the resource. - .filter(operation -> identifierIndex.getOperationBindingType(resource, - operation) == IdentifierBindingIndex.BindingType.COLLECTION) - // Get their operation shapes. - .flatMap(id -> OptionalUtils.stream(model.getShape(id).flatMap(Shape::asOperationShape))) - // Find collection operations which improperly bind all the resource identifiers. - .filter(operation -> hasAllIdentifiersBound(model, resource, operation, identifierIndex)) - .map(operation -> error(operation, + for (ShapeId operationId : resource.getAllOperations()) { + if (bindingIndex.getOperationBindingType(resource, operationId) != BindingType.COLLECTION) { + continue; + } + + OperationShape operation = model.expectShape(operationId, OperationShape.class); + if (hasAllIdentifiersBound(model, resource, operation, bindingIndex)) { + events.add(error(operation, format( - "This operation is bound as a collection operation on the `%s` resource, but it improperly " - + "binds all of the identifiers of the resource to members of the operation input.", + "This operation is bound as a collection operation on the `%s` resource, but it " + + "improperly binds all of the identifiers of the resource to members of the " + + "operation input.", resource.getId()))); + } + } } - private Stream validateInstanceBindings( + private void validateInstanceBindings( Model model, ResourceShape resource, - IdentifierBindingIndex bindingIndex + IdentifierBindingIndex bindingIndex, + List events ) { - return resource.getAllOperations() - .stream() - // Find all instance operations bound to the resource. - .filter(operation -> bindingIndex.getOperationBindingType(resource, - operation) == IdentifierBindingIndex.BindingType.INSTANCE) - // Get their operation shapes. - .flatMap(id -> OptionalUtils.stream(model.getShape(id).flatMap(Shape::asOperationShape))) - // Find instance operations which do not bind all of the resource identifiers. - .filter(operation -> !hasAllIdentifiersBound(model, resource, operation, bindingIndex)) - .map(operation -> { - String expectedIdentifiers = createBindingMessage(resource.getIdentifiers()); - String boundIds = createBindingMessage(bindingIndex.getOperationInputBindings(resource, operation)); - return error(operation, - format( - "This operation does not form a valid instance operation when bound to resource `%s`. " - + "All of the identifiers of the resource were not implicitly or explicitly bound " - + "to the input of the operation. Expected the following identifier bindings: " - + "[%s]. Found the following identifier bindings: [%s]", - resource.getId(), - expectedIdentifiers, - boundIds)); - }); + for (ShapeId operationId : resource.getAllOperations()) { + if (bindingIndex.getOperationBindingType(resource, operationId) != BindingType.INSTANCE) { + continue; + } + + OperationShape operation = model.expectShape(operationId, OperationShape.class); + if (!hasAllIdentifiersBound(model, resource, operation, bindingIndex)) { + String expectedIdentifiers = createBindingMessage(resource.getIdentifiers()); + String boundIds = createBindingMessage(bindingIndex.getOperationInputBindings(resource, operation)); + events.add(error(operation, + format( + "This operation does not form a valid instance operation when bound to resource `%s`. " + + "All of the identifiers of the resource were not implicitly or explicitly " + + "bound to the input of the operation. Expected the following identifier " + + "bindings: [%s]. Found the following identifier bindings: [%s]", + resource.getId(), + expectedIdentifiers, + boundIds))); + } + } } private boolean hasAllIdentifiersBound( diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/transform/RemoveShapesTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/transform/RemoveShapesTest.java index 463c196b6a5..195d806658d 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/transform/RemoveShapesTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/transform/RemoveShapesTest.java @@ -9,8 +9,10 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasKey; import static org.hamcrest.Matchers.not; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.ArrayList; import java.util.Arrays; @@ -41,9 +43,11 @@ import software.amazon.smithy.model.shapes.StructureShape; import software.amazon.smithy.model.shapes.UnionShape; import software.amazon.smithy.model.traits.AuthDefinitionTrait; +import software.amazon.smithy.model.traits.ErrorTrait; import software.amazon.smithy.model.traits.MixinTrait; import software.amazon.smithy.model.traits.ProtocolDefinitionTrait; import software.amazon.smithy.model.traits.ReadonlyTrait; +import software.amazon.smithy.utils.ListUtils; public class RemoveShapesTest { @@ -182,6 +186,31 @@ public void removesTaggedUnionMembersWhenRemoved() { assertContainerMembersAreRemoved(container, Arrays.asList(a, b)); } + @Test + public void removesOperationComponentsWhenStructureRemoved() { + ShapeId operationId = ShapeId.fromParts("ns.foo", "C"); + StructureShape a = StructureShape.builder().id("ns.foo#A").build(); + StructureShape b = StructureShape.builder() + .id("ns.foo#A") + .addTrait(new ErrorTrait("client")) + .build(); + OperationShape c = OperationShape.builder() + .id(operationId) + .input(a) + .output(a) + .addError(b) + .build(); + + Model model = Model.builder().addShapes(a, b, c).build(); + ModelTransformer transformer = ModelTransformer.create(); + Model result = transformer.removeShapes(model, ListUtils.of(a, b)); + + assertEquals(1, result.shapes().count()); + assertFalse(result.expectShape(operationId, OperationShape.class).getInput().isPresent()); + assertFalse(result.expectShape(operationId, OperationShape.class).getOutput().isPresent()); + assertTrue(result.expectShape(operationId, OperationShape.class).getErrors().isEmpty()); + } + @Test public void removesOperationsFromResourcesWhenOperationRemoved() { ResourceShape container = ResourceShape.builder()