Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Make UnreferencedShape an opt-in linter instead
Browse files Browse the repository at this point in the history
UnreferencedShape is no longer an on-by-default validator that ensures
shapes must be connected to a service shape. There are plenty of cases
where teams might create models that define shared shapes and don't
necessarily connect those shapes to services. Emitting an
UnreferencedShape validation event for these cases is harmless, but
also annoying and desensitizing developers for when a shape is actually
unreferenced.

This commit instead adds a new smithy-linters validator called
UnreferencedShape that allows an optional "rootShapeSelector" to be
provided that defines the "root" shapes that all other shape must be
connected to. It defaults to "service". You might want to customize
this to indicate that other shapes are your roots (e.g., maybe there
is a specific trait that makes a shape a root).

Closes #2093
mtdowling committed Jan 30, 2024
1 parent 3fa1c8e commit be1a157
Showing 23 changed files with 374 additions and 129 deletions.
52 changes: 52 additions & 0 deletions docs/source-2.0/guides/model-linters.rst
Original file line number Diff line number Diff line change
@@ -36,6 +36,58 @@ Linters in ``smithy-linters``
The ``smithy-linters`` package defines the following linters.


.. _UnreferencedShape:

UnreferencedShape
=================

Emits when a shape is not connected to the rest of the model. If no
configuration is provided, the linter will check if a shape is connected to
the closure of any service shape. A selector can be provided to define a
custom set of "root" shapes to customize how the linter determines if a shape
is unreferenced. Shapes that are connected through the :ref:`idref-trait`
are considered connected.

Rationale
Just like unused variables in code, removing unused shapes from a model
makes the model easier to maintain.

Default severity
``NOTE``

Configuration
.. list-table::
:header-rows: 1
:widths: 20 20 60

* - Property
- Type
- Description
* - rootShapeSelector
- ``string``
- A :ref:`selector <selectors>` that specifies the root shape(s)
from which to detect if other shapes are connected. Defaults
to "service", meaning any shape connected to any service shape
in the model is considered referenced.

Example:

.. code-block:: smithy
$version: "2"
metadata validators = [
{
// Find shapes that aren't connected to service shapes or shapes
// marked with the trait, smithy.example#myCustomTrait.
name: "UnreferencedShape"
configuration: {
rootShapeSelector: ":is(service, [trait|smithy.example#myCustomTrait])"
}
}
]
.. _AbbreviationName:

AbbreviationName
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

package software.amazon.smithy.linters;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Objects;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.neighbor.UnreferencedShapes;
import software.amazon.smithy.model.node.ExpectationNotMetException;
import software.amazon.smithy.model.node.ObjectNode;
import software.amazon.smithy.model.selector.Selector;
import software.amazon.smithy.model.selector.SelectorSyntaxException;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.validation.AbstractValidator;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.model.validation.ValidatorService;

