Aspnetcore: Resolve public API that exposes on 'pubternal' API

[natemcmaster]

I tried enabling the 'pubternal' analyzer on this entire project and found 12 cases of public API which depends on pubternal. What should we do about these?

Options:

• Remove the API (breaking change)
• Suppress the analyzer error
• Rename API to be public (breaking change)

area-mvc: @mkArtakMSFT @JamesNK

src/Features/JsonPatch/src/Adapters/AdapterFactory.cs(16,24): error PUB0001: Pubternal type ('Microsoft.AspNetCore.JsonPatch.Internal.IAdapter') usage in public API [src/Features/JsonPatch/src/Microsoft.AspNetCore.JsonPatch.csproj]
src/Features/JsonPatch/src/Adapters/IAdapterFactory.cs(20,9): error PUB0001: Pubternal type ('Microsoft.AspNetCore.JsonPatch.Internal.IAdapter') usage in public API [src/Features/JsonPatch/src/Microsoft.AspNetCore.JsonPatch.csproj]
src/Http/Routing/src/Template/TemplateBinder.cs(45,13): error PUB0001: Pubternal type ('Microsoft.AspNetCore.Routing.Internal.UriBuildingContext') usage in public API [src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj]
src/Http/Routing/src/Template/TemplateBinder.cs(65,13): error PUB0001: Pubternal type ('Microsoft.AspNetCore.Routing.Internal.UriBuildingContext') usage in public API [src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj]
src/Http/Routing/src/Tree/TreeRouteBuilder.cs(43,13): error PUB0001: Pubternal type ('Microsoft.AspNetCore.Routing.Internal.UriBuildingContext') usage in public API [src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj]
src/Http/Routing/src/Tree/TreeRouteBuilder.cs(63,13): error PUB0001: Pubternal type ('Microsoft.AspNetCore.Routing.Internal.UriBuildingContext') usage in public API [src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj]
src/Http/Routing/src/Tree/TreeRouter.cs(47,13): error PUB0001: Pubternal type ('Microsoft.AspNetCore.Routing.Internal.UriBuildingContext') usage in public API [src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj]
src/Middleware/ResponseCaching/src/ResponseCachingMiddleware.cs(33,13): error PUB0001: Pubternal type ('Microsoft.AspNetCore.ResponseCaching.Internal.IResponseCachingPolicyProvider') usage in public API [src/Middleware/ResponseCaching/src/Microsoft.AspNetCore.ResponseCaching.csproj]
src/Middleware/ResponseCaching/src/ResponseCachingMiddleware.cs(34,13): error PUB0001: Pubternal type ('Microsoft.AspNetCore.ResponseCaching.Internal.IResponseCachingKeyProvider') usage in public API [src/Middleware/ResponseCaching/src/Microsoft.AspNetCore.ResponseCaching.csproj]

area-servers: @muratg @Tratcher

src/Hosting/Hosting/src/Startup/ConventionBasedStartup.cs(17,39): error PUB0001: Pubternal type ('Microsoft.AspNetCore.Hosting.Internal.StartupMethods') usage in public API [src/Hosting/Hosting/src/Microsoft.AspNetCore.Hosting.csproj]

area-dataprotection: @muratg @blowdart

src/DataProtection/DataProtection/src/KeyManagement/XmlKeyManager.cs(65,83): error PUB0001: Pubternal type ('Microsoft.AspNetCore.DataProtection.Internal.IActivator') usage in public API [src/DataProtection/DataProtection/src/Microsoft.AspNetCore.DataProtection.csproj]
src/DataProtection/DataProtection/src/KeyManagement/XmlKeyManager.cs(75,83): error PUB0001: Pubternal type ('Microsoft.AspNetCore.DataProtection.Internal.IActivator') usage in public API [src/DataProtection/DataProtection/src/Microsoft.AspNetCore.DataProtection.csproj]

cc @pakrym @davidfowl

[muratg]

I think for each of these, we'll need to assess if making a breaking change is acceptable.

(Also, a third option, we can consider making pubternal type public.)

[Tratcher]

ConventionBasedStartup should have been pubternal as well. It's not a type anyone is likely to use directly, it could be changed with minimal risk.

[pakrym]

For XmlKeyManager.cs I think we should just suppress the error. We can neither remove IActivator argument or make it public.

[blowdart]

Yup, leave data protection as is.

[muratg]

OK, @Tratcher @pakrym could you prep the necessary PRs? Thanks!

[pakrym]

@natemcmaster do you have a PR for this? We can push out fixes directly to it.

[natemcmaster]

I didn't start one yet. I wanted to wait for feedback from @JamesNK @mkArtakMSFT on the Routing APIs and what should be done there. I'd like to just make Internal.Analyzers.Sdk apply to all non-test projects, but didn't want to blindly suppress all the errors.

[JamesNK]

src/Http/Routing/src/Template/TemplateBinder.cs(45,13): error PUB0001: Pubternal type ('Microsoft.AspNetCore.Routing.Internal.UriBuildingContext') usage in public API [src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj]
src/Http/Routing/src/Template/TemplateBinder.cs(65,13): error PUB0001: Pubternal type ('Microsoft.AspNetCore.Routing.Internal.UriBuildingContext') usage in public API [src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj]
src/Http/Routing/src/Tree/TreeRouteBuilder.cs(43,13): error PUB0001: Pubternal type ('Microsoft.AspNetCore.Routing.Internal.UriBuildingContext') usage in public API [src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj]
src/Http/Routing/src/Tree/TreeRouteBuilder.cs(63,13): error PUB0001: Pubternal type ('Microsoft.AspNetCore.Routing.Internal.UriBuildingContext') usage in public API [src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj]
src/Http/Routing/src/Tree/TreeRouter.cs(47,13): error PUB0001: Pubternal type ('Microsoft.AspNetCore.Routing.Internal.UriBuildingContext') usage in public API [src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj]

