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

Validate listener names at application startup. #501

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

mwhittaker
Copy link
Member

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 mwhittaker requested a review from spetrovic77 August 4, 2023 21:21
@mwhittaker mwhittaker self-assigned this Aug 4, 2023
Base automatically changed from validate to main August 10, 2023 15:18
Copy link
Contributor

@spetrovic77 spetrovic77 left a comment

Choose a reason for hiding this comment

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

A lot of people in our workshops ran into a problem where they added a listener to weaver.Main, but forgot to run weaver generate.

Do you know if we print a helpful error in that case?

@@ -101,7 +100,7 @@ func fillListeners(impl any, get func(name string) (net.Listener, string, error)

// The listener's name is the field name, unless a tag is present.
name := t.Name
if tag := t.Tag.Get("weaver"); tag != "" {
if tag, ok := t.Tag.Lookup("weaver"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

This was intentional: I was writing an empty service and wanted to use the empty listener name. 🤣

Thanks for fixing!

@mwhittaker
Copy link
Member Author

A lot of people in our workshops ran into a problem where they added a listener to weaver.Main, but forgot to run weaver generate.

Do you know if we print a helpful error in that case?

I don't think we do. Let me work on that in another PR.

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 mwhittaker force-pushed the validate_listener_names branch from 5ae0192 to 522830c Compare August 10, 2023 16:34
@mwhittaker mwhittaker merged commit 968d7ba into main Aug 10, 2023
@mwhittaker mwhittaker deleted the validate_listener_names branch August 10, 2023 16:40
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