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.

Sign up for GitHub

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 ability to elide targets for mixins/resources #1231

Merged
merged 1 commit into from
Jun 1, 2022
Merged

Add ability to elide targets for mixins/resources #1231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
77 changes: 60 additions & 17 deletions docs/source/1.0/guides/migrating-idl-1-to-2.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,14 +199,6 @@ having to copy and paste them. The following model:

namespace smithy.example

resource User {
identifiers: {
email: String,
id: String,
},
read: GetUser
}

operation GetUser {
input: GetUserInput,
output: GetUserOutput
Expand Down Expand Up @@ -240,15 +232,6 @@ Can be updated to:

namespace smithy.example

resource User {
identifiers: {
email: String
id: String
username: String
},
read: GetUser
}

@mixin
structure UserIdentifiers {
@required
Expand All @@ -274,6 +257,66 @@ that otherwise have to be copied and pasted.
work.


Use the target elision syntax sugar to reduce boilerplate
---------------------------------------------------------

Resource shapes contain a set of identifiers, but when writing structures that
contain those identifiers you have to duplicate those definitions entirely. In
IDL 2.0, you can use the target elision syntax with a structure bound to a
resource. For example:

.. code-block:: smithy

$version: "2.0"

namespace smithy.example

resource User {
identifiers: {
id: String
email: String
}
}

// The `for` syntax here determines which resource should be checked.
structure UserDetails for User {
// With this syntax, the target is automatically inferred from the
// resource.
$id

// Uncomment this to include an email member. Unlike with mixins, you
// must opt in to the members that you want to include. This allows you
// to have partial views of a resource, such as in a create operation
// that does not bind all of the identifiers.
// $email

address: String
}

This syntax can also be used with mixins to more succinctly add additional
traits to included members.

.. code-block:: smithy

$version: "2.0"

namespace smithy.example

@mixin
structure UserIdentifiers {
id: String
email: String
}

structure UserDetails with [UserIdentifiers] {
@required
$id

@required
$email
}


Remove unsightly commas
-----------------------

Expand Down
116 changes: 113 additions & 3 deletions docs/source/1.0/spec/core/idl.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,12 +190,14 @@ The Smithy IDL is defined by the following ABNF:
enum_shape_statement :`enum_type_name` `ws` `identifier` [`mixins`] `ws` `enum_shape_members`
enum_type_name :"enum" / "intEnum"
enum_shape_members :"{" `ws` 1*(`trait_statements` `identifier` `ws`) "}"
shape_members :"{" `ws` *(`shape_member_kvp` `ws`) "}"
shape_member_kvp :`trait_statements` `identifier` `ws` ":" `ws` `shape_id`
shape_members :"{" `ws` *(`trait_statements` `shape_member` `ws`) "}"
shape_member :`shape_member_kvp` / `shape_member_elided`
shape_member_kvp :`identifier` `ws` ":" `ws` `shape_id`
shape_member_elided :"$" `identifier`
list_statement :"list" `ws` `identifier` [`mixins`] `ws` `shape_members`
set_statement :"set" `ws` `identifier` [`mixins`] `ws` `shape_members`
map_statement :"map" `ws` `identifier` [`mixins`] `ws` `shape_members`
structure_statement :"structure" `ws` `identifier` [`mixins`] `ws` `shape_members`
structure_statement :"structure" `ws` `identifier` ["for" `shape_id`] [`mixins`] `ws` `shape_members`
union_statement :"union" `ws` `identifier` [`mixins`] `ws` `shape_members`
service_statement :"service" `ws` `identifier` [`mixins`] `ws` `node_object`
operation_statement :"operation" `ws` `identifier` [`mixins`] `ws` `inlineable_properties`
Expand Down Expand Up @@ -1408,6 +1410,11 @@ and defines a :ref:`read <read-lifecycle>` operation:
}
}

.. seealso::

The :ref:`target elision syntax <idl-target-elision>` for an easy way to
define structures that reference resource identifiers without having to
repeat the target definition.

.. _idl-mixins:

Expand Down Expand Up @@ -1438,6 +1445,109 @@ For example:
string SensitiveText with [SensitiveString]


.. _idl-target-elision:

Target Elision
JordonPhillips marked this conversation as resolved.
Show resolved Hide resolved
kstich marked this conversation as resolved.
Show resolved Hide resolved
--------------