Those are older routing APIs that I'm not familiar with. TreeRouter and TreeRouteBuilder are unlikely to be called by users. TemplateBinder might be called by users. Thoughts @rynowak?

[Tratcher]

Resolved ConventionBasedStartup by making the public type pubternal.

[pakrym]

@mkArtakMSFT can you please assign someone for JsonPatch and Routing

@JunTaoLuo - ResponseCaching

[anurse]

Ping on this. It looks like the servers area work is done here. @mkArtakMSFT is someone going to look at this from the MVC side?

[rynowak]

For Routing, I can take care of this.

For JsonPatch - I think we should leave those APIs in place. We know that people implement those interfaces, and I don't really want to break people if it's cosmetic (moving namespaces). Is there a supression so that this doesn't get flagged again?

[anurse]

Is there a supression so that this doesn't get flagged again?

It's a C# analyzer, but I don't think there's a built-in way to suppress those by line. I believe you could either <NoWarn> them in the project or we can drop a ruleset file.

They're defined here https://github.com/aspnet/Extensions/blob/ff039b9cd82b5a092adf01c5d8dacce342f67852/src/TestingUtils/Internal.AspNetCore.Analyzers/src/PubturnalityDescriptors.cs#L10 . I think PUB0001 is the one you want to suppress.

NOTE: I don't believe these analyzers are enabled, @natemcmaster was just trying it out. Though it would be nice to turn them on ;).

[natemcmaster]

Correct, I want to enable these analyzers to avoid making more mistakes, but wanted to make sure we reviewed existing mistakes first. It sounds like we have reviewed enough to try again enabling the pubternal analyzer.

[rynowak]

For the routing cases, I'm looking at this and I think the best we can do is make these constructors obsolete in this release and provide another way to access these types. TemplateBinder is constructed in user code, and I think the best fix is to add a DI factory for it.

[rynowak]

PR for routing https://github.com/aspnet/AspNetCore/pull/9305

[JunTaoLuo]

The ResponseCaching APIs should never have been made public, as they were more experimental and we didn't want to officially support them. Since marking them as internal (i.e. the remove approach) is breaking, maybe we should just mark these as obsolete and remove in the next release.

[Tratcher]

@JunTaoLuo we haven't hesitated to move other pubternal types to internal this release. Do you have any reason to believe many people are using the pubternal types in response caching?

[JunTaoLuo]

Oh perfect, I'll do that then. I thought that would be considered "too breaking".

[JunTaoLuo]

FYI The outstanding pubternal APIs:

src/Features/JsonPatch/src/Adapters/AdapterFactory.cs(16,24): error PUB0001: Pubternal type ('Microsoft.AspNetCore.JsonPatch.Internal.IAdapter') usage in public API [src/Features/JsonPatch/src/Microsoft.AspNetCore.JsonPatch.csproj]
src/Features/JsonPatch/src/Adapters/IAdapterFactory.cs(20,9): error PUB0001: Pubternal type ('Microsoft.AspNetCore.JsonPatch.Internal.IAdapter') usage in public API [src/Features/JsonPatch/src/Microsoft.AspNetCore.JsonPatch.csproj]

src/DataProtection/DataProtection/src/KeyManagement/XmlKeyManager.cs(65,83): error PUB0001: Pubternal type ('Microsoft.AspNetCore.DataProtection.Internal.IActivator') usage in public API [src/DataProtection/DataProtection/src/Microsoft.AspNetCore.DataProtection.csproj]
src/DataProtection/DataProtection/src/KeyManagement/XmlKeyManager.cs(75,83): error PUB0001: Pubternal type ('Microsoft.AspNetCore.DataProtection.Internal.IActivator') usage in public API [src/DataProtection/DataProtection/src/Microsoft.AspNetCore.DataProtection.csproj]

[Tratcher]

ConventionBasedStartup was resolved by making the public type pubternal.
https://github.com/aspnet/AspNetCore/issues/6975#issuecomment-468479541

[mkArtak]

Thanks @JunTaoLuo.
@pranavkm please do the needful for the JSONPatch APIs.

[davidfowl]

Has this been done for extensions as well or are we only looking at ASP.NET Core?

[natemcmaster]

We're inconsistent right now. Some areas have moved away from using pubternal. Extensions has largely been untouched, with a few exceptions like logging.

[rynowak]

The JSON Patch stuff should be left as-is. Everyone who's using JSON patch is using those APIs. There's no point in breaking everyone who uses something.

[mkArtakMSFT]

@rynowak, why wouldn't we turn these APIs public then?

[rynowak]

They are public in the .NET sense, along with everything else discussed here.

They are in the Microsoft.AspNetCore.JsonPatch.Internal namespace. I don't think it's sensible to change that because of how common it is for users of JsonPatch to use these types.

[anurse]

Unfortunately the transition from "pubternal" to "public" is also breaking because it's a namespace change.

[natemcmaster]

Let's add the pubternal analyzer to all assemblies. If there are any remaining PUB001 errors, let's open granular issues for each and assign to area owners.

[mkArtakMSFT]

Unassigning myself, as there is no more action pending from MVC side.