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

Fix decorator not being applied to transient dependencies #941

Merged
merged 2 commits into from
Sep 22, 2022
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
33 changes: 33 additions & 0 deletions decorate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,39 @@ func TestDecorateSuccess(t *testing.T) {
)
defer app.RequireStart().RequireStop()
})

t.Run("decoration must execute when required by a member of group", func(t *testing.T) {
type Drinks interface {
}
type Coffee struct {
Type string
Name string
Price int
}
type PriceService struct {
DefaultPrice int
}
app := fxtest.New(t,
Copy link
Contributor

Choose a reason for hiding this comment

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

This test would pass even if none of the functions executed. What guarantees the assertions on 337 and 344 are called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Invokes are guaranteed to run (and the fact that it ran is guaranteed by fx.RequireStart().RequireStop() - if any Invokes fail to run, that fails the test). Invoke running successfully means assertion in line 344 are called.

And yes, line 337 in itself does not guarantee the execution, but the assertion in line 344 means both constructors Provided here ran successfully.

fx.Provide(func() *PriceService {
return &PriceService{DefaultPrice: 3}
}),
fx.Decorate(func(service *PriceService) *PriceService {
service.DefaultPrice = 10
return service
}),
fx.Provide(fx.Annotate(func(service *PriceService) Drinks {
assert.Equal(t, 10, service.DefaultPrice)
return &Coffee{Type: "coffee", Name: "Americano", Price: service.DefaultPrice}
}, fx.ResultTags(`group:"drinks"`))),
fx.Provide(fx.Annotated{Group: "drinks", Target: func() Drinks {
return &Coffee{Type: "coffee", Name: "Cold Brew", Price: 4}
}}),
fx.Invoke(fx.Annotate(func(drinks []Drinks) {
assert.Len(t, drinks, 2)
}, fx.ParamTags(`group:"drinks"`))),
)
defer app.RequireStart().RequireStop()
})
}

func TestDecorateFailure(t *testing.T) {
Expand Down
17 changes: 13 additions & 4 deletions module.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,23 +81,32 @@ func (o moduleOption) apply(mod *module) {
type module struct {
parent *module
name string
scope *dig.Scope
scope scope
provides []provide
invokes []invoke
decorators []decorator
modules []*module
app *App
}

// scope is a private wrapper interface for dig.Container and dig.Scope.
// We can consider moving this into Fx using type constraints after Go 1.20
// is released and 1.17 is deprecated.
type scope interface {
Decorate(f interface{}, opts ...dig.DecorateOption) error
Invoke(f interface{}, opts ...dig.InvokeOption) error
Provide(f interface{}, opts ...dig.ProvideOption) error
Scope(name string, opts ...dig.ScopeOption) *dig.Scope
String() string
}

// builds the Scopes using the App's Container. Note that this happens
// after applyModules' are called because the App's Container needs to
// be built for any Scopes to be initialized, and applys' should be called
// before the Container can get initialized.
func (m *module) build(app *App, root *dig.Container) {
if m.parent == nil {
m.scope = root.Scope(m.name)
// TODO: Once fx.Decorate is in-place,
// use the root container instead of subscope.
Comment on lines -99 to -100
Copy link
Collaborator

Choose a reason for hiding this comment

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

heh

m.scope = root
} else {
parentScope := m.parent.scope
m.scope = parentScope.Scope(m.name)
Expand Down