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

Add breakingChanges property to @trait #1193

Merged
merged 1 commit into from
Apr 25, 2022
Merged

Conversation

mtdowling
Copy link
Member

@mtdowling mtdowling commented Apr 17, 2022

Rather than rely on specialized tags like diff.error.add, we can add
breakingChanges rules to traits directly.

Other changes to make this easier to implement include:

  • Add getMember(String) to Shape simplifies the trait diff validator
    significantly, and can be used to simplify many other code paths in
    Smithy if we go back and refactor them.
  • Added toNode and fromNode to various classes like Severity,
    NodePointer, etc.

It is a backward incompatible change to add the following trait to an existing shape:

@trait(breakingChanges: [{change: "add"}])
structure cannotAdd {}

Note: The above trait definition is equivalent to the following:

@trait(
    breakingChanges: [
        {
            change: "add",
            path: "",
            severity: "ERROR"
        }
    ]
)
structure cannotAdd {}

It is a backward incompatible change to add or remove the following trait from an existing shape:

@trait(breakingChanges: [{change: "presence"}])
structure cannotToAddOrRemove {}

It is very likely backward incompatible to change the "foo" member of the following trait or to remove the "baz" member:

@trait(
    breakingChanges: [
        {
            change: "update",
            path: "/foo",
            severity: "DANGER"
        },
        {
            change: "remove",
            path: "/baz",
            severity: "DANGER"
        }
    ]
)
structure fooBaz {
    foo: String,
    baz: String
}

So for example, if the following shape:

@fooBaz(foo: "a", baz: "b")
string Example

Is changed to:

@fooBaz(foo: "b")
string Example

Then the change to the foo member from "a" to "b" is backward incompatible, as is the removal of the baz member.

Referring to list and set members

The JSON pointer can path into the members of a list or set using a member segment.

In the following example, it is a breaking change to change values of lists or sets in instances of the names trait:

@trait(
    breakingChanges: [
        {
            change: "update",
            path: "/names/member"
        }
    ]
)
structure names {
    names: NameList
}

@private
list NameList {
    member: String
}

So for example, if the following shape:

@names(names: ["Han", "Luke"])
string Example

Is changed to:

@names(names: ["Han", "Chewy"])
string Example

Then the change to the second value of the names member is backward incompatible because it changed from Luke to Chewy.

Referring to map members

Members of a map shape can be referenced in a JSON pointer using key
and value.

The following example defines a trait where it is backward incompatible to remove a key value pair from a map:

@trait(
    breakingChanges: [
        {
            change: "remove",
            path: "/key"
        }
    ]
)
map jobs {
    key: String,
    value: String
}

So for example, if the following shape:

@jobs(Han: "Smuggler", Luke: "Jedi")
string Example

Is changed to:

@jobs(Luke: "Jedi")
string Example

Then the removal of the "Han" entry of the map is flagged as backward incompatible.

The following example detects when values of a map change.

@trait(
    breakingChanges: [
        {
            change: "update",
            path: "/value"
        }
    ]
)
map jobs {
    key: String,
    value: String
}

So for example, if the following shape:

@jobs(Han: "Smuggler", Luke: "Jedi")
string Example

Is changed to:

@jobs(Han: "Smuggler", Luke: "Ghost")
string Example

Then the change to Luke's mapping from "Jedi" to "Ghost" is backward incompatible.

Note:

  • Using the "update" change type with a map key has no effect.
  • Using any change type other than "update" with map values has no
    effect.

What this does not support

Note that the current syntax of JSON pointers and validation does not support referring to specific values or a list or set or specific entries in a map. For example, you can't say it's a breaking change to update the first element of a list using /0. Nor can you say it's a breaking change to update a specific named entry for a map like /someKeyName. This was left out for now, but could be added later if needed. For example, kind of special syntax like /[0] and /[foo], or perhaps /member[0] and /key[foo].

Possible future extension

If we ever really needed it, a conditional change type could be added to add more complex diff detection.

@trait(
    breakingChanges: [
        {
            change: "conditinal",
            // these are and-ed together. An "or" and "and" conditional could be added here.
            conditions: [
                // This could add oldValue and newValue to make it even more specific.
                update: {
                    path: "/foo",
                    oldValue: "bye",
                    newValue: "hi"
                },
                remove: {
                    path: "/bar"
                }
            ],
            severity: "ERROR",
            message: "You cannot change /foo from bye to hi and also remove /bar!"
        }
    ]
)

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

