Remove deprecated imports from a few more apps #8696
Conversation
WalkthroughThe changes in this pull request involve updating type hints across multiple files to utilize built-in types instead of those from the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LambdaFunction
participant LambdaJob
User->>LambdaFunction: invoke(db_task, data)
LambdaFunction->>LambdaJob: _call_detector(function, db_task, labels)
LambdaJob->>LambdaJob: Check label compatibility
LambdaJob-->>LambdaFunction: Return results
LambdaFunction-->>User: Return response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
cvat/apps/iam/utils.py (1)
16-16: LGTM! Consider adding docstring.The type hint change from
Tuple[bytes, str]totuple[bytes, str]is correct and aligns with PEP 585 (Python 3.9+).Consider adding a docstring to explain:
- The purpose of the OPA bundle
- The format of the returned tuple (bundle bytes and ETag)
- The caching behavior
@functools.lru_cache(maxsize=None) def get_opa_bundle() -> tuple[bytes, str]: + """Generate a tarball containing OPA (Open Policy Agent) rules. + + Returns: + tuple[bytes, str]: A tuple containing: + - bytes: The gzipped tar bundle of Rego policy files + - str: The BLAKE2b hash of the bundle (ETag) + + Note: + Results are cached indefinitely using @lru_cache. + Cache is invalidated when rules are added via add_opa_rules_path(). + """ bundle_file = io.BytesIO()cvat/apps/iam/serializers.py (2)
Line range hint
1-156: Add tests for authentication-related changesThe PR checklist indicates that no tests were added. Given that this file handles critical authentication functionality, please consider adding tests to verify:
- Error handling in the registration flow
- Type compatibility with both dict and User objects
- Validation error formatting
Would you like me to help create a test suite for these changes?
Review error details exposure in validation error handling
Consider limiting the detail provided by
as_serializer_errorto prevent potential information disclosure in authentication contexts.
cvat/apps/iam/serializers.py: Lines 19-25🔗 Analysis chain
Line range hint
89-94: Review error details exposure in validation error handlingWhile the error handling improvement is good, consider reviewing the security implications of exposing detailed validation errors through
as_serializer_error. In authentication contexts, it's often better to provide generic error messages to prevent information disclosure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar error handling patterns in authentication-related files rg -g '!*.{json,md,yml}' -A 3 'DjangoValidationError.*ValidationError' cvat/apps/iam/Length of output: 87
Script:
#!/bin/bash # Search for usages of 'as_serializer_error' in the iam app rg --glob '!*.{json,md,yml}' 'as_serializer_error' cvat/apps/iam/Length of output: 158
cvat/apps/iam/permissions.py (1)
37-37: LGTM! Consider extending type hint modernization.The type hint updates to use built-in collection types (
list[str],dict[str, Any]) align with PEP 585 and improve code readability.Consider updating other type hints in the file to use built-in collection types consistently. For example:
url: struser_id: intgroup_name: Optional[str]org_id: Optional[int]org_owner_id: Optional[int]org_role: Optional[str]scope: strobj: Optional[Any]These attributes in the
OpenPolicyAgentPermissionclass already use the correct format, but maintaining consistency throughout the codebase would be beneficial.Also applies to: 87-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
cvat/apps/events/tests/test_events.py(2 hunks)cvat/apps/iam/permissions.py(3 hunks)cvat/apps/iam/rules/tests/generate_tests.py(1 hunks)cvat/apps/iam/serializers.py(2 hunks)cvat/apps/iam/utils.py(1 hunks)cvat/apps/lambda_manager/tests/test_lambda.py(2 hunks)cvat/apps/lambda_manager/views.py(4 hunks)
🔇 Additional comments (10)
cvat/apps/iam/rules/tests/generate_tests.py (2)
10-13: LGTM! Good modernization of imports.
The change from typing.Sequence to collections.abc.Sequence follows modern Python best practices and aligns with PEP 585.
10-13: Verify Python version compatibility.
Since collections.abc.Sequence as a generic type requires Python 3.9+, let's verify the project's Python version requirements.
✅ Verification successful
Python version compatibility verified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Python version requirements in the project
# Check setup.py or pyproject.toml for Python version constraint
fd -e toml -e py setup pyproject | xargs rg -A 5 'python_requires|python_version'
# Check if there are any Python version related configuration files
fd -e python-version
Length of output: 1019
cvat/apps/iam/serializers.py (1)
35-35: LGTM: Type hints are accurate and helpful
The type hints correctly specify that these methods can handle both dict and User objects, with precise return type annotations.
Also applies to: 39-39
cvat/apps/events/tests/test_events.py (2)
45-45: LGTM! Type hint modernization looks good
The change from List[dict] to list[dict] aligns with PEP 585 for built-in generics while maintaining the same type safety.
8-8: Verify Python version compatibility for type hints
While modernizing type hints to use built-in types, Optional is still imported from typing. This suggests Python < 3.10 compatibility is needed (where Optional wasn't available as |None).
cvat/apps/iam/permissions.py (1)
11-11: LGTM! Good practice with imports.
The import changes follow best practices:
- Using
Sequencefromcollections.abc - Retaining
Any, Optional, TypeVarfromtypingas they don't have built-in equivalents
Also applies to: 14-14
cvat/apps/lambda_manager/views.py (3)
15-15: LGTM! Correct usage of typing imports
The imports of Any and Optional from the typing module are necessary as these types don't have built-in equivalents.
234-237: LGTM! Proper usage of built-in type hints
The change from Dict[str, Any] to dict[str, Any] aligns with PEP 585, using built-in types as generic types.
657-657: LGTM! Consistent type hint modernization
The changes correctly update the type hints to use built-in types while maintaining the nested structure:
dict[str, dict[str, Any]]for labelsdict[str, str]for mapping
Also applies to: 659-659
cvat/apps/lambda_manager/tests/test_lambda.py (1)
1443-1443: LGTM! Good modernization of type hints.
The change from Dict to dict aligns with Python 3.9+ type hinting best practices, where built-in collection types can be used directly for type annotations. This improves code readability while maintaining the same functionality.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8696 +/- ##
===========================================
+ Coverage 74.18% 74.25% +0.06%
===========================================
Files 401 401
Lines 43496 43496
Branches 3950 3950
===========================================
+ Hits 32269 32296 +27
+ Misses 11227 11200 -27
|
Motivation and context
This is a continuation of #8626.
How has this been tested?
Checklist
developbranch[ ] I have created a changelog fragment[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Bug Fixes
invokemethod to raiseValidationErrorwith appropriate messages for missing mandatory arguments during lambda function invocation.New Features
Documentation
Tests