Having to completely redefine a :ref:`resource identifier <resource-identifiers>`
to use it in a structure or redefine a member from a :ref:`mixin` to add
additional traits can be cumbersome and potentially error-prone. The
:token:`type elision syntax <smithy:shape_member_elided>` can be used to cut
down on that repetition by prefixing the member name with a ``$``. If a member
is prefixed this way, its target will automatically be set to the target of a
mixin member with the same name. The following example shows how to elide the
target for a member inherited from a mixin:

.. code-block:: smithy

$version: "2.0"

namespace smithy.example

@mixin
structure IdBearer {
id: String
}

structure IdRequired with [IdBearer] {
@required
$id
}

Additionally, structure shapes can reference a :ref:`resource <idl-resource>`
shape to define members that represent the resource's identifiers without having
to redefine the target shape. In addition to prefixing a member with ``$``, the
structure must also add ``for`` followed by the resource referenced in
the shape's definition before any mixins are specified.

To resolve elided types, first check if any bound resource defines an
identifier that case-sensitively matches the elided member name. If a match is
found, the type targeted by that identifier is used for the elided type. If no
identifier matches the elided member name, mixin members are case-sensitively
checked, and if a match is found, the type targeted by the mixin member is
used as the elided type. It is an error if neither the resource or mixin
members matches an elided member name.

The following example shows a structure reusing an identifier definition from
a resource:

.. code-block:: smithy

$version: "2.0"

namespace smithy.example

resource User {
identifiers: {
name: String
uuid: String
}
}

structure UserSummary for User {
$name
age: Short
}

Note that the ``UserSummary`` structure does not attempt to define the
``uuid`` identifier. When referencing a resource in this way, only the
identifiers that are explicitly referenced are added to the structure. This
allows structures to define subsets of identifiers, which can be useful for
operations like create operations where some of those identifiers may be
generated by the service.

Structures may only reference one resource shape in this way.

When using both mixins and a resource reference, the referenced resource will
be checked first. The following example is invalid:

.. code-block:: smithy

$version: "2.0"

namespace smithy.example

resource User {
identifiers: {
uuid: String
}
}

@mixin
structure UserIdentifiers {
uuid: Blob
}

// This is invalid because the `uuid` member's target is set to
// String, which then conflicts with the UserIdentifiers mixin.
structure UserSummary for User with [UserIdentifiers] {
$uuid
}




.. _documentation-comment:

