bgmulinari / B1SLayer

A lightweight SAP Business One Service Layer client for .NET
MIT License
136 stars 47 forks source link

Skip not functioning properly? #34

Closed grantsanders closed 1 year ago

grantsanders commented 1 year ago
    var response = await _serviceLayer
        .Request("Items")
        .Select("ItemCode,ItemName,Mainsupplier,SupplierCatalogNo")
        .Skip(numberToSkip)
        .WithPageSize(2500)
        .GetAsync<IEnumerable<Item>>();

Here's my code- Not sure what the deal is here, could be an issue on my side, but when I use the skip method, everything past my page size gets truncated. For example, if numberToSkip here was equal to 2499, I would only recieve one object, and if numberToSkip was 2500+, I would recieve nothing.

For the time being I'm using this workaround:

    var response = await _serviceLayer
        .Request("Items")
        .Select("ItemCode,ItemName,Mainsupplier,SupplierCatalogNo")
        .Skip(numberToSkip)
        .WithPageSize(numberToSkip + 2500)   // 2500 here is my desired page size
        .GetAsync<IEnumerable<Item>>();

To just resize the page incrementally based on the number of objects to be skipped.

Thanks for your time, love this library

bgmulinari commented 1 year ago

Hi, @grantsanders. I'm not sure I understand your problem. How many Items do you have in total in your database? If your skip number is greater than the total number of records, it's normal for the request to return an empty result, since you're skipping more than what you have in the table.

In your code I also didn't understand why you are adding numberToSkip + 2500 in the WithPageSize() method.

Keep in mind:

WithPageSize: maximum number of records to be returned per request Skip: first n records to be excluded from the result

I did a test in my database, where I only have 54 items in total, and here's the result:

int numberToSkip = 50;

var items = await serviceLayer
    .Request("Items")
    .Select("ItemCode,ItemName,Mainsupplier,SupplierCatalogNo")
    .Skip(numberToSkip)
    .WithPageSize(2500)
    .GetAsync();

// Only 4 items returned, as expected

Also, if you are doing this to get all items from your database, you can do this more easily with the GetAllAsync() method.

grantsanders commented 1 year ago

Thanks for the quick reply!

In this particular case, the database contains over 80,000 records. I guess I expected B1S to function similarly to how requests do directly in the service layer- if you make a call in postman, for example, and specify that you want 2500 records, but also specify that you want to skip 2500 records, you would receive records 2501-5000 in the database. I'm adding to WithPageSize dynamically to achieve similar behavior with B1SLayer.

I guess my use case is peculiar though- I'm being tasked with essentially making a wrapper for the service layer, and I'm trying to mimic its behavior as much as I can. The GetAllAsync method would work in this case, but our client specifically requested for us to batch the data.

bgmulinari commented 1 year ago

I see. It should work exactly like you described and the result with B1SLayer should be the same as what you see in Postman, provided all the request parameters are the same.

Here you can see my request above made with B1SLayer captured with Fiddler Classic:

image

Did you test your request in Postman with the same request parameters and got a different result? If so, we could compare the resulting requests with Fiddler to understand what's happening.

grantsanders commented 1 year ago

I believe so- My thinking was that the WithPageSize method would set the Prefer header on the request, is that the case? If so then the requests (or at least, as far as I'm aware) were identical. Not sure how B1SLayer handles that under the hood?

Let me show an example response in postman:

https://app.screencast.com/oMOusComNmkmx

This is a call to my API, where the Items controller is hitting this method:

    [EnableQuery]
    [HttpGet(Name = "GetItems")]
    public async Task<IEnumerable<Item>> Get(ODataQueryOptions options)
    {

        int numberToSkip = options.Skip?.Value ?? 0;

        var itemTotal = await _serviceLayer
            .Request("Items")
            .GetCountAsync();

        var response = await _serviceLayer
            .Request("Items")
            .Select("ItemCode,ItemName,Mainsupplier,SupplierCatalogNo")
            .Skip(numberToSkip)
            .WithPageSize(2500)
            .GetAsync<IEnumerable<Item>>();

        return response;
    }

Here is a call directly to the service layter, you can see the header I'm sending for page size here.

https://app.screencast.com/sXxQiqH1Jv2fH

The call directly to the service layer returns 2500 items, starting from position 2499 as determined by the skip header. My api call, however, returns 1.

It seems like WithPageSize is specifying the amount of results to return, and then skip is applied afterwards, while in the service layer these operations happen in reverse- the starting position is determined by the skip value and then the maxpageresults applies afterwards.

Is this intended behavior of the MaxPageResults method?

Again, appreciate your prompt replies!

bgmulinari commented 1 year ago

The WithPageSize() method applies the B1S-PageSize header value to the request, as you can see in my image in the previous comment, which is mentioned in the Service Layer user manual. I'm guessing that B1SLayer should be using odata.maxpagesize like you instead of B1S-PageSize.

I see that you're using OData v4 (b1s/v2 in the URL instead of b1s/v1), which I think it could be related to this odd behaviour with the B1S-PageSize header parameter. Can you test using your current code using OData v3 (b1s/v1)?

Anyway, the code bellow should fix the issue for now:

var response = await _serviceLayer
    .Request("Items")
    .Select("ItemCode,ItemName,Mainsupplier,SupplierCatalogNo")
    .Skip(numberToSkip)
    .WithHeader("Prefer", "odata.maxpagesize=2500")
    .GetAsync<IEnumerable<Item>>();
grantsanders commented 1 year ago

The plot thickens- when I use that header, I get the same result as when I just use WithPageSize. Is it possible that B1Slayer is truncating the extra results? Currently I'm sticking to my hacky method from earlier where I'm adding to the variable I use for the WithPageSize method for every call.

I'll try running odata v3 and get back to you

Update: Specifying Odata v3 does not change behavior

bgmulinari commented 1 year ago

This is very odd, @grantsanders. I don't believe it's possible for B1SLayer to be truncating the results since it simply does a deserialization with the given request response.

Can you use Fiddler Classic like I showed previously to compare the requests from B1SLayer and Postman? Fiddler is a tool that captures HTTP requests being made locally and logs them, so it's very useful for this kind of scenario.

If in Postman and B1SLayer the request parameters are the same (query string and headers), the resulting request and its response captured in Fiddler should also be the same.

grantsanders commented 1 year ago

Having trouble configuring fiddler with my VPN- Will get back to this later tonight when I have more time

grantsanders commented 1 year ago

Well, I've found the solution- It was definitely my mistake:

In my API controller, I was passing the Skip property from the request URL into a variable (numToSkip) to be used by B1SLayer. What I did not realize was that by passing the Skip property in the URL, with the name Skip, it actually executes that OData filter on the entire request- so the skip filter was passing correctly through to B1SLayer, and returning the correct data- but then I was actually truncating it myself by skipping through THOSE results.

I've fixed the issue by just renaming the query variable from "skip" in the request to "skipTo". Works flawlessly now.

Thanks again for your prompt replies! Appreciate you and this library.

bgmulinari commented 1 year ago

Thanks for the clarification, @grantsanders. I thought this could be an issue with the B1S-PageSize header parameter, but I guess it works fine then and I don't need to change it.