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

Make serviceName in HttpClient builder extensions obsolete, add new overloads #1725

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MattKotsenas
Copy link
Contributor

@MattKotsenas MattKotsenas commented May 2, 2022

Currently, the HttpClient builder extensions (i.e.
AddMicrosoftIdentityUserAuthenticationHandler and
AddMicrosoftIdentityAppAuthenticationHandler) take the service name as
the first parameter.

As far as I can tell, this value is only used to
name and subsequently retrieve the named options, all of which is
internal to the setup (or in other words, the user never needs to refer
to the client by this name).

Furthermore, these calls are always part of an .AddHttpClient() call,
which is also named, either explicitly, or implicitly with the type
name.

As a result, confusing or even incorrect use of the extensions is
possible when working with multiple clients. Consider a setup like this:

services
    .AddHttpClient("NamedClient1")
    .AddMicrosoftIdentityAppAuthenticationHandler("Service1",
	Configuration.GetRequiredSection("Service1"));

services
    .AddHttpClient("NamedClient2")
    .AddMicrosoftIdentityAppAuthenticationHandler("Service1",
	Configuration.GetRequiredSection("Service2"));

In this example, the HttpClient objects are named differently, and they
pull from different config sections, however due to a copy / paste
error, they both use the service name "Service1". In this case, despite
being two different builder chains and two different clients, the
configuration of Service1 will be overwritten by the values of Service2
because of the name collision.

In short, the service name appears to be unnecessary and a potential
source of confusion and bugs, as it isn't clear from the user's
perspective what it's used for.

In order to push users towards safer behavior while not making a breaking
change, we add new overloads that are missing the serviceName parameter,
and mark the existing methods as Obsolete.

@MattKotsenas
Copy link
Contributor Author

As mentioned above, opening this to at least start a discussion around if it makes sense to remove the "serivceName" parameter, given that it seems redundant with the client name, and has confused some people on my team.

If there's agreement on removing it, I defer to the team about how best to communicate that (i.e. major version bumps, additional overloads, obsolete annotations, etc.) but didn't want to get ahead of myself before there's conensus.

Also tagging @qetza , who wrote the initial implementation, in case there are uses I'm missing.

Thanks!

@MattKotsenas
Copy link
Contributor Author

Hey there! Friendly ping to see if this is a change worth considering. Also pinging @jennyf19 for visibility.

If you have any questions, please let me know. Thanks!

@jennyf19
Copy link
Collaborator

Hi @MattKotsenas This would be a breaking change for us, so not sure if we want to do that atm. @jmprieur for perspective.

@MattKotsenas
Copy link
Contributor Author

MattKotsenas commented May 16, 2022

Oh yes, as-is definitely would be a breaking change. I think the first question is "is this something you would ever accept?", since I don't see a purpose to name the service config, but that doesn't mean there is no purpose :-P

If so, then we can focus on either how to land a breaking change, or if there's something we'd want to do to make it not blocking (i.e. new overrides or marking deprecated). Let me know if that makes sense, or if you have questions!

Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

Proposing a simpler change for the same effect

@MattKotsenas MattKotsenas force-pushed the feature/remove-service-name-from-http-client branch from a7045e6 to 2f031c3 Compare May 18, 2022 17:43
@MattKotsenas MattKotsenas changed the title Remove serviceName from HttpClient builder extensions Make serviceName in HttpClient builder extensions obsolete, add new overloads May 18, 2022
@MattKotsenas
Copy link
Contributor Author

Thanks @jmprieur! I believe I updated this change as you suggested; would you mind taking another look and letting me know if there are any questions or concerns? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants