Skip to content

Commit

Permalink
Add service id to the error message to aid debugging (#2483)
Browse files Browse the repository at this point in the history
  • Loading branch information
sugmanue authored Dec 6, 2024
1 parent 9563d77 commit f8ac947
Show file tree
Hide file tree
Showing 10 changed files with 194 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ private AwsTagIndex(Model model) {
}

public static AwsTagIndex of(Model model) {
return new AwsTagIndex(model);
return model.getKnowledge(AwsTagIndex.class, AwsTagIndex::new);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import static software.amazon.smithy.aws.traits.tagging.TaggingShapeUtils.TAG_RESOURCE_OPNAME;
import static software.amazon.smithy.aws.traits.tagging.TaggingShapeUtils.UNTAG_RESOURCE_OPNAME;

import java.util.LinkedList;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import software.amazon.smithy.model.FromSourceLocation;
Expand All @@ -36,15 +36,15 @@ public final class ServiceTaggingValidator extends AbstractValidator {
@Override
public List<ValidationEvent> validate(Model model) {
AwsTagIndex awsTagIndex = AwsTagIndex.of(model);
List<ValidationEvent> events = new LinkedList<>();
List<ValidationEvent> events = new ArrayList<>();
for (ServiceShape service : model.getServiceShapesWithTrait(TagEnabledTrait.class)) {
events.addAll(validateService(service, awsTagIndex));
}
return events;
}

private List<ValidationEvent> validateService(ServiceShape service, AwsTagIndex awsTagIndex) {
List<ValidationEvent> events = new LinkedList<>();
List<ValidationEvent> events = new ArrayList<>();
TagEnabledTrait trait = service.expectTrait(TagEnabledTrait.class);

Optional<ShapeId> tagResourceId = awsTagIndex.getTagResourceOperation(service.getId());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

package software.amazon.smithy.aws.traits.tagging;

import java.util.LinkedList;
import java.util.ArrayList;
import java.util.List;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.knowledge.TopDownIndex;
Expand All @@ -30,7 +30,7 @@
public final class TagEnabledServiceValidator extends AbstractValidator {
@Override
public List<ValidationEvent> validate(Model model) {
List<ValidationEvent> events = new LinkedList<>();
List<ValidationEvent> events = new ArrayList<>();
TopDownIndex topDownIndex = TopDownIndex.of(model);
AwsTagIndex tagIndex = AwsTagIndex.of(model);
for (ServiceShape service : model.getServiceShapesWithTrait(TagEnabledTrait.class)) {
Expand All @@ -44,7 +44,7 @@ private List<ValidationEvent> validateService(
AwsTagIndex tagIndex,
TopDownIndex topDownIndex
) {
List<ValidationEvent> events = new LinkedList<>();
List<ValidationEvent> events = new ArrayList<>();
TagEnabledTrait trait = service.expectTrait(TagEnabledTrait.class);

int taggableResourceCount = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
package software.amazon.smithy.aws.traits.tagging;

import java.util.AbstractMap;
import java.util.ArrayList;
import java.util.Collection;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand All @@ -40,7 +40,7 @@
public final class TaggableResourceValidator extends AbstractValidator {
@Override
public List<ValidationEvent> validate(Model model) {
List<ValidationEvent> events = new LinkedList<>();
List<ValidationEvent> events = new ArrayList<>();
TopDownIndex topDownIndex = TopDownIndex.of(model);
AwsTagIndex tagIndex = AwsTagIndex.of(model);
for (ServiceShape service : model.getServiceShapes()) {
Expand Down Expand Up @@ -72,7 +72,7 @@ private List<ValidationEvent> validateResource(
ServiceShape service,
AwsTagIndex awsTagIndex
) {
List<ValidationEvent> events = new LinkedList<>();
List<ValidationEvent> events = new ArrayList<>();
// Generate danger if resource has tag property in update API.
if (awsTagIndex.isResourceTagOnUpdate(resource.getId())) {
Shape operation = resource.getUpdate().isPresent()
Expand All @@ -87,7 +87,6 @@ private List<ValidationEvent> validateResource(
// list or tag keys input or output member
// 2. Tagging via APIs specified in the @taggable trait which are validated
// through the tag property, and must be resource instance operations
//Caution: avoid short circuiting behavior.
boolean isServiceWideTaggable = awsTagIndex.serviceHasTagApis(service.getId());
boolean isInstanceOpTaggable = isTaggableViaInstanceOperations(model, resource);

Expand All @@ -96,9 +95,11 @@ private List<ValidationEvent> validateResource(
+ " It must use the `aws.api@arn` trait."));
}

if (!(isServiceWideTaggable || isInstanceOpTaggable)) {
events.add(error(resource, "Resource does not have tagging CRUD operations and is not compatible"
+ " with service-wide tagging operations."));
if (!isServiceWideTaggable && !isInstanceOpTaggable) {
events.add(error(resource, String.format("Resource does not have tagging CRUD operations and is not"
+ " compatible with service-wide tagging operations"
+ " for service `%s`.",
service.getId())));
}

return events;
Expand Down Expand Up @@ -174,7 +175,7 @@ private boolean exactlyOne(
}

private Collection<Map.Entry<MemberShape, Shape>> collectMemberTargetShapes(ShapeId ioShapeId, Model model) {
Collection<Map.Entry<MemberShape, Shape>> collection = new LinkedList<>();
Collection<Map.Entry<MemberShape, Shape>> collection = new ArrayList<>();
for (MemberShape memberShape : model.expectShape(ioShapeId).members()) {
collection.add(new AbstractMap.SimpleImmutableEntry<>(
memberShape, model.expectShape(memberShape.getTarget())));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,10 @@ static boolean isTagPropertyInInput(
Model model,
ResourceShape resource
) {
PropertyBindingIndex propertyBindingIndex = PropertyBindingIndex.of(model);
Optional<String> property = resource.expectTrait(TaggableTrait.class).getProperty();
if (property.isPresent()) {
if (operationId.isPresent()) {
if (operationId.isPresent()) {
PropertyBindingIndex propertyBindingIndex = PropertyBindingIndex.of(model);
Optional<String> property = resource.expectTrait(TaggableTrait.class).getProperty();
if (property.isPresent()) {
OperationShape operation = model.expectShape(operationId.get()).asOperationShape().get();
Shape inputShape = model.expectShape(operation.getInputShape());
return isTagPropertyInShape(property.get(), inputShape, propertyBindingIndex);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
[DANGER] example.weather#Weather: Shape `example.weather#ListTagsForResource` does not satisfy 'ListTagsForResource' operation requirements. | ServiceTagging
[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations. | TaggableResource
[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations for service `example.weather#Weather`. | TaggableResource
[WARNING] example.weather#Weather: Service marked `aws.api#tagEnabled` trait does not have consistent tagging operations implemented: {TagResource, UntagResource, and ListTagsForResource}. | TagEnabledService
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
[WARNING] example.weather#Weather: Service marked `aws.api#TagEnabled` is missing an operation named 'TagResource.' | ServiceTagging
[WARNING] example.weather#Weather: Service marked `aws.api#TagEnabled` is missing an operation named 'UntagResource.' | ServiceTagging
[WARNING] example.weather#Weather: Service marked `aws.api#tagEnabled` trait does not have consistent tagging operations implemented: {TagResource, UntagResource, and ListTagsForResource}. | TagEnabledService
[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations. | TaggableResource
[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations for service `example.weather#Weather`. | TaggableResource
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[WARNING] example.weather#Forecast: Resource is likely missing `aws.api#taggable` trait. | TaggableResource
[WARNING] example.weather#AnotherService: Service has resources with `aws.api#taggable` applied but does not have the `aws.api#tagEnabled` trait. | TaggableResource
[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations for service `example.weather#AnotherService`. | TaggableResource
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
$version: "2.0"

metadata suppressions = [
{
id: "UnstableTrait",
namespace: "example.weather"
}
]

namespace example.weather

use aws.api#arn
use aws.api#taggable
use aws.api#tagEnabled

@tagEnabled
service Weather {
version: "2006-03-01",
resources: [City]
operations: [GetCurrentTime, TagResource, UntagResource, ListTagsForResource]
}

@internal
service AnotherService {
version: "2006-03-01"
resources: [AnotherResouce]
}

@arn(template: "resource/{resourceId}/another")
resource AnotherResouce {
identifiers: {
cityId: CityId
}
resources: [
City
]
}

structure Tag {
key: String
value: String
}

list TagList {
member: Tag
}

list TagKeys {
member: String
}

operation TagResource {
input := {
@required
arn: String
@length(max: 128)
tags: TagList
}
output := { }
}

operation UntagResource {
input := {
@required
arn: String
@required
tagKeys: TagKeys
}
output := { }
}

operation ListTagsForResource {
input := {
@required
arn: String
}
output := {
@length(max: 128)
tags: TagList
}
}

@arn(
template: "city/{cityId}/forecast/{forecastId}"
)
resource Forecast {
identifiers: {
cityId: CityId
forecastId: ForecastId
}
}

@taggable(property: "tags")
@arn(template: "city/{CityId}")
resource City {
identifiers: { cityId: CityId }
properties: {
name: String
coordinates: CityCoordinates
}
read: GetCity
resources: [Forecast]
}

@pattern("^[A-Za-z0-9 ]+$")
string ForecastId

@pattern("^[A-Za-z0-9 ]+$")
string CityId

@readonly
operation GetCity {
input: GetCityInput
output: GetCityOutput
errors: [NoSuchResource]
}

@input
structure GetCityInput {
// "cityId" provides the identifier for the resource and
// has to be marked as required.
@required
cityId: CityId
}

@output
structure GetCityOutput {
// "required" is used on output to indicate if the service
// will always provide a value for the member.
@required
name: String,

@required
coordinates: CityCoordinates
}

// This structure is nested within GetCityOutput.
structure CityCoordinates {
@required
latitude: Float,

@required
longitude: Float,
}

// "error" is a trait that is used to specialize
// a structure as an error.
@error("client")
structure NoSuchResource {
@required
resourceType: String
}

@readonly
operation GetCurrentTime {
input: GetCurrentTimeInput
output: GetCurrentTimeOutput
}

@input
structure GetCurrentTimeInput {}

@output
structure GetCurrentTimeOutput {
@required
time: Timestamp
}

Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
[WARNING] example.weather#Weather: Service marked `aws.api#TagEnabled` is missing an operation named 'UntagResource.' | ServiceTagging
[WARNING] example.weather#Weather: Service marked `aws.api#tagEnabled` trait does not have consistent tagging operations implemented: {TagResource, UntagResource, and ListTagsForResource}. | TagEnabledService
[ERROR] example.weather#City: Tag property must be a list shape targeting a member containing a pair of strings, or a Map shape targeting a string member. | TagResourcePropertyType
[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations. | TaggableResource
[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations for service `example.weather#Weather`. | TaggableResource

0 comments on commit f8ac947

Please sign in to comment.