-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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 InMemory database throws an argument exception on Nullable<>.ToString #35299
Comments
@wilfriedb can you share the listing of the unit test that is failing (or other form of full repro of this issue)? I'm trying the following:
and it works fine. Expression tree we create here is
which is has no problems calling ToString method on |
Hello @maumar, I'm in the process of making a minimal repo that will reproduce the issue. There are many, many expressions in this code therefore this process is not going fast . When I've finished this I will post an update here. |
Hello @maumar here is a minimal repo that reproduces the issue: https://github.com/wilfriedb/EFCoreReproduce In this repo, the unit test will fail with an exception, as mentioned above. Also, with a minimal change in the file MapperProfile.cs, as indicated in the comments, the test will succeed. Fun fact, the DateOnly field ValueDate on the PaymentModel class, on which the exception occurs, is not nullable. But when debugging in the file InMemoryExpressionTranslatingExpressionVisitor, the field (in an expression) is promoted (demoted?) to a nullable DateOnly. This is also the case in EF Core 8, but on line 888, in EF Core 9 the if statement is skipped (the condition evaluates to false), but in EF Core 8 the if statement is entered (the condition evaluates to true). |
I have the same issue, here is the 1 file repro https://github.com/domagojmedo/EfCore35299 You can just comment and uncomment line in .csproj |
Shouldn't this at least be added to breaking changes for EF 9? |
I ran into this, doesn't really understand how it could be fixed / worked around. Do we need to downgrade? Can i ran EF Core 8 with .net 9? |
You can, that's what I did. Using [8.0.11] in test project |
regression introduced in #33706 efcore/src/EFCore.InMemory/Query/Internal/InMemoryExpressionTranslatingExpressionVisitor.cs Line 892 in 59fc95b
we add a null check and only if the object is not null, we convert the object to non-null, to match the type and call the method. Problem is that we don't do that for Nullable<>.ToString - the way that we check for it is by method name only, so we accidentally also filtered out non-nullable ToString. We should check the declaring type as well |
workaround is to convert the argument to nullable before calling ToString(): AuthorId = ((int?)x.Author!.Id).ToString() |
Yes, but that means we have to write our EF code differently just because of tests and that's not something I'm very keen on doing. Think we're gonna stay on version 8 and hope it's fixed in 10 |
…n on Nullable<>.ToString Problem was that when rewriting the (inmemory) query, sometimes we change the nullability of the expression fragment. (e.g. when accessing a property on an optional entity). We then have a logic to compensate for the change. E.g. when the modified fragment was a caller of a method, we add a null check and only call the method (as non-nullable argument) if the value is not null. This way we can retain the method signature as it was. In 9, we added some exemptions to this logic, e.g. `GetValueOrDefault`, or `Nullable<>.ToString`, which don't need the compensation. Problem was that we were matching the ToString method incorrectly, only looking at the method name, so we would also match non-nullable ToString(). Fix is to look at the declaring type of the ToString method as well to make sure it's nullable<T>, otherwise apply the compensation. Fixes #35299
…n on Nullable<>.ToString Problem was that when rewriting the (inmemory) query, sometimes we change the nullability of the expression fragment. (e.g. when accessing a property on an optional entity). We then have a logic to compensate for the change. E.g. when the modified fragment was a caller of a method, we add a null check and only call the method (as non-nullable argument) if the value is not null. This way we can retain the method signature as it was. In 9, we added some exemptions to this logic, e.g. `GetValueOrDefault`, or `Nullable<>.ToString`, which don't need the compensation. Problem was that we were matching the ToString method incorrectly, only looking at the method name, so we would also match non-nullable ToString(). Fix is to look at the declaring type of the ToString method as well to make sure it's nullable<T>, otherwise apply the compensation. Fixes #35299
After upgrading to EF Core 9, a unit test using the InMemory database throws this exception:
This is from the Microsoft.EntityFrameworkCore.InMemory package.
When I look into this package code where the exception originates, I see the following change between version 8 and version 9:
EF Core 8:
Namespace Microsoft.EntityFrameworkCore.InMemory.Query.Internal
InMemoryExpressionTranslatingExpressionVisitor.cs line 888:
EF Core 9:
InMemoryExpressionTranslatingExpressionVisitor.cs line 887:
So the new code adds explicitly a
Nullable<>.ToString
according to the comment, but this new code throws an exception at runtime because it cannot be called!(For using an EF Core InMemory database in a unit test, yes I know that's not a good practice, but we inherited this Solution)
The Microsoft.EntityFrameworkCore.SqlServer package does not have this bug, it exhibits the expected behavior.
This seems a regression to me.. It is also interesting to see that the code makes an exception for the
int
value type, but not for any other value type.EF Core version: 9.0
Database provider: Microsoft.EntityFrameworkCore.InMemor
Target framework: .NET 0.0)
Operating system: Windows 11 21H2
IDE: Visual Studio 2022 17.12.3
The text was updated successfully, but these errors were encountered: