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

Added support for paginating the JSONSearchServlet #1715

Merged
merged 2 commits into from Aug 2, 2017

Conversation

@mjpitz
Copy link

commented Aug 2, 2017

Right now, this search servlet doesn't support paginating results. This can be problematic for large code bases that have more then the hard configured limit of MAXRESULTS. Paginating will keep the endpoint performant and allow these larger companies to get at data easily.

@mjpitz

This comment has been minimized.

Copy link
Author

commented Aug 2, 2017

Submitted Signed OCA

@mjpitz mjpitz force-pushed the mjpitz:master branch from 939a219 to c20b917 Aug 2, 2017

@vladak

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2017

while you're at it, can use getIntParameter() for other parameters. Otherwise this looks good.

@mjpitz

This comment has been minimized.

Copy link
Author

commented Aug 2, 2017

Can do. I wanted to keep this change as minimal as possible.

@vladak

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2017

As long as the other changes are present as separate changesets that's fine, will merge them without squashing.

@mjpitz

This comment has been minimized.

Copy link
Author

commented Aug 2, 2017

Sounds good. I only see 1 other param in this servlet (maxResults).

The real value would be to apply the same method call across the project. That seems a bit out of scope for the course of this change. Want me to update the one other case in this class or do a separate ticket for the rest of the params?

@mjpitz

This comment has been minimized.

Copy link
Author

commented Aug 2, 2017

Applied to the one other param in the current servlet. I can go through the rest of the project in another PR

@vladak

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2017

That's what I had in mind, merging.

@vladak vladak merged commit 1856e12 into oracle:master Aug 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk No new vulnerabilities
Details

@tarzanek tarzanek added this to the 1.1 milestone Aug 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.