DbFunction definition returning nullable value type causes incorrect SQL to get generated

[andreikarkkanen]

I have AsQueryable extension method that solves SQL Server cache pollution problem using STRING_SPLIT approach. The following invocation has different results depending of TargetFramework

For EFCore 6.0.11 it produces
exec sp_executesql N'SELECT [c].[CustomerId] FROM [Customers] AS [c] WHERE EXISTS ( SELECT 1 FROM STRING_SPLIT(@__source_1, @__separator_2) AS [s] WHERE CONVERT(UNIQUEIDENTIFIER, [s].[Value]) = [c].[CustomerId])',N'@__source_1 nvarchar(4000),@__separator_2 nvarchar(4000)',@__source_1=N'009cf046-a859-4849-8de7-9854bd00f7de,b38c95f2-e5aa-4926-a315-a23ce6b6578c',@__separator_2=N','

and for EFCore 7.0.0 it has
SELECT [c].[CustomerId] FROM [Customers] AS [c] WHERE 0 = 1

The full code of the program is in the attachment

ConsoleApp1.zip

[roji]

The problem is with the DbFunction configuration of the ToGuid function:

modelBuilder.HasDbFunction(type.GetMethod(nameof(ToGuid))!)
    .HasTranslation(t => new SqlFunctionExpression(
        functionName: "CONVERT",
        arguments: new[] { new SqlFragmentExpression("UNIQUEIDENTIFIER"), t[0] },
        nullable: true,
        argumentsPropagateNullability: new[] { false, true },
        typeof(Guid?), <-- Change to Guid
        typeMapping: null))
    .IsBuiltIn();

Although the function may return null, in EF's SQL tree, the CLR types are never nullable (since nullability is handled differently on the SQL side). Changing the function definition to return Guid instead of Guid? generates the correct SQL.

Note that we have a debug assertion against this, but of course it doesn't occur in Release. We should add model validation against DbFunctions with nullable value return types.

For reference, the reason this started failing in 7.0 is f1bcf13, which changed the Contains(foo) to be rewritten to Any(x => object.Equals(x, foo), as opposed to the previous Any(x => x == foo). The 6.0 equality gets translated in RelationalSqlTranslatingExpressionVisitor.VisitBinary, which doesn't care about the nullability of the two sides. But the 7.0 object.Equals gets to EqualsTranslator, which returns false since the left and right side have different nullability.

[andreikarkkanen]

Thank you so much, I wouldn't have figured it out on my own.

[RichardD2]

This is now throwing an exception in EF Core 8.0 which wasn't present in EF Core 7. However, it doesn't seem to be documented as a "breaking change", which it technically is.

[ajcvickers]

@RichardD2 When something was not working correctly in a previous version and we fix that, as was the case here, we generally don't consider that a breaking change since your code wasn't working correctly before.

[RichardD2]

@ajcvickers In my case, I was using TRY_PARSE, and it was working in EF7 - at least in my tests.

It was even mostly working for other people:
NBS.EntityFrameworkCore.SqlServer.TryParse

The only issue reported with EF7 was a missing AS in the generated query for a very complex projection, which I have yet to be able to reproduce.

Changing that to throw an exception at runtime certainly seems like a breaking change to me. :)

[ajcvickers]

Note for triage: make sure we are not being overly aggressive validating this.

[ajcvickers]

@RichardD2 I checked with the team, and while this may have been working for you in some cases, that was accidental, and the code should be changed going forward.