[Discussion] Making "pubinternal" types in MVC internal

[pranavkm]

In ASP.NET Core, pubinternal types are types that are declared as public but put in an .Internal namespace. While these types are public they have no support policy and are subject to breaking changes. Unfortunately accidental use of these types has been common, resulting in breaking changes to these projects and limiting our ability to maintain the framework.

In ASP.NET Core 3.0, we are updating all pubinternal types in MVC to either be public in a supported namespace, or internal as appropriate. This includes types in the following namespaces:

• Microsoft.AspNetCore.Mvc.Formatters.Xml.Internal
• Microsoft.AspNetCore.Mvc.Cors.Internal
• Microsoft.AspNetCore.Mvc.ViewFeatures.Internal
• Microsoft.AspNetCore.Mvc.Formatters.Json.Internal
• Microsoft.AspNetCore.Mvc.RazorPages.Internal
• Microsoft.AspNetCore.Mvc.DataAnnotations.Internal
• Microsoft.AspNetCore.Mvc.TagHelpers.Internal
• Microsoft.AspNetCore.Mvc.Internal
• Microsoft.AspNetCore.Mvc.Razor.Internal
• Microsoft.AspNetCore.Mvc.Formatters.Internal
• Microsoft.AspNetCore.Mvc.Core.Internal
• Microsoft.AspNetCore.Mvc.ModelBinding.Internal

If there are specific types in these namespaces that are critical to your applications, please file an issue in https://github.com/aspnet/Mvc and we will consider making the requested types public.

[Sebazzz]

Unfortunately accidental use of these types has been common.

Is that an autocompletion problem? What about hiding through [EditorBrowsable(EditorBrowsableState.Never)] in release builds?

[pranavkm]

cc @khellang and @poke since you were involved in the discussion in the PR I sent out earlier. Here are some of the more specific reasons why we're considering doing this:

• Many aspects of MVC’s design ask extenders to inherit or new up a type. Sometimes these types would inject a pubternal type and then were subject to being broken. E.g.:

  • https://github.com/aspnet/Mvc/blob/release/2.2/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs#L33-L35
  • https://github.com/aspnet/Mvc/blob/a67d9363e22be8ef63a1a62539991e1da3a6e30e/src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerFactory.cs#L32-L34

A library such as FluentValidation which uses the first type is now using an unsupported API in a primary scenario. Libraries using pubinternal types break our ability to evolve the type without affecting the ecosystem and this is problematic.

In 2.1.0 we started using an analyzer to flag these kinds of incorrect usages. However this limits the utility for pubinternal and has the same overall result as using real public or internal types.

• Since pubinternal types show up as part of IDE autocompletion, it is incredibly easy for 3rd party libraries to unwittingly use these types in their regular code paths. E.g:

  • https://github.com/OrchardCMS/OrchardCore/blob/d729a2048a4390380d99e7be9f7968ff8f8e9271/src/OrchardCore/OrchardCore.Mvc.Core/ModularTenantRouteBuilder.cs#L21
  • https://github.com/OData/WebApi/blob/d36fe5910b8fee50ff46ba335c36a228332aaec9/src/Microsoft.AspNetCore.OData/Routing/ODataActionSelector.cs#L38

• Although breaking changes to pubinternal types are allowed in a minor release, the guidance isn't very clear in patch releases. We do not add/modify/remove public APIs in a patch release and this leaves pubinternal types in a precarious position. In a recent patch release, we had to discuss and ultimately abandon a patch fix that required adding a new parameter to a pubinternal method. For us to produce effective patch releases, we need the ability to modify internal APIs.

[dasMulli]

Is this decision limited to MVC or have there been discussions about doing something similar for EF Core? (asking because we currently have tools that use some pubinternal EF types because they are really useful for doing weird things and saved us a lot of time - but we fully accept that there is no support policy or API stability whatsoever)

[pranavkm]

@Sebazzz that's an idea that I hadn't considered and it would certainly help in accidental use. That said, it doesn't help with our servicing patch story which is one of the primary reasons we want to consider moving away from it's use.

@dasMulli, this is limited to the MVC repo. We had an internal discussion about this and the EF Core team was happy with their current use of pub internal types.

[Jetski5822]

@sebastienros are we affected??

[sebastienros]

No, it's taken care of

[jeremycook]

