PhenoApps / Field-Book

https://fieldbook.phenoapps.org
GNU General Public License v2.0
51 stars 54 forks source link

[BUG] Last page not retrieved for recursive methods getPlotDetails() and getTraits() #352

Closed BrapiCoordinatorSelby closed 3 years ago

BrapiCoordinatorSelby commented 3 years ago

From @khaled-alshamaa in #344

Describe the bug Fix the condition to stop the BrAPI pagination recursive loop. It was not retrieving the last page in both getPlotDetails() and getTraits() methods in both classes BrAPIServiceV1 and BrAPIServiceV2!

To Reproduce Steps to reproduce the behavior: We tested version 5.0.7 of PhenoApps/Field-Book and found an issue in BrAPI pagination. When we imported a trial that has 2424 plots using BrAPI connection, we got a notification message that all 2424 plots has been imported successfully (I believe this value come from the getTotalPages() method in response). But when we switch to the collect page, we found only 2000 plots available (BrAPI page size kept 1000 as default). So, somehow, the last page was not retrieved.

Expected behavior The last page of data should be retrieved.

BrapiCoordinatorSelby commented 3 years ago

quoted from @BrapiCoordinatorSelby in #344

hi @khaled-alshamaa I was not able to reproduce your issue on the existing 5.0.7 code. Are there any more details you can provide? Were you able to step through the code, if so what were the values of page and response.getMetadata().getPagination().getTotalPages() in each step?

To reproduce the issue, I created a study on the BrAPI Test Server with 24 plots in it, and I changed the page size to 10. I was able to step through the existing 5.0.7 code and watch all 3 pages downloaded successfully. I'm concerned that there is some other issue going on, and the code you changed is not the root cause.

BrapiCoordinatorSelby commented 3 years ago

quoted from @khaled-alshamaa in #344

Dears @trife @BrapiCoordinatorSelby, I am sorry to be late in response to this, I just back from a short vacation.

Back to business, first, I am not a Java programmer! I did a kind of static code analysis to resolve this issue. But I make a mistake when I supposed that page variable (i.e., response.getMetadata().getPagination().getCurrentPage()) start counting from 1 while it starts from 0 as per BrAPI definition. Therefore, my updates did not fix the issue as I expected :-(

Regardless of this fact, the issue is real and exists. Whenever importing a new field book using BrAPI that has plots that exceed the API page size, the last page records/plots will not be available in the Collect section (even if the page size is a factor of the number of plots). Regardless of the import notification message that shows correctly the total number of plots (because it comes from a different source: response.getMetadata().getPagination().getTotalCount()).

khaled-alshamaa commented 3 years ago

It is always the last page! I tried it with a different study with 98 plots and page size 20, and I got only 80 plots in the collect section. For curiosity, I change the page size to 14 expecting to get 7 whole pages, but again, plots available in the collect section are only 84! So, it is simply the whole last page, whatever it is complete or just have a few records.

khaled-alshamaa commented 3 years ago

The server is BMS, and I tried with two different servers v18, and IBP test server v19.2 (https://www.bms-uat-test.net/) and got the same problem! Android Field-book setting used BrAPI v1. When I tried to query the data manually via BrAPI request to download the JSON as Peter asked for using the following URL (the total study plots are 98, page size = 40, and I call for all the 3 pages):

https:///brapi/v1/studies/{studyDbId}/observationunits?observationLevel=plot&page=[0,1, and 2]&pageSize=40

A quick check of these JSON files, I directly noticed that the problem is at the BMS side, not Android Field-Book. Sorry guys to waste your time on this issue!

Simply the first two pages (i.e., page 0 and page 1) are identical, and have the first 40 plots records, while the 3rd page (page 2) has the next 40 plots records (i.e., starting from plot 41 until plot 80). Therefore, it never retrieves the last page content properly, because someone missed up counting pages like 0, 1, and 2 vs. 1, 2, and 3 (which will return an error message that page number 3 is out of range).

I will contact the IBP developer team regarding this issue, and sorry again for wasting your time.

May you please close this ticket :-)

BrapiCoordinatorSelby commented 3 years ago

No worries @khaled-alshamaa, happy to help