WebApi icon indicating copy to clipboard operation
WebApi copied to clipboard

$select doesn't work with custom IQueryable implementation

Open ideafixxxer opened this issue 3 years ago • 6 comments

It seems that the current implementation of OData components is hard-wired to MS implementations of IQueryable, such as EF, EF.Core etc. The logic behind adding Null Check Expression verifies if data source provider is EFClassic, and only in this case skips the null check (see SelectExpandBinder.cs, line 800). To determine whether it is EFClassic or not, the namespace is compared with pre-hardcoded list of namespaces. This is not an extensible solution. If another EF-like implementation of IQueryable constructs SQL query, it will be constructed with error.

Assemblies affected

Microsoft.AspNet.OData.Shared

Reproduce steps

There are no "simple" steps to repro it as a custom implementation of IQueryable is required. You can try and remove EFClassic namespace from the hardcoded list. Add $select to ODataQueryOptions and try to apply it to a IQueryable instance.

Expected result

I would expect ODataQuerySettings.HandleNullPropagation handle this situation. If it is not supposed to, maybe add another setting, instead of hardcoding.

Actual result

Here is an example of generated Lambda

.Lambda #Lambda18<System.Func`2[InvoiceModel,Microsoft.AspNet.OData.Query.Expressions.SelectExpandBinder+SelectSome`1[InvoiceModel]]>(InvoiceModel $$it)
{
    .New Microsoft.AspNet.OData.Query.Expressions.SelectExpandBinder+SelectSome`1[InvoiceModel]()
    {
        ModelID = .Constant<Microsoft.AspNet.OData.Query.Expressions.LinqParameterContainer+TypedLinqParameterContainer`1[System.String]>(Microsoft.AspNet.OData.Query.Expressions.LinqParameterContainer+TypedLinqParameterContainer`1[System.String]).TypedProperty,
        Container = .New Microsoft.AspNet.OData.Query.Expressions.PropertyContainer+NamedProperty`1[System.Nullable`1[System.Int32]]()
        {
            Name = "Id",
            Value = .If ($$it == null) {
                null
            } .Else {
                (System.Nullable`1[System.Int32])$$it.Id
            }
        }
    }
}

The generated SQL cannot be processed by SQL Server.

ideafixxxer avatar Jun 07 '21 22:06 ideafixxxer

Hi all, we are facing the exact same problem and I was going to open a new issue before finding this one. I mentioned the topic in #2537 . Is there any plan about a review of this part? Thanks.

f-camera avatar Sep 13 '21 16:09 f-camera

@ideafixxxer @f-camera there's work introduced to make it possible to provide your own SelectExpandBinder implementation, which is what handles the $select and $expand logic: https://github.com/OData/AspNetCoreOData/pull/296

Do you think this would help address the issue you're facing?

habbes avatar Sep 21 '21 08:09 habbes

Hi @habbes, thank you for the info. It looks promising, but I have few questions/comments:

  • is the PR OData/AspNetCoreOData#296 to be followed by some corresponding work on OData/WebApi?
  • If yes, is there already any plan about it?
  • I've got the base idea and some details, but I'm not sure of my complete understanding: at first sight, at least for our case, it looks a bit of an overkill. As far as our attempts went, I verified that our minimal need is to influence the decision on this line of code https://github.com/OData/WebApi/blob/67f82a668a558de6dcf7221279fca22072547c4d/src/Microsoft.AspNet.OData.Shared/Query/Expressions/SelectExpandBinder.cs#L804 since our custom Providers are not recognized ("mapped") by https://github.com/OData/WebApi/blob/67f82a668a558de6dcf7221279fca22072547c4d/src/Microsoft.AspNet.OData.Shared/Query/HandleNullPropagationOptionHelper.cs#L71 but at the same time they actually "wrap" EF6 or EFCore. If they were recognized, unless we discover other difficulties, the work done by the current SelectExpandBinder.cs would be fine, as it has been fine so far. So, if possible, I'd prefer to avoid having to provide a full custom implementation of SelectExpandBinder.

This issue is preventing us from adopting more recent releases (we are at 7.1.0 so far) and finally take advantage of the performance related fixes I was looking for in #2537 .

Thank you for every clarification you may provide.

f-camera avatar Sep 21 '21 12:09 f-camera

@f-camera I do agree that it does sound like it would be overkill for your scenario. Maybe we should expose an option to allow the user to select how the null-check propagation is handled since this seems to be a common source of errors. What do you think @KenitoInc @xuzhg ?

habbes avatar Nov 28 '21 22:11 habbes

@f-camera @habbes We can disable NullPropagation by setting ODataQuerySettings.HandleNullPropagation = HandleNullPropagationOption.False

KenitoInc avatar Dec 01 '21 07:12 KenitoInc

Hi @ideafixxxer you can disable/configure null propagation by setting the HandleNullPropagation property in [EnableQuery], for example:

[EnableQuery(HandleNullPropagation = HandleNullPropagationOption.False)]
public IQueryable<SomeEntity> Get() { /* ... */ }

Let us know if this solves the issue.

habbes avatar Feb 01 '22 08:02 habbes