search

zompinc / efcore-extensions

Provides window (analytics) functions and binary functions for EF Core. Providers: SQL Server, SQLite, Postgres.
MIT License
60 stars 6 forks source link

UseWindowFunctions and target framework versions vs dependency versions problem #4

Closed Ephron-WL closed 1 year ago

Ephron-WL commented 1 year ago

The set of classes you define to support the call to UseWindowFunctions have an issue. You are using preprocessor directives to evaluate whether the user is running net6.0 or net7.0 assuming that the Microsoft.EntityFrameworkCore.SqlServer dependency is also versioned consistent with the target framework. However, as you see in the image below, even Microsoft.EntityFrameworkCore.SqlServer 7 is targeting net6.0:

image

My issue is that I'm using Microsoft.EntityFrameworkCore.SqlServer 7 while targeting net6.0, which leads to exceptions when using your library, since your code is compiling as if I'm using Microsoft.EntityFrameworkCore.SqlServer 6 and clearly they introduced breaking changes in 7.

I'm not really sure how you should solve this issue, however. Maybe you instantiate some object from Microsoft.EntityFrameworkCore.SqlServer, which will load the assembly into the runtime and then you can use reflection to obtain the assembly version loaded. Using that you can identify which version of the service objects to load. What do you think? I'd be happy to submit a PR with the solution if you are amenable to it.

virzak commented 1 year ago

@Ephron-WL thanks for reporting and you're right, before .NET 6 + EF Core 7 isn't covered at this time.

The other option is to push 2 versions. One for LTS (.NET 6) and one for STS (.NET 7). That's what packages like Entity Framework Extensions do.

Not entirely sure what your solution looks like, is there an existing example of this? Also, reflection should be avoided, shouldn't it?

Ephron-WL commented 1 year ago

@Ephron-WL thanks for reporting and you're right, before .NET 6 + EF Core 7 isn't covered at this time.

The other option is to push 2 versions. One for LTS (.NET 6) and one for STS (.NET 7). That's what packages like Entity Framework Extensions do.

Yes, that seems like an obvious solution that I missed. Do you just want to do that?

Not entirely sure what your solution looks like, is there an existing example of this? Also, reflection should be avoided, shouldn't it?

Yes, normally. The reflection is minimal and is only called once, when the DbContext is created. If you are pooling them then reuse of the DbContext won't even call this again. Below is my approach and then I'd check the assembly version in the runtime to identify to which set of classes to use. Because your idea above would be consistent with Microsoft's own handling of Microsoft.EntityFrameworkCore.SqlServer, it is probably the best. While I think my creative approach would be acceptable, the advantages might be minor.

image

virzak commented 1 year ago

I'm implementing my way, but might run into some issues with nbgv. Also really unsure about which way is better. If you have a PR (even a draft), feel free to submit so we can compare which way is better.

virzak commented 1 year ago

Fixed in https://github.com/zompinc/efcore-extensions/commit/2a5277ad59223e98b227d4b6acd649af996dbcce by targeting to .NET 6

I ended up dropping the EF Core 6 support going forward, since .NET 6 is LTS, but EF Core 6 isn't. If anyone wants it back, we can deal with it then.

Let me know if this doesn't resolve the issue for you and we'll reopen.