Skip to content

Paginate query responses and document assets#32

Open
ejanalysis wants to merge 7 commits into
mainfrom
codex-query-row-limit
Open

Paginate query responses and document assets#32
ejanalysis wants to merge 7 commits into
mainfrom
codex-query-row-limit

Conversation

@ejanalysis

@ejanalysis ejanalysis commented May 24, 2026

Copy link
Copy Markdown
Contributor

Implement pagination for /query responses instead of a fixed first-page cap. Adds page and limit parameters, bounds limit at 500 rows per page, returns a consistent results plus pagination envelope, validates invalid pagination inputs with HTTP 400 responses, and documents the response contract in the README.\n\nAlso keeps the static assets route documentation from the original PR.

@ejanalysis

Copy link
Copy Markdown
Contributor Author

/query needs either pagination, a limit on number of row returned, or a way to select fewer columns. Otherwise broad cutoffs like 0.80 can fail with a 500 code from response size

@ejanalysis

Copy link
Copy Markdown
Contributor Author

The limit does not have to be 100 -- change it if you think it should be higher, before merging.

@ejanalysis ejanalysis requested a review from ericnost May 24, 2026 12:22
@ericnost

Copy link
Copy Markdown
Contributor

Thanks for this @ejanalysis! Yes, the API needs to be more like an API.... I'm not sure capping at 100 or even 500 rows is a great solution. Pagination is, but it will take some development and testing. This, generated with the help of Gemini through a Google search, seems like the bones of what we want. We'd be adding a page parameter, then on each call to the endpoint, the API would do the whole calculation, returning the results corresponding to the requested page (e.g. if the limit is 100 per page, then page 2 would return results 101-200, or 100-199 depending on indexing).

#* Get a paginated list of items
#* @param page The page number (default 1)
#* @param limit Number of items per page (default 100)
#* @get /data
function(page = 1, limit = 100) {
  
  # Ensure parameters are numeric
  page  <- as.numeric(page)
  limit <- as.numeric(limit)
  
  # Calculate index range
  start_idx <- ((page - 1) * limit) + 1
  end_idx <- page * limit
  
  # Assuming 'my_dataset' is available in your R environment
  total_rows <- nrow(my_dataset)
  
  # Handle out-of-bounds pages gracefully
  if (start_idx > total_rows) {
    return(list(
      data = list(),
      metadata = list(
        current_page = page,
        total_items = total_rows,
        total_pages = ceiling(total_rows / limit)
      )
    ))
  }
  
  # Extract the requested slice of data
  paginated_data <- my_dataset[start_idx:min(end_idx, total_rows), ]
  
  # Return combined data and metadata
  list(
    data = paginated_data,
    metadata = list(
      current_page = page,
      limit = limit,
      total_items = total_rows,
      total_pages = ceiling(total_rows / limit)
    )
  )
}

This example has limit as a parameter, but I might just hard code it to something like 500. Or, leave it as a parameter, but make it no more than 500.

ejanalysis and others added 2 commits June 28, 2026 14:50
Bring the query row-limit branch up to date with main (handoff endpoints, CORS,
/assets mount, single-sourced base URL, Swagger @tag annotations).

Conflict resolution:
- rest_controller.r (/query roxygen): keep BOTH the row-limit description and
  main's `@tag Data` annotation; the row-limiting code (query_limit <- 100L)
  merged cleanly.
- README.md: take main's richer Model/Base-URLs intro (drops the stale
  "three endpoints" one-liner); keep this branch's new "## Query" section
  (documents the 100-row limit) plus main's Handoff/CORS/Assets; drop this
  branch's now-stale root-path "## Assets" (main moved assets to /assets/<name>).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ejanalysis

Copy link
Copy Markdown
Contributor Author

I just assigned the rewrite (per your comments) and testing of this to Codex, so let's see what it can come up with.

@ejanalysis ejanalysis changed the title Limit query responses and document assets Paginate query responses and document assets Jul 3, 2026
@ejanalysis ejanalysis requested a review from Copilot July 3, 2026 18:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the /query endpoint to support explicit pagination (page/limit) and returns a consistent { results, pagination } response envelope, with corresponding README documentation and new unit tests for the pagination helper/endpoint wrapper.

Changes:

  • Added page and limit parameters to /query, including input validation and HTTP 400 mapping for invalid pagination inputs.
  • Introduced query_pagination.R helpers (paginate_query_results(), query_endpoint_response()) and wired them into rest_controller.r.
  • Documented the paginated /query response contract in README.md and added testthat coverage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
tests/test-query-pagination.R Adds tests covering pagination behavior, validation, and 400 mapping for invalid inputs.
rest_controller.r Wires /query to the new pagination-aware response helper and documents new params.
README.md Documents /query pagination parameters and the {results, pagination} response envelope.
query_pagination.R Implements pagination helpers, input validation, and endpoint response assembly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread query_pagination.R
Comment thread query_pagination.R Outdated
Comment thread README.md
@ejanalysis

Copy link
Copy Markdown
Contributor Author

Addressed the remaining review comments on #32. Changes pushed in 1d8491f: added integer-overflow validation for pagination inputs, fixed empty-result pagination metadata, and imported pandas in the README query example. Verification: Rscript tests/test-query-pagination.R, parse check, and Plumber /query route smoke all passed.

— Codex

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

API needs to use Pagination to handle large responses from "query" endpoint (&/or "data" endpoint?)

3 participants