fix: more filter_id issues#1730
Merged
Merged
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:
WalkthroughThis PR extends filter resolution across v1 endpoints by centralizing a new ChangesFilter-based deletion and filter-scoped retrieval
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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
![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: 1
🧹 Nitpick comments (2)
tests/integration/sdk/test_filters.py (1)
193-199: ⚡ Quick winMake the not-found checks assert the actual contract.
These two tests currently pass on any
OpenRAGError, including unrelated auth or transport failures. Please assert the 404/not-found details from the exception so they really validate the newfilter_idpath.Also applies to: 249-255
tests/unit/api/test_v1_filter_resolution.py (1)
14-55: ⚡ Quick winAdd direct tests for
resolve_filter_id.This file only locks down
merge_filter_overrides. The new shared path that loads a saved filter, strips wildcard dimensions, and returns 404s is still untested, so regressions there will only show up through higher-level integration failures.🤖 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 `@tests/unit/api/test_v1_filter_resolution.py` around lines 14 - 55, Add direct unit coverage for resolve_filter_id in this test module, since it is the shared path that loads a saved filter, removes wildcard dimensions, and handles missing filters with 404 behavior. Create focused tests around resolve_filter_id’s successful lookup and normalization behavior, plus the not-found case, using the existing filter-loading and merge helpers only as needed to keep the assertions at the resolve_filter_id level.
🤖 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/_filter_resolution.py`:
- Around line 55-59: The current logic swallows JSONDecodeError for
query_data_raw and falls back to an empty dict, allowing a malformed saved
filter to produce an unfiltered/default search; instead, detect invalid JSON and
fail closed: on json.JSONDecodeError for query_data_raw (the block handling
query_data_raw → query_data) raise/return an explicit error (e.g., propagate the
exception or return a validation/HTTP error) so the caller handling filter
resolution knows the filter is invalid rather than treating it as {}. Locate the
code that reads query_data_raw and ensure the exception is not converted into an
empty dict (adjust the except branch in the query_data parsing logic used by the
filter resolution routine).
---
Nitpick comments:
In `@tests/unit/api/test_v1_filter_resolution.py`:
- Around line 14-55: Add direct unit coverage for resolve_filter_id in this test
module, since it is the shared path that loads a saved filter, removes wildcard
dimensions, and handles missing filters with 404 behavior. Create focused tests
around resolve_filter_id’s successful lookup and normalization behavior, plus
the not-found case, using the existing filter-loading and merge helpers only as
needed to keep the assertions at the resolve_filter_id level.
🪄 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: 52c1ae51-99be-463a-9ddd-48d3790a3fa4
📒 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.pytests/integration/sdk/test_documents.pytests/integration/sdk/test_filters.pytests/unit/api/test_v1_filter_resolution.py
Comment on lines
+55
to
+59
| if isinstance(query_data_raw, str): | ||
| try: | ||
| query_data = json.loads(query_data_raw) | ||
| except json.JSONDecodeError: | ||
| query_data = {} |
Contributor
There was a problem hiding this comment.
Don't fail open on malformed saved filter JSON.
If query_data is invalid JSON here, the code silently resolves it to {}, which becomes an unfiltered/default search/chat request. That turns a corrupted filter_id into broader access than intended instead of failing closed.
Proposed fix
if isinstance(query_data_raw, str):
try:
query_data = json.loads(query_data_raw)
except json.JSONDecodeError:
- query_data = {}
+ raise HTTPException(
+ status_code=500,
+ detail={"error": f"Filter {filter_id} has invalid query_data"},
+ )
else:
query_data = query_data_raw or {}📝 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
| if isinstance(query_data_raw, str): | |
| try: | |
| query_data = json.loads(query_data_raw) | |
| except json.JSONDecodeError: | |
| query_data = {} | |
| if isinstance(query_data_raw, str): | |
| try: | |
| query_data = json.loads(query_data_raw) | |
| except json.JSONDecodeError: | |
| raise HTTPException( | |
| status_code=500, | |
| detail={"error": f"Filter {filter_id} has invalid query_data"}, | |
| ) |
🤖 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/_filter_resolution.py` around lines 55 - 59, The current logic
swallows JSONDecodeError for query_data_raw and falls back to an empty dict,
allowing a malformed saved filter to produce an unfiltered/default search;
instead, detect invalid JSON and fail closed: on json.JSONDecodeError for
query_data_raw (the block handling query_data_raw → query_data) raise/return an
explicit error (e.g., propagate the exception or return a validation/HTTP error)
so the caller handling filter resolution knows the filter is invalid rather than
treating it as {}. Locate the code that reads query_data_raw and ensure the
exception is not converted into an empty dict (adjust the except branch in the
query_data parsing logic used by the filter resolution routine).
phact
force-pushed
the
pr-1666-filter-id-main
branch
from
b9a93ae to
5d8d20d
Compare
github-actions
Bot
added
lgtm
bug
Change assignments to the request body to use bracket notation (body['filename'], body['filter_id']) instead of dot notation. This ensures the string keys are set explicitly on the Record<string, string> and avoids relying on declared property names.
edwinjosechittilappilly
force-pushed
the
pr-1666-filter-id-main
branch
from
a0d4ac4 to
b389aab
Compare
github-actions
Bot
added
bug
github-actions
Bot
added
bug
Collaborator
|
@phact I have pushed fixes to main, ideally the test should pass now |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/api/v1/_filter_resolution.py (1)
57-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed on malformed
query_datainstead of silently broadening scope.
json.JSONDecodeErroris still converted to{}, which can turn a bad saved filter into default/unfiltered behavior. This should return an explicit error.Proposed fix
if isinstance(query_data_raw, str): try: query_data = json.loads(query_data_raw) except json.JSONDecodeError: - query_data = {} + raise HTTPException( + status_code=500, + detail={"error": f"Filter {filter_id} has invalid query_data"}, + ) else: query_data = query_data_raw or {}🤖 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/_filter_resolution.py` around lines 57 - 61, The current handling of a malformed query_data_raw silently falls back to an empty dict; change this to fail closed by catching json.JSONDecodeError and returning an explicit client error instead of setting query_data = {}. In the block that checks isinstance(query_data_raw, str) replace the silent fallback with raising an HTTP error (e.g. HTTPException with status 400 and a clear detail like "malformed query_data JSON") so callers get a validation error; reference the variables query_data_raw and query_data and the exception json.JSONDecodeError to locate and update the logic accordingly.
🤖 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.
Duplicate comments:
In `@src/api/v1/_filter_resolution.py`:
- Around line 57-61: The current handling of a malformed query_data_raw silently
falls back to an empty dict; change this to fail closed by catching
json.JSONDecodeError and returning an explicit client error instead of setting
query_data = {}. In the block that checks isinstance(query_data_raw, str)
replace the silent fallback with raising an HTTP error (e.g. HTTPException with
status 400 and a clear detail like "malformed query_data JSON") so callers get a
validation error; reference the variables query_data_raw and query_data and the
exception json.JSONDecodeError to locate and update the logic accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bb983e9-7f29-47ec-bab6-a9aa49800787
📒 Files selected for processing (8)
sdks/python/openrag_sdk/documents.pysrc/api/v1/_filter_resolution.pysrc/api/v1/chat.pysrc/api/v1/documents.pysrc/api/v1/search.pytests/integration/sdk/test_documents.pytests/integration/sdk/test_filters.pytests/unit/api/test_v1_filter_resolution.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/integration/sdk/test_documents.py
- tests/integration/sdk/test_filters.py
- sdks/python/openrag_sdk/documents.py
- tests/unit/api/test_v1_filter_resolution.py
- src/api/v1/documents.py
- src/api/v1/chat.py
- src/api/v1/search.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
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests