Make IDataProtectionKeyContext settable

[amcasey]

Background and Motivation

I understand that it's required when persisting keys to EF, so we should reflect that in the interface. Note that it's included in our docs.

PR: https://github.com/dotnet/aspnetcore/pull/52644

Proposed API

namespace Microsoft.AspNetCore.DataProtection.EntityFrameworkCore;

public interface IDataProtectionKeyContext
{
-    DbSet<DataProtectionKey> DataProtectionKeys { get; }
+    DbSet<DataProtectionKey> DataProtectionKeys { get; set; }
}

Usage Examples

class MyContext : IDataProtectionKeyContext
{
    public DbSet<DataProtectionKey> DataProtectionKeys { get; set; } = null!;
}

Alternative Designs

It might be possible to use a default interface member that throws, but I wouldn't guess you could add a single accessor to an existing property.

Risks

Obviously, it's a breaking change. I understand it only breaks people who are already persisting keys incorrectly.

[ghost]

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.

[halter73]

API Review Notes:

• This is meant to save people who think they're done after they auto-implement IDataProtectionKeyContext.
  • We could introduce a new interface, but no one would likely call or implement it making it useless.
• This should not be source breaking unless your code is already broken (unless the setter was internal?)
  • Are there other ways this could be initialized?
• This will be binary breaking. We expect there would be type load exception for IDataProtectionKeyContext implementations that have the setter, but aren't rebuilt with this change.
  • Could this be mitigated by a default no-op setter?
• If we are too afraid of the binary break, we could ship an analyzer instead?
• Is this valid? If so, we wouldn't want to break it.

  DbSet<DataProtectionKey> DataProtectionKeys { get; } = new MyDbSet<DataProtectionKey>();

• It looks like we found at least one seemingly valid usage that would have a source break with this change at https://github.com/alexyakunin/BoardGames/blob/54dd0b2a7684e2cf579482a463bbe7f711c01e9d/src/HostServices/AppDbContext.cs because of the protected setters. That's all we can find using https://grep.app/search?current=4&q=DataProtectionKeys%20%7B though.

@ajcvickers @csharpfritz Do you have any opinions?

As of now, we think that we should probably go with an analyzer if anything.

[ajcvickers]

From an EF perspective, the DbSet property does not need to be, and ideally should not be, settable.

[csharpfritz]

From an EF perspective, the DbSet property does not need to be, and ideally should not be, settable.

That's interesting... how do you work with the collection if the DbSet is not settable?

This syntax is invalid because DbSet is abstract:

public DbSet<DataProtectionKey> DataProtectionKeys { get; } = new DbSet<DataProtectionKey>();

We can initialize the property in the constructor with syntax like:

DataProtectionKeys = Set<DataProtectionKey>();

Either way... we can agree that the auto-generated syntax is invalid. I think I'd favor an analyzer that throws an error on this misconfiguration.

[ajcvickers]

@csharpfritz My personal favorite pattern is:

public DbSet<DataProtectionKey> DataProtectionKeys => Set<DataProtectionKey>();

You can provide a public or private setter, and then EF will initialize the properties for you. Or you can call Set in the constructor and initialize the properties explicitly there.

The main point here is that consumers will never call the setter, so my preference is to not include it, and certainly it should not be required by the interface.

[amcasey]

API rejected. Thanks, @ajcvickers!