Add pagination and optional query params to config list endpoint#725
Open
Add pagination and optional query params to config list endpoint#725
Conversation
📝 WalkthroughWalkthroughAdded query-based filtering and "has_more" pagination metadata to config and document list flows; CRUD read methods now return (items, has_more); docs updated to document Changes
Sequence DiagramsequenceDiagram
participant Client
participant APIRoute
participant CRUD
participant DB as Database
Client->>APIRoute: GET /configs?query=prefix&skip=0&limit=100
APIRoute->>CRUD: read_all(query='prefix', skip=0, limit=100)
CRUD->>DB: SELECT ... WHERE name ILIKE 'prefix%' AND project_id=... AND deleted_at IS NULL LIMIT 101 OFFSET 0
DB-->>CRUD: rows (up to 101)
CRUD->>CRUD: truncate to 100 if > limit\nset has_more = (len > limit)
CRUD-->>APIRoute: (configs[:limit], has_more)
APIRoute->>Client: 200 { data: [...], metadata: { has_more: true/false } }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/crud/document/document.py (1)
36-77:⚠️ Potential issue | 🔴 CriticalCritical: Return type change breaks all existing tests that expect a list.
The return type change from
list[Document]totuple[list[Document], bool]causes failures in test code:
backend/app/tests/crud/documents/documents/test_crud_document_read_many.py(lines 27–114): Multiple assertions fail—len(docs)returns 2 instead of the actual document count, andall(x.is_deleted is False for x in crud.read_many())attempts to iterate over(list, bool), causing a TypeError.backend/app/tests/crud/documents/documents/test_crud_document_update.py(lines 21–25):len(before)andlen(after)will both return 2.All test assertions must unpack the tuple:
docs, has_more = crud.read_many(...)instead of treating the result as a list. The API route atbackend/app/api/routes/documents.py:84is already correctly unpacking the tuple.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/crud/document/document.py` around lines 36 - 77, The read_many function now returns (list[Document], bool) but tests (and some callers) still treat its return as a plain list; update every callsite and test to unpack the tuple (e.g., docs, has_more = crud.read_many(...)) and then use docs for length/iteration and has_more for pagination assertions; search for usages of read_many in tests like the ones in test_crud_document_read_many and test_crud_document_update and replace direct iterations/len(calls) with the unpacked docs variable accordingly.
🧹 Nitpick comments (3)
backend/app/api/routes/documents.py (1)
97-97: Prefer dict literal overdict()call.Static analysis (Ruff C408) flags this as unnecessary. Use a dict literal for consistency and minor performance improvement.
Suggested fix
- return APIResponse.success_response(results, metadata=dict(has_more=has_more)) + return APIResponse.success_response(results, metadata={"has_more": has_more})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/documents.py` at line 97, Replace the dict() call used for the metadata argument with a dict literal to satisfy Ruff C408: in the return that calls APIResponse.success_response(results, metadata=...), change the metadata from dict(has_more=has_more) to a literal mapping with the key "has_more" and the has_more variable as its value; ensure you update the expression where results and has_more are passed to APIResponse.success_response.backend/app/api/routes/config/config.py (1)
66-69: Use a literal for metadata instead ofdict(...).This avoids Ruff C408 and is more idiomatic.
♻️ Suggested cleanup
return APIResponse.success_response( data=configs, - metadata=dict(has_more=has_more) + metadata={"has_more": has_more} )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/config/config.py` around lines 66 - 69, Replace the use of dict(...) when calling APIResponse.success_response with a literal dict: change metadata=dict(has_more=has_more) to use a mapping literal (e.g. metadata={"has_more": has_more}) in the return that invokes APIResponse.success_response so the call (return APIResponse.success_response(data=configs, metadata=...)) uses a literal and avoids Ruff C408; update the metadata argument where it appears in this function.backend/app/crud/config/config.py (1)
106-108: RedundantNoneguard onlimitcan be removed.Given
limit: intin this method signature,limit is not Noneis unnecessary here.🧹 Suggested simplification
- if limit is not None and len(configs) > limit: + if len(configs) > limit: has_more = True configs = configs[:limit]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/crud/config/config.py` around lines 106 - 108, The conditional check redundantly tests "limit is not None" even though the function signature declares limit: int; update the block in the function that manipulates configs (the code using variables limit, configs, has_more) to simply check "if len(configs) > limit:" then set has_more = True and trim configs = configs[:limit]; remove the unnecessary None guard so the logic uses the integer limit directly while keeping has_more and slicing behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/docs/config/list.md`:
- Line 1: Update the docs to state that the query performs a prefix match
against Config.name (i.e., it uses ilike(f"{query}%")) rather than a general
substring search; explicitly mention "prefix match on Config.name" and give an
example like searching "env" will match "env-prod" but not "my-env" to prevent
client confusion.
In `@backend/app/api/routes/config/config.py`:
- Around line 53-59: The function list_configs is missing an explicit return
type; update its signature to include a return annotation that matches the
actual returned value (for example a list of your config schema or a FastAPI
response model), e.g. annotate list_configs with the correct return type after
the parameter list (keeping current_user: AuthContextDep and session: SessionDep
intact); ensure the chosen type (e.g., list[ConfigSchema], ConfigListResponse,
or Response) matches the implementation and imported types so type checking
passes.
---
Outside diff comments:
In `@backend/app/crud/document/document.py`:
- Around line 36-77: The read_many function now returns (list[Document], bool)
but tests (and some callers) still treat its return as a plain list; update
every callsite and test to unpack the tuple (e.g., docs, has_more =
crud.read_many(...)) and then use docs for length/iteration and has_more for
pagination assertions; search for usages of read_many in tests like the ones in
test_crud_document_read_many and test_crud_document_update and replace direct
iterations/len(calls) with the unpacked docs variable accordingly.
---
Nitpick comments:
In `@backend/app/api/routes/config/config.py`:
- Around line 66-69: Replace the use of dict(...) when calling
APIResponse.success_response with a literal dict: change
metadata=dict(has_more=has_more) to use a mapping literal (e.g.
metadata={"has_more": has_more}) in the return that invokes
APIResponse.success_response so the call (return
APIResponse.success_response(data=configs, metadata=...)) uses a literal and
avoids Ruff C408; update the metadata argument where it appears in this
function.
In `@backend/app/api/routes/documents.py`:
- Line 97: Replace the dict() call used for the metadata argument with a dict
literal to satisfy Ruff C408: in the return that calls
APIResponse.success_response(results, metadata=...), change the metadata from
dict(has_more=has_more) to a literal mapping with the key "has_more" and the
has_more variable as its value; ensure you update the expression where results
and has_more are passed to APIResponse.success_response.
In `@backend/app/crud/config/config.py`:
- Around line 106-108: The conditional check redundantly tests "limit is not
None" even though the function signature declares limit: int; update the block
in the function that manipulates configs (the code using variables limit,
configs, has_more) to simply check "if len(configs) > limit:" then set has_more
= True and trim configs = configs[:limit]; remove the unnecessary None guard so
the logic uses the integer limit directly while keeping has_more and slicing
behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c47e6852-6170-4dd0-833a-d4e8974c31e2
📒 Files selected for processing (5)
backend/app/api/docs/config/list.mdbackend/app/api/routes/config/config.pybackend/app/api/routes/documents.pybackend/app/crud/config/config.pybackend/app/crud/document/document.py
| @@ -1,3 +1,7 @@ | |||
| query: Search string used for partial matching across configurations | |||
There was a problem hiding this comment.
Clarify query matching semantics in the docs.
Current wording suggests broad partial matching, but the implementation applies a prefix match on Config.name (ilike(f"{query}%")). Please make this explicit to avoid client-side confusion.
✏️ Suggested doc tweak
-query: Search string used for partial matching across configurations
+query: Optional prefix search string for configuration names (case-insensitive)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| query: Search string used for partial matching across configurations | |
| query: Optional prefix search string for configuration names (case-insensitive) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/docs/config/list.md` at line 1, Update the docs to state that
the query performs a prefix match against Config.name (i.e., it uses
ilike(f"{query}%")) rather than a general substring search; explicitly mention
"prefix match on Config.name" and give an example like searching "env" will
match "env-prod" but not "my-env" to prevent client confusion.
…as_more functionality
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Ayush8923
approved these changes
Mar 30, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/app/tests/crud/documents/documents/test_crud_document_read_many.py (1)
78-96: Consider adding tests forhas_morepagination behavior.Since this PR adds the
has_moreflag and this file is the primary test suite forread_many(), it would be valuable to add tests verifying thehas_morelogic:
has_more=Truewhen more records exist beyond the limithas_more=Falsewhen all records fit within the limithas_more=Falsewhenlimit=0Example test:
def test_has_more_true_when_records_exceed_limit( self, db: Session, store: DocumentStore, ) -> None: crud = DocumentCrud(db, store.project.id) limit = self._ndocs // 2 # Request fewer than total docs, has_more = crud.read_many(limit=limit) assert len(docs) == limit assert has_more is True def test_has_more_false_when_all_records_returned( self, db: Session, store: DocumentStore, ) -> None: crud = DocumentCrud(db, store.project.id) docs, has_more = crud.read_many(limit=self._ndocs + 10) assert len(docs) == self._ndocs assert has_more is False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/crud/documents/documents/test_crud_document_read_many.py` around lines 78 - 96, Add tests in test_crud_document_read_many.py that assert the new has_more pagination flag returned by DocumentCrud.read_many: create a test (e.g., test_has_more_true_when_records_exceed_limit) that calls DocumentCrud(db, store.project.id).read_many(limit=<fewer than total, e.g., self._ndocs // 2>) and asserts len(docs) == limit and has_more is True; create a test (e.g., test_has_more_false_when_all_records_returned) that calls read_many with limit larger than total (e.g., self._ndocs + 10) and asserts len(docs) == self._ndocs and has_more is False; and add a test (e.g., test_has_more_false_when_limit_zero) that calls read_many(limit=0) and asserts not docs and has_more is False. Ensure you use DocumentCrud and the existing db and store fixtures so tests mirror the style of existing tests.backend/app/api/routes/config/config.py (1)
66-66: Prefer dict literal for metadata construction.At line 66,
dict(has_more=has_more)should be replaced with a dictionary literal to comply with Ruff C408.Suggested patch
- return APIResponse.success_response(data=configs, metadata=dict(has_more=has_more)) + return APIResponse.success_response(data=configs, metadata={"has_more": has_more})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/config/config.py` at line 66, The return uses dict(has_more=has_more) which triggers Ruff C408; replace that call with a dictionary literal so APIResponse.success_response(data=configs, metadata={'has_more': has_more}) is used instead, locating the change in the return statement that calls APIResponse.success_response with data=configs and metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/tests/api/routes/configs/test_config.py`:
- Around line 477-481: The test currently checks prefix filtering with a
substring assertion; update the assertion that inspects returned_names (the list
comprehension variable `returned_names = [c["name"] for c in data["data"]]`) so
it uses startswith semantics instead of substring containment—replace the
`all("search" in name for name in returned_names)` check with an assertion that
every name starts with "search" (e.g., use `all(name.startswith("search") for
name in returned_names)`) to match the prefix filtering implemented in the CRUD
logic (`ilike(f"{query}%")`).
---
Nitpick comments:
In `@backend/app/api/routes/config/config.py`:
- Line 66: The return uses dict(has_more=has_more) which triggers Ruff C408;
replace that call with a dictionary literal so
APIResponse.success_response(data=configs, metadata={'has_more': has_more}) is
used instead, locating the change in the return statement that calls
APIResponse.success_response with data=configs and metadata.
In `@backend/app/tests/crud/documents/documents/test_crud_document_read_many.py`:
- Around line 78-96: Add tests in test_crud_document_read_many.py that assert
the new has_more pagination flag returned by DocumentCrud.read_many: create a
test (e.g., test_has_more_true_when_records_exceed_limit) that calls
DocumentCrud(db, store.project.id).read_many(limit=<fewer than total, e.g.,
self._ndocs // 2>) and asserts len(docs) == limit and has_more is True; create a
test (e.g., test_has_more_false_when_all_records_returned) that calls read_many
with limit larger than total (e.g., self._ndocs + 10) and asserts len(docs) ==
self._ndocs and has_more is False; and add a test (e.g.,
test_has_more_false_when_limit_zero) that calls read_many(limit=0) and asserts
not docs and has_more is False. Ensure you use DocumentCrud and the existing db
and store fixtures so tests mirror the style of existing tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d26a7699-fc3f-4cca-8679-10b3e575aec3
📒 Files selected for processing (6)
backend/app/api/routes/config/config.pybackend/app/crud/config/config.pybackend/app/tests/api/routes/configs/test_config.pybackend/app/tests/crud/config/test_config.pybackend/app/tests/crud/documents/documents/test_crud_document_read_many.pybackend/app/tests/crud/documents/documents/test_crud_document_update.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/crud/config/config.py
Comment on lines
+477
to
+481
| returned_names = [c["name"] for c in data["data"]] | ||
| assert "search-alpha" in returned_names | ||
| assert "search-beta" in returned_names | ||
| assert all("search" in name for name in returned_names) | ||
| assert "other-config" not in returned_names |
There was a problem hiding this comment.
Tighten filter assertion to match prefix semantics.
At Line [480], all("search" in name ...) validates substring behavior, but backend/app/crud/config/config.py applies prefix filtering (ilike(f"{query}%")). Assert startswith to prevent false positives.
Suggested patch
- assert all("search" in name for name in returned_names)
+ assert all(name.lower().startswith("search") for name in returned_names)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| returned_names = [c["name"] for c in data["data"]] | |
| assert "search-alpha" in returned_names | |
| assert "search-beta" in returned_names | |
| assert all("search" in name for name in returned_names) | |
| assert "other-config" not in returned_names | |
| returned_names = [c["name"] for c in data["data"]] | |
| assert "search-alpha" in returned_names | |
| assert "search-beta" in returned_names | |
| assert all(name.lower().startswith("search") for name in returned_names) | |
| assert "other-config" not in returned_names |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/tests/api/routes/configs/test_config.py` around lines 477 - 481,
The test currently checks prefix filtering with a substring assertion; update
the assertion that inspects returned_names (the list comprehension variable
`returned_names = [c["name"] for c in data["data"]]`) so it uses startswith
semantics instead of substring containment—replace the `all("search" in name for
name in returned_names)` check with an assertion that every name starts with
"search" (e.g., use `all(name.startswith("search") for name in returned_names)`)
to match the prefix filtering implemented in the CRUD logic
(`ilike(f"{query}%")`).
Ayush8923
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue: #732
Summary
This PR adds an optional query param for name-based search and a has_more flag in the response metadata, enabling efficient server-side filtering and accurate pagination.
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
New Features
Documentation
Tests