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 registrations when app is launched. #500

Merged
merged 2 commits into from
Aug 10, 2023
Merged

Validate registrations when app is launched. #500

merged 2 commits into from
Aug 10, 2023

Conversation

mwhittaker
Copy link
Member

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:

$ 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:

// In weaver package.
type Interface interface { isComponentInterface() }
type Ref[T Interface] struct { ... }
type Implements[T Interface] struct { ... }
func (Implements[T]) isComponentInterface()

// In user's code.
type ExampleComponent {
    weaver.Interface
    Foo(context.Context) error
}

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.

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 mwhittaker requested a review from spetrovic77 August 4, 2023 20:42
@mwhittaker mwhittaker self-assigned this Aug 4, 2023
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.
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.

Very nice!

"github.com/ServiceWeaver/weaver/runtime/codegen"
)

// validateRegistrations validates the provided registrations, returning an
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/an/a/

@spetrovic77 spetrovic77 merged commit b6bfeab into main Aug 10, 2023
@spetrovic77 spetrovic77 deleted the validate branch August 10, 2023 15:18
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants