Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validator to warn on ignored idempotency token trait #2358

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

package software.amazon.smithy.model.validation.validators;

import static java.lang.String.format;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
import java.util.TreeSet;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.knowledge.NeighborProviderIndex;
import software.amazon.smithy.model.neighbor.NeighborProvider;
import software.amazon.smithy.model.neighbor.Relationship;
import software.amazon.smithy.model.neighbor.RelationshipType;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.shapes.StructureShape;
import software.amazon.smithy.model.traits.IdempotencyTokenTrait;
import software.amazon.smithy.model.traits.MixinTrait;
import software.amazon.smithy.model.traits.Trait;
import software.amazon.smithy.model.validation.AbstractValidator;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.utils.ListUtils;

/**
* Emits warnings when a structure member has an idempotency token trait that will be ignored.
*/
public class IdempotencyTokenIgnoredValidator extends AbstractValidator {
sugmanue marked this conversation as resolved.
Show resolved Hide resolved

@Override
public List<ValidationEvent> validate(Model model) {
if (!model.getAppliedTraits().contains(IdempotencyTokenTrait.ID)) {
return Collections.emptyList();
}
List<ValidationEvent> events = new ArrayList<>();
NeighborProvider reverse = NeighborProviderIndex.of(model).getReverseProvider();
for (MemberShape memberShape : model.getMemberShapes()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use getMemberShapesWithTrait(IdempotencyToken.class)

Shape container = model.expectShape(memberShape.getContainer());
// Skip non-structures (invalid) and mixins (handled at mixed site).
if (!container.isStructureShape() || container.hasTrait(MixinTrait.class)) {
continue;
}

if (memberShape.hasTrait(IdempotencyTokenTrait.class)) {
Trait trait = memberShape.expectTrait(IdempotencyTokenTrait.class);
events.addAll(checkRelationships(container.asStructureShape().get(), memberShape, trait, reverse));
}
}
return events;
}

private List<ValidationEvent> checkRelationships(
StructureShape containerShape,
MemberShape memberShape,
Trait trait,
NeighborProvider reverse
) {

// Store relationships so we can emit one event per ignored binding.
Map<RelationshipType, List<ShapeId>> ignoredRelationships = new TreeMap<>();
List<ValidationEvent> events = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor but we could just pass in the mutable list of events from the validate method rather than needing to create intermediate lists

List<Relationship> relationships = reverse.getNeighbors(containerShape);
int checkedRelationshipCount = relationships.size();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this doesn't do anything now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 yeah, let me remove that.

for (Relationship relationship : relationships) {
// Skip members of the container.
if (relationship.getRelationshipType() == RelationshipType.MEMBER_CONTAINER) {
checkedRelationshipCount--;
continue;
}
if (relationship.getRelationshipType() != RelationshipType.INPUT) {
ignoredRelationships.merge(relationship.getRelationshipType(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be simplified to just use ignoredRelationships.computeIfAbsent? Might not need mergeShapeIdsList either at that point.

ListUtils.of(relationship.getShape().getId()), this::mergeShapeIdLists);
}
}

// If we detected invalid relationships, build the right event message.
if (!ignoredRelationships.isEmpty()) {
events.add(emit(memberShape, trait, checkedRelationshipCount, ignoredRelationships));
}

return events;
}

private List<ShapeId> mergeShapeIdLists(List<ShapeId> shapeIds1, List<ShapeId> shapeIds2) {
List<ShapeId> shapeIds = new ArrayList<>();
shapeIds.addAll(shapeIds1);
shapeIds.addAll(shapeIds2);
return shapeIds;
}

private ValidationEvent emit(
MemberShape memberShape,
Trait trait,
int checkedRelationshipCount,
Map<RelationshipType, List<ShapeId>> ignoredRelationships
) {
String mixedIn = memberShape.getMixins().isEmpty() ? "" : " mixed in";
String message = "The `%s` trait applied to this%s member is ";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we say something more clear like "The idempotencyToken trait only has an effect when applied to a top-level operation input member, but it was applied and ignored in the following contexts: " ... etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change as described.

if (checkedRelationshipCount == ignoredRelationships.size()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why omit the context here? It looks like we emit the event on the member that has the trait, but don't say how the containing shape was referenced in a way that caused the trait to be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove.

message += "ignored in all contexts.";
} else {
message += "ignored in some contexts: " + formatIgnoredRelationships(ignoredRelationships);
}
return warning(memberShape, trait, format(message, Trait.getIdiomaticTraitName(trait), mixedIn));
}

private String formatIgnoredRelationships(Map<RelationshipType, List<ShapeId>> ignoredRelationships) {
List<String> relationshipTypeBindings = new ArrayList<>();
for (Map.Entry<RelationshipType, List<ShapeId>> ignoredRelationshipType : ignoredRelationships.entrySet()) {
StringBuilder stringBuilder = new StringBuilder(ignoredRelationshipType.getKey().toString()
.toLowerCase(Locale.US)
.replace("_", " "));
Set<String> bindings = new TreeSet<>();
for (ShapeId binding : ignoredRelationshipType.getValue()) {
bindings.add(binding.toString());
}
stringBuilder.append(": [").append(String.join(", ", bindings)).append("]");
relationshipTypeBindings.add(stringBuilder.toString());
}
return "{" + String.join(", ", relationshipTypeBindings) + "}";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ software.amazon.smithy.model.validation.validators.HttpResponseCodeSemanticsVali
software.amazon.smithy.model.validation.validators.HttpUriGreedyLabelValidator
software.amazon.smithy.model.validation.validators.HttpUriConflictValidator
software.amazon.smithy.model.validation.validators.HttpUriFormatValidator
software.amazon.smithy.model.validation.validators.IdempotencyTokenIgnoredValidator
software.amazon.smithy.model.validation.validators.JsonNameValidator
software.amazon.smithy.model.validation.validators.LengthTraitValidator
software.amazon.smithy.model.validation.validators.MediaTypeValidator
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[WARNING] io.smithy.example#StructureOne$stringMember: The `idempotencyToken` trait applied to this member is ignored in some contexts: {member target: [io.smithy.example#StructureTwo$structureOne]} | IdempotencyTokenIgnored
[WARNING] io.smithy.example#StructureThree$stringMember: The `idempotencyToken` trait applied to this mixed in member is ignored in some contexts: {output: [io.smithy.example#OperationFour]} | IdempotencyTokenIgnored


Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
$version: "2"

namespace io.smithy.example

service SmithyExample {
operations: [
OperationOne
OperationTwo
OperationThree
OperationFour
]
}

operation OperationOne {
input := {
@idempotencyToken
stringMember: String
integerMember: Integer
}
output: Unit
}

operation OperationTwo {
input: StructureOne
output: Unit
}

operation OperationThree {
input: StructureTwo
output: Unit
}

operation OperationFour {
input: StructureThree
output: StructureThree
}


structure StructureOne {
@idempotencyToken
stringMember: String
integerMember: Integer
}


structure StructureTwo {
stringMember: String
structureOne: StructureOne
}


@mixin
structure StructureMixin {
@idempotencyToken
stringMember: String
}


structure StructureThree with [StructureMixin] {
integerMember: Integer
}

Loading