@mtdowling mtdowling requested a review from a team as a code owner April 17, 2022 06:37
@JordonPhillips
Copy link
Contributor

Note that the current syntax of JSON pointers and validation does not support referring to specific values or a list or set or specific entries in a map. For example, you can't say it's a breaking change to update the first element of a list using /0. Nor can you say it's a breaking change to update a specific named entry for a map like /someKeyName. This was left out for now, but could be added later if needed. For example, kind of special syntax like /[0] and /[foo], or perhaps /member[0] and /key[foo].

We could bring back the idea of jmespath-lite to address those referencing issues

@mtdowling mtdowling force-pushed the traits-with-diff-property branch from 878127e to 15e38ef Compare April 19, 2022 16:50
@mtdowling
Copy link
Member Author

We could bring back the idea of jmespath-lite to address those referencing issues

Yeah I considered JMESPath here, but there were some advantages to JSON Pointer:

  • We already have an implementation in smithy-model
  • It's more visually related to the model
  • JSON pointer results in simpler paths. JMESPath is more complex than JSON Pointer because it can do a lot more. I don't think we need the added power here though.

Here are examples of how this would look with JMESPath. Given the map key example, I don't think we'd be able to get away with "JMESPath-lite" unfortunately.

Match on the entire trait:

  • Pointer: ""
  • JMESPath: "@"
  • Very few models will ever specify an empty path or "@" and should instead
    just omit a path.

Match on specific structure/union members:

  • Pointer: "/foo/bar/baz"
  • JMESPath: "foo.bar.bar"
  • This is fine and not a meaningful difference.

Match on list/set members:

  • Pointer: "/someList/member"
  • JMESPath: "someList[*]"
  • The JMESPath expression is a bit more disconnected from the model. The above
    paths mean to traverse into a structure/union member named "someList" and
    then yields the values of each member in the list.

Match on map values:

  • Pointer: "/someMap/value"
  • JMESPath: "someMap.*"
  • Like the list/set path examples, this fine, but the JMESPath expression feels
    a bit disconnected from the model. It paths into a structure/union member
    named "someMap" and yields each map entry's value.

Match on keys:

  • Pointer: "/key"
  • JMESPath: "keys(@).*"
  • The JMESPath expression here is pretty bad because it requires functions and
    a lot more syntax. The JMESPath-lite idea isn't a reality if we want to
    support this. This is useful for detecting things like removing an entry
    from a map.

Match on specific map key value pairs:

Note that this is not a proposed feature of this proposal. We don't have this
use case today. This is just a potential approach we could take if we ever
needed it.

  • Pointer: "/someMap/[someKey]"
  • JMESPath: "someMap.someKey"
  • The JMESPath expression is a bit simpler here.

Match on specific list index:

Note that this is not a proposed feature of this proposal. We don't have this
use case today. This is just a potential approach we could take if we ever
needed it.

  • Pointer: "/someList/[0]"
  • JMESPath: "someList[0]"
  • The JMESPath expression it a bit simpler here.

@mtdowling mtdowling force-pushed the traits-with-diff-property branch from 15e38ef to d7a3b13 Compare April 22, 2022 23:52
@mtdowling mtdowling requested a review from gosar April 22, 2022 23:52
Rather than rely on specialized tags like `diff.error.add`, we can add
`breakingChanges` rules to traits directly.

Other changes to make this easier to implement include:

* Add getMember(String) to Shape simplifies the trait diff validator
  significantly, and can be used to simplify many other code paths in
  Smithy if we go back and refactor them.
* Added toNode and fromNode to various classes like Severity,
  NodePointer, etc.
* Migrate diff.error.* tags to breakingChanges
* Update smithy-diff docs to state that diff tags are deprecated.
@mtdowling mtdowling force-pushed the traits-with-diff-property branch from d7a3b13 to d3f437a Compare April 23, 2022 02:10
@mtdowling mtdowling requested a review from gosar April 23, 2022 02:10
@mtdowling mtdowling merged commit a2ce817 into main Apr 25, 2022
@mtdowling mtdowling deleted the traits-with-diff-property branch June 13, 2022 05:06
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