-
Notifications
You must be signed in to change notification settings - Fork 89
More predictable handling of trait/validator classes model loader #1709
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
Conversation
| // Loading the upstream model | ||
| val upstreamModel = Model | ||
| .assembler() | ||
| .assembler(validatorClassLoader) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should make the classes (validators, trait services) visible to the assembler - but notably not making the models visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
ok I think the easiest way to test this will be to use a custom (non-alloy) trait class in a plugin/CLI test - as proven by polyvariant/smithy4s-bsp#10 this finally allows us to use those properly. the other thing would still be nice to test though, I'll need to think of a library that we can use as the example (or make a plugin test like that somehow) |
| @@ -0,0 +1,4 @@ | |||
| # check if the app runs successfully. | |||
| # if it doesn't compile, it could be because the transformation wasn't applied. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: it is WAY too easy to misconfigure a transformation, hence this extra care
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW we had no other tests that involved user-provided transformations/validators, apparently. There's no software.amazon.smithy.build.ProjectionTransformer or software.amazon.smithy.model.validation.Validator in the test resources. I think these tests suffice, but maybe we should have a more minimal one for both too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe indeed
Workaround / fix for smithy-lang/smithy#2596. Even fixes #336.
tl;dr without passing the classloader to
assembler(), we can run into strange cases where the validator gets loaded and the trait services don't - meaning the validators won't behave like they're supposed to.PR Checklist (not all items are relevant to all PRs)