Expand ODataSettings with additional properties to enable more configuration

[darjanbogdan]

With this change I aim to expand custom ODataSettings to support additional configuration properties which are naturally exposed in ODataQuerySettings type to allow dynamic assignment of them.

There is a difference in EFCore part in which HandleNullPropagation was set to False for some reason, default value is Default. So would be good to check that once again, although tests are passing.

I was thinking to completely remove ODataSettings and use ODataQuerySettings but that would be a breaking change which would be good to avoid at this moment.

[BlaiseD]

HandleNullPropagation is false because EFCore handles null checks and usually throws when they're included.

You'll want to pass ODataSettings to GetQueryableExpression where the PageSize may replace $top when present - not ToFilterExpression. (Does that change work? It shouldn't because the sorting expressions are added elsewhere.)

We handle most of the expressions ourselves so the usage of ODataQuerySettings in ToFilterExpression may be removed at some point in favor of the FilterHelper.

You'll also want to add tests to account for the new logic.

Make sense?

[darjanbogdan]

Thanks for the prompt response and explanations!

I'll revert the changes I did and expand ODataSettings with PageSize prop only. EnsureStableOrdering doesn't have to be exposed nor configurable in this case.

Due to my lack of knowledge and overall understanding I won't change ToFilterExpression in favor of FilterHelper as I'm not confident I will do it right.

So, for now GetQueryableExpression will be adapted to support ODataSettings and additional tests will be added to cover the changes.

[darjanbogdan]

I finally had time to spend investigating behavior of both approaches mine and the one you described as preferred.

First thing to clarify is a difference between $top parameter and PageSize property as their responsibilities are different.
PageSize property is used (responsible) to limit the result set on the server, no matter of the $top presence or value.

To give an example, here is the arbitrary fixture:
ResultSet: 50 - total number of items which can be fetched
PageSize: 20 - server defined, can't be manipulated by the client
$top: 30 - client defined/manipulated

So, some GET /resources?$top=30 request should return 20 items only, along with next page URL which has $top=10&$skip=20 params appended. So, even though client is requesting top 30 items, it needs to do 2 roundtrips to the server to fetch the whole set, due to constraints on the server's page size.

Described behavior is supported by OData lib when passed through ODataQuerySettings. Basically changes done in this PR are working correctly, if the $filter is defined. The main reason is in filterOption.ApplyTo call which is done on FilterQueryOption instead of ODataQueryOptions<TModel>. I guess there is a good reason for it, so do you have an idea how to enhance this implementation to avoid introducing some hacks to pass "dummy" filter option object just to apply page size limitation?

I've also tried with the approach you mentioned, through GetQueryableExpression method, but it doesn't work correctly. Because $top and PageSize are used for the same thing - client side pagination. So, if we try the example request above, server will actually return 30 items in the first request, which would be a breaking change from the expected behavior of OData endpoint.

Now, I'm not sure how to proceed with this change, so any help would be appreciated! :)

Thanks!

[BlaiseD]

I'm saying something like the following where the server configuration takes precedence over $top.

        private static int? GetPageSize(TopQueryOption topQueryOption, ODataSettings oDataSettings)
        {
            if (oDataSettings?.PageSize.HasValue == true)
                return oDataSettings?.PageSize;

            return topQueryOption?.Value;
        }

You'd send nextLink back with the same request. You're saying that causes two a second roundtrip?

[darjanbogdan]

The problem in using such method is that PageSize and $top would be used in a mutually exclusive manner, while it should be mutually inclusive.

If GetPageSize is implemented likewise, it would effectively overwrite $top which is sent by the client, and that would be wrong. $top needs to stay intacted, and PageSize should be used to tweak the ApplyTo method's logic and behavior.

And yes, nextLink would be sent within the response of the first request, but the params should be as described above. That means, for some client in order to get full result set, it needs to send 2 requests to fetch all 30 items. In first request 20, in second request 10.

[BlaiseD]

So, some GET /resources?$top=30 request should return 20 items only, along with next page URL which has $top=10&$skip=20 params appended. So, even though client is requesting top 30 items, it needs to do 2 roundtrips to the server to fetch the whole set, due to constraints on the server's page size.

So this is the behavior you want - you're not saying two trips is a problem - I misunderstood. Also $top is not being used for paging in your scenario - it limits the complete result set.

Please add the tests so that the expected behavior is clear - and we'll go from there. The first test should assert the expected nextLink for the second request.

It still sounds to me like updating the OrderBy method expression is the way to go - but tests first please.

[darjanbogdan]

I've added few tests, 3 of them are failing due to inconsistencies we discussed. I hope this will give you needed clarity what I'm trying to achieve with these changes.

It boils down to this example:

Test(await Get<CoreBuilding, TBuilding>("/corebuilding?$top=3", pageSize: 2));

void Test(ICollection<CoreBuilding> collection)
{
    Assert.Equal(2, collection.Count); // fails, collection.Count = 3
}

[darjanbogdan]

I've updated implementation to closely follow your recommendation, and tests are passing, so nextPageLink is working as expected! I was confused for some reason, when tested manually, as usual unit tests to the rescue :)

[BlaiseD]

Restoring some of the signatures. We want to match any previous callers. Didn't notice any code adding nextLink - should me similar to Total count for AspNetCore and AspNet.

Still reviewing.

[darjanbogdan]

Aha, code for nextLink doesn't have to be added as it's supported out of the box via extension method from MS OData library. If you check added test, you'll see the following: options.Request.GetNextPageLink(pageSize: 2);. That should be enough, right?

Thanks for the support in this!

[BlaiseD]

If [EnableQuery(PageSize=##)] automatically sets nextLink then I think we want the same behavior - defined based on current URL, $skip, $top and PageSize - unless there's a reason not to.

[darjanbogdan]

Good point, wasn't aware that's the default behavior of EnableQuery, will add the missing part.

[BlaiseD]

Looks good overall. Can you add the new tests to AutoMapper.OData.EF6.Tests.GetTests also?

Thanks for the PR. This feature has been requested a few times.

[BlaiseD]

• Need a double check on int? GetPageSize(TopQueryOption topQueryOption) - what happens with $top less than pageSize.
• Add corresponding tests for EF6.

[darjanbogdan]

• Logic for GetPageSize is adapted to support additional scenario. Unit tests for that logic are also added.
• Missing tests for EF6 are added.
• I have removed [Obsolete] attribute since it was only applied for GetAsync, you might want to consider decorating Get and GetQueryAsync methods as well. For now, I think it's not needed, otherwise we can still add? Maybe we can just remove those methods in the next major release?