Documentation comment
Expand Down
83 changes: 36 additions & 47 deletions smithy-model/src/main/java/software/amazon/smithy/model/loader/AbstractMutableModelFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ abstract class AbstractMutableModelFile implements ModelFile {
private final Set<ShapeId> allShapeIds = new HashSet<>();
private final Map<ShapeId, AbstractShapeBuilder<?, ?>> shapes = new LinkedHashMap<>();
private final Map<ShapeId, Map<String, MemberShape.Builder>> members = new HashMap<>();
private final Map<ShapeId, Set<ShapeId>> pendingShapes = new HashMap<>();
private final Map<ShapeId, List<PendingShapeModifier>> pendingModifications = new HashMap<>();
private final Map<ShapeId, Set<ShapeId>> pendingDependencies = new HashMap<>();
private final List<ValidationEvent> events = new ArrayList<>();
private final MetadataContainer metadata = new MetadataContainer(events);
private final TraitFactory traitFactory;
Expand Down Expand Up @@ -106,7 +107,13 @@ void onShape(AbstractShapeBuilder<?, ?> builder) {
}

void addPendingMixin(ShapeId shape, ShapeId mixin) {
pendingShapes.computeIfAbsent(shape, id -> new LinkedHashSet<>()).add(mixin);
addPendingModification(shape, new ApplyMixin(mixin, events));
}

void addPendingModification(ShapeId shape, PendingShapeModifier pendingModification) {
pendingDependencies.computeIfAbsent(shape, id -> new LinkedHashSet<>())
.addAll(pendingModification.getDependencies());
pendingModifications.computeIfAbsent(shape, id -> new ArrayList<>()).add(pendingModification);
}

private SourceException onConflict(AbstractShapeBuilder<?, ?> builder, AbstractShapeBuilder<?, ?> previous) {
Expand Down Expand Up @@ -181,13 +188,15 @@ public final CreatedShapes createShapes(TraitContainer resolvedTraits) {
List<Shape> resolvedShapes = new ArrayList<>(shapes.size());
List<PendingShape> pendingMixins = new ArrayList<>();

for (Map.Entry<ShapeId, Set<ShapeId>> entry : pendingShapes.entrySet()) {
for (Map.Entry<ShapeId, List<PendingShapeModifier>> entry : this.pendingModifications.entrySet()) {
ShapeId subject = entry.getKey();
Set<ShapeId> mixins = entry.getValue();
List<PendingShapeModifier> pendingModifications = entry.getValue();
Set<ShapeId> dependencies = pendingDependencies.getOrDefault(subject, Collections.emptySet());
AbstractShapeBuilder<?, ?> builder = shapes.get(entry.getKey());
Map<String, MemberShape.Builder> builderMembers = claimMembersOfContainer(builder.getId());
shapes.remove(entry.getKey());
pendingMixins.add(createPendingShape(subject, builder, builderMembers, mixins, traitContainer));
pendingMixins.add(createPendingShape(
subject, builder, builderMembers, dependencies, pendingModifications, traitContainer));
}

// Build members and add them to top-level shapes.
Expand Down Expand Up @@ -223,55 +232,21 @@ private PendingShape createPendingShape(
AbstractShapeBuilder<?, ?> builder,
Map<String, MemberShape.Builder> builderMembers,
Set<ShapeId> mixins,
List<PendingShapeModifier> pendingModifications,
TraitContainer resolvedTraits
) {
return PendingShape.create(subject, builder, mixins, shapeMap -> {
// Build normal members first.
for (MemberShape.Builder memberBuilder : builderMembers.values()) {
for (PendingShapeModifier pendingModification : pendingModifications) {
pendingModification.modifyMember(builder, memberBuilder, resolvedTraits, shapeMap);
}
buildShape(memberBuilder, resolvedTraits).ifPresent(builder::addMember);
}
// Add each mixin and ensure there are no member conflicts.
for (ShapeId mixin : mixins) {
Shape mixinShape = shapeMap.get(mixin);
for (MemberShape member : mixinShape.members()) {
ShapeId targetId = builder.getId().withMember(member.getMemberName());
Map<ShapeId, Trait> introducedTraits = traitContainer.getTraitsForShape(targetId);

MemberShape introducedMember = null;
if (builderMembers.containsKey(member.getMemberName())) {
introducedMember = builderMembers.get(member.getMemberName())
.addMixin(member)
.build();

if (!introducedMember.getTarget().equals(member.getTarget())) {
// Members cannot be redefined if their targets conflict.
MemberShape.Builder conflict = builderMembers.get(member.getMemberName());
events.add(ValidationEvent.builder()
.severity(Severity.ERROR)
.id(Validator.MODEL_ERROR)
.shapeId(conflict.getId())
.sourceLocation(conflict.getSourceLocation())
.message("Member conflicts with an inherited mixin member: " + member.getId())
.build());
}
} else if (!introducedTraits.isEmpty()) {
// Build local member copies before adding mixins if traits
// were introduced to inherited mixin members.
introducedMember = MemberShape.builder()
.id(targetId)
.target(member.getTarget())
.source(member.getSourceLocation())
.addTraits(introducedTraits.values())
.addMixin(member)
.build();
}

if (introducedMember != null) {
builder.addMember(introducedMember);
}
}
builder.addMixin(mixinShape);

for (PendingShapeModifier pendingModification : pendingModifications) {
pendingModification.modifyShape(builder, builderMembers, resolvedTraits, shapeMap);
}

buildShape(builder, resolvedTraits).ifPresent(result -> shapeMap.put(result.getId(), result));
});
}
Expand All @@ -285,10 +260,24 @@ private <S extends Shape, B extends AbstractShapeBuilder<? extends B, S>> Option
builder.addTrait(trait);
}
return Optional.of(builder.build());
} catch (IllegalStateException e) {
if (builder.getShapeType() == ShapeType.MEMBER && ((MemberShape.Builder) builder).getTarget() == null) {
mtdowling marked this conversation as resolved.
Show resolved Hide resolved
events.add(ValidationEvent.builder()
.severity(Severity.ERROR)
.id(Validator.MODEL_ERROR)
.shapeId(builder.getId())
.sourceLocation(builder.getSourceLocation())
.message("Member target was elided, but no bound resource or mixin contained a matching "
+ "identifier or member name.")
.build());
return Optional.empty();
}
throw e;
} catch (SourceException e) {
events.add(ValidationEvent.fromSourceException(e, "", builder.getId()));
resolvedTraits.clearTraitsForShape(builder.getId());
return Optional.empty();
}
}

}
Loading