#1518 Create building the `IServiceCollection` #1986

ArwynFr · 2024-03-03T10:44:30Z

Fixes #1518

Dependency Injection extensions build the service provider #1518

Proposed Changes

Replace the statement building the ServiceCollection by a new ServiceProvider by copying the ServiceDescriptor

Beware that this does not fix the problem of Steeltoe discovery client that might still build the service provider.
This is just a hotfix, there is still room for a larger refactor of removing the IConfiguration from the OcelotBuilder.

raman-m · 2024-03-04T17:07:07Z

Hi Adrien!
Thank you for created PR!

I've added PR to March'24 milestone. So, it will be released during first days of April. No objections?
Or should I prioritize it adding to the current February'24 release?
But I guess we have to have some time for debates, discussions and writing tests.

ArwynFr · 2024-03-04T21:49:06Z

Hey Raman,

As said on the issue, I'm not involved anymore in development of Ocelot-based software.
So I personally have no rush in the publication of this PR.

It is a mere hotfix regarding any aspnet 6+ compatibility, to prevent some apps to crash.
There is still a long way to go in adopting WebApplicationBuilder and options pattern.

raman-m · 2024-03-12T15:00:32Z

@ArwynFr
Could you explain me please?
What kind of app crashing do you fix by this tiny PR?

raman-m

Thanks for English text typo corrections in XML docs! 😄

raman-m · 2024-03-12T15:27:00Z

src/Ocelot/DependencyInjection/ServiceCollectionExtensions.cs

+        var descriptor = services.Where(descriptor => descriptor.ServiceType == typeof(IConfiguration)).FirstOrDefault();
+        var provider = new ServiceCollection().Add(descriptor).BuildServiceProvider();
+        var configuration = provider.GetRequiredService<IConfiguration>();


Why not to define a private helper to search for IConfiguration object?
Also, the XML description need to point out to the developer that the interface object is derived from DI and must be registered in DI container anyway!

This is my fault when I reviewed #1655 and didn't pay my attention to the fact we add overloaded version without the interface object 👇

Ocelot/src/Ocelot/DependencyInjection/ServiceCollectionExtensions.cs

Lines 48 to 52 in 5a3c55e

public static IOcelotBuilder AddOcelotUsingBuilder(this IServiceCollection services, Func<IMvcCoreBuilder, Assembly, IMvcCoreBuilder> customBuilder)

{

var configuration = services.BuildServiceProvider().GetRequiredService<IConfiguration>();

return new OcelotBuilder(services, configuration, customBuilder);

}

So, 😮 But Ocelot Builder has required 2nd argument:

Ocelot/src/Ocelot/DependencyInjection/OcelotBuilder.cs

Line 51 in 5a3c55e

public OcelotBuilder(IServiceCollection services, IConfiguration configurationRoot, Func<IMvcCoreBuilder, Assembly, IMvcCoreBuilder> customBuilder = null)

The fact that OcelotBuilder requires an IConfiguration is actually the problem in the first place.
This builder should only focus on registering service descriptors with their lifetime managers.
Services would access configuration by injection of IOptionsMonitor<T> rather than an IConfiguration reference.
Overloads with IConfiguration parameters would then be the ones to deprecate.

I wasn't sure deduplication of 3 lines of code was worth writing a separate helper.
Also in my mind this code is expected to go away once the IConfiguration references are gone.

@raman-m I've added some default behavior in case the IConfiguration wasn't registered.
Note that Tom's code reads the configuration from current working path, which may be different from the executable's path.

raman-m · 2024-03-12T15:39:08Z

src/Ocelot/DependencyInjection/ServiceCollectionExtensions.cs

