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

Implement mixins #889

Merged
merged 4 commits into from
Aug 20, 2021
Merged

Implement mixins #889

merged 4 commits into from
Aug 20, 2021

Conversation

mtdowling
Copy link
Member

@mtdowling mtdowling commented Aug 13, 2021

This massive PR implements the mixins proposal, but with the following
change that will be updated as a followup on the proposal:

The idea of redefining inherited members was removed, along with the
special (inherit) syntax. Feedback on this syntax was that it was
awkward. Redefining a member is now prohibited. Instead, only apply is
allowed. To make it easier to apply multiple traits to a single shape,
particularly a member, a block form of apply was added in a previous
commit.

This change adds:

  1. The ability to create mixins using the @mixin trait.
  2. The ability to restrict which members are inherited by shapes that
    use a mixin using localTraits on the @mixin trait.
  3. The ability to use a mixin with a shape in the IDL using with.
  4. The ability to use a mixin in the JSON AST using mixins.
  5. The ability to serialize mixins in the IDL and AST.
  6. The ability to query mixin relationships in selectors using the
    mixin relationship. This also ensures that mixins are considered
    part of a service closure.
  7. The ability to remove mixins from a model, and update all shapes that
    depend on the mixin.
  8. The ability to "flatten" mixins out of the model, meaning that the
    mixin is removed, but any members or traits it provides to shapes
    that use it are copied onto those shapes.
  9. Validation to ensure no mixin cycles or "with" statements are added
    that point to shapes that don't exist or that aren't marked with
    the @mixin trait.
  10. Mixins are flattened out of models when converting to JSON Schema
    and to OpenAPI.
  11. When a mixin is updated, any shape that uses that mixin is also
    updated.

Implementation notes:

  • Mixins were added to the Shape type. This made it far easier to
    implement everything rather than introducing something like a
    HasMixins interface implemented on structure, union, and member
    shapes or using visitors. This design decision is similar to what was
    already done with the members() method on Shape, which just makes
    Shape generally easir to use.

  • Shapes implicitly hold on to references to their mixins, but the mixin
    shapes are not directly exposed in the shape's API (only ShapeIds).
    This ensures that if we need to do any refactoring later, we can
    without making that a hard design requirement (it isn't so far!).

  • Missing validation about member shapes matching their container IDs
    was added because it was needed to ensure mixin members were added
    correctly.

  • An updated ListUtils.of() method was added for the case when there are
    two items. This was done because the members() method is now used
    each time a shape is created, which previously created an ArrayList
    each time the members of MapShape were requested.

    Followups:

    • Add specifications around mixins.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

This massive PR implements the mixins proposal, but with the following
change that will be updated as a followup on the proposal:

The idea of redefining inherited members was removed, along with the
special `(inherit)` syntax. Feedback on this syntax was that it was
awkward. Redefining a member is now prohibited. instead, only apply is
allowed. To make it easier to apply multiple traits to a single shape,
particularly a member, a block form of apply was added in a previous
commit.

This change adds:
1. The ability to create mixins using the `@mixin` trait.
2. The ability to restrict which members are inherited by shapes that
   use a mixin using `localTraits` on the `@mixin` trait.
3. The ability to use a mixin with a shape in the IDL using `with`.
4. The ability to use a mixin in the JSON AST using `mixins`.
5. The ability to serialize mixins in the IDL and AST.
6. The ability to query mixin relationships in selectors using the
   `mixin` relationship. This also ensures that mixins are considered
   part of a service closure.
7. The ability to remove mixins from a model, and update all shapes that
   depend on the mixin.
8. The ability to "flatten" mixins out of the model, meaning that the
   mixin is removed, but any members or traits it provides to shapes
   that use it are copied onto those shapes.
9. Validation to ensure no mixin cycles or "with" statements are added
   that point to shapes that don't exist or that aren't marked with
   the `@mixin` trait.
10. Mixins are flattened out of models when converting to JSON Schema
    and to OpenAPI.
11. When a mixin is updated, any shape that uses that mixin is also
    updated.

Implementation notes:
* Mixins were added to the Shape type. This made it far easier to
  implement everything rather than introducing something like a
  `HasMixins` interface implemented on structure, union, and member
  shapes or using visitors. This design decision is similar to what was
  already done with the `members()` method on `Shape`, which just makes
  `Shape` generally easir to use.
* Shapes implicitly hold on to references to their mixins, but the mixin
  shapes are not directly exposed in the shape's API (only ShapeIds).
  This ensures that if we need to do any refactoring later, we can
  without making that a hard design requirement (it isn't so far!).
* Missing validation about member shapes matching their container IDs
  was added because it was needed to ensure mixin members were added
  correctly.
* An updated ListUtils.of() method was added for the case when there are
  two items. This was done because the `members()` method is now used
  each time a shape is created, which previously created an ArrayList
  each time the members of MapShape were requested.

  Followups:
  * Update the mixins design doc.
  * Add specifications around mixins.
Mixins are now properly serialized depending on how many mixins there
are and if there are members. This also fixes a bug in how mixin member
copies were created -- they were previoulsy carrying over local traits
of their parents, which is unwanted, so now a separate builder is used.
Copy link
Contributor

@JordonPhillips JordonPhillips left a comment

Choose a reason for hiding this comment

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

There doesn't seem to be any tests for either model serializer, so those will be needed.

Copy link
Contributor

@JordonPhillips JordonPhillips left a comment

Choose a reason for hiding this comment

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

Meant to do request changes not comment

@mtdowling
Copy link
Member Author

There doesn't seem to be any tests for either model serializer, so those will be needed.

These are tested using the integration test runner defined in sofware/amazon/smithy/model/shapes/idl-serialization/cases/mixins.smithy and the AST serialization is covered via the model loading integration tests that require .smithy models to be loaded and .json models to be created (software/amazon/smithy/model/loader/valid/mixins/).

@mtdowling mtdowling merged commit 80252d9 into idl-1.1 Aug 20, 2021
@mtdowling mtdowling deleted the implement-mixins branch September 21, 2021 22:59
@mtdowling mtdowling restored the implement-mixins branch September 22, 2021 20:07
@mtdowling mtdowling deleted the implement-mixins branch September 22, 2021 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants