Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

search-after pagination and ancestry membership implementation #397

Merged
merged 41 commits into from
Dec 13, 2023

Conversation

alexdunnjpl
Copy link
Contributor

DO NOT MERGE UPON PASSING REVIEW
This PR exists to separate out these changes from those in the membership-overhaul branch, in an attempt to make review less miserable.

🗒️ Summary

Replaces start/limit pagination with sort/search-after.

Instead of supplying qparams like ?start=100&limit=50, pagination is accomplished with qparams like ?sort=_id&search-after=lidvidOfLastDocumentOfPreviousPage

Breaks membership endpoints until #386 is merged into this branch

⚙️ Test Data and/or Report

Tests outstanding - need to be modified/written to reflect new pagination paradigm.

♻️ Related Issues

Fixes #352

@alexdunnjpl
Copy link
Contributor Author

alexdunnjpl commented Dec 5, 2023

Outstanding issues:

  • sort/search-after properties must use dot notation
  • summary must convert back to dot notation

… building queries.

This change is completed in later commit 4067428e and squashing is not possible
…entation GroupConstraintImpl.union()""

This reverts commit badc29b.
alexdunnjpl and others added 15 commits December 5, 2023 12:32
…ggregateProduct

this is more-correct, and necessary to allow inheritance of RefLogicNonAggregateProduct.memberOf()
…uct in the event that the name is unrecognized.

see comment for justification
This change completes earlier commit 10a57795
Replace remaining string PDS product ids with PdsProductIdentifier objects
@alexdunnjpl alexdunnjpl changed the title search-after pagination implementation search-after pagination and ancestry membership implementation Dec 5, 2023
@tloubrieu-jpl
Copy link
Member

retrieved 334440 products in 8.6 minutes in pages of 500 products. This fullfil the requirement on the page being returned in less that 1 second on the initial test case.

Integration test still need to run.

@jordanpadams
Copy link
Member

@alexdunnjpl do we know what is up with the regression tests and why they are failing?

@alexdunnjpl
Copy link
Contributor Author

@jordanpadams yep - unrelated to this PR.

/usr/local/bin/postman-waits-for-test-data.sh: line 49: syntax error: unterminated quoted string

Should be a simple fix

@jordanpadams
Copy link
Member

@alexdunnjpl just fixed that bug in the integration tests, but it is now failing for what looks like actual failures... https://github.com/NASA-PDS/registry-api/actions/runs/7107883089/job/19616830041

It looks like the integration tests are working on main, so I think we need to update some tests as part of this PR?

@jordanpadams
Copy link
Member

In the interest of the Sprint completion tomorrow. Going to merge this change and track test fixes here: #404

@jordanpadams jordanpadams merged commit 1351c17 into main Dec 13, 2023
1 of 2 checks passed
@jordanpadams jordanpadams deleted the search-after branch December 13, 2023 22:29
@alexdunnjpl
Copy link
Contributor Author

It looks like the integration tests are working on main, so I think we need to update some tests as part of this PR?

@jordanpadams do the integration tests actually run as part of the unstable I&D action? Looks like it's just roundup and build

@jordanpadams
Copy link
Member

@alexdunnjpl we probably need to update that since the branch CIVD does run the tests: https://github.com/NASA-PDS/registry-api/blob/main/.github/workflows/branch-cicd.yaml

@jordanpadams
Copy link
Member

@alexdunnjpl updated the unstable build to include this as well. It will still build and tag the docker image as expected, but it will fail the tests and turn up red if the tests fail.

https://github.com/NASA-PDS/registry-api/blob/main/.github/workflows/unstable-cicd.yaml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pagination performance does not meet requirements
3 participants