I see both sides but ultimately prefer y'all being able to move forward with confidence. It has been very nice in a few rare cases to just depend on a .Internal type instead of having to find the source code for it and copy-paste to reproduce the same outcome. In the future when I need to do that I will open an issue suggesting that an internal class be promoted to public. Thank you for announcing this.

[PeteX]

A while ago I wrote some code that allowed Kestrel to be started up without ASP.NET. I did it because I was curious to know if it could be done, and the answer seems to be yes, but unless I've missed something you need Microsoft.AspNetCore.Hosting.Internal and Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.

Standalone Kestrel seems potentially useful. It uses less memory than the normal setup, and might be appropriate when someone is using HTTP as a transport, but not writing a website.

I should add that if these APIs go away, you won't actually break anything for me. I never took it any further precisely because I would be depending on .Internal namespaces, which didn't seem like a good idea.

Edit: Posting this made me look at the problem again, and I've been able to rewrite it to avoid using any .Internal types. So thanks @pranavkm for the explanation about this discussion, and it's not a problem anyway!

[pranavkm]

@PeteX we're specifically changing types in the namespaces listed above. Neither of the namespaces you listed above are affected by this particular change \ announcement.

[tillig]

It'd be nice to see some more specific info on things you might be considering exposing as public. Stuff we've used in various ways in one manner or another that I can see include things like...

• Exceptions (e.g., AmbiguousActionException)
• Classes named "default" that have both public constructors and virtual methods (e.g., DefaultFilterProvider) - these seem like reasonable jumping-off points for custom logic so you don't have to copy/paste the whole thing.
• Static helpers (e.g., ViewEnginePath).

Most of this stuff is right in Mvc.Internal. I see other things like ICorsAuthorizationFilter is also in an internal namespace and that seems to actually be the extension point for creating that type of filter, so... yeah. An actual breakdown to know what's going where might help save time in figuring out what needs to be argued from the community.

[pranavkm]

@tillig my current plan was to only make pubinternal types public if they were previously exposed as part of a public API.

• AmbiguousActionException - End point routing does it's own exception: https://github.com/aspnet/Routing/blob/8c4f187c225e2e6d73383aa204dd2bc5d22c6b1e/src/Microsoft.AspNetCore.Routing/Matching/AmbiguousMatchException.cs. It might be useful to see if there's a more meaningful contract that you could use instead of having two match two separate types. cc @JamesNK \ @rynowak

• I'm hesitant about types like Default*. The general recommendation would be to use the contract (i.e. IFilterProvider) rather than the concrete type. More specifically with provider types (contracts that have an OnProvidersExecuting and OnProvidersExecuted) that are fairly common in MVC, you'd register an additional provider that mutates the results of the default implementation. It's an alternative to deriving from the default implementation and we've been successful using this pattern in the framework (e.g. support for ApiController is implemented in a secondary provider)

• Marker interfaces like ICorsAuthorizationFilter certainly seem like a good candidate to be made public. That said, I'd treat it on par with a feature request and it would be useful to have an issue for the ask so it can be triaged and tracked.

[tillig]

@pranavkm

to only make pubinternal types public if they were previously exposed as part of a public API

I'm not sure that narrows it down. For example, looking at the AmbiguousActionException and AmbiguousMatchException examples... ASP.NET MVC [classic] appears to have used the System.Reflection.AmbiguousMatchException from the BCL so it could be argued that from a contract standpoint the exception was public. If we're only looking at ASP.NET Core... that's just as hard to say because things in the .Internal namespace that are marked public are public now [sort of].

Anyway, it'd be potentially much easier to have a discussion if there was some sort of list of candidates that will move to be internal or a list of candidates to keep public. I recognize that's possibly a long list, but it's really hard to look through a codebase for usages or hypothesize use cases for things if we don't really know what we're looking for.

[pranavkm]

@tillig that's a fairly long list. As part of making types in the Microsoft.AspNetCore.Mvc.Core assembly internal in this PR - aspnet/Mvc#8646, there are close to 100 types that are being made internal. I'm not sure it'll be very productive to list them in this discussion.

That said, I'm happy for you to join to the feedback and identify types that may be of interest to you. I'll make sure to loop you in in PRs to other assemblies. Additionally, I'm using aspnet/Mvc#8694 to review previously pub internal types that were made public .

[tillig]

public interface IActionSelector has docs that say it throws AmbiguousActionException but that exception just got made internal. I added a comment to #8646.