search

ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.28k stars 1.63k forks source link

#1518 Create building the `IServiceCollection` #1986

Closed ArwynFr closed 5 months ago

ArwynFr commented 5 months ago

Fixes #1518

Proposed Changes

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

ArwynFr commented 5 months ago

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

ArwynFr commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

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.

ggnaegi commented 5 months ago

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

raman-m commented 5 months ago

I've tested locally unit tests coverage by commands

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

Report header image

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 commented 5 months ago

@raman-m will look into it this weekend

raman-m commented 5 months ago

Coverage 100% →

Report header

image

Private methods: coverage is good

image

I like these pictures! And you, Adrien? 😉

raman-m commented 5 months ago

@ArwynFr Where are you?

raman-m commented 5 months ago

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

raman-m commented 5 months ago

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