@@ -17,15 +18,17 @@ public static class ServiceCollectionExtensions
    /// <returns>An <see cref="IOcelotBuilder"/> object.</returns>
    public static IOcelotBuilder AddOcelot(this IServiceCollection services)
    {
-        var configuration = services.BuildServiceProvider().GetRequiredService<IConfiguration>();
+        var descriptor = services.Where(descriptor => descriptor.ServiceType == typeof(IConfiguration)).FirstOrDefault();


Not an issue, but my questions:

How many IConfiguration objects can be registered in DI? If more than one instance, how to detect exactly ocelot.json one?

Could not the IConfiguration be registered at all? Seems not, because of appsettings.json...

IConfiguration is a singleton. When adding multiple configuration sources, these are mixed together as a single configuration context, where the latest source overrides former ones in case of configuration key duplication.

Technically you could provoke an error if you provide a new ServiceCollection(), but these methods are designed to be used with WebApplicationBuilder which does formally creates an IConfiguration instance.

Perhpas this could cause problems with slimmer versions of the builder introduced in dotnet 8.

By flattening multiple sources in a single IConfiguration context, the framework allows to easily implement "configuration overriding" features. For instance the default WebApplicationBuilder registers appsettings.json, then user secret file, and then environment variables as configuration sources. This allows secret files to override appsettings.json and environment variables to override both.

Also using Builder without an IConfiguration allows to register configuration sources AFTER the registration of the services. This in turns allow to do some fancy stuff like configuration bootstraping (read configuration from file, create services to access a remote storage using this config, then provide the rest of the app with config values from that storage).

Which is exacly the kind of problems Ocelot had tried to solve with Consul/Eureka.

If you want to isloate ocelot.json, you need to have a separate IConfiguration instance, using a separate ConfigurationBuilder for instance and not have it injected in the DI container. This is NOT what currently happens and NEITHER is what is described in the documentation.

Currently all configuration (ocelot routes and aspnet config) are mixed together in a single IConfiguration

ArwynFr · 2024-03-12T16:14:57Z

@ArwynFr Could you explain me please? What kind of app crashing do you fix by this tiny PR?

See discussion in #1518

Applications built using a dotnet 6+ with WebApplicationBuilder instead of Host/Startup, and using AddOcelot() override without the IConfiguration parameter will likely fail to start or crash. This is because the method builds the service provider which is expected not to happen with AddXXX methods. This in turns puts the provider in a weird state where resolving services could fail, throw an uncaught exception and provoke failure to start.

ggnaegi

Ok, we should keep in mind it's a quick fix, but yes, it will work. As of the issue with the Eureka Service Discovery, it's probably related to the Steeltoe extension methods: https://github.com/ggnaegi/Ocelot/blob/42ac3ca51829006bbe2e63bc684ab3a3caaccf1e/src/Ocelot.Provider.Eureka/OcelotBuilderExtensions.cs#L12
As we can read in their latest commit here:
SteeltoeOSS/Steeltoe@e934355
@ArwynFr Maybe it would make sense that you include the package update (3.6) in your PR, do you agree?

ArwynFr · 2024-03-12T18:51:51Z

@ggnaegi Steeltoe is facing the same problem as Ocelot, but they have chosen to enforce the IConfiguration, pushing the problem towards the developer instead of solving it. Since we are already passing the configuration instance, upgrading the library won't change much. But it won't help with removing the OcelotBuilder.Configuration neither.

ggnaegi · 2024-03-12T19:23:37Z

@ggnaegi Steeltoe is facing the same problem as Ocelot, but they have chosen to enforce the IConfiguration, pushing the problem towards the developer instead of solving it. Since we are already passing the configuration instance, upgrading the library won't change much. But it won't help with removing the OcelotBuilder.Configuration neither.

@ArwynFr Yes, but they were, at least until version 3.5 using the same

var configuration = services.BuildServiceProvider().GetRequiredService<IConfiguration>();

as ocelot did.
Since we know that, why would we keep something potentially hazardous in Ocelot? Yes, IConfiguration shouldn't be null, but we might have some edge cases. It was only some assumptions. We could create a new PR for the library update too.

ArwynFr · 2024-03-12T19:29:41Z

We could create a new PR for the library update too.

Since it's my first PR to Ocelot, I'd rather have the lib dependency update separated please.

src/Ocelot/DependencyInjection/ServiceCollectionExtensions.cs

ggnaegi · 2024-03-12T19:37:57Z

We could create a new PR for the library update too.

Since it's my first PR to Ocelot, I'd rather have the lib dependency update separated please.

Ok, fine

ggnaegi

Ok, thanks @ArwynFr !

raman-m

My suggestions below. And I worry about this

src/Ocelot/DependencyInjection/ServiceCollectionExtensions.cs

raman-m · 2024-03-19T15:07:02Z

I've tested locally unit tests coverage by commands

dotnet test --collect:"XPlat Code Coverage" which generated results with 046fc5a0-c6b6-48a5-9442-90302febc4e4 GUID
reportgenerator -reports:"d:\github\Ocelot\ArwynFr\Ocelot\test\Ocelot.UnitTests\TestResults\046fc5a0-c6b6-48a5-9442-90302febc4e4\coverage.cobertura.xml" -targetdir:"d:\github\Ocelot\ArwynFr\Ocelot\test\Ocelot.UnitTests\TestResults" -reporttypes:Html

And I see bad coverage... Also I have totally missed this thing during code review, Adrien 🙃

Report header

Do you like tests? 😉
Our dev process requires unit testing for new code.
⚠️ So, this PR merging will be blocked until added unit tests to cover new methods❕

ArwynFr · 2024-03-19T18:30:07Z

@raman-m will look into it this weekend

…uck! Hunting for `new FileInfo("")` bug... Done!

raman-m · 2024-03-20T19:01:35Z

Coverage 100% →

Ocelot.DependencyInjection.ServiceCollectionExtensions - Coverage Report.zip

Report header

Private methods: coverage is good

I like these pictures! And you, Adrien? 😉

raman-m

Ready for delivery! ✅

Code review ✔️
Team approvals ✔️
Unit testing ✔️
Acceptance tests ❔ It would be crazy to try to emulate a basic application crash scenario by trying to juggle startup configurations 🤣
Updated docs ❔ not required for system updates 🆗

@ArwynFr
Because this PR is like a hotfix: it contains pretty system updates then I believe we can go without docs updating.

My input

Defined new ServiceCollectionExtensionsTests class which contains exactly unit testing for private methods and public too
Added unit tests to OcelotBuilderTests with new functionality coverage
Switched off annoying reminder about Obsolete definitions aka CS0618 Warn , but for test projects only
The ultimate goal is 100% coverage of ServiceCollectionExtensions class achieved.
Fixed ugly a little duck duck bug in the GetMergedOcelotJson method of ConfigurationBuilderExtensions class. So, null value of the IWebHostEnvironment env argument led to empty string passing to the cstor in line of this code: new FileInfo(environmentFile). Well... It was my design gap. 😢 And especially these unit testing challenge helped me to uncover this Duck bug 🦆 🐛
That's why I love unit testing 😍

Yahoo! We can merge, @ArwynFr, right? 😋
We haven't made any breaking changes, right? 😳😁

raman-m · 2024-03-21T07:45:43Z

@ArwynFr Where are you?

raman-m · 2024-03-21T08:53:52Z

@ArwynFr I have no time to wait for a simple action to do. I don't want to delay release too.
Merging...

raman-m · 2024-03-21T16:15:22Z

@ArwynFr Thank you for the contribution!
This hotfix will be released next week...

ArwynFr marked this pull request as draft March 3, 2024 11:32

ArwynFr marked this pull request as ready for review March 3, 2024 16:50

fix: create building the servicecollection

36292a7

raman-m changed the title ~~fix: create building the servicecollection~~ #1518 Create building the IServiceCollection Mar 4, 2024

raman-m assigned ArwynFr Mar 4, 2024

raman-m added bug Identified as a potential bug proposal Proposal for a new functionality in Ocelot Spring'24 Spring 2024 release labels Mar 4, 2024

raman-m added this to the March'24 milestone Mar 4, 2024

ArwynFr mentioned this pull request Mar 7, 2024

Dependency Injection extensions build the service provider #1518

Closed

raman-m added the Configuration Ocelot feature: Configuration label Mar 12, 2024

raman-m requested review from RaynaldM, raman-m and ggnaegi March 12, 2024 15:14

raman-m requested changes Mar 12, 2024

View reviewed changes

ggnaegi approved these changes Mar 12, 2024

View reviewed changes

fix: handle default configuration

ed7f180

ggnaegi requested changes Mar 12, 2024

View reviewed changes

src/Ocelot/DependencyInjection/ServiceCollectionExtensions.cs Outdated Show resolved Hide resolved

src/Ocelot/DependencyInjection/ServiceCollectionExtensions.cs Outdated Show resolved Hide resolved

RaynaldM approved these changes Mar 18, 2024

View reviewed changes

chore: extension methods invocation

712ec51

ggnaegi approved these changes Mar 18, 2024

View reviewed changes

RaynaldM approved these changes Mar 18, 2024

View reviewed changes

Merge branch 'develop' into ArwynFr/issue1518-hotfix

b5e04f8

raman-m added Feb'24 February 2024 release and removed Spring'24 Spring 2024 release labels Mar 18, 2024

raman-m modified the milestones: March'24, February'24 Mar 18, 2024

raman-m requested changes Mar 18, 2024

View reviewed changes

raman-m added 2 commits March 19, 2024 12:43

Update src/Ocelot/DependencyInjection/ServiceCollectionExtensions.cs

715a364

Code review by @raman-m

293c1d9

No warnings for CS0618

7ca818b

raman-m added 2 commits March 20, 2024 21:30

Given developer forgot to add environment, then?... Ups! Your Duck, d…

990bf9b

…uck! Hunting for `new FileInfo("")` bug... Done!

Coverage 100%

cef1396

raman-m approved these changes Mar 20, 2024

View reviewed changes

raman-m added the hotfix Gitflow: Hotfix issue, PR related to hotfix branch label Mar 20, 2024

raman-m merged commit ded4d7e into ThreeMammals:develop Mar 21, 2024
2 checks passed

raman-m mentioned this pull request Mar 29, 2024

Release 23.2 #2023

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1518 Create building the `IServiceCollection` #1986

#1518 Create building the `IServiceCollection` #1986

ArwynFr commented Mar 3, 2024 •

edited by raman-m

Loading

raman-m commented Mar 4, 2024

ArwynFr commented Mar 4, 2024

raman-m commented Mar 12, 2024

raman-m left a comment •

edited

Loading

raman-m Mar 12, 2024

raman-m Mar 12, 2024

ArwynFr Mar 12, 2024

ArwynFr Mar 12, 2024

ArwynFr Mar 12, 2024

raman-m Mar 12, 2024

ArwynFr Mar 12, 2024

ArwynFr Mar 12, 2024

ArwynFr Mar 12, 2024

ArwynFr Mar 12, 2024

ArwynFr commented Mar 12, 2024

ggnaegi left a comment •

edited

Loading

ArwynFr commented Mar 12, 2024

ggnaegi commented Mar 12, 2024

ArwynFr commented Mar 12, 2024

ggnaegi commented Mar 12, 2024

ggnaegi left a comment

raman-m left a comment •

edited

Loading

raman-m commented Mar 19, 2024 •

edited

Loading

ArwynFr commented Mar 19, 2024

raman-m commented Mar 20, 2024

raman-m left a comment •

edited

Loading

raman-m commented Mar 21, 2024

raman-m commented Mar 21, 2024

raman-m commented Mar 21, 2024

	public static IOcelotBuilder AddOcelotUsingBuilder(this IServiceCollection services, Func<IMvcCoreBuilder, Assembly, IMvcCoreBuilder> customBuilder)
	{
	var configuration = services.BuildServiceProvider().GetRequiredService<IConfiguration>();
	return new OcelotBuilder(services, configuration, customBuilder);
	}

#1518 Create building the IServiceCollection #1986

#1518 Create building the IServiceCollection #1986

Conversation

ArwynFr commented Mar 3, 2024 • edited by raman-m Loading

Fixes #1518

Proposed Changes

raman-m commented Mar 4, 2024

ArwynFr commented Mar 4, 2024

raman-m commented Mar 12, 2024

raman-m left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArwynFr commented Mar 12, 2024

ggnaegi left a comment • edited Loading

Choose a reason for hiding this comment

ArwynFr commented Mar 12, 2024

ggnaegi commented Mar 12, 2024

ArwynFr commented Mar 12, 2024

ggnaegi commented Mar 12, 2024

ggnaegi left a comment

Choose a reason for hiding this comment

raman-m left a comment • edited Loading

Choose a reason for hiding this comment

raman-m commented Mar 19, 2024 • edited Loading

ArwynFr commented Mar 19, 2024

raman-m commented Mar 20, 2024

Coverage 100% →

Report header

Private methods: coverage is good

raman-m left a comment • edited Loading

Choose a reason for hiding this comment

Ready for delivery! ✅

My input

raman-m commented Mar 21, 2024

raman-m commented Mar 21, 2024

raman-m commented Mar 21, 2024

#1518 Create building the `IServiceCollection` #1986

#1518 Create building the `IServiceCollection` #1986

ArwynFr commented Mar 3, 2024 •

edited by raman-m

Loading

raman-m left a comment •

edited

Loading

ggnaegi left a comment •

edited

Loading

raman-m left a comment •

edited

Loading

raman-m commented Mar 19, 2024 •

edited

Loading

raman-m left a comment •

edited

Loading