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

Add target framework netstandard 2.0 so Auth0Net.DependencyInjection can be used in .NET Framework and .NET 5.0 #24

Merged
merged 5 commits into from
Mar 8, 2024
Merged

Add target framework netstandard 2.0 so Auth0Net.DependencyInjection can be used in .NET Framework and .NET 5.0 #24

merged 5 commits into from
Mar 8, 2024

Conversation

stevenvolckaert
Copy link
Contributor

I've successfully used Auth0Net.DependencyInjection in a .NET 8.0 Blazor Web Application project to integrate with Auth0's Management API.

I now need to do the same in three ASP.NET web applications running on .NET Framework 4.8. These can unfortunately not be migrated to .NET Core: their codebase is too large.

This pull request adds netstandard2.0 as a target framework, and I've added net48 to the test project's target frameworks to ensure test coverage for .NET Framework 4.8.

Since System.Net.Http.SocketsHttpHandler is not supported in netstandard2.0, I've excluded it with a preprocessor directive, so the default implementation is used instead.

        return services.AddHttpClient<IManagementConnection, HttpClientManagementConnection>()
#if !NETSTANDARD2_0
            .ConfigurePrimaryHttpMessageHandler(() =>
                new SocketsHttpHandler()
                {
                    PooledConnectionLifetime = TimeSpan.FromMinutes(2)
                })
#endif

@Hawxy could you please review and comment here if necessary?

Note: I updated the Auth0Net.DependencyInjection version to 3.1.1.

@Hawxy
Copy link
Owner

Hawxy commented Mar 7, 2024

Whilst I appreciate the contribution, I'm not exactly thrilled that it's in the form of some tech debt.

I now need to do the same in three ASP.NET web applications running on .NET Framework 4.8.

Could you proxy the requests via a central API or is direct access a firm requirement? Are those projects using a modern enough IoC container to work with the extensions in this project (implements IServiceCollection)?

Since System.Net.Http.SocketsHttpHandler is not supported in netstandard2.0

Just a note that this will result in the client breaking if a DNS record gets changed, although that probably doesn't matter.

@@ -0,0 +1,27 @@
// Original source code copied from
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer the use of Simon Cropp's Polyfill package than having this in the codebase.

@stevenvolckaert
Copy link
Contributor Author

Hi Hawxy, thanks for your review.

The requests to Auth0 could be proxied over a new REST service, but this adds considerable overhead for us: A new service to build and maintain, new DNS entries for 4 environments, and a new pipeline to build and deploy the service to the environments. We would like to avoid this overhead, if possible.

Since the Auth0 official packages do target netstandard2.0, and supporting this target framework was quite straightforward, I assumed a PR here adding netstandard2.0 support would be the best solution.

I wasn't aware of the Polyfill package that you mentioned; I'll look into this shortly.

Could you give some additional info regarding the DNS problem that System.Net.Http.SocketsHttpHandler solves? I'd be very interested what is all about.

@Hawxy
Copy link
Owner

Hawxy commented Mar 7, 2024

The requests to Auth0 could be proxied over a new REST service, but this adds considerable overhead for us

Gotcha.

Could you give some additional info regarding the DNS problem

https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient-guidelines

In this case I use a hybrid solution that provides the benefit of IHttpClientFactory (logging + extensions + DNS fix) but without the downside of clients needing to be registered as transient, thus the override of the handler + infinite timeout. The DNS issue itself is an edge case that you're unlikely to run into but just something to be aware of.

@stevenvolckaert
Copy link
Contributor Author

I've replaced IsExternalInit.cs with package reference Polyfill.

I see now I forgot to answer your question:

Are those projects using a modern enough IoC container to work with the extensions in this project (implements IServiceCollection)?

Yes, all three ASP.NET projects use Microsoft.Extensions.DependencyInjection.ServiceProvider, and its services are added through the IServiceCollection interface when each application starts.

Thanks for pointing out the DNS behavior problem, I understand now what it is about.

The Recommended use heading states:

To summarize recommended HttpClient use in terms of lifetime management, you should use either long-lived clients and set PooledConnectionLifetime (.NET Core and .NET 5+) or short-lived clients created by IHttpClientFactory.

Because we use short-lived HttpClient instances injected by Microsoft.Extensions.DependencyInjection, or by creating named clients by injecting IHttpClientFactory, I assume the DNS behavior problem won't affect us.

If you think it's useful, I'd be happy to write some documentation pointing to this recommended use, as part of this PR.

@Hawxy
Copy link
Owner

Hawxy commented Mar 7, 2024

I've replaced IsExternalInit.cs with package reference Polyfill

Thanks. The package needs to be added to the library itself for the netstandard target and not the test project as netfx consumers of the library will require the polyfill.

I assume the DNS behavior problem won't affect us.

My comments have nothing to do with your own application code, this is specific to how I designed this library. I use long-lived injected httpclients due to the auth0 clients being singletons for perf reasons. I'm configuring SocketsHttpHandler to get around this issue, which you've put behind a compilation flag. You'll get a single, long lived handler for the lifetime of the application whilst using the auth0 clients under netfx, thus the dns issue may appear, as unlikely as it is.

some documentation pointing to this recommended use

I don't feel this is necessary.

I'll merge this in tomorrow and get a new release out.

@stevenvolckaert
Copy link
Contributor Author

Thanks for your review and comments. I've moved the Polyfill package reference to the Auth0Net.DependencyInjection project, as you requested.

@Hawxy Hawxy merged commit 721bce5 into Hawxy:main Mar 8, 2024
2 checks passed
@stevenvolckaert stevenvolckaert deleted the feature/add-netstandard2.0-target-framework branch March 11, 2024 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants