Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EF Core 6 RC1 breaks extensions due to a change in a contract #26022

Closed
koenbeuk opened this issue Sep 14, 2021 · 11 comments
Closed

EF Core 6 RC1 breaks extensions due to a change in a contract #26022

koenbeuk opened this issue Sep 14, 2021 · 11 comments

Comments

@koenbeuk
Copy link

koenbeuk commented Sep 14, 2021

File a bug

After updating my project to EF Core 6 RC1, extensions installed with the project stopped working due to a contract change:

Before: https://github.com/dotnet/efcore/blob/v5.0.10/src/EFCore/Infrastructure/DbContextOptionsExtensionInfo.cs#L49

public abstract class DbContextOptionsExtensionInfo {
    ....
    public abstract long GetServiceProviderHashCode();
}

After: https://github.com/dotnet/efcore/blob/v6.0.0-rc.1.21452.10/src/EFCore/Infrastructure/DbContextOptionsExtensionInfo.cs#L49

public abstract class DbContextOptionsExtensionInfo {
    ....
    public abstract int GetServiceProviderHashCode();
}

GetServiceProviderHashCode has a new return type of int rather than long. This causes the following exception in production when loading an extension that was compiled against a previous EF Core version:

System.TypeLoadException: 'Method 'GetServiceProviderHashCode' in type 'ExtensionInfo' from assembly 'EntityFrameworkCore.Projectables, Version=1.0.0.0, Culture=neutral, PublicKeyToken=fc5550082a9c642c' does not have an implementation.'

A simple reproduction using: EntityFrameworkCore.Projectables:

 var serviceProvider = new ServiceCollection()
    .AddDbContext<ApplicationDbContext>(options => {
        options
             .UseSqlite(dbConnection)
             .UseProjectables() // Include our custom extension
         })
    .BuildServiceProvider();

// Causes System.TypeLoadException when targeting EF Core 6 RC1. Runs fine when targeting EF Core 6 preview 7 or earlier (including EF Core 5 and 3.1)
var dbContext = serviceProvider.GetRequiredService<ApplicationDbContext>(); 

EF Core version: 6.0.0-rc.1.21451.13
Database provider: (e.g. Microsoft.EntityFrameworkCore.SqlServer)
Target framework: (e.g. .NET 6.0 RC 1)
Operating system: Windows
IDE: (e.g. Visual Studio 2022 17.4)

@koenbeuk koenbeuk changed the title EF Core 6 rc1 breaks extensions EF Core 6 RC1 breaks extensions due to a change in a contract Sep 14, 2021
@ajcvickers
Copy link
Member

Documenting this is tracked here: dotnet/EntityFramework.Docs#3406

@koenbeuk
Copy link
Author

@ajcvickers I'm not convinced this needs to be a breaking change. The breaking part seems to be moving from a long to int. This is probably because the HashCode type is now being used which builds on an integer. Instead I would suggest the following changes:

  • Changed the DbContextOptionsExtensionInfo.GetServiceProviderHashCode return type back to long. The new implementation using HashCode can still work due to implicit convertibility between long < int.
  • Give DbContextOptionsExtensionInfo.ShouldUseSameServiceProvider a default implementation that simply compares hashcodes.

I'm happy to issue a PR if that helps into taking this into consideration.

@ajcvickers
Copy link
Member

ajcvickers commented Sep 15, 2021

@koenbeuk It's more involved than that. I already discussed with @AndriySvyryd whether there was a reasonable way to avoid this. He can explain more if you want to pursue this.

@AndriySvyryd
Copy link
Member

The primary break is that extensions need to implement ShouldUseSameServiceProvider, the issue was that we were just comparing hash codes, which has led to incorrect results due to collisions. To implement it properly extensions will need to release a new version and it won't be backwards compatible.

@koenbeuk
Copy link
Author

@AndriySvyryd Understood and I agree that it's an important change. However it does force us extension developers to take dependency on EF Core 6. In my last comment I proposed a change that would allow this new extension model to exist without breaking backward combability. Is my proposal technically sound? and if so, can we consider implementing this? (As mentioned, I'm happy to issue a PR)

@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 15, 2021

@koenbeuk You can have multiple builds/packages from same code base.

@koenbeuk
Copy link
Author

koenbeuk commented Sep 15, 2021

@ErikEJ That's what I'm doing now to support EF Core 3.1 and EF Core 5 in another one of my projects. The problem I have with this approach is that its breaking semver. As I now have a 1.x branch as well as a 2.x branch that target different EF Core versions. I could certainly create a 3.x branch but that only adds confusion and I rather stick with semver.

@AndriySvyryd
Copy link
Member

@koenbeuk Without this forcing breaking change it's unlikely that the extensions will add an implementation. And if they do the new extension version will not be backwards compatible with previous versions of EF Core, so it wouldn't save any work or avoid versioning changes.

@koenbeuk
Copy link
Author

koenbeuk commented Sep 15, 2021

@AndriySvyryd Not trying to beat on a dead horse here, I get the impression that decisions have already been made 😄. Just to ensure that we understand each other:

These 2 proposed changes:

  • Change the DbContextOptionsExtensionInfo.GetServiceProviderHashCode return type back to long.
  • Give DbContextOptionsExtensionInfo.ShouldUseSameServiceProvider a default implementation that simply compares hashcodes (produced by GetServiceProviderHashCode).

Would fix this breaking change while still allowing extension authors to give an implementation to ShouldUseSameServiceProvider.


it's unlikely that the extensions will add an implementation

An analyzer could suggest or even enforce an implementation

And if they do the new extension version will not be backwards compatible with previous versions of EF Core

In my proposal we still only have 1 extension model. Providing a custom implementation only affects EF Core 6 and beyond. Not providing an implementation means that there is still the chance of hash collisions.

@AndriySvyryd
Copy link
Member

Would fix this breaking change while still allowing extension authors to give an implementation to ShouldUseSameServiceProvider.
Providing a custom implementation only affects EF Core 6 and beyond. Not providing an implementation means that there is still the chance of hash collisions.

Yes, I understand that your proposal allows extensions to work with EF Core 6 without fixing the original issue. But this issue is hard to debug and workaround when it happens, so we decided to take a break and fix it for good this time.

Note that 3.1 and 5.0 will go out of support next year: Releases

@koenbeuk
Copy link
Author

koenbeuk commented Sep 15, 2021

Closing this issue for now as that this is a deliberate breaking change. Documentation is already tracked as a separate issue. Thanks for the attention and suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants