OData / AspNetCoreOData

ASP.NET Core OData: A server library built upon ODataLib and ASP.NET Core
Other
458 stars 158 forks source link

Canonical functions not returning null for null parameters #1310

Open Xriuk opened 2 months ago

Xriuk commented 2 months ago

Assemblies affected ASP.NET Core OData 8.2.4"

Describe the bug At least one of the built-in/canonical functions do not return null when one of their parameter is null. A concat with a non-nullable string property on a nullable complex property, does not return null but an empty string.

Data Model

// Complex
public class VatNumber{
    public string CountryCode {get; set;}

    public string Number {get; set;}
}

// Entity
public class Customer{
    public int Id {get; set;}

    public VatNumber? VatNumber {get; set;} // Nullable/optional
}

EDM (CSDL) Model

<ComplexType Name="VatNumber">
    <Property Name="CountryCode" Type="Edm.String" Nullable="false" MaxLength="2" />
    <Property Name="Number" Type="Edm.String" Nullable="false" MaxLength="20" />
</ComplexType>
...
<EntityType Name="Customer">
    <Key>
        <PropertyRef Name="Id" />
    </Key>
    <Property Name="Id" Type="int" Nullable="false" />
    <Property Name="VatNumber" Type="Models.VatNumber" />
</EntityType>

Request/Response

/odata/Customers/?$compute=concat(VatNumber/CountryCode, VatNumber/Number) as FullVAT&$select=FullVAT

The response contain FullVAT property as an empty string for customers which do not have a VAT number. The resulting SQL contains: COALESCE([c].[VatNumber_Country], N'') + COALESCE([c].[VatNumber_Number], N'')

Expected behavior Coalesce should not be used, I don't know if this is OData's fault or EF Core's. Anyway OData should perform some null checks I guess.

Additional context Section 5.1.1.4 URL Conventions

If a parameter of a canonical function is null, the function returns null.

julealgon commented 2 months ago

Can you provide an explicit example of how the final string looks like today, and how it should look like?

Xriuk commented 2 months ago

The final string should be the concatenation of the specified strings for users with a VAT number (eg: "AT1234567"), or a null value for users without a VAT number. Now the string is returned correctly for users with a VAT number, while a empty string is returned for users without one.

julealgon commented 2 months ago

@Xriuk oh I see now. I had slightly misunderstood your example. I see that both arguments for the concatenation are inside of the VatNumber model now.

Xriuk commented 2 months ago

I see that both arguments for the concatenation are inside of the VatNumber model now.

Yes, and they themselves are not nullable inside VatNumber, but VatNumber itself can be nullable. I don't know if maybe this is not causing the nullability to propagate to the returned value or if null values are not handled at all.

xuzhg commented 2 months ago

If we don't have the null propagate setting, it's by default

[EnableQuery]
public IActionResult Get()
{
    return Ok(_context.Customers);
}

You will get "empty string" in the payload:

image

And yes, the Database has the similar log as:

image

If you have the following configuration on the query setting:

[EnableQuery(HandleNullPropagation = HandleNullPropagationOption.True)]
public IActionResult Get()
{
    return Ok(_context.Customers);
}

You can get the following 'null' result. image

Here's the output of Database log: image

xuzhg commented 2 months ago

See my test sample at: https://github.com/xuzhg/WebApiSample/tree/main/v8.x/NullValueSample

Xriuk commented 2 months ago

HandleNullPropagationOption.True seems to solve the issue. Is there any reason why this is configurable and may be disabled by default (since it looks like the default option depends on the query provider)?

The specification does not seem to allow customization, and seems pretty explicit to me. Section 5.1.1.4 URL Conventions

If a parameter of a canonical function is null, the function returns null.

Xriuk commented 1 month ago

This solution is conflicting with $expand/$select query options as it generates a lot of ternary operators with null checks (eg: $it == null ? null : $it.Prop == null ? null : $it == null ? null : $it.Prop), which sometimes cannot be translated from Linq to SQL (in my case).

IMO there should be at least 2 separate options to handle nulls, one to handle generated code like properties/navigations and one to handle null for canonical functions.