#2178 DI service resolution from scoped HttpContext request services for the IConsulServiceBuilder service

[Niksson]

Fixes #2178

• In v23.3.4 ConsulProviderFactory can't resolve IConsulServiceBuilder via IServiceProvider #2178

This fixes the issue with DI service resolution for Consul provider mentioned in #2178

Proposed Changes

• ConsulProviderFactory - resolve the IConsulServiceBuilder from the HttpContext.RequestServices
• Provide a unit test that detects a scope validation error for ConsulProviderFactory

Some notes:

1. I'm not sure if the test that detects a scope validation error is strictly required. By enabling scope validation in service provider all code that tries to resolve scoped services from singletons should make the tests red.
2. This PR's scope is Consul provider only. In acceptance tests I made the scope validation option opt-in, to not break other tests. It may be good to force it when the other providers are also fixed
3. ConsulProviderFactory.Get creates both Consul and PollConsul providers, so the fix should cover both of them

If there is anything that can be changed to improve quality - do not hesitate to propose changes

Docs

• Best practices: Dependency injection in ASP.NET Core
• ASP.NET Core Web Host | Scope validation
• Using scoped services inside singletons by Sam Walpole

[raman-m]

Thank you for the high-quality PR. I suggest writing acceptance tests, if applicable.

[raman-m]

Does reading context services implicitly create a scope? I'll demonstrate the difference in the following code:

- var scope = provider.CreateScope();
- var serviceBuilder = scope.ServiceProvider.GetService<IConsulServiceBuilder>(); // consume data in default/custom builder
+ var context = contextAccessor.HttpContext;
+ var serviceBuilder = context.RequestServices.GetService<IConsulServiceBuilder>(); // consume data in default/custom builder

Thus, are the two solutions equivalent?

[Niksson]

AFAIK ASP.NET creates a DI scope for HttpContext of each HTTP request (see this link for more info).
Technically, the solutions are equivalent, but I thought that since we already access HttpContext via IHttpContextAccessor, it would be logical to access the scope of the HTTP request instead of creating a new one.

[raman-m]

see this link for more info

Yep, good to know!
Bu gaga, Tom should read these best DI practices in 2017-2020 when designing everything as singletons and delegates → Design services for dependency injection

---

🆗 Not an issue. Your solution should function properly.

[raman-m]

Trigger pipeline 🤣

Please ask me to initiate new builds. Are you not able to click the buttons in the Checks? 🤔

[Niksson]

Please ask me to initiate new builds.

Okay, will do. There was a test that was flaky, so I just triggered another run by the only way I know ;)
Unfortunately, there were no other buttons that allow me to retry checks.

[Niksson]

IMO new acceptance tests are not required here:

• The existing ones and the future ones should detect invalid service resolution if the scope validation is turned on in helpers. I didn't do it for every acceptance test since the scope of this PR is limited.
• More technical reason: I don't know of a way to have a non-scoped (aka root) service provider in HttpContext while using TestHost 😅

[raman-m]

2. This PR's scope is Consul provider only. In acceptance tests I made the scope validation option opt-in, to not break other tests. It may be good to force it when the other providers are also fixed

Fair enough, but how many tests would break if we bypass the parameter and enforce scope validation for all tests? It would be logically preferable to resolve all issues with scoped services in one PR, but that seems to be beyond the current PR's scope.

Tomorrow, I plan to conduct research to determine how to comprehensively address all issues with scoped services...

[raman-m]

I've identified numerous instances where scope validation is disabled in unit tests: specifically, a search for ".BuildServiceProvider(" reveals about 25 files with manual construction of service collections without validation. Unfortunately, we cannot address these in the current PR. 😢 But the final number of items ~50+ 🤯

[raman-m]

Regrettably, the search for "WebHostBuilder" revealed a significant issue with disabled scopes validation in tests and the core library 🤯 This wasn't an issue when services were primarily designed as singletons by Tom and contributors. However, as we began to introduce more scoped services, it became apparent that our testing projects were not equipped to handle them. The subsequent task will be to enable scope validation across all areas: unit tests, acceptance tests, and the core library. At this time, it is not feasible to tackle this extensive problem within this PR.

During refactoring, it is sensible to:

• Relocate the creation of the IWebHostBuilder objects in tests to the Ocelot.Testing project to enable the reuse of the helper across all testing projects. It is crucial to focus on C# statements containing new WebHostBuilder() expressions, necessitating the implementation of a wrapper.

[raman-m]

Development Complete❕

[raman-m]

@Niksson This PR has been merged.
Thank you for your contribution! 👍