Refactor RQId / RQIdManager #8306
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes in the CVAT application significantly enhance the management of request identifiers by introducing new classes and refactoring existing code. This update replaces the previous approach of using string-based identifiers with a more structured object-oriented design, utilizing enumerations for actions and targets. These improvements promote type safety, clarity, and maintainability across various modules involved in data import and export operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RequestHandler
participant RQId
participant Database
User->>RequestHandler: Initiate request (Import/Export)
RequestHandler->>RQId: Create request ID (Action, Target)
RQId-->>RequestHandler: Return request ID
RequestHandler->>Database: Process Import/Export with ID
Database-->>RequestHandler: Return result
RequestHandler-->>User: Send 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 as PR comments)
Additionally, you can add 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: 1
Outside diff range, codebase verification and nitpick comments (1)
cvat/apps/engine/rq_job_handler.py (1)
Line range hint
93-135: Improve error handling inparsemethod.The
parsemethod could benefit from more specific error messages to aid debugging and user understanding.except ValueError as ex: raise ValueError(f"Invalid RQ ID format: {rq_id!r}. Error: {str(ex)}") from ex except Exception as ex: raise ValueError(f"An unexpected error occurred while parsing RQ ID: {rq_id!r}. Error: {str(ex)}") from ex
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- cvat/apps/engine/background_operations.py (3 hunks)
- cvat/apps/engine/backup.py (3 hunks)
- cvat/apps/engine/mixins.py (4 hunks)
- cvat/apps/engine/models.py (2 hunks)
- cvat/apps/engine/permissions.py (2 hunks)
- cvat/apps/engine/rq_job_handler.py (3 hunks)
- cvat/apps/engine/serializers.py (5 hunks)
- cvat/apps/engine/task.py (2 hunks)
- cvat/apps/engine/views.py (21 hunks)
Additional comments not posted (28)
cvat/apps/engine/rq_job_handler.py (3)
35-35: MarkingRQIdas immutable is a good practice.The use of
@attrs.frozen()ensures that instances ofRQIdare immutable, which is beneficial for maintaining data integrity.
37-58: Use of enumerations improves type safety.Replacing string types with enumerations for
action,target, andsubresourceenhances type safety and reduces the risk of errors due to invalid values.
61-63: Ensure consistent validation logic.The
__attrs_post_init__method enforces validation logic foractionandtarget. Ensure that similar validation is applied elsewhere if needed.cvat/apps/engine/mixins.py (2)
278-281: Use ofRQIdenhances clarity and maintainability.The refactoring to use
RQIdfor request identifiers improves the structure and clarity of the code.
Line range hint
479-498: Renaming torq_id_factoryimproves semantic clarity.The renaming of
rq_id_templatetorq_id_factorybetter reflects its purpose and enhances code readability.cvat/apps/engine/background_operations.py (2)
347-355: Use ofRQIdimproves type safety and clarity.The transition to using
RQIdfor request identifiers enhances type safety and reduces the likelihood of errors.
586-592: Use ofRQIdenhances consistency and maintainability.The adoption of
RQIdfor request identifiers aligns with the refactoring goals and improves the consistency of the codebase.cvat/apps/engine/models.py (4)
1199-1204: Good use of Django'sTextChoices.The
RequestStatusclass clearly defines the possible statuses for a request, enhancing readability and maintainability.
1206-1209: Well-defined action choices.The
RequestActionclass usesTextChoicesto define actions, which improves type safety and reduces the use of hardcoded strings.
1211-1214: Clear target definitions withTextChoices.The
RequestTargetclass effectively usesTextChoicesto specify request targets, aligning with best practices for clarity and maintainability.
1216-1219: Structured subresource definitions.The
RequestSubresourceclass provides a structured way to handle subresources, which promotes consistency and reduces errors.cvat/apps/engine/backup.py (3)
42-46: Improved import statements with new enumerations.The import of
RQIdand related enumerations (RequestAction,RequestTarget,RequestSubresource) enhances code clarity and reduces the risk of errors associated with hardcoded strings.
1018-1021: Refactored request ID generation inimport_project.The use of the
RQIdclass to generate request IDs is a clear improvement. It encapsulates the parameters and uses the.render()method to produce the final ID, promoting a more structured approach.
1044-1047: Refactored request ID generation inimport_task.Similar to
import_project, theimport_taskfunction now uses theRQIdclass for generating request IDs. This change enhances maintainability and aligns with the refactored design.cvat/apps/engine/permissions.py (2)
1230-1230: Consistent use oftargetinstead ofresource.The change from
parsed_rq_id.resourcetoparsed_rq_id.targetis consistent with the refactor. Ensure this change is applied throughout the file.Also applies to: 1239-1239
1244-1244: Error message update aligns with refactor.The error message now correctly references
target, maintaining consistency with the refactor.cvat/apps/engine/task.py (2)
30-30: LGTM! Imports for RequestAction and RequestTarget.The addition of
RequestActionandRequestTargetimports aligns with the refactoring goals to use enums for better clarity and maintainability.
53-53: LGTM! Improved request ID generation.The change to generate the request ID using the
RQIdconstructor withRequestAction.CREATEandRequestTarget.TASKenhances clarity and maintainability.Additionally, ensure that this new usage of
RQIdis consistent across the codebase.Verification successful
Consistent Usage of
RQIdConstructor Across CodebaseThe
RQIdconstructor is consistently used with enums (RequestActionandRequestTarget) across multiple files, confirming the refactoring's alignment with the codebase's design goals.
- Files with consistent usage:
cvat/apps/engine/rq_job_handler.pycvat/apps/engine/task.pycvat/apps/engine/backup.pycvat/apps/engine/mixins.pycvat/apps/engine/views.pycvat/apps/engine/background_operations.pyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `RQId` constructor across the codebase to ensure consistency. # Test: Search for the `RQId` constructor usage. Expect: Consistent usage with enums. rg --type python -A 5 $'RQId('Length of output: 92
Script:
#!/bin/bash # Description: Verify the usage of the `RQId` constructor across the codebase to ensure consistency. # Test: Search for the `RQId` constructor usage. Expect: Consistent usage with enums. rg --type python -A 5 'RQId('Length of output: 92
Script:
#!/bin/bash # Description: Verify the usage of the `RQId` constructor across the codebase to ensure consistency. # Test: Search for the `RQId` constructor usage. Expect: Consistent usage with enums. rg --type python -A 5 'RQId\('Length of output: 63
Script:
#!/bin/bash # Description: Verify the usage of the `RQId` constructor across the codebase to ensure consistency. # Test: Search for the `RQId` constructor usage. Expect: Consistent usage with enums. rg --type py -A 5 'RQId\('Length of output: 3591
cvat/apps/engine/serializers.py (3)
2206-2206: Use of centralized choice field is approved.The change to use
models.RequestTarget.choicesimproves maintainability by centralizing choice definitions.
2234-2234: Centralized choice field usage is approved.The update to use
models.RequestStatus.choicesenhances consistency and maintainability by centralizing choice definitions.
2302-2305: Use of enumerations for actions and subresources is approved.Replacing string literals with
models.RequestAction.IMPORTandmodels.RequestSubresource.BACKUPenhances type safety and maintainability.cvat/apps/engine/views.py (7)
283-284: Transition toIMPORT_RQ_ID_FACTORYis approved.The use of
functools.partialforIMPORT_RQ_ID_FACTORYenhances flexibility and readability by allowing dynamic request ID generation.
861-862: Transition toIMPORT_RQ_ID_FACTORYis approved.The use of
functools.partialforIMPORT_RQ_ID_FACTORYinTaskViewSetenhances flexibility and readability, similar to the change inProjectViewSet.
1731-1732: Transition toIMPORT_RQ_ID_FACTORYis approved.The use of
functools.partialforIMPORT_RQ_ID_FACTORYinJobViewSetaligns with the changes in other viewsets, enhancing flexibility and readability.
Line range hint
3029-3043: Use ofrq_id_factoryenhances modularity.The transition to using
rq_id_factoryfor generatingrq_idimproves the modularity and reusability of the code.
Line range hint
3145-3157: Use ofrq_id_factoryenhances modularity.The transition to using
rq_id_factoryfor generatingrq_idin_import_project_datasetimproves the modularity and reusability of the code, consistent with changes in_import_annotations.
3326-3328: Consistent use ofRQId.parseis approved.The use of
RQId.parsefor parsingrq_idensures consistency with the refactoredRQIdclass, aligning with the refactoring objectives.
3363-3365: Consistent use ofRQId.parseis approved.The use of
RQId.parsefor parsingrq_idin_get_rq_job_by_idensures consistency with the refactoredRQIdclass, similar to_get_rq_jobs_from_queue.
| def render( | ||
| self, | ||
| ) -> str: | ||
| if "import" == action: | ||
| return f"{action}:{resource}-{identifier}-{subresource}" | ||
| elif "export" == action: | ||
| if anno_format is None: | ||
| common_prefix = f"{self.action}:{self.target}-{self.identifier}" | ||
|
|
||
| if RequestAction.IMPORT == self.action: | ||
| return f"{common_prefix}-{self.subresource}" | ||
| elif RequestAction.EXPORT == self.action: | ||
| if self.format is None: | ||
| return ( | ||
| f"{action}:{resource}-{identifier}-{subresource}-by-{user_id}" | ||
| f"{common_prefix}-{self.subresource}-by-{self.user_id}" | ||
| ) | ||
| format_to_be_used_in_urls = anno_format.replace(" ", "_").replace(".", "@") | ||
| return f"{action}:{resource}-{identifier}-{subresource}-in-{format_to_be_used_in_urls}-format-by-{user_id}" | ||
| elif "create" == action: | ||
| assert "task" == resource | ||
| return f"{action}:{resource}-{identifier}" | ||
|
|
||
| format_to_be_used_in_urls = self.format.replace(" ", "_").replace(".", "@") | ||
| return f"{common_prefix}-{self.subresource}-in-{format_to_be_used_in_urls}-format-by-{self.user_id}" | ||
| elif RequestAction.CREATE == self.action: | ||
| return common_prefix | ||
| else: | ||
| raise ValueError(f"Unsupported action {action!r} was found") | ||
| assert False, f"Unsupported action {self.action!r} was found" |
There was a problem hiding this comment.
Refactor render method for clarity and maintainability.
The render method is well-structured but could benefit from further refactoring to reduce complexity and improve readability.
def render(self) -> str:
common_prefix = f"{self.action}:{self.target}-{self.identifier}"
if self.action == RequestAction.IMPORT:
return f"{common_prefix}-{self.subresource}"
if self.action == RequestAction.EXPORT:
format_str = f"-in-{self.format.replace(' ', '_').replace('.', '@')}-format" if self.format else ""
return f"{common_prefix}-{self.subresource}{format_str}-by-{self.user_id}"
if self.action == RequestAction.CREATE:
return common_prefix
raise AssertionError(f"Unsupported action {self.action!r} was found")
SpecLad
force-pushed
the
rq-id-refactor
branch
from
b1cc426
to
ea83864
Compare
|
@Marishka17 Mind taking a look? |
cvat/apps/engine/rq_job_handler.py
Outdated
| ) | ||
|
|
||
| def __attrs_post_init__(self) -> None: | ||
| if self.action == RequestAction.CREATE and self.target != RequestTarget.TASK: |
There was a problem hiding this comment.
there are also a few more cases to be handled:
| if self.action == RequestAction.CREATE and self.target != RequestTarget.TASK: | |
| if ( | |
| self.action == RequestAction.CREATE | |
| and self.target != RequestTarget.TASK | |
| or self.action == RequestAction.IMPORT | |
| and ( | |
| self.target != RequestTarget.PROJECT | |
| and self.subresource == RequestSubresource.DATASET | |
| or self.target == RequestTarget.PROJECT | |
| and self.subresource == RequestSubresource.ANNOTATIONS | |
| ) | |
| or self.action in (RequestAction.EXPORT, RequestAction.IMPORT) | |
| and self.target == RequestTarget.JOB | |
| and self.subresource == RequestSubresource.BACKUP | |
| ): |
There was a problem hiding this comment.
I decided to redo this function entirely. My original check was adapted from an assert in RQIdManager.build, but on second thought, I don't think it's really needed, because it's not RQId's responsibility to decide which actions can be applied to which resources.
OTOH, it is its responsibility to make sure that the ID can be rendered to a string that can then be parsed back to the same ID. So I added checks that make sure certain fields are present/absent, depending on the action.
* Merge these into one class. These classes clearly deal with the same concept, so it doesn't make sense to divide the logic into two classes. * Turn `build` into an instance method (`render`). That way, the validation logic can be reused between it and the `RQId` constructor. Adjust the fields so that the first three fields can be specified as positional arguments. * Make the class frozen (I don't see a compelling case to mutate it). * Change string fields into corresponding enums. This reduces the amount of hardcoded string literals everywhere. Note that I had to move the enums into the `models` module to avoid a circular import. * Rename `resource` to `target`, because that's the name of the enum and the corresponding field in the API.
`RQId` wants an integer, but `pk` is a string.
SpecLad
force-pushed
the
rq-id-refactor
branch
from
83bc384
to
b80790c
Compare
|
Motivation and context
Merge these into one class. These classes clearly deal with the same concept, so it doesn't make sense to divide the logic into two classes.
Turn
buildinto an instance method (render). That way, the validation logic can be reused between it and theRQIdconstructor. Adjust the fields so that the first three fields can be specified as positional arguments.Make the class frozen (I don't see a compelling case to mutate it).
Change string fields into corresponding enums. This reduces the amount of hardcoded string literals everywhere. Note that I had to move the enums into the
modelsmodule to avoid a circular import.Rename
resourcetotarget, because that's the name of the enum and the corresponding field in the API.How has this been tested?
Unit tests.
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
New Features
RequestStatus,RequestAction,RequestTarget, andRequestSubresource.Improvements
Bug Fixes
Documentation