public final class UnreferencedShapeValidator extends AbstractValidator {

public static final class Config {

private Selector rootShapeSelector = Selector.parse("service");

/**
* The Selector used to find root shapes, and any shape that is not connected to any of the returned root
* shapes is considered unreferenced.
*
* <p>Defaults to "service" if not set.
*
* @return Returns the root selector;
*/
public Selector getRootShapeSelector() {
return rootShapeSelector;
}

public void setRootShapeSelector(Selector rootShapeSelector) {
this.rootShapeSelector = Objects.requireNonNull(rootShapeSelector);
}
}

public static final class Provider extends ValidatorService.Provider {
public Provider() {
super(UnreferencedShapeValidator.class, configuration -> {
Config config = new Config();
ObjectNode node = configuration.expectObjectNode()
.expectNoAdditionalProperties(Collections.singleton("rootShapeSelector"));
node.getStringMember("rootShapeSelector").ifPresent(rootShapeNode -> {
try {
config.setRootShapeSelector(Selector.parse(rootShapeNode.getValue()));
} catch (SelectorSyntaxException e) {
throw new ExpectationNotMetException("Error parsing `rootShapeSelector`: " + e.getMessage(),
rootShapeNode);
}
});
return new UnreferencedShapeValidator(config);
});
}
}

private final Config config;

private UnreferencedShapeValidator(Config config) {
this.config = config;
}

@Override
public List<ValidationEvent> validate(Model model) {
List<ValidationEvent> events = new ArrayList<>();

for (Shape shape : new UnreferencedShapes(config.rootShapeSelector).compute(model)) {
events.add(note(shape, "This shape is unreferenced. It has no modeled connections to shapes "
+ "that match the following selector: `" + config.rootShapeSelector + "`"));
}

return events;
}
}
Original file line number Diff line number Diff line change
@@ -10,3 +10,4 @@ software.amazon.smithy.linters.ShouldHaveUsedTimestampValidator$Provider
software.amazon.smithy.linters.StandardOperationVerbValidator$Provider
software.amazon.smithy.linters.StutteredShapeNameValidator$Provider
software.amazon.smithy.linters.MissingSensitiveTraitValidator$Provider
software.amazon.smithy.linters.UnreferencedShapeValidator$Provider
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[WARNING] smithy.example#Baz: This shape is unreferenced. It has no modeled connections to shapes that match the following selector: `[trait|smithy.example#root]` | UnreferencedShape
[WARNING] smithy.example#Bam: This shape is unreferenced. It has no modeled connections to shapes that match the following selector: `[trait|smithy.example#root]` | UnreferencedShape
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
$version: "2.0"

metadata validators = [
{
name: "UnreferencedShape"
// Elevate the severity so that NOTE events don't slip through.
severity: "WARNING"
configuration: {
rootShapeSelector: "[trait|smithy.example#root]"
}
}
]

namespace smithy.example

// Considered referenced because it's a trait.
@trait
structure root {}

// Considered referenced because of the root trait.
@root
structure MyAbc {
MyString: MyString
}

// Considered referenced because it's referenced by MyAbc$MyString
string MyString

// Considered referenced because of the root trait.
@root
string MyDef

// Unreferenced.
string Baz

// Unreferenced.
string Bam
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[ERROR] -: Error creating `UnreferencedShape` validator: Error parsing | Model
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
$version: "2.0"

metadata validators = [
{
name: "UnreferencedShape"
configuration: {
rootShapeSelector: "!!!!"
}
}
]
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[NOTE] smithy.example#Baz: This shape is unreferenced. It has no modeled connections to shapes that match the following selector: `service` | UnreferencedShape
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
$version: "2.0"

metadata validators = [
{
name: "UnreferencedShape"
}
]

namespace smithy.example

// A "root" shape, so does not emit.
service MyService {
operations: [
MyOperation
]
}

// Connected.
operation MyOperation {
input := {
foo: MyString
}
}

// Connected.
string MyString

// Not connected.
string Baz
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[NOTE] smithy.example#Baz: This shape is unreferenced. It has no modeled connections to shapes that match the following selector: `service` | UnreferencedShape
[NOTE] smithy.example#Bam: This shape is unreferenced. It has no modeled connections to shapes that match the following selector: `service` | UnreferencedShape
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
$version: "2.0"

metadata validators = [
{
name: "UnreferencedShape"
configuration: {
rootShapeSelector: "service"
}
}
]

namespace smithy.example

service MyService1 {
operations: [
MyOperation1
]
}

service MyService2 {
operations: [
MyOperation2
]
}

operation MyOperation1 {
input := {
foo: MyAbc
}
}

operation MyOperation2 {
output := {
bar: MyDef
}
}

structure MyAbc {
MyString: MyString
}

string MyString

string MyDef

string Baz

string Bam
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[NOTE] smithy.example#Baz: This shape is unreferenced. It has no modeled connections to shapes that match the following selector: `service` | UnreferencedShape
[NOTE] smithy.example#Bam: This shape is unreferenced. It has no modeled connections to shapes that match the following selector: `service` | UnreferencedShape
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
$version: "2.0"

