Remove unused SaveChanges APIs in EF DbContext Interfaces

[beforan]

Quick note first: I've tried to look around at existing questions related to this area (#3507 #2067) and frankly that makes this more confusing.

Question

IConfigurationDbContext and IPersistedGrantDbContext both expect implementors to provide the following method in addition to the expected DbSets:

Task<int> SaveChangesAsync();

However, EF Core's DbContext provides an implementation that takes 1 optional parameter, which should presumably suffice for any call sites working with these interfaces.

Furthermore, as per the issues linked above, I see that IdentityServer4's EF Stores don't actually call SaveChangesAsync() (this is a work in progress as per #3674), which suggests to me the interfaces don't need the method implementing as it will never be called anyway.

Really I'm just making sure I don't need to do something important when implementing these interfaces, and if I have noticed this unnecessary requirement, maybe the interfaces can be updated to reflect that.

[scottbrady91]

I don't remember exactly why. I think it dates back to Entity Framework Core 1.x method signatures. It's something that can be updated, but unfortunately, it's a breaking change.

You're not missing anything. The default implementation just calls base: https://github.com/IdentityServer/IdentityServer4/blob/master/src/EntityFramework.Storage/src/DbContexts/ConfigurationDbContext.cs#L82

[brockallen]

Yep. I think in a 4.0 we can revisit it.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[brockallen]

Yea, there are 2 Save APIs -- any objections to removing both (on the config interface)? We'd remove just the sync one on the grants interface.

[brockallen]

Done

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.