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

request pagination #5207

Closed
wants to merge 5 commits into from
Closed

Conversation

gbrvalerio
Copy link

@gbrvalerio gbrvalerio commented Nov 9, 2023

  • This draft PR creates the basis for implementing Pagination in storage requests

Tasks

Core

  • Create Pagination class
  • Adapt Request class to include pagination
  • Adapt DatabaseStorage to handle the request pagination
  • Remove the limit prop from the Order class
  • Fix current usage of the Request class to use Pagination

Implementation

  • Add skip and limit url params to the list devices endpoint
    • Update Docs
    • Update query
  • Add skip and limit url params to the list users endpoint
    • Update Docs
    • Update query
  • Add skip and limit url params to the base class SimpleObjectResource (affects Groups, Calendars, Orders)
    • Update Docs
    • Update query
  • Add skip and limit url params to the list drivers endpoint
    • Update Docs
    • Update query

Databases Tested

  • Postgres
  • H2
  • MS SQL

@tananaev

Comment on lines 319 to 331
if(pagination.getSkip() > 0) {
result.append(" OFFSET ");
result.append(pagination.getSkip());
}
if (pagination.getLimit() > 0) {
if (databaseType.equals("Microsoft SQL Server")) {
result.append(" OFFSET 0 ROWS FETCH FIRST ");
result.append(order.getLimit());
result.append(pagination.getLimit());
result.append(" ROWS ONLY");
} else {
result.append(" LIMIT ");
result.append(order.getLimit());
result.append(pagination.getLimit());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which databases have you tested?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PG so far

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to test at least H2, MySQL, Postgres and MS SQL. Is there an SQL standard for it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code wise, the path is correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only did a brief look, but I think it makes sense. I would separate database changes and report changes though.

@gbrvalerio
Copy link
Author

@tananaev regarding docs: is it automatically updated or should I do some manual work here? If so, can you point me where should I pay attention and update?

ty

@tananaev
Copy link
Member

What docs do you need to update?

@gbrvalerio
Copy link
Author

the api ones to include the new skip and limit url params in the endpoints

@tananaev
Copy link
Member

Well, we need to split the PR first.

@gbrvalerio
Copy link
Author

closed in favor of #5208 and #5209

@gbrvalerio gbrvalerio closed this Nov 10, 2023
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.

None yet

2 participants