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

ConfigureHttpClientDefaults isn't an extension method in the reference assembly #90911

Closed
davidfowl opened this issue Aug 22, 2023 · 15 comments · Fixed by #90916
Closed

ConfigureHttpClientDefaults isn't an extension method in the reference assembly #90911

davidfowl opened this issue Aug 22, 2023 · 15 comments · Fixed by #90916

Comments

@davidfowl
Copy link
Member

davidfowl commented Aug 22, 2023

This PR added ConfigureHttpClientDefaults but it turns out there's a ref/def mismatch that's causing this API to only show up as a static method instead of an extension method:

Ref:
https://github.com/dotnet/runtime/blob/3bda6e0013ddb5b48a7b2a89fd84bf4fbbed0e37/src/libraries/Microsoft.Extensions.Http/ref/Microsoft.Extensions.Http.cs#L60C83-L60C105

Def:

public static IServiceCollection ConfigureHttpClientDefaults(this IServiceCollection services, Action<IHttpClientBuilder> configure)

cc @karelz @JamesNK @CarnaViire @stephentoub

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 22, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 22, 2023
@ghost
Copy link

ghost commented Aug 22, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR added ConfigureHttpClientDefaults but it turns out there's a ref/def mismatch that's causing this API to only show up as a static method instead of an extension method:

Ref:
https://github.com/dotnet/runtime/blob/3bda6e0013ddb5b48a7b2a89fd84bf4fbbed0e37/src/libraries/Microsoft.Extensions.Http/ref/Microsoft.Extensions.Http.cs#L60C83-L60C105

Def:

public static IServiceCollection ConfigureHttpClientDefaults(this IServiceCollection services, Action<IHttpClientBuilder> configure)

cc @karelz @JamesNK @CarnaViire @stephentoub

Author: davidfowl
Assignees: -
Labels:

untriaged, area-Extensions-HttpClientFactory, needs-area-label

Milestone: -

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 22, 2023
@ShreyasJejurkar
Copy link
Contributor

I wonder why PublicApiAnalyzer didn't fail this or warn about it!? 🤔

@MihaZupan MihaZupan removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 22, 2023
@MihaZupan MihaZupan added this to the 8.0.0 milestone Aug 22, 2023
@MihaZupan MihaZupan removed the untriaged New issue has not been triaged by the area owner label Aug 22, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 22, 2023
@CarnaViire
Copy link
Member

Reopening for backport

@CarnaViire CarnaViire reopened this Aug 22, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 22, 2023
@CarnaViire CarnaViire self-assigned this Aug 22, 2023
@ShreyasJejurkar
Copy link
Contributor

I wonder why PublicApiAnalyzer didn't fail this or warn about it!? 🤔

This looks like a valid bug in analyzer, we should create issue at given repo. I don't know which repo manages this analyzer. I am out as of now, can someone create a issue for this!?

Thanks,

@CarnaViire
Copy link
Member

I wonder why PublicApiAnalyzer didn't fail this or warn about it!? 🤔

cc @ericstj @carlossanlop @ViktorHofer it looks like there might be a bug in API Compat

@ViktorHofer
Copy link
Member

I wonder why PublicApiAnalyzer didn't fail this or warn about it!? 🤔

The public API comparison of a contract and implementation assembly is handled by APICompat, not by PublicApiAnalyzer.

I would describe this case as a case that isn't yet covered by APICompat's validation. We have many other cases for which we have issues that aren't yet covered by the tool. See https://github.com/dotnet/sdk/issues?q=is%3Aopen+is%3Aissue+label%3AArea-ApiCompat

@CarnaViire
Copy link
Member

CarnaViire commented Aug 22, 2023

Filed an issue dotnet/sdk#34824

@ShreyasJejurkar
Copy link
Contributor

Filed an issue dotnet/sdk#34824

Great! Thanks!

I guess we can close this issue now! 😇

@CarnaViire
Copy link
Member

It's open for the backport to release/8.0 branch

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 22, 2023
@karelz
Copy link
Member

karelz commented Aug 22, 2023

Fixed in 8.0 (RC2) in PR #90920 and in 9.0 (main) in PR #90916

@karelz
Copy link
Member

karelz commented Aug 22, 2023

13 hours from report to fix in both main and 8.0. That is FAST! Kudos @ShreyasJejurkar and @CarnaViire!

@CarnaViire
Copy link
Member

Kudos to @ShreyasJejurkar as well 🥳

@karelz
Copy link
Member

karelz commented Aug 22, 2023

Right, @ShreyasJejurkar did the original PR, I entirely forgot. Sorry -- kudos to @ShreyasJejurkar as well! (fixed above)

@ShreyasJejurkar
Copy link
Contributor

13 hours from report to fix in both main and 8.0. That is FAST! Kudos @ShreyasJejurkar and @CarnaViire!

haha, Thanks @karelz and @CarnaViire. I don't think there is anything to appreciate, to be honest, this was a simple fix! 😅
Thanks to @davidfowl who tried the new API actually and found this bug, otherwise I don't know when we would have caught this!

But the major issue is on the API Compact side where this got missed from flagging, I hope there are no more like these cases in the build.

I don't know (am yet to explore) what the code volume needs to go in APICompact to solve this issue, but if anyone here has an idea if it's trivial, it would be great to get that fix so that we can catch this kind of things before release!

@ViktorHofer Thanks for the pointers to APICompact, I wasn't aware as I used to get errors from PublicApiAnalyzer. 😄

But btw this Public API Shipped/UnShipped part of SDK? I mean customers can also use that, is it? I thought this is specifically designed for dotnet repos! 🤔

@ViktorHofer
Copy link
Member

ViktorHofer commented Aug 23, 2023

But btw this Public API Shipped/UnShipped part of SDK? I mean customers can also use that, is it? I thought this is specifically designed for dotnet repos! 🤔

The Public API Analyzers uses this concept of shipped/unshipped files to track API changes. That analyzer is publicly available to use: https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md

APICompat / PackageValidation is the tool that we use here in runtime, which is also publicly available and even part of the .NET SDK and can be enabled by setting a property. See the documentation here: https://learn.microsoft.com/en-us/dotnet/fundamentals/package-validation/overview

@ghost ghost locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants