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

Verify registered listeners when starting the app. #561

Merged
merged 1 commit into from
Aug 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dev/build_and_test
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function cmd_vet() {

function cmd_lint() {
if ! exists staticcheck; then
printf "staticcheck not found; install via\ngo install honnef.co/go/tools/cmd/staticcheck@0.4.3\n" >&2
printf "staticcheck not found; install via\ngo install honnef.co/go/tools/cmd/staticcheck@v0.4.3\n" >&2
spetrovic77 marked this conversation as resolved.
Show resolved Hide resolved
exit 1
fi

Expand Down
2 changes: 1 addition & 1 deletion fill.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,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, ok := t.Tag.Lookup("weaver"); ok {
if !isValidListenerName(tag) {
if !isValidListenerName(name) {
return fmt.Errorf("FillListeners: listener tag %s is not a valid Go identifier", tag)
}
name = tag
Expand Down
1 change: 1 addition & 0 deletions godeps.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ github.com/ServiceWeaver/weaver
github.com/ServiceWeaver/weaver/runtime/codegen
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp
go.opentelemetry.io/otel/trace
golang.org/x/exp/slices
log/slog
math/rand
net
Expand Down
10 changes: 10 additions & 0 deletions internal/tool/generate/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,16 @@ func extractComponent(opt Options, pkg *packages.Package, file *ast.File, tset *
return nil, err
}

// Check that listener names are unique.
spetrovic77 marked this conversation as resolved.
Show resolved Hide resolved
seenLis := map[string]struct{}{}
for _, lis := range listeners {
if _, ok := seenLis[lis]; ok {
return nil, errorf(pkg.Fset, spec.Pos(),
"component implementation %s declares multiple listeners with name %s. Please disambiguate.", formatType(pkg, impl), lis)
}
seenLis[lis] = struct{}{}
}

// Warn the user if the component has a mistyped Init method. Init methods
// are supposed to have type "func(context.Context) error", but it's easy
// to forget to add a context.Context argument or error return. Without
Expand Down
30 changes: 30 additions & 0 deletions internal/tool/generate/testdata/errors/listeners_name_clash.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright 2022 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

// ERROR: multiple listeners with name bar

// Multiple listeners with the same name.
package foo

import (
"github.com/ServiceWeaver/weaver"
)

type foo interface{}

type impl struct {
weaver.Implements[foo]
bar weaver.Listener
_ weaver.Listener `weaver:"bar"`
}
16 changes: 13 additions & 3 deletions validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

"github.com/ServiceWeaver/weaver/internal/reflection"
"github.com/ServiceWeaver/weaver/runtime/codegen"
"golang.org/x/exp/slices"
)

// validateRegistrations validates the provided registrations, returning an
Expand All @@ -48,16 +49,25 @@ func validateRegistrations(regs []*codegen.Registration) error {
if _, ok := intfs[v.Type]; !ok {
// T is not a registered component interface.
err := fmt.Errorf(
"component implementation struct %v has field %v, but component %v was not registered; maybe you forgot to run 'weaver generate'",
"component implementation struct %v has component reference field %v, but component %v was not registered; maybe you forgot to run 'weaver generate'",
reg.Impl, f.Type, v.Type,
)
errs = append(errs, err)
}

case f.Type == reflection.Type[Listener]():
// f is a weaver.Listener.
if tag, ok := f.Tag.Lookup("weaver"); ok && !isValidListenerName(tag) {
err := fmt.Errorf("component implementation struct %v has invalid listener tag %q", reg.Impl, tag)
name := f.Name
if tag, ok := f.Tag.Lookup("weaver"); ok {
if !isValidListenerName(tag) {
err := fmt.Errorf("component implementation struct %v has invalid listener tag %q", reg.Impl, tag)
errs = append(errs, err)
continue
}
name = tag
}
if !slices.Contains(reg.Listeners, name) {
err := fmt.Errorf("component implementation struct %v has a listener field %v, but listener %v hasn't been registered; maybe you forgot to run 'weaver generate'", reg.Impl, name, name)
errs = append(errs, err)
}
}
Expand Down
51 changes: 43 additions & 8 deletions validate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,18 +34,21 @@ func TestValidateNoRegistrations(t *testing.T) {
// TestValidateValidRegistrations tests that validateRegistrations succeeds on
// a set of valid registrations.
func TestValidateValidRegistrations(t *testing.T) {
type foo struct{}
type bar struct{}
type foo interface{}
type bar interface{}
type fooImpl struct {
Ref[bar]
Listener `weaver:"foo"`
Listener `weaver:"lis1"`
_ Listener `weaver:"lis2"`
lis3 Listener //lint:ignore U1000 Present for code generation.
}
type barImpl struct{ Ref[foo] }
regs := []*codegen.Registration{
{
Name: "foo",
Iface: reflection.Type[foo](),
Impl: reflection.Type[fooImpl](),
Name: "foo",
Iface: reflection.Type[foo](),
Impl: reflection.Type[fooImpl](),
Listeners: []string{"lis1", "lis2", "lis3"},
},
{
Name: "bar",
Expand All @@ -61,7 +64,7 @@ func TestValidateValidRegistrations(t *testing.T) {
// TestValidateUnregisteredRef tests that validateRegistrations fails when a
// component has a weaver.Ref on an unregistered component.
func TestValidateUnregisteredRef(t *testing.T) {
type foo struct{}
type foo interface{}
type fooImpl struct{ Ref[io.Reader] }
regs := []*codegen.Registration{
{
Expand All @@ -83,7 +86,7 @@ func TestValidateUnregisteredRef(t *testing.T) {
// TestValidateInvalidListenerNames tests that validateRegistrations fails on
// invalid listener names.
func TestValidateInvalidListenerNames(t *testing.T) {
type foo struct{}
type foo interface{}
type fooImpl struct {
_ Listener `weaver:""` // empty name
_ Listener `weaver:" "` // whitespace name
Expand Down Expand Up @@ -114,3 +117,35 @@ func TestValidateInvalidListenerNames(t *testing.T) {
}
}
}

// TestValidateUnregisteredListeners tests that validateRegistrations fails on
// listener names that haven't been registered.
func TestValidateUnregisteredListener(t *testing.T) {
type foo interface{}
type fooImpl struct {
foo Listener //lint:ignore U1000 Present for code generation.
bar Listener //lint:ignore U1000 Present for code generation.
baz Listener //lint:ignore U1000 Present for code generation.
}

regs := []*codegen.Registration{
{
Name: "foo",
Iface: reflection.Type[foo](),
Impl: reflection.Type[fooImpl](),
Listeners: []string{"foo"},
},
}
err := validateRegistrations(regs)
if err == nil {
t.Fatal("unexpected validateRegistrations success")
}
for _, want := range []string{
`listener bar hasn't been registered`,
`listener baz hasn't been registered`,
} {
if !strings.Contains(err.Error(), want) {
t.Errorf("validateRegistrations: got %q, want %q", err, want)
}
}
}