metadata validators = [
{
name: "UnreferencedShape"
configuration: {
rootShapeSelector: "service"
}
}
]

namespace smithy.example

service MyService {
operations: [
MyOperation
]
}

operation MyOperation {
input := {
foo: MyAbc
}
output := {
bar: MyDef
}
}

structure MyAbc {
MyString: MyString
}

string MyString

string MyDef

string Baz

string Bam
Original file line number Diff line number Diff line change
@@ -38,6 +38,3 @@
[DANGER] ns.baz#lowerStructureTrait$UpperCamelCase: Member shape member name, `UpperCamelCase`, is not lower camel case; members in the ns.baz namespace must all use lower camel case. | DefaultCamelCase
[DANGER] ns.foo#Structure$lowerCamelCase: Member shape member name, `lowerCamelCase`, is not upper camel case; members in the MyService must all use upper camel case. | DefaultCamelCase
[DANGER] ns.foo#Structure$snake_case: Member shape member name, `snake_case`, is not upper camel case; members in the MyService must all use upper camel case. | DefaultCamelCase

[NOTE] ns.foo#Enum: The enum shape is not connected to from any service shape. | UnreferencedShape
[NOTE] ns.foo#IntEnum: The intEnum shape is not connected to from any service shape. | UnreferencedShape
Original file line number Diff line number Diff line change
@@ -21,31 +21,49 @@
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.knowledge.NeighborProviderIndex;
import software.amazon.smithy.model.loader.Prelude;
import software.amazon.smithy.model.selector.Selector;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.traits.TraitDefinition;
import software.amazon.smithy.utils.FunctionalUtils;

