Cleanup ReCaptcha services

[MikeAlhayek]

Based on a discussion we had today @sebastienros comment

@jtkech this may reduce some memory allocation reported in #14117

[sebastienros]

This is assuming that IHttpClientFactory is registered at the tenant level and not the host level so that each tenant for the same resolved name will get a different instance, because it has a custom configuration (Api Url).

But if we didn't set the base uri and have it instead in the POST call then we can even share the same client for all services in the tenant without base Uri.

[MikeAlhayek]

I think here the options are evaluated every time the new client is created. So it would use the current tenant's options. We may have to remove the ReCaptchSettings from the singleton service too

[MikeAlhayek]

@sebastienros I did some cleanup. I think ReCaptchaService is okay to make it scoped. Since there is no abstractions here, I removed ReCaptchaClient. I think it's over engineering and adds unnecessary complexity. Now, we register ReCaptchaService as scoped service which should be good in this case. Also, we now use IHttpClientFactory. I also replace Newtonsoft with System.Text.Json as an added bonus.

[sebastienros]

IHttpClientFactory, was it removed in the meantime?

[MikeAlhayek]

We still use it. We register HttpClient for the type, so injecting HttpClient in the constructor will return the same instance for that type

[jtkech]

@MikeAlhayek

Yes I know see #14117 (comment) and then #14117 (comment).

I already made something locally, here what I did #14335.

I will look at what you did.

[MikeAlhayek]

@jtkech thank you! I think my approach simplify the process even more by eliminating the need for ReCaptchaClient. let me know what you think

[jtkech]

Yes I first thought about it but then I tried to make as few changes as possible.

Good to read https://learn.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests

In #14335 I kept the typed client pattern.

For the sake of brevity, this guidance shows the most structured way to use IHttpClientFactory, which is to use Typed Clients (Service Agent pattern).

But I have no personal preference, both may do the same.

[jtkech]

If the typed client is now tied to ReCaptchaService, because it is also injected by ReCaptchaLoginFilter on each request, an HttpClient will be still created, by using the IHttpClientFactory but still on each request.

One way would be to make ReCaptchaService a singleton but not good to hold a fully built HttpClient a too long time.

I will add to #14348 the simple fix I did in #14335 which resolves the client lazily, because they are both related to #14117, I may change the lifetime of ReCaptchaService, at least a scoped service, maybe a singleton if it no longer injects the client, will see.

But not incompatible with what you did here because other changes and cleanups look good.

[jtkech]

As said here https://github.com/OrchardCMS/OrchardCore/pull/14333/files#r1341924709

If the typed client is now tied to ReCaptchaService, because it is also injected by ReCaptchaLoginFilter on each request, an HttpClient will be still created, by using the IHttpClientFactory but still on each request.

So here the HttpClient is still created on each request.

One solution was to let the HttpClient tied to the RecaptchaClient service but only resolve it on demand, as already done in #14348 that was merged.

Or no longer add a typed client (tied to a service) and in ReCaptchaService inject and then use explicitly the IHttpClientFactory on demand.

[MikeAlhayek]

@jtkech so do you rather inject IHttpClientFactory (as I originally had in the ReCaptchaService), or change ReCaptchaService to a singleton instead?

[jtkech]

Not a singleton as holding an HttpClient a too long time is not good.

Yes, one solution is to explicitly use the factory on demand.

What I did in #14348 was to keep the typed client pattern and just resolve the tied service on demand.

[jtkech]

I can make a PR for whatever solution we prefer

[MikeAlhayek]

@jtkech sorry saw your last comment a bit late. I did a follow up PR to address your recommendation.
Note that we have to user either named or types HttpClient since we are using a policy/behavior for that client.

[jtkech]

No problem, will look at it