Fix decorator not being applied to transient dependencies

[sywhang]

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.

[codecov]

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

[abhinav]

heh

[zenthangplus]

Thank, could you publish a release?

[sywhang]

@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]

@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]

@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]

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

[zenthangplus]

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]

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]

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

[sywhang]

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]

@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]

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

[jkanywhere]

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

[sywhang]

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.