fix: Add filter_id support and v1 filter resolution#1666
Open
edwinjosechittilappilly wants to merge 11 commits into
Open
fix: Add filter_id support and v1 filter resolution#1666edwinjosechittilappilly wants to merge 11 commits into
edwinjosechittilappilly wants to merge 11 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds filter_id support for document deletion and result scoping across Python/TypeScript SDKs and FastAPI endpoints, implements a shared resolve_filter_id helper, updates chat/search/documents endpoints to use resolved filters, adjusts MCP exposure, and expands integration tests validating filter scoping and deletion behavior. ChangesFilter-based document deletion and filtering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
github-actions
Bot
added
backend
github-actions
Bot
added
bug
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class filter_id support across the v1 API surface (chat/search/documents) by resolving knowledge filters server-side, extends document deletion to support deleting by filter_id, and updates both SDKs + integration tests to exercise the new behavior.
Changes:
- Introduce
api.v1._filter_resolution.resolve_filter_id()and wirefilter_idinto v1 chat/search/documents behavior (including wildcard stripping and override rules). - Extend
DELETE /v1/documents+ Python/TypeScript SDKs to support deleting by eitherfilenameorfilter_id(mutually exclusive), returning richer deletion metadata. - Expand SDK integration tests (Python + TypeScript) to validate that
filter_idtruly scopes retrieval/deletion behavior (including streaming + not-found + validation cases).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/sdk/test_filters.py | Adds end-to-end filter scoping tests for chat/search (including streaming and not-found). |
| tests/integration/sdk/test_documents.py | Adds delete-by-filter_id integration tests and SDK-side validation cases. |
| src/mcp_http/server.py | Updates MCP tool descriptions and route mapping behavior (GETs as tools; excludes ingest). |
| src/api/v1/search.py | Accepts filter_id and resolves filter values for v1 search requests. |
| src/api/v1/documents.py | Extends v1 documents DELETE to support deleting all filenames referenced by a filter. |
| src/api/v1/chat.py | Accepts filter_id and resolves filter values for v1 chat (streaming + non-streaming). |
| src/api/v1/_filter_resolution.py | New shared helper for resolving filter_id into concrete filters/limit/threshold with wildcard stripping. |
| sdks/typescript/tests/integration.test.ts | Strengthens TS integration tests to validate actual scoping and delete-by-filter behavior. |
| sdks/typescript/src/types.ts | Extends delete response typing and introduces delete options type. |
| sdks/typescript/src/documents.ts | Updates documents.delete to accept filename or { filterId } and preserve filename-idempotency. |
| sdks/python/openrag_sdk/models.py | Extends delete response model to include filter deletion metadata. |
| sdks/python/openrag_sdk/documents.py | Updates documents.delete to support filter_id and preserve filename-idempotency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+137
to
+153
| resolved_filters = body.filters | ||
| resolved_limit = body.limit | ||
| resolved_score_threshold = body.score_threshold | ||
| if body.filter_id: | ||
| resolved = await resolve_filter_id( | ||
| body.filter_id, | ||
| knowledge_filter_service, | ||
| user_id=user.user_id, | ||
| jwt_token=jwt_token, | ||
| ) | ||
| # Inline values override per-field; defaults (10 / 0) fall back to the filter. | ||
| if not body.filters: | ||
| resolved_filters = resolved["filters"] | ||
| if body.limit == 10: | ||
| resolved_limit = resolved["limit"] | ||
| if body.score_threshold == 0: | ||
| resolved_score_threshold = resolved["score_threshold"] |
Comment on lines
+52
to
+67
| resolved_filters = body.filters | ||
| resolved_limit = body.limit | ||
| resolved_score_threshold = body.score_threshold | ||
| if body.filter_id: | ||
| resolved = await resolve_filter_id( | ||
| body.filter_id, | ||
| knowledge_filter_service, | ||
| user_id=user.user_id, | ||
| jwt_token=None, | ||
| ) | ||
| if not body.filters: | ||
| resolved_filters = resolved["filters"] | ||
| if body.limit == 10: | ||
| resolved_limit = resolved["limit"] | ||
| if body.score_threshold == 0: | ||
| resolved_score_threshold = resolved["score_threshold"] |
Comment on lines
+122
to
+140
| total_deleted = 0 | ||
| for fname in filenames: | ||
| payload, _status = await delete_documents_by_filename_core( | ||
| filename=fname, | ||
| session_manager=session_manager, | ||
| user_id=user.user_id, | ||
| jwt_token=None, | ||
| ) | ||
| results.append(payload) | ||
| total_deleted += payload.get("deleted_chunks", 0) or 0 | ||
|
|
||
| return JSONResponse( | ||
| { | ||
| "success": True, | ||
| "deleted_chunks": total_deleted, | ||
| "filenames": filenames, | ||
| "filter_id": body.filter_id, | ||
| "per_file": results, | ||
| } |
Comment on lines
+19
to
+30
| def _strip_wildcards(filters: dict[str, Any] | None) -> dict[str, list[str]]: | ||
| """Drop `["*"]` and empty lists from each filter dimension.""" | ||
| if not filters: | ||
| return {} | ||
| cleaned: dict[str, list[str]] = {} | ||
| for key in _FILTER_DIMENSIONS: | ||
| values = filters.get(key) | ||
| if not values or not isinstance(values, list): | ||
| continue | ||
| if "*" in values: | ||
| continue | ||
| cleaned[key] = values |
| if not result.get("success"): | ||
| raise HTTPException( | ||
| status_code=404, | ||
| detail={"error": f"Filter {filter_id} not found"}, |
| "description": ( | ||
| "Check the status of an ingestion task. " | ||
| "Use the task_id returned from openrag_ingest." | ||
| "Check the status of an ingestion task. Use the task_id returned from openrag_ingest." |
github-actions
Bot
added
bug
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
sdks/python/openrag_sdk/models.py (1)
100-103: ⚡ Quick winGive
per_filea real model.
per_file: list[dict]leaves a public SDK field unvalidated and untyped, so malformed per-file payloads won't surface until consumer code touches them. A small nested model would keep the delete-by-filter contract self-describing.♻️ Proposed fix
+class DeleteDocumentPerFileResult(BaseModel): + """Per-file result from filter-based document deletion.""" + + success: bool + deleted_chunks: int = 0 + filename: str | None = None + message: str | None = None + error: str | None = None + + class DeleteDocumentResponse(BaseModel): """Response from document deletion.""" @@ - per_file: list[dict] | None = None + per_file: list[DeleteDocumentPerFileResult] | None = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/python/openrag_sdk/models.py` around lines 100 - 103, The public SDK model exposes per_file as list[dict], which is untyped and unvalidated; create a small nested Pydantic model (e.g., PerFileInfo or PerFileDelete) with typed fields (for example filename: str, data_source: str, success: bool, details: dict | None) and replace the per_file annotation in the parent model with list[PerFileInfo] | None, updating imports and any references to use the new PerFileInfo class so per-file payloads are validated and self-describing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/v1/chat.py`:
- Around line 147-153: The current checks treat falsy values as "not provided"
so explicit inputs like filters={}, limit=10 or score_threshold=0 get ignored;
change the override logic in chat.py to use presence-based checks (e.g., test
whether the request field was actually provided on body rather than truthiness)
when setting resolved_filters, resolved_limit and resolved_score_threshold so
you only fall back to resolved["..."] when the caller truly omitted the field
(for example by using body.__fields_set__ or explicit is not None /
attribute-presence checks) and accept empty/zero values as valid overrides.
In `@src/api/v1/documents.py`:
- Around line 151-185: The route handler currently embeds the full filter-based
deletion orchestration (resolving filters via resolve_filter_id, enforcing
safety, looping calls to delete_documents_by_filename_core, aggregating
total_deleted and per_file results) which should be moved into a dedicated
helper/service; create a new function (e.g., delete_documents_by_filter or
DocumentFilterDeletionService) that accepts filter_id, knowledge_filter_service,
session_manager, user_id and jwt_token, performs resolve_filter_id, validates
filenames, calls delete_documents_by_filename_core for each filename, aggregates
deleted_chunks and per-file payloads, and returns a structured result or raises
appropriate exceptions; then simplify the route to only call that helper,
translate exceptions to proper HTTPResponse/status codes, ensure dependency
injection via existing dependencies, and update the route's response model
typing to reflect the helper's returned structure.
---
Nitpick comments:
In `@sdks/python/openrag_sdk/models.py`:
- Around line 100-103: The public SDK model exposes per_file as list[dict],
which is untyped and unvalidated; create a small nested Pydantic model (e.g.,
PerFileInfo or PerFileDelete) with typed fields (for example filename: str,
data_source: str, success: bool, details: dict | None) and replace the per_file
annotation in the parent model with list[PerFileInfo] | None, updating imports
and any references to use the new PerFileInfo class so per-file payloads are
validated and self-describing.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 089b05e0-09ee-4e06-9a55-af9a9c991f5e
📒 Files selected for processing (12)
sdks/python/openrag_sdk/documents.pysdks/python/openrag_sdk/models.pysdks/typescript/src/documents.tssdks/typescript/src/types.tssdks/typescript/tests/integration.test.tssrc/api/v1/_filter_resolution.pysrc/api/v1/chat.pysrc/api/v1/documents.pysrc/api/v1/search.pysrc/mcp_http/server.pytests/integration/sdk/test_documents.pytests/integration/sdk/test_filters.py
✅ Files skipped from review due to trivial changes (1)
- sdks/typescript/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/api/v1/_filter_resolution.py
- sdks/python/openrag_sdk/documents.py
- src/api/v1/search.py
- sdks/typescript/src/documents.ts
- tests/integration/sdk/test_documents.py
- tests/integration/sdk/test_filters.py
- sdks/typescript/tests/integration.test.ts
- src/mcp_http/server.py
Comment on lines
+147
to
+153
| # Inline values override per-field; defaults (10 / 0) fall back to the filter. | ||
| if not body.filters: | ||
| resolved_filters = resolved["filters"] | ||
| if body.limit == 10: | ||
| resolved_limit = resolved["limit"] | ||
| if body.score_threshold == 0: | ||
| resolved_score_threshold = resolved["score_threshold"] |
Contributor
There was a problem hiding this comment.
Use presence-based override checks here.
This only treats truthy filters and non-default limit / score_threshold values as overrides. That means filters={}, limit=10, and score_threshold=0 all fall back to the saved filter instead of honoring the caller's explicit input, which breaks the per-field override contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/v1/chat.py` around lines 147 - 153, The current checks treat falsy
values as "not provided" so explicit inputs like filters={}, limit=10 or
score_threshold=0 get ignored; change the override logic in chat.py to use
presence-based checks (e.g., test whether the request field was actually
provided on body rather than truthiness) when setting resolved_filters,
resolved_limit and resolved_score_threshold so you only fall back to
resolved["..."] when the caller truly omitted the field (for example by using
body.__fields_set__ or explicit is not None / attribute-presence checks) and
accept empty/zero values as valid overrides.
Comment on lines
+151
to
+185
| if body.filter_id: | ||
| resolved = await resolve_filter_id( | ||
| body.filter_id, | ||
| knowledge_filter_service, | ||
| user_id=user.user_id, | ||
| jwt_token=None, | ||
| ) | ||
| filenames = resolved["filters"].get("data_sources") or [] | ||
| if not filenames: | ||
| return JSONResponse( | ||
| {"error": "Filter has no specific data_sources to delete"}, | ||
| status_code=400, | ||
| ) | ||
|
|
||
| results = [] | ||
| total_deleted = 0 | ||
| for fname in filenames: | ||
| payload, _status = await delete_documents_by_filename_core( | ||
| filename=fname, | ||
| session_manager=session_manager, | ||
| user_id=user.user_id, | ||
| jwt_token=None, | ||
| ) | ||
| results.append(payload) | ||
| total_deleted += payload.get("deleted_chunks", 0) or 0 | ||
|
|
||
| return JSONResponse( | ||
| { | ||
| "success": True, | ||
| "deleted_chunks": total_deleted, | ||
| "filenames": filenames, | ||
| "filter_id": body.filter_id, | ||
| "per_file": results, | ||
| } | ||
| ) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract filter-based deletion orchestration out of the route.
This handler now resolves filters, enforces safety rules, iterates deletions, and aggregates response state. Please move that flow behind a dedicated helper/service and keep the route focused on HTTP validation/translation. As per coding guidelines, "src/api/**/*.py: FastAPI routes. Public/stable endpoints belong under src/api/v1/. Verify dependency injection via src/dependencies.py, response model typing, and correct HTTP status codes. No business logic in route handlers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/v1/documents.py` around lines 151 - 185, The route handler currently
embeds the full filter-based deletion orchestration (resolving filters via
resolve_filter_id, enforcing safety, looping calls to
delete_documents_by_filename_core, aggregating total_deleted and per_file
results) which should be moved into a dedicated helper/service; create a new
function (e.g., delete_documents_by_filter or DocumentFilterDeletionService)
that accepts filter_id, knowledge_filter_service, session_manager, user_id and
jwt_token, performs resolve_filter_id, validates filenames, calls
delete_documents_by_filename_core for each filename, aggregates deleted_chunks
and per-file payloads, and returns a structured result or raises appropriate
exceptions; then simplify the route to only call that helper, translate
exceptions to proper HTTPResponse/status codes, ensure dependency injection via
existing dependencies, and update the route's response model typing to reflect
the helper's returned structure.
Add server-side resolution for filter_id and extend document deletion to accept a filter_id. Introduces api.v1._filter_resolution to normalize a filter_id into concrete filters/limits/score_thresholds and to strip wildcards. v1 endpoints (chat, search, documents) now accept filter_id and merge resolved values with inline overrides; documents DELETE can delete all filenames referenced by a filter_id (with wildcard/empty data_sources rejected to avoid mass deletion). SDK updates (Python and TypeScript) allow DocumentsClient.delete to accept either filename or filter_id (mutually exclusive), include additional response fields (filenames, filter_id, per_file), and preserve idempotent semantics for filename deletes while surfacing filter-not-found errors. Tests updated/added to cover filter_id behavior, inline overrides, streaming, and validation. Additional minor logging/validation and type updates included.
Expand the DELETE /v1/documents component description to indicate it can delete single or multiple documents and requires exactly one of `filename` or `filter_id`. Also notes that wildcards are rejected for safety, improving API documentation and removing ambiguity about deletion semantics.
Ensure API-key authenticated searches set the auth context and pass the user's JWT to the search service so downstream tools can resolve the user. Adds import for set_auth_context, calls it when handling API-key requests (mirroring v1 chat), and passes user.jwt_token to search_service.search instead of None. Also cleans up an unused typing import.
Clarify search component docs to document `filter_id` and inline `filters` (inline filters override per-field). Document that /v1/documents/ingest is intentionally not customized/exposed via MCP because multipart/form-data uploads are not supported by FastMCP's from_fastapi conversion; clients should use the HTTP API or SDK to ingest. Add a RouteMap to explicitly exclude POST /v1/documents/ingest and consolidate route maps to expose all /v1/ routes (including GET) as MCP tools, with a note explaining that GETs are exposed as tools to better support LLM agent workflows.
edwinjosechittilappilly
force-pushed
the
fix-v1-endpoints
branch
from
7ab355d to
6a13a75
Compare
github-actions
Bot
added
bug
Contributor
|
Actionable comments posted: 0 |
github-actions
Bot
added
bug
github-actions
Bot
added
bug
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/api/v1/search.py (2)
53-68: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMove filter merge/resolution orchestration out of the route handler.
This block is endpoint business logic and should live in a service/helper so the route stays as transport + validation only.
As per coding guidelines
src/api/**/*.py: “No business logic in route handlers.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/v1/search.py` around lines 53 - 68, Extract the filter-resolution/merge logic from the route handler in src/api/v1/search.py into a new service/helper function (e.g., resolve_and_merge_filters) that accepts the request body (or its fields), knowledge_filter_service, user_id and returns resolved_filters, resolved_limit, and resolved_score_threshold; inside that helper call the existing resolve_filter_id(...) and apply the same merge rules (use resolved["filters"] when body.filters is empty, resolved["limit"] when body.limit == 10, and resolved["score_threshold"] when body.score_threshold == 0). Replace the inline block in the route with a single call to this helper and ensure the route remains only transport/validation while resolve_filter_id, resolved_filters, resolved_limit, resolved_score_threshold, body, knowledge_filter_service, and user.user_id are used to locate and wire up the change.
63-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse presence-based override checks instead of truthiness/default-value checks.
Lines 63-68 treat
{},10, and0as “not provided”, so explicit inline overrides can be ignored.🔧 Minimal fix
- resolved_filters = body.filters - resolved_limit = body.limit - resolved_score_threshold = body.score_threshold + provided_fields = body.model_fields_set + resolved_filters = body.filters + resolved_limit = body.limit + resolved_score_threshold = body.score_threshold @@ - if not body.filters: + if "filters" not in provided_fields: resolved_filters = resolved["filters"] - if body.limit == 10: + if "limit" not in provided_fields: resolved_limit = resolved["limit"] - if body.score_threshold == 0: + if "score_threshold" not in provided_fields: resolved_score_threshold = resolved["score_threshold"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/v1/search.py` around lines 63 - 68, The override logic currently uses falsy/default-value checks (if not body.filters, if body.limit == 10, if body.score_threshold == 0) which treats valid values like {} , 0, or 10 as “not provided”; change these to presence-based checks (e.g. test for None / presence) so explicit values are honored: update the conditions around body.filters, body.limit, and body.score_threshold to check for None (or an explicit "is provided" flag) and only fall back to resolved["filters"], resolved["limit"], and resolved["score_threshold"] when the field is actually missing; look for the block that assigns resolved_filters, resolved_limit, and resolved_score_threshold in search.py and replace the falsy/default comparisons with presence checks.src/api/v1/chat.py (1)
17-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the unused
get_api_key_user_asyncimport to unblock Ruff.CI is currently failing with F401 at Line 18.
🔧 Minimal fix
from dependencies import ( - get_api_key_user_async, get_chat_service, get_knowledge_filter_service, get_session_manager, require_api_key_permission, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/v1/chat.py` around lines 17 - 19, Remove the unused import get_api_key_user_async from the dependencies import list in src/api/v1/chat.py to satisfy Ruff F401; keep the needed get_chat_service import and ensure any references to get_api_key_user_async are not required elsewhere in this module before removing it.
♻️ Duplicate comments (2)
src/api/v1/search.py (1)
57-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForward the caller JWT to
resolve_filter_id.Line 61 passes
jwt_token=None, which can fail filter resolution for JWT/OIDC-authenticated requests and is inconsistent with chat’s implementation.🔧 Minimal fix
resolved = await resolve_filter_id( body.filter_id, knowledge_filter_service, user_id=user.user_id, - jwt_token=None, + jwt_token=user.jwt_token, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/v1/search.py` around lines 57 - 62, The call to resolve_filter_id in the search handler currently passes jwt_token=None which breaks JWT/OIDC-backed filter resolution; change the call to forward the caller's JWT instead of None by replacing jwt_token=None with the request's JWT token (e.g., jwt_token variable from the handler or the auth context) so the call becomes resolve_filter_id(body.filter_id, knowledge_filter_service, user_id=user.user_id, jwt_token=caller_jwt); ensure you reference the same token variable used elsewhere in this module (the handler's JWT/auth token).src/api/v1/chat.py (1)
149-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse presence-based overrides so explicit
{},10, and0are honored.Current checks treat falsy/default-valued inputs as omitted, so caller-provided overrides can be dropped.
🔧 Minimal fix
- resolved_filters = body.filters - resolved_limit = body.limit - resolved_score_threshold = body.score_threshold + provided_fields = body.model_fields_set + resolved_filters = body.filters + resolved_limit = body.limit + resolved_score_threshold = body.score_threshold @@ - if not body.filters: + if "filters" not in provided_fields: resolved_filters = resolved["filters"] - if body.limit == 10: + if "limit" not in provided_fields: resolved_limit = resolved["limit"] - if body.score_threshold == 0: + if "score_threshold" not in provided_fields: resolved_score_threshold = resolved["score_threshold"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/v1/chat.py` around lines 149 - 154, The code is using falsy/value-equality checks that drop explicit overrides (e.g., {} for filters, 10 and 0 for numeric fields); update the presence checks to use "is None" semantics so explicit values are preserved: replace the conditions that reference body.filters, body.limit == 10, and body.score_threshold == 0 with presence checks that only fallback to resolved["filters"], resolved["limit"], and resolved["score_threshold"] when body.filters, body.limit, or body.score_threshold are None respectively (refer to body.filters, body.limit, body.score_threshold and resolved_filters/resolved_limit/resolved_score_threshold to locate the logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/api/v1/chat.py`:
- Around line 17-19: Remove the unused import get_api_key_user_async from the
dependencies import list in src/api/v1/chat.py to satisfy Ruff F401; keep the
needed get_chat_service import and ensure any references to
get_api_key_user_async are not required elsewhere in this module before removing
it.
In `@src/api/v1/search.py`:
- Around line 53-68: Extract the filter-resolution/merge logic from the route
handler in src/api/v1/search.py into a new service/helper function (e.g.,
resolve_and_merge_filters) that accepts the request body (or its fields),
knowledge_filter_service, user_id and returns resolved_filters, resolved_limit,
and resolved_score_threshold; inside that helper call the existing
resolve_filter_id(...) and apply the same merge rules (use resolved["filters"]
when body.filters is empty, resolved["limit"] when body.limit == 10, and
resolved["score_threshold"] when body.score_threshold == 0). Replace the inline
block in the route with a single call to this helper and ensure the route
remains only transport/validation while resolve_filter_id, resolved_filters,
resolved_limit, resolved_score_threshold, body, knowledge_filter_service, and
user.user_id are used to locate and wire up the change.
- Around line 63-68: The override logic currently uses falsy/default-value
checks (if not body.filters, if body.limit == 10, if body.score_threshold == 0)
which treats valid values like {} , 0, or 10 as “not provided”; change these to
presence-based checks (e.g. test for None / presence) so explicit values are
honored: update the conditions around body.filters, body.limit, and
body.score_threshold to check for None (or an explicit "is provided" flag) and
only fall back to resolved["filters"], resolved["limit"], and
resolved["score_threshold"] when the field is actually missing; look for the
block that assigns resolved_filters, resolved_limit, and
resolved_score_threshold in search.py and replace the falsy/default comparisons
with presence checks.
---
Duplicate comments:
In `@src/api/v1/chat.py`:
- Around line 149-154: The code is using falsy/value-equality checks that drop
explicit overrides (e.g., {} for filters, 10 and 0 for numeric fields); update
the presence checks to use "is None" semantics so explicit values are preserved:
replace the conditions that reference body.filters, body.limit == 10, and
body.score_threshold == 0 with presence checks that only fallback to
resolved["filters"], resolved["limit"], and resolved["score_threshold"] when
body.filters, body.limit, or body.score_threshold are None respectively (refer
to body.filters, body.limit, body.score_threshold and
resolved_filters/resolved_limit/resolved_score_threshold to locate the logic).
In `@src/api/v1/search.py`:
- Around line 57-62: The call to resolve_filter_id in the search handler
currently passes jwt_token=None which breaks JWT/OIDC-backed filter resolution;
change the call to forward the caller's JWT instead of None by replacing
jwt_token=None with the request's JWT token (e.g., jwt_token variable from the
handler or the auth context) so the call becomes
resolve_filter_id(body.filter_id, knowledge_filter_service,
user_id=user.user_id, jwt_token=caller_jwt); ensure you reference the same token
variable used elsewhere in this module (the handler's JWT/auth token).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 013eba90-89ca-487f-b96f-a5ce0d07400f
📒 Files selected for processing (3)
src/api/v1/chat.pysrc/api/v1/documents.pysrc/api/v1/search.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/v1/documents.py
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
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.
Add server-side resolution for filter_id and extend document deletion to accept a filter_id. Introduces api.v1._filter_resolution to normalize a filter_id into concrete filters/limits/score_thresholds and to strip wildcards. v1 endpoints (chat, search, documents) now accept filter_id and merge resolved values with inline overrides; documents DELETE can delete all filenames referenced by a filter_id (with wildcard/empty data_sources rejected to avoid mass deletion). SDK updates (Python and TypeScript) allow DocumentsClient.delete to accept either filename or filter_id (mutually exclusive), include additional response fields (filenames, filter_id, per_file), and preserve idempotent semantics for filename deletes while surfacing filter-not-found errors. Tests updated/added to cover filter_id behavior, inline overrides, streaming, and validation. Additional minor logging/validation and type updates included.
Summary by CodeRabbit
New Features
Bug Fixes
Tests