-
Notifications
You must be signed in to change notification settings - Fork 254
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
Validate registrations when app is launched. #500
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR introduces a validateRegistrations function that checks the validity of component registrations. For now, it simply checks that every weaver.Ref[T] field refers to a registered component interface T. This mistake is actually quite common, when you add a component and forget to run `weaver generate`. Before this PR, if a non-main component had a weaver.Ref[T] to an unregistered component, the app would run but silently not work. With this change, all weavelets will imediately crash, and for `go run .` and `weaver multi deploy`, the app will terminate immediately with a helpful error message: ```shell $ weaver multi deploy weaver.toml start main process: NewEnvelope: connect to weavelet: read protobuf length: EOF -----BEGIN STDERR----- 2023/08/04 13:31:50 component implementation struct main.odd has field weaver.Ref[main.this_is_not_a_registered_component], but component main.this_is_not_a_registered_component was not registered; maybe you forgot to run 'weaver generate' -----END STDERR----- exit status 1 ``` >> Alternatives Srdjan had a great idea of checking the validity of weaver.Ref[T]s at compile time, but I couldn't think of any way to do that. If we do think of something, we should switch to it. I did have one idea that turned out to be terrible. We could require users to embed a special `weaver.Interface` interace into their component interfaces: ```go // In weaver package. type Interface interface { isComponentInterface() } type Ref[T Interface] struct { ... } type Implements[T Interface] struc { ... } func (Implements[T]) isComponentInterface() // In user's code. type ExampleComponent { weaver.Interface } ``` This is not only ugly and super janky, it also doesn't work. It checks that a `weaver.Ref[T]` refers to a component interface `T`, but it doesn't check that `T` was actually registered. If you forget to run `weaver generate`, it won't be.
mwhittaker
added a commit
that referenced
this pull request
Aug 4, 2023
This PR extends `validateRegistrations` to validate listener names when a Service Weaver application begins. As with PR #500, this PR aims to catch permanent and easily avoidable programming errors immediately. I also updated the website documentation to describe legal listener names.
mwhittaker
added a commit
that referenced
this pull request
Aug 4, 2023
This PR extends the `validateRegistrations` function to also validate listener names when a Service Weaver application begins. As with PR #500, this PR aims to catch permanent and easily avoidable programming errors immediately. I also fixed a small bug in listener name validation. Previously, if a listener had a `weaver:""` annotation, it was being ignored. Now, it is being rejected (since `""` is not a valid listener name). I also updated the website documentation to describe legal listener names.
mwhittaker
added a commit
that referenced
this pull request
Aug 4, 2023
This PR extends the `validateRegistrations` function to also validate listener names when a Service Weaver application begins. As with PR #500, this PR aims to catch permanent and easily avoidable programming errors immediately. I also fixed a small bug in listener name validation. Previously, if a listener had a `weaver:""` annotation, it was being ignored. Now, it is being rejected (since `""` is not a valid listener name). I also updated the website documentation to describe legal listener names.
spetrovic77
approved these changes
Aug 10, 2023
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.
Very nice!
"github.com/ServiceWeaver/weaver/runtime/codegen" | ||
) | ||
|
||
// validateRegistrations validates the provided registrations, returning an |
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.
nit: s/an/a/
mwhittaker
added a commit
that referenced
this pull request
Aug 10, 2023
This PR extends the `validateRegistrations` function to also validate listener names when a Service Weaver application begins. As with PR #500, this PR aims to catch permanent and easily avoidable programming errors immediately. I also fixed a small bug in listener name validation. Previously, if a listener had a `weaver:""` annotation, it was being ignored. Now, it is being rejected (since `""` is not a valid listener name). I also updated the website documentation to describe legal listener names.
mwhittaker
added a commit
that referenced
this pull request
Aug 10, 2023
This PR extends the `validateRegistrations` function to also validate listener names when a Service Weaver application begins. As with PR #500, this PR aims to catch permanent and easily avoidable programming errors immediately. I also fixed a small bug in listener name validation. Previously, if a listener had a `weaver:""` annotation, it was being ignored. Now, it is being rejected (since `""` is not a valid listener name). I also updated the website documentation to describe legal listener names.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces a
validateRegistrations
function that checks the validity of component registrations. For now, it simply checks that everyweaver.Ref[T]
field refers to a registered component interfaceT
. This mistake is actually quite common, when you add a component and forget to runweaver generate
.Before this PR, if a non-main component had a
weaver.Ref[T]
to an unregistered component, the app would run but silently not work. With this change, all weavelets will imediately crash, and forgo run .
andweaver multi deploy
, the app will terminate immediately with a helpful error message:Alternatives
Srdjan had a great idea of checking the validity of
weaver.Ref[T]
s at compile time, but I couldn't think of any way to do that. If we do think of something, we should switch to it.I did have one idea that turned out to be terrible. We could require users to embed a special
weaver.Interface
interace into their component interfaces:This is not only ugly and super janky, it also doesn't work. It checks that a
weaver.Ref[T]
refers to a component interfaceT
, but it doesn't check thatT
was actually registered.