/**
* Finds shapes that are not connected to a service shape, are not trait
* definitions, are not referenced by trait definitions, and are not
* referenced in trait values through
* Finds shapes that are not connected to a "root" shape, are not trait definitions, are not referenced by trait
* definitions, and are not referenced in trait values through
* {@link software.amazon.smithy.model.traits.IdRefTrait}.
*
* <p>The "root" shapes defaults to all service shapes in the model. You can customize this by providing a selector
* that considers every matching shape a root shape. For example, a model might consider all shapes marked with
* a trait called "root" to be a root shape.
*
* <p>Prelude shapes are never considered unreferenced.
*/
public final class UnreferencedShapes {

private static final Selector SERVICE_SHAPES = Selector.parse("service");

private final Selector rootShapeSelector;
private final Predicate<Shape> keepFilter;

/**
* @param keepFilter Predicate that if matched keeps a shape from being unreferenced.
* @param rootShapeSelector Selector that returns the root shapes to traverse from (defaults to all services).
*/
public UnreferencedShapes(Predicate<Shape> keepFilter, Selector rootShapeSelector) {
this.keepFilter = keepFilter;
this.rootShapeSelector = rootShapeSelector;
}

public UnreferencedShapes() {
this(FunctionalUtils.alwaysTrue());
}

/**
* @param keepFilter Predicate that if matched keeps a shape from being unreferenced.
*/
public UnreferencedShapes(Selector selector) {
this(FunctionalUtils.alwaysTrue(), selector);
}

public UnreferencedShapes(Predicate<Shape> keepFilter) {
this.keepFilter = keepFilter;
this(keepFilter, SERVICE_SHAPES);
}

/**
@@ -60,21 +78,26 @@ public Set<Shape> compute(Model model) {
Walker shapeWalker = new Walker(providerWithIdRefRelationships);

// Find all shapes connected to any service shape.
Set<Shape> connected = new HashSet<>();
for (Shape service : model.getServiceShapes()) {
connected.addAll(shapeWalker.walkShapes(service));
Set<ShapeId> connected = new HashSet<>();

// Stop traversing into trees that are already traversed.
Predicate<Relationship> traversed = rel -> !connected.contains(rel.getNeighborShapeId());

Set<Shape> rootShapes = rootShapeSelector.select(model);
for (Shape root : rootShapes) {
shapeWalker.iterateShapes(root, traversed).forEachRemaining(shape -> connected.add(shape.getId()));
}

// Don't remove shapes that are traits or connected to traits.
for (Shape trait : model.getShapesWithTrait(TraitDefinition.class)) {
connected.addAll(shapeWalker.walkShapes(trait));
shapeWalker.iterateShapes(trait, traversed).forEachRemaining(shape -> connected.add(shape.getId()));
}

// Any shape that wasn't identified as connected to a service is considered unreferenced.
// Any shape that wasn't identified as connected to a root is considered unreferenced.
Set<Shape> result = new HashSet<>();
for (Shape shape : model.toSet()) {
if (!shape.isMemberShape()
&& !connected.contains(shape)
&& !connected.contains(shape.getId())
&& !Prelude.isPreludeShape(shape)
&& keepFilter.test(shape)) {
result.add(shape);
Original file line number Diff line number Diff line change
@@ -27,7 +27,11 @@
/**
* Adds a validation note event for each shape in the model that is not
* connected to a service shape.
*
* <p>This validator is deprecated and no longer applied by default.
* Use the UnreferencedShapeValidator from smithy-linters instead.
*/
@Deprecated
public final class UnreferencedShapeValidator extends AbstractValidator {
@Override
public List<ValidationEvent> validate(Model model) {
Original file line number Diff line number Diff line change
@@ -52,6 +52,5 @@ software.amazon.smithy.model.validation.validators.TraitTargetValidator
software.amazon.smithy.model.validation.validators.TraitValueValidator
software.amazon.smithy.model.validation.validators.UnionValidator
software.amazon.smithy.model.validation.validators.UnitTypeValidator
software.amazon.smithy.model.validation.validators.UnreferencedShapeValidator
software.amazon.smithy.model.validation.validators.UnstableTraitValidator
software.amazon.smithy.model.validation.validators.XmlNamespaceTraitValidator
Original file line number Diff line number Diff line change
@@ -17,20 +17,16 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.notNullValue;
import static org.junit.jupiter.api.Assertions.assertThrows;

import java.util.Arrays;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.node.ObjectNode;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.validation.NodeValidationVisitorTest;
import software.amazon.smithy.model.validation.ValidatedResult;
import software.amazon.smithy.model.validation.ValidationEvent;
import software.amazon.smithy.model.validation.ValidationEventDecorator;
@@ -40,23 +36,19 @@

public class ValidationEventDecoratorTest {

static final String HINT = "Consider connecting this structure to a service";
static final String UNREFERENCED_SHAPE_EVENT_ID = "UnreferencedShape";
static final Set<ShapeId> STRUCT_SHAPE_IDS = SetUtils.of(ShapeId.from("ns.foo#Structure"),
ShapeId.from("ns.foo#Structure2"),
ShapeId.from("ns.foo#Structure3"),
ShapeId.from("ns.foo#Structure4"));
static final String HINT = "We had to deprecate this";
static final String SHAPE_EVENT_ID = "DeprecatedShape";
static final Set<ShapeId> MATCHING_SHAPE_IDS = SetUtils.of(ShapeId.from("smithy.example#Foo$a"));

@Test
public void canDecorateValidationEvents() {
ValidatedResult<Model> result = Model.assembler()
.addImport(NodeValidationVisitorTest.class.getResource("node-validator"
+ ".json"))
.validatorFactory(testFactory(new DummyHintValidationEventDecorator()))
.assemble();
.addImport(getClass().getResource("validation-event-decorator-test.smithy"))
.validatorFactory(testFactory(new TestValidationEventDecorator()))
.assemble();
for (ValidationEvent event : result.getValidationEvents()) {
ShapeId eventShapeId = event.getShapeId().orElse(null);
if (STRUCT_SHAPE_IDS.contains(eventShapeId)) {
if (MATCHING_SHAPE_IDS.contains(eventShapeId)) {
assertThat(event.getHint().isPresent(), equalTo(true));
assertThat(event.getHint().get(), equalTo(HINT));
} else {
@@ -69,10 +61,9 @@ public void canDecorateValidationEvents() {
public void exceptionsAreNotCaughtWhenDecoratorsThrow() {
assertThrows(RuntimeException.class, () -> {
Model.assembler()
.addImport(NodeValidationVisitorTest.class.getResource("node-validator"
+ ".json"))
.validatorFactory(testFactory(new ThrowingValidationEventDecorator()))
.assemble();
.addImport(getClass().getResource("validation-event-decorator-test.smithy"))
.validatorFactory(testFactory(new ThrowingValidationEventDecorator()))
.assemble();
});
}

@@ -96,11 +87,11 @@ public Optional<Validator> createValidator(String name, ObjectNode configuration
};
}

static class DummyHintValidationEventDecorator implements ValidationEventDecorator {

static class TestValidationEventDecorator implements ValidationEventDecorator {
@Override
public boolean canDecorate(ValidationEvent ev) {
return ev.containsId(UNREFERENCED_SHAPE_EVENT_ID) && ev.getMessage().contains("The structure ");
System.out.println(ev);
return ev.containsId(SHAPE_EVENT_ID);
}

@Override
@@ -110,15 +101,14 @@ public ValidationEvent decorate(ValidationEvent ev) {
}

static class ThrowingValidationEventDecorator implements ValidationEventDecorator {

@Override
public boolean canDecorate(ValidationEvent ev) {
return ev.containsId(UNREFERENCED_SHAPE_EVENT_ID) && ev.getMessage().contains("The structure ");
return ev.containsId(SHAPE_EVENT_ID);
}

@Override
public ValidationEvent decorate(ValidationEvent ev) {
throw new RuntimeException("ups");
throw new RuntimeException("oops!");
}
}
}
Original file line number Diff line number Diff line change
@@ -22,8 +22,6 @@
[ERROR] ns.foo#NInput$a: This `a` structure member is marked with the `httpLabel` trait, but no corresponding `http` URI label could be found when used as the input of the `ns.foo#N` operation. | HttpLabelTrait
[ERROR] ns.foo#PInput$a: The `a` structure member corresponds to a greedy label when used as the input of the `ns.foo#P` operation. This member targets (integer: `ns.foo#Integer`), but greedy labels must target string shapes. | HttpLabelTrait
[ERROR] ns.foo#RInput$b: `httpHeader` binding of `x-foo` conflicts with the `httpPrefixHeaders` binding of `ns.foo#RInput$a` to ``. `httpHeader` bindings must not case-insensitively start with any `httpPrefixHeaders` bindings. | HttpPrefixHeadersTrait
[NOTE] ns.foo#BadError: The structure shape is not connected to from any service shape. | UnreferencedShape
[NOTE] ns.foo#BadErrorMultipleBindings: The structure shape is not connected to from any service shape. | UnreferencedShape
[ERROR] ns.foo#GInput$b: Trait `httpHeader` cannot be applied to `ns.foo#GInput$b`. This trait may only be applied to shapes that match the following selector: structure > :test(member > :test(boolean, number, string, timestamp, list > member > :test(boolean, number, string, timestamp))) | TraitTarget
[ERROR] ns.foo#GOutput$b: Trait `httpHeader` cannot be applied to `ns.foo#GOutput$b`. This trait may only be applied to shapes that match the following selector: structure > :test(member > :test(boolean, number, string, timestamp, list > member > :test(boolean, number, string, timestamp))) | TraitTarget
[ERROR] ns.foo#HInput$a: Trait `httpHeader` cannot be applied to `ns.foo#HInput$a`. This trait may only be applied to shapes that match the following selector: structure > :test(member > :test(boolean, number, string, timestamp, list > member > :test(boolean, number, string, timestamp))) | TraitTarget
Original file line number Diff line number Diff line change
@@ -12,16 +12,13 @@
[DANGER] ns.foo#Long: Selector capture matched selector: long | long
[DANGER] ns.foo#Long: Selector capture matched selector: number | number
[DANGER] ns.foo#Long: Selector capture matched selector: simpleType | simpleType
[NOTE] ns.foo#Long: The long shape is not connected to from any service shape. | UnreferencedShape
[DANGER] ns.foo#Float: Selector capture matched selector: :is(long, float, boolean) | any
[DANGER] ns.foo#Float: Selector capture matched selector: float | float
[DANGER] ns.foo#Float: Selector capture matched selector: number | number
[DANGER] ns.foo#Float: Selector capture matched selector: simpleType | simpleType
[NOTE] ns.foo#Float: The float shape is not connected to from any service shape. | UnreferencedShape
[DANGER] ns.foo#Boolean: Selector capture matched selector: :is(long, float, boolean) | any
[DANGER] ns.foo#Boolean: Selector capture matched selector: boolean | boolean
[DANGER] ns.foo#Boolean: Selector capture matched selector: simpleType | simpleType
[NOTE] ns.foo#Boolean: The boolean shape is not connected to from any service shape. | UnreferencedShape
[DANGER] ns.foo#Blob: Selector capture matched selector: [trait|mediaType$='plain'] | traitEndsWith
[DANGER] ns.foo#Blob: Selector capture matched selector: [trait|mediaType*='PLAIN' i] | traitContainsCaseInsensitive
[DANGER] ns.foo#Blob: Selector capture matched selector: [trait|mediaType*='plain'] | traitContains
@@ -33,10 +30,8 @@
[DANGER] ns.foo#Blob: Selector capture matched selector: [trait|smithy.api#mediaType*=plain] | traitContains
[DANGER] ns.foo#Blob: Selector capture matched selector: blob | blob
[DANGER] ns.foo#Blob: Selector capture matched selector: simpleType | simpleType
[NOTE] ns.foo#Blob: The blob shape is not connected to from any service shape. | UnreferencedShape
[DANGER] ns.foo#List: Selector capture matched selector: :not(:is([trait|error], simpleType)) | not
[DANGER] ns.foo#List: Selector capture matched selector: list | list
[NOTE] ns.foo#List: The list shape is not connected to from any service shape. | UnreferencedShape
[DANGER] ns.foo#List$member: Selector capture matched selector: :not(:is([trait|error], simpleType)) | not
[DANGER] ns.foo#List$member: Selector capture matched selector: :test(member > [id='ns.foo#String']) | memberTargetsString
[DANGER] ns.foo#List$member: Selector capture matched selector: > | valid-neighbor-only
@@ -90,8 +85,6 @@
[DANGER] ns.foo#OperationBInput$id: Selector capture matched selector: > | valid-neighbor-only
[DANGER] ns.foo#OperationBInput$id: Selector capture matched selector: member | member
[DANGER] ns.foo#UtcTimestamp: Selector capture matched selector: simpleType | simpleType
[NOTE] ns.foo#UtcTimestamp: The timestamp shape is not connected to from any service shape. | UnreferencedShape
[DANGER] other.ns#String: Selector capture matched selector: [id|name="String"] | shapeName
[DANGER] other.ns#String: Selector capture matched selector: [id|name='String'] | shapeName
[DANGER] other.ns#String: Selector capture matched selector: simpleType | simpleType
[NOTE] other.ns#String: The string shape is not connected to from any service shape. | UnreferencedShape

This file was deleted.

This file was deleted.

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

namespace smithy.example

@deprecated
string MyString

string OtherString

structure Foo {
a: MyString
b: OtherString
}

0 comments on commit be1a157

Please sign in to comment.