Skip to content
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

Canonical functions not returning null for null parameters #1310

Open
Xriuk opened this issue Sep 9, 2024 · 8 comments
Open

Canonical functions not returning null for null parameters #1310

Xriuk opened this issue Sep 9, 2024 · 8 comments

Comments

@Xriuk
Copy link

Xriuk commented Sep 9, 2024

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.

@Xriuk Xriuk added the bug Something isn't working label Sep 9, 2024
@julealgon
Copy link
Contributor

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

@Xriuk
Copy link
Author

Xriuk commented Sep 9, 2024

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
Copy link
Contributor

@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
Copy link
Author

Xriuk commented Sep 9, 2024

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
Copy link
Member

xuzhg commented Sep 11, 2024

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
Copy link
Member

xuzhg commented Sep 11, 2024

@xuzhg xuzhg added investigated and removed bug Something isn't working labels Sep 11, 2024
@Xriuk
Copy link
Author

Xriuk commented Sep 12, 2024

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
Copy link
Author

Xriuk commented Oct 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants