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

Fix ignored binding checks for mixins #1969

Merged
merged 1 commit into from
Sep 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
Expand Up @@ -22,6 +22,7 @@
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.HttpHeaderTrait;
Expand All @@ -31,6 +32,7 @@
import software.amazon.smithy.model.traits.HttpQueryParamsTrait;
import software.amazon.smithy.model.traits.HttpQueryTrait;
import software.amazon.smithy.model.traits.HttpResponseCodeTrait;
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;
Expand Down Expand Up @@ -70,6 +72,12 @@ public List<ValidationEvent> validate(Model model) {
List<ValidationEvent> events = new ArrayList<>();
NeighborProvider reverse = NeighborProviderIndex.of(model).getReverseProvider();
for (MemberShape memberShape : model.getMemberShapes()) {
Shape container = model.expectShape(memberShape.getContainer());
// Skip non-structures (invalid) and mixins (handled at mixed site).
if (!container.isStructureShape() || container.hasTrait(MixinTrait.class)) {
continue;
}

// Gather all traits that are HTTP member binding.
// Keep the trait instance around so that it can be used it later for source location.
Map<ShapeId, Trait> traits = new HashMap<>();
Expand All @@ -82,11 +90,9 @@ public List<ValidationEvent> validate(Model model) {
// The traits set is now the HTTP binding traits that are ignored outside
// the top level of an operation's components.
if (!traits.isEmpty()) {
StructureShape containerShape = model.expectShape(memberShape.getContainer(), StructureShape.class);

// All relationships and trait possibilities are checked at once to de-duplicate
// several parts of the iteration logic.
events.addAll(checkRelationships(containerShape, memberShape, traits, reverse));
events.addAll(checkRelationships(container.asStructureShape().get(), memberShape, traits, reverse));
}
}
return events;
Expand Down Expand Up @@ -151,19 +157,16 @@ private List<ValidationEvent> checkRelationships(
// immediate grabbing of next traits is all that's necessary.
if (!ignoredRelationships.isEmpty()) {
if (!ignoredOutsideInputTraits.isEmpty()) {
ShapeId traitId = ignoredOutsideInputTraits.iterator().next();
events.add(emit("Input", memberShape, traits, traitId,
checkedRelationshipCount, ignoredRelationships));
Trait trait = traits.get(ignoredOutsideInputTraits.iterator().next());
events.add(emit("Input", memberShape, trait, checkedRelationshipCount, ignoredRelationships));

} else if (!ignoredOutsideOutputTraits.isEmpty()) {
ShapeId traitId = ignoredOutsideOutputTraits.iterator().next();
events.add(emit("Output", memberShape, traits, traitId,
checkedRelationshipCount, ignoredRelationships));
Trait trait = traits.get(ignoredOutsideOutputTraits.iterator().next());
events.add(emit("Output", memberShape, trait, checkedRelationshipCount, ignoredRelationships));
} else {
// The traits list is always non-empty here, so just grab the first.
ShapeId traitId = traits.keySet().iterator().next();
events.add(emit("TopLevel", memberShape, traits, traitId,
checkedRelationshipCount, ignoredRelationships));
Trait trait = traits.get(traits.keySet().iterator().next());
events.add(emit("TopLevel", memberShape, trait, checkedRelationshipCount, ignoredRelationships));
}
}

Expand All @@ -180,18 +183,18 @@ private List<ShapeId> mergeShapeIdLists(List<ShapeId> shapeIds1, List<ShapeId> s
private ValidationEvent emit(
String type,
MemberShape memberShape,
Map<ShapeId, Trait> traits,
ShapeId traitId,
Trait trait,
int checkedRelationshipCount,
Map<RelationshipType, List<ShapeId>> ignoredRelationships
) {
String message = "The `%s` trait applied to this member is ";
String mixedIn = memberShape.getMixins().isEmpty() ? "" : " mixed in";
String message = "The `%s` trait applied to this%s member is ";
if (checkedRelationshipCount == ignoredRelationships.size()) {
message += "ignored in all contexts.";
} else {
message += format("ignored in some contexts: %s", formatIgnoredRelationships(ignoredRelationships));
message += "ignored in some contexts: " + formatIgnoredRelationships(ignoredRelationships);
}
return warning(memberShape, traits.get(traitId), format(message, Trait.getIdiomaticTraitName(traitId)), type);
return warning(memberShape, trait, format(message, Trait.getIdiomaticTraitName(trait), mixedIn), type);
}

private String formatIgnoredRelationships(Map<RelationshipType, List<ShapeId>> ignoredRelationships) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,6 @@
[WARNING] smithy.example#NestedBindings$stringMap2: The `httpPrefixHeaders` trait applied to this member is ignored in some contexts: {member target: [smithy.example#IgnoredBindingOperationInput$nestedBindings, smithy.example#IgnoredBindingOperationOutput$nestedBindings]} | HttpBindingTraitIgnored.TopLevel
[WARNING] smithy.example#NestedBindings$code: The `httpResponseCode` trait applied to this member is ignored in some contexts: {member target: [smithy.example#IgnoredBindingOperationInput$nestedBindings, smithy.example#IgnoredBindingOperationOutput$nestedBindings], input: [smithy.example#NestedBindingOperation]} | HttpBindingTraitIgnored.Output
[WARNING] smithy.example#NestedBindings$payload: The `httpPayload` trait applied to this member is ignored in some contexts: {member target: [smithy.example#IgnoredBindingOperationInput$nestedBindings, smithy.example#IgnoredBindingOperationOutput$nestedBindings]} | HttpBindingTraitIgnored.TopLevel
[WARNING] smithy.example#MixedBindingOperationOutput$query: The `httpQuery` trait applied to this mixed in member is ignored in all contexts. | HttpBindingTraitIgnored.Input
[WARNING] smithy.example#MixedBindingOperationInput$code: The `httpResponseCode` trait applied to this mixed in member is ignored in all contexts. | HttpBindingTraitIgnored.Output
[WARNING] smithy.example#MixedBindingOperationOutput$code: The `httpResponseCode` trait applied to this mixed in member is ignored in some contexts: {member target: [smithy.example#MixedBindingOperationInput$content]} | HttpBindingTraitIgnored.Output
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ $version: "2"
namespace smithy.example

service InvalidExample {
version: "2020-12-29",
version: "2020-12-29"
operations: [
IgnoredBindingOperation
NestedBindingOperation
],
MixedBindingOperation
]
}

@http(method: "POST", uri: "/ignored-binding")
Expand Down Expand Up @@ -77,3 +78,27 @@ map StringMap {
key: String
value: String
}

@http(method: "POST", uri: "/mixed-binding")
operation MixedBindingOperation {
input: MixedBindingOperationInput
output: MixedBindingOperationOutput
}

structure MixedBindingOperationInput with [MixedQuery, MixedResponseCode] {
content: MixedBindingOperationOutput
}

structure MixedBindingOperationOutput with [MixedQuery, MixedResponseCode] {}

@mixin
structure MixedQuery {
@httpQuery("query")
query: String
}

@mixin
structure MixedResponseCode {
@httpResponseCode
code: Integer
}