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

Conversation

sywhang
Copy link
Contributor

@sywhang sywhang commented Sep 22, 2022

There is a bug that only manifests when decorating objects in Fx where transient dependency types are not properly decorated. For example, if type A depends on type B, and type B depends on type C which has a decorator, it fails to recognize that.

This only occurs when all constructors and the decorators for all these types are provided at the "root" level fx.App.

This happens because fx injects a fake "root" Scope between the actual root Container (which is where constructors are provided to by default).

Fixed by making fx stop create this fake root Scope and use the dig.Container directly.

Fixes #940.

There is a bug that only manifests when decorating objects in Fx
where transient dependency types are not properly decorated. For
example, if type A depends on type B, and type B depends on type C
which has a decorator, it fails to recognize that.

This only occurs when all constructors and the decorators for all
these types are provided at the "root" level fx.App.

This happens because fx injects a fake "root" Scope between the
actual root Container (which is where constructors are provided to
by default).

Fixed by making fx stop create this fake root Scope and use the
dig.Container directly.

Fixes uber-go#940.
@codecov
Copy link

codecov bot commented Sep 22, 2022

Codecov Report

Merging #941 (5f425f6) into master (f8aaaeb) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #941   +/-   ##
=======================================
  Coverage   98.72%   98.72%           
=======================================
  Files          30       30           
  Lines        1337     1337           
=======================================
  Hits         1320     1320           
  Misses         11       11           
  Partials        6        6           
Impacted Files Coverage Δ
module.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines -99 to -100
// TODO: Once fx.Decorate is in-place,
// use the root container instead of subscope.
Copy link
Collaborator

Choose a reason for hiding this comment

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

heh

@zenthangplus
Copy link

Thank, could you publish a release?

@sywhang
Copy link
Contributor Author

sywhang commented Sep 23, 2022

@zenthangplus Sure, I can. But can you wait until Monday, please? I'll need to verify the new version with Uber's Go Monorepo and we don't do releases on Friday to avoid causing issues over the weekend for services.

@zenthangplus
Copy link

@sywhang i've tested, fx.Decorate work well, but seems fx.Replace is not work as expected. As i known, Replace is equivalent with Decorate.

@sywhang
Copy link
Contributor Author

sywhang commented Sep 23, 2022

@sywhang i've tested, fx.Decorate work well, but seems fx.Replace is not work as expected. As i known, Replace is equivalent with Decorate.

Can you be more specific? What's the code you wrote to test it?

@zenthangplus
Copy link

@sywhang I've add two test cases, please take a look: #942

@zenthangplus
Copy link

Oops, seems have some problem on master branch. When upgrading to latest code on master branch, my app is not run correctly, i don't know why, but i'm investigating.

I'm using the bellow function to handle request (for testing only).

httpHandler := func(w http.ResponseWriter, r *http.Request) {
		ctx := context.Background()
		ctxWithResponseWriter := context.WithValue(ctx, "w", w)
		ctxWithRequest := context.WithValue(ctxWithResponseWriter, "r", r)
		_ = app.Start(ctxWithRequest)
	}

and append hook

lc.Append(
		fx.Hook{
			OnStart: func(ctx context.Context) error {
				log.Info("start application") // Problem: only show once time
				r := ctx.Value("r").(*http.Request)
				w := ctx.Value("w").(*httptest.ResponseRecorder)
				httpServer.Handler.ServeHTTP(w, r)
				return nil
			},
		},
	)

But when run a test suite with two test cases, the last test case seems not run correctly. OnStart hook seems only run once time.

Then i've tried to revert to v1.18.1 and it works correctly. Seems latest code on master has problem.

@abhinav
Copy link
Collaborator

abhinav commented Sep 27, 2022

Thanks for catching that @zenthangplus.
Would you mind creating a new issue for this? We nearly missed the comment.
If your investigation yields a reproduction of the bug, that would be really helpful for us to fix this.

@zenthangplus
Copy link

zenthangplus commented Sep 28, 2022

@abhinav i've checked, it related to this MR #931, it not been released yet

@sywhang
Copy link
Contributor Author

sywhang commented Sep 28, 2022

Hey @zenthangplus, could you please specify what you mean? OnStart/OnStop hooks are only supposed to run once. If you were relying on it being run twice, that's not the intended behavior.

@zenthangplus
Copy link

zenthangplus commented Sep 28, 2022

@sywhang In my case, i've wrote test for integration api testing. every http call to my api will be handled by this function.

httpHandler := func(w http.ResponseWriter, r *http.Request) {
		ctx := context.Background()
		ctxWithResponseWriter := context.WithValue(ctx, "w", w)
		ctxWithRequest := context.WithValue(ctxWithResponseWriter, "r", r)
		_ = app.Start(ctxWithRequest)
	}

Before it works well, but after this MR #931 app.Start is only allow to run once. I can change my logic to handle app.Stop before handle another request but i think this is breaking change and it break my test logic.

(again, it not related to this MR)

@abhinav
Copy link
Collaborator

abhinav commented Sep 28, 2022

@zenthangplus I've created #945. Let's continue the discussion there.

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.

sywhang added a commit to sywhang/fx that referenced this pull request Oct 11, 2022
There is a bug that only manifests when decorating objects in Fx
where transient dependency types are not properly decorated. For
example, if type A depends on type B, and type B depends on type C
which has a decorator, it fails to recognize that.

This only occurs when all constructors and the decorators for all
these types are provided at the "root" level fx.App.

This happens because fx injects a fake "root" Scope between the
actual root Container (which is where constructors are provided to
by default).

Fixed by making fx stop create this fake root Scope and use the
dig.Container directly.

Fixes uber-go#940.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

bug: Decorator is not applied properly
4 participants