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 configurable diff evaluator #2056

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

milesziemer
Copy link
Contributor

Adds a smithy-diff evaluator which can be configured via metadata property diffEvaluators within the model. Unlike the similar EmitEach/EmitNoneSelectorValidators, this diff evaluator is not loaded using SPI, and the ability to create your own custom configurable diff evaluators has not been exposed. Instead, smithy-diff just looks at the new model's metadata and loads any diff evaluators directly. We can expose creating custom configurable diff evaluators later if there is a need, but this diff evaluator should be enough for almost every use case, and we can also extend it easily with more configuration options.

This evaluator works as follows:

  1. Get a subset of shapes to match based on the appliesTo property. Currently either added shapes or removed shapes.
  2. Optionally filter this subset of shapes further with a selector configured in the filter property, which runs on either the new or old model depending on appliesTo.
  3. Run the configured selector on either the new model or old model depending on appliesTo.
  4. Match shapes returned by selector to the set of shapes from 2, and emit events based on emitCondition.

This functionality can be extended later by adding more options for emitCondition and appliesTo. For example, there is currently no configuration option for looking at changed shapes, but you could imaging wanting to see which shapes don't match in the old model, but do in the new model, which would be a new appliesTo. Another emitCondition could be IfAnyDontMatch or something.

Also adds a validator to smithy-diff for this metadata property so you'll know when the model is being build if evaluators have been configured incorrectly. This requires having a dependency on smithy-diff in the model package where you're configuring the diff evaluators, but the alternative is stick the validation in smithy-model, creating an implicit dependency on smithy-diff.

See the added test case for an example.

Next Steps:

  • Should also add to our docs for this once we know it's concrete.
  • Another thing to look at would be moving some of this functionality into smithy-model, since its similar to how the current configurable validators work and we could provide more configuration options for those.

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

Adds a smithy-diff evaluator which can be configured via metadata
property `diffEvaluators` within the model. Unlike the similar
EmitEach/EmitNoneSelectorValidators, this diff evaluator is not
loaded using SPI, and the ability to create your own custom
configurable diff evaluators has not been exposed. Instead,
smithy-diff just looks at the new model's metadata and loads
any diff evaluators directly. We can expose creating custom
configurable diff evaluators later if there is a need, but this
diff evaluator should be enough for almost every use case, and
we can also extend it easily with more configuration options.

This evaluator works as follows:
1. Get a subset of shapes to match based on the `appliesTo`
property. Currently either added shapes or removed shapes.
2. Optionally filter this subset of shapes further with a
selector configured in the `filter` property, which runs
on either the new or old model depending on `appliesTo`.
3. Run the configured `selector` on either the new model or
old model depending on `appliesTo`.
4. Match shapes returned by `selector` to the set of shapes
from 2, and emit events based on `emitCondition`.

This functionality can be extended later by adding more options
for `emitCondition` and `appliesTo`. For example, there is
currently no configuration option for looking at changed shapes,
but you could imaging wanting to see which shapes don't match
in the old model, but do in the new model, which would be a new
`appliesTo`. Another `emitCondition` could be `IfAnyDontMatch`
or something.

Also adds a validator to smithy-diff for this metadata property so
you'll know when the model is being build if evaluators have been
configured incorrectly. This requires having a dependency on
smithy-diff in the model package where you're configuring the diff
evaluators, but the alternative is stick the validation in
smithy-model, creating an implicit dependency on smithy-diff.
@milesziemer milesziemer force-pushed the add-configurable-diff-evaluator branch from 273a14c to 5c8f6a9 Compare November 30, 2023 15:50
selector: "[trait|default = 0]"
}
{
id: "AddedMemberWithoutClientOptional"
Copy link
Member

Choose a reason for hiding this comment

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

Can you provide an example of IfAnyMatch that is more realistic?

id: "RemovedRootShape"
message: "Removed root shape."
emitCondition: "ForEachMatch"
appliesTo: "RemovedShapes"
Copy link
Member

Choose a reason for hiding this comment

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

Can we get more removed examples?

{
id: "AddedMemberWithoutClientOptional"
message: "One of the added members does not have `@clientOptional` trait."
emitCondition: "IfAnyMatch"
Copy link
Member

Choose a reason for hiding this comment

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

Is there any use case for IfNoneMatch?

@@ -0,0 +1,54 @@
$version: "2.0"

metadata diffEvaluators = [
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should emulate validators: https://smithy.io/2.0/spec/model-validation.html#validators. So more like:

{
    id: "AddedInternalOperation"
    name: "EmitEachMatch|EmitIfNoneMatch|EmitIfAnyMatch|..."
    severity: "NOTE"
    configuration: {
        appliesTo: "AddedShapes|RemovedShapes|ChangedShapes"
        selector: "operation [trait|internal]"
    }
}

This keeps the door open for future configurable diff evaluators, and maybe even the ability to configure built-in evaluators. This could also, if we really look into the future, potentially cover metadata diffs and trait diffs too.

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.

2 participants