SharePoint / PnP-JS-Core

Code moved to https://github.com/pnp/pnpjs. This repository is archived.
Other
379 stars 231 forks source link

Skip operation is useless when using it with orderBy #757

Closed babkenmes closed 6 years ago

babkenmes commented 6 years ago

Category

[ ] Enhancement

[x] Bug

[ ] Question

Version

[3.0.1]

In your wiki page there is written that

The library also supports the OData operations select and expand for instances (such as a list or an item) as well we the select, filter, expand, orderBy, skip, and top operations for collections (collection of lists, or items).

Link

I assume that support OData operations means support what OData documentation says. If it is wrong assumption you can skip (really skip) the OData documentation part.

Odata documentation says

A data service URI with a $skip System Query Option identifies a subset of the Entries in the Collection of Entries identified by the Resource Path section of the URI. That subset is defined by seeking N Entries into the Collection and selecting only the remaining Entries (starting with Entry N+1). N is an integer greater than or equal to zero specified by this query option. If a value less than zero is specified, the URI should be considered malformed.

In an example it also shown clearly that items are skipped from collection when they are sorted. Link

It turns out that you skip not by the position of the entries but by id.

With this it would be fine if skipping was done after orderBy but it does not even respect orderBy it just skips from Id of an item. Which makes useless skip operation when one needs to sort something.

example We have items Id: 1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19

pnp.sp.crossDomainWeb(addinweb,hostweb).lists.getByTitle("Cases").items.orderBy("Created",false).top(2).skip(15).get()

Will return 19 and 18 items. We can not iterate through the list using skip and orderBy operations at the same time.

koltyakov commented 6 years ago

Unfortunately, AFAIK, $skiptoken works only for the combination of ordering by ID. This is REST API issue and therefore something which can't be fixed within the library.

patrick-rodgers commented 6 years ago

As @koltyakov says, this is how the underlying SharePoint rest API works. skip expects an item id.

You can iterate through the list using the getPaged method that will respect orderby as it gets the skip token that will have the correct next item id in it. Perhaps this will help you?

const page1 = await pnp.sp.web.lists.getByTitle("OrderByList").items.orderBy("Title").top(10).select("ID", "Title").getPaged();

const page2 = await page1.getNext();
koltyakov commented 6 years ago

With getPaged, there are also situations when paged data collection will be incorrect. It's not the library's issue as REST API bug.

The issue can be easily reproduced if to order by a field with non-unique values.

Example:

image

(21 item in the list).

Then requesting with pagination, top/10:

image

nextPage forms a skip token with the last value of the ordered field(s), i.e.:

$skiptoken=Paged=TRUE&p_Title=HEK&p_ID=5&$orderby=Title+asc&$top=10

So, if a non-unique value is in the last item in a paged collection, next paged collection will be incorrect.

That's what I was able to find out. Not sure if there is a uservoice for fixing this bug.

koltyakov commented 6 years ago

An interesting thing, if to add orderBy('Id') at the end of the ordering conditions looks like nextPage starts working well. Oh, maybe is a real workaround for this.

const items = pnp.sp.web.lists.getByTitle('Suppliers').items;

items.select('Id,Title').orderBy('Title').orderBy('Id').top(10)
    .getPaged()
    .then(r => {
        console.log(r.results.map(r => `${r.Id}-${r.Title}`));
        return r.getNext();
    })
    .then(r => {
        console.log(r.results.map(r => `${r.Id}-${r.Title}`));
        return r.getNext();
    })
    .then(r => {
        console.log(r.results.map(r => `${r.Id}-${r.Title}`));
        return r.getNext();
    });

image