Make types in Microsoft.AspNetCore.DataProtection.KeyManagement.Internal internal

[amcasey]

Background and Motivation

IInternalXmlKeyManager and ICacheableKeyRingProvider are explicitly test hooks and the other types are implementation details. They aren't included in our public API documentation since they aren't intended for external consumption.

PR: #54707

Proposed API

-namespace Microsoft.AspNetCore.DataProtection.KeyManagement.Internal;

-public sealed class CacheableKeyRing {}
-public struct DefaultKeyResolution {}
-public interface ICacheableKeyRingProvider {}
-public interface IDefaultKeyResolver {}
-public interface IInternalXmlKeyManager {}
-public interface IKeyRing {}
-public interface IKeyRingProvider {}

Usage Examples

Alternative Designs

Risks

There's very little reason to implement any of these interfaces, since the test hooks that accept them are internal. There's very little reason to invoke them, since the members of the returned objects are internal. It is, however, not impossible that people are using them. Furthermore, I don't actually know why they were public in the first place.

[dotnet-policy-service]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[Tratcher]

How does this fit with the API Compat policy of "Never remove/break a public API, and put back anything we removed in the past."?

[amcasey]

How does this fit with the API Compat policy of "Never remove/break a public API, and put back anything we removed in the past."?

I guess I'm struggling to see the harm in removing an undocumented type you can't instantiate, subtype, or access the members of. But, yes, it would technically be a violation of our ironclad guarantee against breaking changes.

[Tratcher]

@joperezr

[amcasey]

Does our compat guarantee require that they continue to function as they do now or just that they continue to exist? If I made copies in Microsoft.AspNetCore.DataProtection.KeyManagement (no ".Internal") and left these orphaned, would that suffice?

Motivating example: I introduced a new interface to avoid breaking the existing ICacheableKeyRingProvider - was it important to continue to implement the old test hook in case someone was calling it or could I have just dropped that the implemented interfaces list? (Note that that's not the final shape of the PR.)

[amcasey]

Related: can I mark these types as Obsolete or is that also breaking?

[amcasey]

API rejected. Bend over backwards to not change things.