Fix to #35299 - EF Core InMemory database throws an argument exception on Nullable<>.ToString #35763
Conversation
There was a problem hiding this comment.
PR Overview
This PR addresses an issue where the compensation logic was incorrectly applied to non-nullable ToString calls by refining the matching criteria for Nullable.ToString. The changes include updating the method call condition in the expression visitor and adding a new functional test to verify the fix.
- Updated logic in InMemoryExpressionTranslatingExpressionVisitor to check the declaring type for Nullable.ToString.
- Added a new functional test in GearsOfWarQueryInMemoryTest to cover the behavior on non-nullable properties.
Reviewed Changes
| File | Description |
|---|---|
| test/EFCore.InMemory.FunctionalTests/Query/GearsOfWarQueryInMemoryTest.cs | Added a functional test to ensure proper handling of ToString on non-nullable properties of an optional entity. |
| src/EFCore.InMemory/Query/Internal/InMemoryExpressionTranslatingExpressionVisitor.cs | Refined the condition to correctly exclude Nullable.ToString by verifying the method's DeclaringType. |
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/EFCore.InMemory/Query/Internal/InMemoryExpressionTranslatingExpressionVisitor.cs:891
- [nitpick] The updated condition for excluding Nullable.ToString now includes a check on DeclaringType, which makes the logic complex. Consider adding inline comments or refactoring this condition into a helper method for better readability and maintainability.
&& !(@object!.Type.IsNullableValueType()
|
added the test to InMemory only - there is a discrepancy in the results for this scenario: in memory returns null (which is what it returned in 8, this change essentially reverts to the 8.0 behavior, fixing the regression we made in 9), whereas relational returns empty string (we append coalesce "") to those queries |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…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
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, orNullable<>.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, otherwise apply the compensation.Fixes #35299