Allow quality reports to regular users #8511
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 changes in this pull request enhance the quality report generation capabilities of the application by allowing regular users to compute quality reports, previously restricted to certain roles. Modifications include the introduction of new constants and predicates for permissions, updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant QualityReportManager
participant QualityReportPermission
User->>API: Request to create quality report
API->>QualityReportPermission: Check permissions
QualityReportPermission-->>API: Permissions valid
API->>QualityReportManager: Schedule quality check job
QualityReportManager-->>API: Job scheduled
API-->>User: Quality report creation success
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (9)
changelog.d/20241004_201336_mzhiltso_allow_quality_reports_for_regular_users.md (1)
1-4: LGTM! Consider adding a brief impact statement.The changelog entry accurately reflects the PR objectives and follows a good format. It correctly identifies the change as allowing regular users to compute quality reports via the Server API.
To provide more context, consider adding a brief statement about the impact or benefit of this change. For example:
### Changed - [Server API] Quality report computation is now allowed to regular users + (improves accessibility and empowers users to generate custom reports) (https://github.com/cvat-ai/cvat/pull/8511)This addition would help readers quickly understand the significance of the change.
cvat/apps/quality_control/serializers.py (1)
No existing tests found for quality report serializers
It appears that there are no test files related to quality reports or serializers. To ensure the reliability of the
QualityReportCreateSerializerand related components:
- Create unit tests for
QualityReportCreateSerializercovering scenarios wheretask_idis provided and omitted.- Develop integration tests to verify the end-to-end creation of quality reports, ensuring proper interaction with other serializers and system components.
🔗 Analysis chain
Line range hint
1-70: Ensure comprehensive testing of the quality report creation processWhile the change is isolated to the
QualityReportCreateSerializer, it may have implications for how other parts of the system interact with it. To ensure the robustness of this change:
- Update existing unit tests to cover scenarios where
task_idis not provided.- Add integration tests to verify the end-to-end process of creating quality reports for both admin and regular users.
- Test the interaction between
QualityReportCreateSerializerand other serializers in this file, especiallyQualityReportSerializerandQualitySettingsSerializer.To help with testing, you can use the following script to identify relevant test files:
This will help you locate the appropriate test files to update or add new tests.
Also applies to: 71-184
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find relevant test files for quality report functionality echo "Test files related to quality reports:" fd -e py test.*quality.*report echo "\nTest files related to serializers:" fd -e py test.*serializerLength of output: 340
Script:
#!/bin/bash # Description: Find relevant test files for quality report functionality using fd echo "Test files related to quality reports:" fd -e py -i 'test.*quality.*report' . echo "\nTest files related to serializers:" fd -e py -i 'test.*serializer' .Length of output: 244
tests/python/rest_api/test_quality_control.py (3)
587-609: LGTM: Well-structured parameterized test with a minor suggestion.The
test_user_create_report_in_sandbox_taskmethod is a well-implemented parameterized test that covers both allowed and forbidden scenarios for creating quality reports in sandbox tasks. It effectively uses helper methods to improve readability and maintainability.Consider extracting the task selection logic into a separate helper method to improve readability. For example:
def _get_sandbox_task(tasks, jobs, users): return next( t for t in tasks if t["organization"] is None and not users[t["owner"]["id"]]["is_superuser"] and not any(j for j in jobs if j["task_id"] == t["id"] and j["type"] == "ground_truth") )This would simplify the main test method and make it easier to understand at a glance.
611-663: LGTM: Comprehensive parameterized test with suggestions for improvement.The
test_user_create_report_in_org_taskmethod is a well-structured parameterized test that covers various scenarios for creating quality reports in organization tasks. It effectively uses helper methods and parameterization to test different user roles and permissions.Consider the following improvements to enhance readability and maintainability:
- Extract the task selection logic into a separate helper method:
def _get_org_task(tasks, jobs, users, is_org_member, is_task_staff, org_role, is_staff): for user in users: if user["is_superuser"]: continue task = next( ( t for t in tasks if t["organization"] is not None and is_task_staff(user["id"], t["id"]) == is_staff and is_org_member(user["id"], t["organization"], role=org_role) and not any( j for j in jobs if j["task_id"] == t["id"] and j["type"] == "ground_truth" ) ), None, ) if task is not None: return user, task return None, None
- Simplify the main test method using the extracted helper:
def test_user_create_report_in_org_task( self, tasks, jobs, users, is_org_member, is_task_staff, org_role, is_staff, allow, admin_user, ): user, task = self._get_org_task(tasks, jobs, users, is_org_member, is_task_staff, org_role, is_staff) assert task is not None, "No suitable task found for the test" self.create_gt_job(admin_user, task["id"]) if allow: self._test_create_report_200(user["username"], task["id"]) else: self._test_create_report_403(user["username"], task["id"])These changes will improve the readability of the test method and make it easier to maintain and update in the future.
573-663: Overall assessment: Well-structured tests with room for minor improvements.The new changes introduce well-designed test methods for quality report creation in various scenarios. The use of helper methods and parameterized tests enhances the overall test coverage and maintainability. While the implementation is generally sound, there are opportunities to improve readability and organization, particularly in the task selection logic for both sandbox and organization tasks.
Consider implementing the suggested improvements for extracting complex logic into helper methods. This will make the tests more readable and easier to maintain in the long run. Additionally, ensure consistent naming conventions and documentation for all test methods to further enhance the overall quality of the test suite.
cvat/apps/quality_control/rules/quality_reports.rego (1)
Line range hint
155-169: Optimize filter conditions to reduce redundancyThe filter conditions contain multiple repetitive checks for organization IDs and user IDs. Consider consolidating these conditions or using helper predicates to simplify the logic and enhance readability.
cvat/apps/quality_control/quality_reports.py (3)
13-13: Remove unused importsleepThe
sleepfunction from thetimemodule is imported but not used in the code. Unused imports can clutter the codebase and should be removed to improve maintainability.Apply this diff to remove the unused import:
-from time import sleep
2231-2231: ReplaceUsing
loggingmodule to log information, which provides more flexibility and control over log levels and outputs.Apply this diff to use logging instead:
+import logging ... -def get_quality_check_job(self, ...): + logger = logging.getLogger(__name__) ... - print(queue.fetch_job(rq_id).get_status()) + logger.info(f"Job {rq_id} status: {queue.fetch_job(rq_id).get_status()}")Ensure that
import loggingis at the top of the file.
2208-2230: Review lock mechanism and exception handling in job schedulingThe use of
get_rq_lock_by_userensures that only one job per user is scheduled at a time. However, it's important to ensure that this lock mechanism doesn't introduce deadlocks or block other important operations.Consider reviewing and testing the locking mechanism under high concurrency to ensure it behaves as expected. Additionally, ensure that exceptions within the locked block are properly handled to release the lock and avoid blocking subsequent operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- changelog.d/20241004_201336_mzhiltso_allow_quality_reports_for_regular_users.md (1 hunks)
- cvat/apps/iam/rules/utils.rego (1 hunks)
- cvat/apps/quality_control/permissions.py (5 hunks)
- cvat/apps/quality_control/quality_reports.py (5 hunks)
- cvat/apps/quality_control/rules/conflicts.rego (3 hunks)
- cvat/apps/quality_control/rules/quality_reports.rego (4 hunks)
- cvat/apps/quality_control/serializers.py (1 hunks)
- cvat/apps/quality_control/views.py (2 hunks)
- cvat/schema.yml (0 hunks)
- tests/python/rest_api/test_quality_control.py (1 hunks)
💤 Files with no reviewable changes (1)
- cvat/schema.yml
🧰 Additional context used
🪛 GitHub Check: CodeQL
cvat/apps/quality_control/permissions.py
[warning] 74-74: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🔇 Additional comments (13)
cvat/apps/iam/rules/utils.rego (1)
61-61: New constantVIEW_STATUSaddedThe addition of
VIEW_STATUS := "view:status"constant is consistent with the existing naming conventions for permission scopes in this file. This new constant likely represents a new permission for viewing status information, which aligns with the PR objective of allowing regular users to compute quality reports.However, there are a few points to consider:
- The PR objectives mention allowing users to "compute" quality reports, but the constant only implies "viewing" status. It's unclear if this fully addresses the requirement.
- There are no accompanying changes to implement the usage of this new constant.
To ensure this change is properly implemented and used, let's check for its usage:
✅ Verification successful
Verified Usage of
VIEW_STATUSConstantThe
VIEW_STATUSconstant defined incvat/apps/iam/rules/utils.regois utilized incvat/apps/quality_control/rules/quality_reports.regoasinput.scope == utils.VIEW_STATUS. This usage aligns with the PR objective of allowing regular users to compute quality reports for tasks by defining the necessary permission scope.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the new VIEW_STATUS constant rg --type rego "VIEW_STATUS" # Search for any new rules or changes related to quality reports or status viewing rg --type rego -i "quality report|view status"Length of output: 135
Script:
#!/bin/bash # Search for usage of the new VIEW_STATUS constant in .rego files rg "VIEW_STATUS" --glob "*.rego" # Search for any new rules or changes related to quality reports or status viewing in .rego files rg -i "quality report|view status" --glob "*.rego"Length of output: 235
cvat/apps/quality_control/serializers.py (1)
70-70: LGTM! Consider adding explanatory comment and verify impact.The change to make
task_idoptional aligns with the PR objective of allowing regular users to compute quality reports for tasks. This is a good modification.However, I have a few suggestions:
Add a comment explaining why this field is now optional. This will help future developers understand the reasoning behind this change.
Verify that this change doesn't break existing functionality for admin users.
Check if additional validation logic is needed elsewhere in the codebase to handle cases where
task_idis not provided.To verify the impact of this change, you can run the following script:
#!/bin/bash # Description: Check for usages of QualityReportCreateSerializer and potential places that might need additional validation # Search for usages of QualityReportCreateSerializer echo "Usages of QualityReportCreateSerializer:" rg "QualityReportCreateSerializer" --type python # Search for potential validation points related to task_id in views echo "\nPotential validation points for task_id:" rg "task_id.*request\.data" --type pythonThis script will help identify areas of the codebase that might be affected by this change and where additional validation might be needed.
tests/python/rest_api/test_quality_control.py (2)
573-574: LGTM: Well-structured helper method.The
_test_create_report_200method is a concise and well-named helper function for testing the successful creation of a quality report. It follows good practices for test helper methods.
576-585: LGTM: Well-implemented error case helper method.The
_test_create_report_403method is a well-structured helper function for testing the forbidden access scenario when creating a quality report. It uses appropriate context management for the API client and correctly asserts the expected HTTP status code.cvat/apps/quality_control/rules/quality_reports.rego (1)
103-107: Verify permission levels for creating and viewing in sandbox environmentIn the rule:
allow if { input.scope in {utils.CREATE, utils.VIEW} utils.is_sandbox is_task_staff utils.has_perm(utils.WORKER) }Ensure that assigning the
utils.WORKERpermission is appropriate for regular users in the sandbox environment who need to create or view quality reports. Adjust the permission level if necessary to match the intended access control.cvat/apps/quality_control/rules/conflicts.rego (2)
7-7: Verify the necessity of importingdata.tasks.The import statement
import data.taskswas added. Please confirm thatdata.tasksis actually used within this policy. If it's unused, consider removing it to keep the code clean and avoid unnecessary dependencies.
97-102: Review access control conditions forVIEWscope in sandbox environment.In this
allowrule, access is granted if the user isis_task_staffand has theutils.has_perm(utils.WORKER)permission. Verify that regular users who are task staff have theWORKERpermission, or adjust the permission check accordingly to match the intended access control.cvat/apps/quality_control/permissions.py (3)
21-21: Addition oftask_idattribute enhances permission handlingThe introduction of
task_id: Optional[int]in theQualityReportPermissionclass allows for more granular permission checks related to specific tasks.
33-33: Consistent use ofVIEW_STATUSscope improves maintainabilityUsing
cls.Scopes.VIEW_STATUSinstead of a string literal enhances code consistency and reduces the risk of typos.
Line range hint
139-154: Verify handling of potentialNonevalues in resource dataWhen constructing the
datadictionary, attributes liketask,task.owner,task.assignee,task.project, and their respective owners and assignees may beNone. Ensure that the code accounts for these possibilities to preventAttributeError.cvat/apps/quality_control/views.py (1)
275-276: Ensure 'schedule_custom_quality_check_job' is correctly implementedThe method call has been updated to
schedule_custom_quality_check_jobwith the additionalrequestparameter. Verify that this method is defined inQualityReportUpdateManagerand correctly handles therequest,task, anduser_idparameters.To confirm the method's existence and signature, you can run the following script:
✅ Verification successful
'schedule_custom_quality_check_job' is correctly implemented
- The method
schedule_custom_quality_check_jobis defined inQualityReportUpdateManagerwithincvat/apps/quality_control/quality_reports.pywith the parametersrequest,task, anduser_idas required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'schedule_custom_quality_check_job' is defined with the correct parameters. # Test: Search for the method definition. rg --type python -A 5 $'def schedule_custom_quality_check_job(request, task, user_id)' --path 'cvat/apps/quality_control/'Length of output: 556
Script:
#!/bin/bash # Description: Verify that 'schedule_custom_quality_check_job' is defined with the correct parameters. # Corrected Test: Search for the method definition without using the invalid '--path' flag. rg --type python -A 5 'def schedule_custom_quality_check_job\(request, task, user_id\)' cvat/apps/quality_control/Length of output: 147
Script:
#!/bin/bash # Description: Verify that 'schedule_custom_quality_check_job' is defined with the correct parameters. # Corrected Test: Use the correct type identifier 'py' instead of 'python'. rg --type py -A 5 'def schedule_custom_quality_check_job\(request, task, user_id\)' cvat/apps/quality_control/Length of output: 112
Script:
#!/bin/bash # Description: Broadly search for the definition of 'schedule_custom_quality_check_job' without file type restrictions. # Broad Search: Look for any definition of the method, regardless of file type. rg 'def schedule_custom_quality_check_job' cvat/apps/quality_control/Length of output: 210
cvat/apps/quality_control/quality_reports.py (2)
26-28: Verify necessity of newly added importsThe imports for
RqQueue,RqJob, andRqSchedulerhave been added. Ensure that these classes are used in the code. Unused imports can be removed to keep the code clean.Run the following script to check for their usage:
✅ Verification successful
Imports are used and necessary
The imports for
RqQueue,RqJob, andRqSchedulerare utilized inquality_reports.pyas follows:
RqSchedulerin the_get_schedulermethod.RqQueuein the_get_queuemethod.RqJobin theget_quality_check_jobandis_custom_quality_check_jobmethods.These imports are necessary and should be retained.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of RqQueue, RqJob, and RqScheduler # Test: Search for usage of these classes in the codebase rg --type py '\b(RqQueue|RqJob|RqScheduler)\b' cvat/apps/quality_control/ -A 2 # Expected result: Instances where these classes are used should be found.Length of output: 1588
55-55: Verify usage of newly imported utility functionsThe functions
define_dependent_job,get_rq_job_meta, andget_rq_lock_by_userare imported. Ensure that these functions are actually used in the codebase. Unused imports should be removed to keep the code clean.Run the following script to check for their usage:
✅ Verification successful
Imported utility functions are utilized appropriately
The imported functions
define_dependent_job,get_rq_job_meta, andget_rq_lock_by_userare used withinquality_reports.py. No unused imports detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of imported utility functions # Test: Search for usage of the functions in the codebase rg --type py '\b(define_dependent_job|get_rq_job_meta|get_rq_lock_by_user)\b' cvat/apps/quality_control/ -A 2 # Expected result: Instances where these functions are used should be found.Length of output: 1344
| @@ -58,6 +58,7 @@ IMPORT_DATASET := "import:dataset" | |||
| IMPORT_BACKUP := "import:backup" | |||
| EXPORT_BACKUP := "export:backup" | |||
| UPDATE_ORG := "update:organization" | |||
| VIEW_STATUS := "view:status" | |||
There was a problem hiding this comment.
Incomplete implementation of PR objectives
While the addition of the VIEW_STATUS constant is a step in the right direction, it appears that the implementation is incomplete based on the PR objectives. The PR aims to "allow regular users to compute quality reports for tasks," but the current changes only add a constant for viewing status.
To fully implement the desired functionality, consider the following:
- Add rules or functions that use the new
VIEW_STATUSconstant to grant permissions to regular users. - Implement the actual computation of quality reports, which may involve changes in other files.
- Ensure that the permission check is integrated into the quality report computation process.
Would you like assistance in drafting the additional rules or functions needed to complete this implementation?
cvat/apps/quality_control/views.py
Outdated
| @@ -290,7 +290,7 @@ def create(self, request, *args, **kwargs): | |||
| if ( | |||
| not rq_job | |||
| or not QualityReportPermission.create_scope_check_status( | |||
| request, job_owner_id=rq_job.meta["user_id"] | |||
| request, job_owner_id=rq_job.meta["user"]["id"] | |||
There was a problem hiding this comment.
Handle potential KeyError when accessing 'rq_job.meta["user"]["id"]'
Accessing rq_job.meta["user"]["id"] assumes that the meta dictionary always contains the user key with an id. If either key is missing, a KeyError will be raised, leading to an unhandled exception. Consider adding error handling to manage this possibility.
Apply this diff to safely access the nested id key:
- request, job_owner_id=rq_job.meta["user"]["id"]
+ try:
+ user_id = rq_job.meta["user"]["id"]
+ except KeyError:
+ raise ValidationError("User ID not found in job metadata")
+ request, job_owner_id=user_id📝 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.
| request, job_owner_id=rq_job.meta["user"]["id"] | |
| try: | |
| user_id = rq_job.meta["user"]["id"] | |
| except KeyError: | |
| raise ValidationError("User ID not found in job metadata") | |
| request, job_owner_id=user_id |
| rq_job = queue.fetch_job(rq_id) | ||
| if rq_job and rq_job.get_status(refresh=False) in ( | ||
| rq.job.JobStatus.QUEUED, | ||
| rq.job.JobStatus.STARTED, | ||
| rq.job.JobStatus.SCHEDULED, | ||
| ): | ||
| raise self.JobAlreadyExists() |
There was a problem hiding this comment.
Handle potential exceptions when fetching job status
Fetching the job status may raise exceptions if the job does not exist or if there's a connection issue with the queue. It's important to handle these exceptions to prevent crashes and provide meaningful error messages.
Consider adding exception handling around the job fetching and status checking:
try:
rq_job = queue.fetch_job(rq_id)
if rq_job and rq_job.get_status(refresh=False) in (
rq.job.JobStatus.QUEUED,
rq.job.JobStatus.STARTED,
rq.job.JobStatus.SCHEDULED,
):
raise self.JobAlreadyExists()
except Exception as e:
logger.error(f"Failed to fetch job status for {rq_id}: {e}")
# Handle the exception as appropriate| class JobAlreadyExists(QualityReportsNotAvailable): | ||
| def __str__(self): | ||
| return "Quality computation job for this task already enqueued" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust inheritance of JobAlreadyExists exception
The JobAlreadyExists exception currently inherits from QualityReportsNotAvailable, which might not be semantically appropriate. Since JobAlreadyExists represents a different kind of error (a job is already enqueued), it would be clearer to inherit from a more general exception class like Exception or define a new base exception class for job scheduling errors.
Apply this diff to adjust the inheritance:
-class JobAlreadyExists(QualityReportsNotAvailable):
+class JobAlreadyExists(Exception):📝 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.
| class JobAlreadyExists(QualityReportsNotAvailable): | |
| def __str__(self): | |
| return "Quality computation job for this task already enqueued" | |
| class JobAlreadyExists(Exception): | |
| def __str__(self): | |
| return "Quality computation job for this task already enqueued" |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8511 +/- ##
===========================================
- Coverage 74.33% 74.28% -0.05%
===========================================
Files 397 397
Lines 43139 43181 +42
Branches 3905 3905
===========================================
+ Hits 32066 32077 +11
- Misses 11073 11104 +31
|
|
|
||
| is_task_owner(task_data, auth_data) if { | ||
| task_data.owner.id == auth_data.user.id | ||
| } |
There was a problem hiding this comment.
Need to move to engine at some point
| allow if { | ||
| input.scope == utils.VIEW | ||
| utils.is_sandbox | ||
| quality_utils.is_task_staff(input.resource.task, input.resource.project, input.auth) | ||
| utils.has_perm(utils.WORKER) | ||
| } | ||
|
|
||
| allow if { | ||
| input.scope == utils.VIEW | ||
| input.auth.organization.id == input.resource.organization.id | ||
| utils.has_perm(utils.USER) | ||
| organizations.has_perm(organizations.MAINTAINER) | ||
| } | ||
|
|
||
| allow if { | ||
| input.scope == utils.VIEW | ||
| quality_utils.is_task_staff(input.resource.task, input.resource.project, input.auth) | ||
| input.auth.organization.id == input.resource.organization.id | ||
| utils.has_perm(utils.WORKER) | ||
| organizations.has_perm(organizations.WORKER) | ||
| } | ||
|
|
There was a problem hiding this comment.
@zhiltsov-max, I've not found any place where these rules are used. Could you let me know why you added them? Are you going to introduce the GET /api/quality/conflicts/<id> endpoint? 👀
Marishka17
force-pushed
the
zm/allow-quality-reports-for-regular-users
branch
from
d18480d
to
678cae5
Compare
…ts to check access rights when checking status of report creation
There was a problem hiding this comment.
LGTM
I've not changed permission rules used when checking the status of an rq job in this PR because right now a new rq job to perform quality report computation will be created for each user that has access to a task (because the rq id template includes user_id). In this case, it seems logical to allow only the rq job owner to check the report creation progress.
However, I don't think it's a desirable approach to allow different users to compute quality reports for the same task at the same moment. I've not changed it in this PR because I cannot figure out why Maxim decided so. I suggest waiting for Maxim's response and fixing it in the next PR.
… POST /api/quality/reports
Marishka17
force-pushed
the
zm/allow-quality-reports-for-regular-users
branch
from
bfe1488
to
13ce04b
Compare
|
Motivation and context
How has this been tested?
Unit tests
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Tests