refactor: Move query parameters validation to beartype #1303
Conversation
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
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
creatorrr
approved these changes
Apr 3, 2025
whiterabbit1983
force-pushed
the
r/refactor-query-params-validation
branch
from
d92f538 to
757a1da
Compare
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 757a1da in 2 minutes and 25 seconds
More details
- Looked at
1211lines of code in18files - Skipped
0files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. agents-api/tests/test_user_queries.py:104
- Draft comment:
Consider using raises(HTTPException) (or a more specific exception type) instead of the generic Exception in this test for consistency and clearer error expectations. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. agents-api/agents_api/common/exceptions/validation.py:4
- Draft comment:
New exception class QueryParamsValidationError is implemented clearly. Consider adding additional context or attributes if needed in the future. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. agents-api/agents_api/common/utils/db_exceptions.py:191
- Draft comment:
The lambda mapping checking for BeartypeCallHintParamViolation relies on string matching (e.g. 'typing.Literal['asc', "desc"]') which is brittle. Consider using error attributes or a more robust method for detecting invalid sort parameters. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While string matching error messages can be fragile, I need to consider: 1) Is there actually a better way to handle this? 2) BeartypeCallHintParamViolation may not expose the literal type information in a more structured way 3) The strings being matched are type hints which are unlikely to change frequently 4) The comment doesn't provide a concrete alternative solution.
I may be underestimating the maintenance burden of string matching. The error format could change in future beartype versions.
Without a concrete better alternative suggested, and given these are Python type hints that are relatively stable, the current approach seems reasonable even if not perfect.
The comment raises a valid concern but doesn't provide an actionable alternative. The current implementation, while not ideal, is pragmatic given the constraints.
4. agents-api/agents_api/queries/agents/list_agents.py:70
- Draft comment:
Remove commented-out explicit sort direction validation. The refactor uses beartype to enforce valid parameters, so this obsolete code should be removed. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. agents-api/agents_api/queries/executions/list_executions.py:106
- Draft comment:
Remove the commented-out HTTPException check for sort field. Rely on type validation via beartype instead of manual checks. - Reason this comment was not posted:
Marked as duplicate.
6. agents-api/agents_api/queries/files/list_files.py:61
- Draft comment:
Remove the commented-out validations for sort direction and field in list_files. The new approach using Annotated and beartype.vale makes these manual checks unnecessary. - Reason this comment was not posted:
Marked as duplicate.
7. agents-api/agents_api/queries/tasks/list_tasks.py:85
- Draft comment:
Remove the commented-out validations for sort direction, limit, and offset in the list_tasks query; the new beartype-based validation handles these checks. - Reason this comment was not posted:
Marked as duplicate.
8. agents-api/agents_api/queries/utils.py:387
- Draft comment:
In the make_num_validator function, using 'nonlocal err_msg' to reassign the error message may be error-prone if the validator is reused. Ensure that the err_msg remains consistent and is not inadvertently modified on subsequent validations. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment raises a theoretical concern about reuse of the validator, but looking at the implementation: 1) The err_msg is only modified if it's None 2) Each call to make_num_validator creates a new closure with its own err_msg 3) The modification is intentional to provide default messages 4) The current implementation is actually a common and valid pattern for default values.
I could be wrong about the closure behavior. There might be edge cases where reusing the same validator instance could cause issues.
The closure creates a new scope each time make_num_validator is called, so each validator instance has its own independent err_msg. The modification is intentional for providing defaults.
The comment should be deleted because it raises a theoretical concern about a pattern that is actually correct and commonly used for providing default error messages.
9. agents-api/agents_api/queries/utils.py:86
- Draft comment:
It looks like the function definition for pg_query is written asdef pg_query[**P](. The use of square brackets here seems unusual for declaring a generic function in Python. Please verify if this syntax is intentional, or if it should be rewritten with standard generic syntax (e.g., using type variables or without the brackets). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. agents-api/agents_api/queries/utils.py:340
- Draft comment:
In the function sanitize_string, the isinstance check on line 340 usesif isinstance(data, list | tuple):. While the union operator is allowed in type annotations, for isinstance it should be a tuple of types. This should be updated toif isinstance(data, (list, tuple)):for correct behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. agents-api/tests/test_files_queries.py:1
- Draft comment:
The header comment on line 1 reads "Tests for entry queries", which appears inconsistent with the file's purpose of testing file queries. Consider updating this comment to reflect that these tests are for file queries, e.g., "Tests for file queries". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_e8c3OZpfCuWFM8HF
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -87,17 +97,11 @@ async def list_docs( | |||
| Raises: | |||
| HTTPException: If invalid parameters are provided. | |||
| """ | |||
| if direction.lower() not in ["asc", "desc"]: | |||
| raise HTTPException(status_code=400, detail="Invalid sort direction") | |||
| # if direction.lower() not in ["asc", "desc"]: | |||
There was a problem hiding this comment.
Remove the commented-out HTTPException checks for sort direction and sort field. With the new beartype validators in place, these checks are redundant.
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.
PR Type
Enhancement, Bug fix, Tests
Description
Refactored query parameter validation to use
beartypeand centralized validation logic.make_num_validatorutility for numeric validation with custom error messages.QueryParamsValidationErrorfor consistent exception handling.Replaced inline validation logic in query functions with
Annotatedandbeartype.vale.Enhanced error handling for invalid query parameters across multiple query modules.
Added comprehensive test cases for invalid query parameter scenarios.
Changes walkthrough 📝
11 files
AddedQueryParamsValidationErrorfor query parameter validation errorsEnhanced exception handling for query parameter validationRemoved redundant validation logic for task validationRefactored query parameter validation for listing agentsRefactored query parameter validation for listing documentsRefactored query parameter validation for listing entriesRefactored query parameter validation for listing executionsRefactored query parameter validation for listing filesRefactored query parameter validation for listing tasksRefactored query parameter validation for listing usersAdded `make_num_validator` utility for numeric validation7 files
Added tests for invalid query parameters in agent queriesAdded tests for invalid query parameters in document queriesAdded tests for invalid query parameters in entry queriesAdded tests for invalid query parameters in execution queriesAdded tests for invalid query parameters in file queriesAdded tests for invalid query parameters in task queriesAdded tests for invalid query parameters in user queriesImportant
Refactor query parameter validation using
beartypeacross multiple query functions, adding new tests for validation errors.beartypefor query parameter validation in functions likelist_agents,list_docs,list_entries,list_executions,list_files,list_tasks, andlist_users.limit,offset,sort_by, anddirectionin these functions.QueryParamsValidationErrorinvalidation.pyfor handling query parameter validation errors.common_db_exceptionsindb_exceptions.pyto mapQueryParamsValidationErrorto HTTP 400 errors.limit,offset,sort_by, anddirectionparameters intest_agent_queries.py,test_docs_queries.py,test_entry_queries.py,test_execution_queries.py,test_files_queries.py,test_task_queries.py, andtest_user_queries.py.This description was created by
for 757a1da. It will automatically update as commits are pushed.