This repository has been archived by the owner. It is now read-only.

Bug in Search Result Paging #535

Closed
toweryb opened this Issue Jul 17, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@toweryb

toweryb commented Jul 17, 2017

Thank you for reporting an issue, suggesting an enhancement, or asking a question. We appreciate your feedback - to help the team understand your
needs please complete the below template to ensure we have the details to help. Thanks!

Category

[ ] Enhancement

[X ] Bug

[ ] Question

Version

Please specify what version of the library you are using: [ 2.0.7 ]

Expected / Desired Behavior / Question

Using pnp.sp.search with a search query, start row, and row limit to get a SearchResults object. When I use getPage(pageNumber) on the object, I should get the proper items within the search results.

Observed Behavior

Upon using getPage(1), which should return the second page, the last item from the first page is repeated as the first item on the second page. Using getPage(0), which should return the first page throws an error with the SharePoint postQuery post as the StartRow sent to SharePoint is -1. I believe the following line of code in the getPage method (in search.ts):
StartRow: rows * (pageNumber - 1) + 1,
Should instead be:
StartRow: rows * pageNumber,
as the pages and rows are zero based.

Steps to Reproduce

Create a search with pnp.sp.search using a start row of 0 and row limit of 2 to get the SearchResults object. Use getPage(1) on the SearchResults object, to see 2nd item repeated, then use getPage(0) to get the postQuery error via SharePoint.

Thanks!

@patrick-rodgers

This comment has been minimized.

Show comment
Hide comment
@patrick-rodgers

patrick-rodgers Jul 20, 2017

Contributor

What environment are you testing this in? On-premises 2013/2016 or O365? As I understand it rows are 1 based not zero based, i.e. 1 should be the first row and asking for row zero would be incorrect. So perhaps we are just off by 1.

Contributor

patrick-rodgers commented Jul 20, 2017

What environment are you testing this in? On-premises 2013/2016 or O365? As I understand it rows are 1 based not zero based, i.e. 1 should be the first row and asking for row zero would be incorrect. So perhaps we are just off by 1.

@toweryb

This comment has been minimized.

Show comment
Hide comment
@toweryb

toweryb Jul 20, 2017

Using O365. If I manually set the StartRow via the query to 0, it returns the first row. If I set StartRow to 1, it returns the second row. That leads me to believe that everything is 0 based.

toweryb commented Jul 20, 2017

Using O365. If I manually set the StartRow via the query to 0, it returns the first row. If I set StartRow to 1, it returns the second row. That leads me to believe that everything is 0 based.

@patrick-rodgers

This comment has been minimized.

Show comment
Hide comment
@patrick-rodgers

patrick-rodgers Jul 21, 2017

Contributor

Hmmm, ok, I'll have to play with it. In my initial testing I thought it was 1 based and that seemed to work - but maybe I got it wrong. Thanks for reporting the issue.

Contributor

patrick-rodgers commented Jul 21, 2017

Hmmm, ok, I'll have to play with it. In my initial testing I thought it was 1 based and that seemed to work - but maybe I got it wrong. Thanks for reporting the issue.

@patrick-rodgers

This comment has been minimized.

Show comment
Hide comment
@patrick-rodgers

patrick-rodgers Aug 1, 2017

Contributor

The getPage method is one based. Meaning it starts at page 1. This is different than the startRow (zero based). I didn't say clearly what I was thinking above and used "row" where I meant page. Pages are 1 based, meaning you start at page 1 and move up. Page 0 in the way it is implemented is incorrect.

So start row 0 should == getPage(1) - and it does in my testing.

I don't see the duplication of search results in my testing.

Thoughts?

Contributor

patrick-rodgers commented Aug 1, 2017

The getPage method is one based. Meaning it starts at page 1. This is different than the startRow (zero based). I didn't say clearly what I was thinking above and used "row" where I meant page. Pages are 1 based, meaning you start at page 1 and move up. Page 0 in the way it is implemented is incorrect.

So start row 0 should == getPage(1) - and it does in my testing.

I don't see the duplication of search results in my testing.

Thoughts?

@toweryb

This comment has been minimized.

Show comment
Hide comment
@toweryb

toweryb Aug 2, 2017

OK. I changed my code to reflect your comments about getPage being one based and I am still not seeing what I expect.

My search query is sorted by a date field, so I would expect items to be sorted when I get values from multiple pages. When I run my query with no RowLimit to retrieve all items (I have 6 items), I get them back in the order I expect like the following:
A, B, C, D, E, F (using letters to designate the items).

So, if I then run the same query with a RowLimit of 2 and StartRow of 0, I get (as expected):
A, B

Then, I ask for page 2 (by calling getPage(2)), the PostQuery to SharePoint sends RowLimit: 2 and StartRow: 3 (which should actually be 2, since the rows are 0 based). This gives me:
D, E
This is not what I would expect (I would expect C, D).

If I then ask for page 1 (by calling getPage(1)), the PostQuery to SharePoint sends RowLimit: 2 and Start Row: 1 (which should actually be 0, since the rows are 0 based).

The problem appears to be in the following line of code of the getPage method in search.ts (as I mentioned in my initial post):
StartRow: rows * (pageNumber - 1) + 1

When I call getPage(1) with a RowLimit of 2, this value ends up as 1 -> 2 * (1 - 1) + 1 instead of 0. When I call getPage(2) with a RowLimit of 2, this value ends up at 2 -> 2 * (2 - 1) + 1 instead of 2. This code seems to be expecting the StartRow to be 1 based rather than 0 based (as you stated it is above).

Since the pages are 1 based, I feel this code should be:
rows * (pageNumber - 1)
This change would give the proper expected StartRow for each entry.

toweryb commented Aug 2, 2017

OK. I changed my code to reflect your comments about getPage being one based and I am still not seeing what I expect.

My search query is sorted by a date field, so I would expect items to be sorted when I get values from multiple pages. When I run my query with no RowLimit to retrieve all items (I have 6 items), I get them back in the order I expect like the following:
A, B, C, D, E, F (using letters to designate the items).

So, if I then run the same query with a RowLimit of 2 and StartRow of 0, I get (as expected):
A, B

Then, I ask for page 2 (by calling getPage(2)), the PostQuery to SharePoint sends RowLimit: 2 and StartRow: 3 (which should actually be 2, since the rows are 0 based). This gives me:
D, E
This is not what I would expect (I would expect C, D).

If I then ask for page 1 (by calling getPage(1)), the PostQuery to SharePoint sends RowLimit: 2 and Start Row: 1 (which should actually be 0, since the rows are 0 based).

The problem appears to be in the following line of code of the getPage method in search.ts (as I mentioned in my initial post):
StartRow: rows * (pageNumber - 1) + 1

When I call getPage(1) with a RowLimit of 2, this value ends up as 1 -> 2 * (1 - 1) + 1 instead of 0. When I call getPage(2) with a RowLimit of 2, this value ends up at 2 -> 2 * (2 - 1) + 1 instead of 2. This code seems to be expecting the StartRow to be 1 based rather than 0 based (as you stated it is above).

Since the pages are 1 based, I feel this code should be:
rows * (pageNumber - 1)
This change would give the proper expected StartRow for each entry.

@patrick-rodgers

This comment has been minimized.

Show comment
Hide comment
@patrick-rodgers

patrick-rodgers Aug 15, 2017

Contributor

Thanks for your analysis. I've made your suggested change in PR #561. Thanks!

Contributor

patrick-rodgers commented Aug 15, 2017

Thanks for your analysis. I've made your suggested change in PR #561. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.