Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add REST API tests for /requests API && test both versions of the export API #8216

Merged
merged 46 commits into from
Aug 21, 2024

Conversation

Marishka17
Copy link
Contributor

@Marishka17 Marishka17 commented Jul 24, 2024

Motivation and context

This PR fixes the following issues:

  • [export API v1] do not reinitialize dataset export process when downloading a result file if a resource (project|task|job) has been updated since the first initialized export request
  • [export API v1] return rq_id for all requests with 202 status code (not only for initialization requests)
  • [requests API] Fixed filtering by format && added resource to allowed filters

REST API tests updates:

  • Added tests to check requests filtration using simple filters
  • Added tests to check specific requests retrieving
  • Updated all tests that export project|task|job datasets|annotations|backups:
    • to test both API versions (including API mixing)
    • to use only appropriate resources by checking the default export location
  • Added fixtures to filter projects/tasks assets
  • Updated default target|source buckets to import/export bucket to exclude the same bucket usage as a data source in several tests (when bucket content is used as task data) and as a bucket for results

How has this been tested?

Checklist

  • I submit my changes into the develop branch
  • 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

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

Summary by CodeRabbit

  • New Features

    • Enhanced job handling for exports, improving error management and job state tracking.
    • Introduced a new resource field in the request handling system to improve data categorization.
    • Added new filtering capabilities for API queries, allowing users to filter by the resource field.
  • Bug Fixes

    • Improved status checks and handling for job requests.
    • Introduced exception handling for forbidden access during project backup attempts.
  • Tests

    • Refactored test suites to improve coverage and ensure compatibility across versions with new methods and exception handling.
    • New tests added to validate request handling functionality.

Copy link
Contributor

coderabbitai bot commented Jul 24, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The recent changes enhance the CVAT codebase by refining job handling, updating request filtering, and improving dataset export functionality. Key updates include introducing optional typing for job parameters, consolidating status checks, and implementing new exception handling for better error management. The testing suite has also been revamped to support API versioning and ensure robust coverage for various scenarios, making the code more maintainable and adaptable.

Changes

Files Change Summary
cvat/apps/engine/background.py Enhanced job handling logic with optional typing, consolidated status checks, and introduced new exception handling.
cvat/apps/engine/filters.py Updated request parameter type from HttpRequest to Request, enhancing filtering clarity.
cvat/apps/engine/views.py Added resource field to RequestViewSet class, enhancing API capabilities.
cvat/schema.yml Introduced a new filter field resource for improved API query capabilities.
tests/python/rest_api/test_jobs.py Refactored and improved dataset export tests; enhanced configurability with API versioning.
tests/python/rest_api/test_projects.py Enhanced project backup tests, introduced exception handling, and refactored to reduce duplication.
tests/python/rest_api/utils.py Separated export functionality by API version and added new import/export functions.
tests/python/shared/fixtures/data.py Added filter_assets function for dynamic resource filtering in tests.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant API
    participant JobManager
    participant Database

    User->>API: Request dataset export
    API->>JobManager: Start export job
    JobManager->>Database: Save job state
    Database-->>JobManager: Job state confirmed
    JobManager->>API: Return job ID
    API-->>User: Provide job ID

    User->>API: Check job status
    API->>JobManager: Get job status
    JobManager->>Database: Retrieve job state
    Database-->>JobManager: Return job state
    JobManager->>API: Send job status
    API-->>User: Provide job status
Loading

🐰 In fields of code, I hop with glee,
Changes and fixes, as sweet as can be!
With each little tweak, our tasks are anew,
Like carrots in gardens, they flourish and grew!
So here’s to the work, so clever and bright,
A warren of joy in the code's gentle light! 🌟


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@Marishka17 Marishka17 marked this pull request as ready for review July 26, 2024 12:01
@Marishka17 Marishka17 changed the title [WIP] Add REST API tests for /requests API && test both versions of the export API Add REST API tests for /requests API && test both versions of the export API Jul 26, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 41d2f04 and 4129851.

Files selected for processing (10)
  • cvat/apps/engine/background.py (16 hunks)
  • cvat/apps/engine/filters.py (4 hunks)
  • cvat/apps/engine/mixins.py (1 hunks)
  • cvat/apps/engine/views.py (4 hunks)
  • cvat/schema.yml (2 hunks)
  • tests/python/rest_api/test_jobs.py (7 hunks)
  • tests/python/rest_api/test_projects.py (16 hunks)
  • tests/python/rest_api/test_requests.py (1 hunks)
  • tests/python/rest_api/test_tasks.py (7 hunks)
  • tests/python/rest_api/utils.py (2 hunks)
Files skipped from review due to trivial changes (1)
  • cvat/apps/engine/mixins.py
Additional comments not posted (85)
tests/python/rest_api/test_requests.py (17)

27-28: LGTM!

The validate_action function is correct and concise.


31-32: LGTM!

The validate_subresource function is correct and concise.


35-36: LGTM!

The validate_status function is correct and concise.


39-40: LGTM!

The validate_id function is correct and concise.


43-44: LGTM!

The validate_resource function is correct and concise.


47-48: LGTM!

The validate_format function is correct and concise.


55-60: LGTM!

The setup method is correct and concise.


62-67: LGTM!

The create_resources method is correct and concise.


69-164: LGTM!

The make_requests method is correct and comprehensive.


166-180: LGTM!

The create_tasks method is correct and concise.


182-193: LGTM!

The create_projects method is correct and concise.


195-200: LGTM!

The get_job_ids method is correct and concise.


215-229: LGTM!

The test_list_request_with_simple_filter method is correct and comprehensive.


236-241: LGTM!

The _test_get_request_200 method is correct and concise.


243-247: LGTM!

The _test_get_request_403 method is correct and concise.


251-290: LGTM!

The test_owner_can_retrieve_request method is correct and comprehensive.


293-314: LGTM!

The test_non_owner_cannot_retrieve_request method is correct and comprehensive.

tests/python/rest_api/utils.py (13)

Line range hint 25-82: LGTM!

The export_v1 function is correct and comprehensive.


85-155: LGTM!

The export_v2 function is correct and comprehensive.


158-186: LGTM!

The export_dataset function is correct and comprehensive.


190-192: LGTM!

The export_project_dataset function is correct and concise.


195-197: LGTM!

The export_task_dataset function is correct and concise.


200-202: LGTM!

The export_job_dataset function is correct and concise.


205-220: LGTM!

The export_backup function is correct and comprehensive.


223-225: LGTM!

The export_project_backup function is correct and concise.


228-230: LGTM!

The export_task_backup function is correct and concise.


233-277: LGTM!

The import_resource function is correct and comprehensive.


279-287: LGTM!

The import_backup function is correct and concise.


290-292: LGTM!

The import_project_backup function is correct and concise.


295-297: LGTM!

The import_task_backup function is correct and concise.

cvat/apps/engine/filters.py (2)

415-419: LGTM!

The filter_queryset method in class NonModelSimpleFilter is correct and concise.


Line range hint 468-472: LGTM!

The filter_queryset method in class NonModelOrderingFilter is correct and concise.

cvat/apps/engine/background.py (5)

86-86: Ensure rq_job parameter is optional.

The method _handle_rq_job_v1 now accepts an optional rq_job parameter, which is a good change to handle cases where the job may not exist.


89-89: Ensure rq_job parameter is optional.

The method _handle_rq_job_v2 now accepts an optional rq_job parameter, which is a good change to handle cases where the job may not exist.


90-105: Ensure proper handling of job statuses.

The method _handle_rq_job_v2 includes logic for handling different job statuses and actions. Ensure that all possible job statuses are handled correctly.


148-155: Ensure proper handling of job cancellation and re-enqueueing.

The method cancel_and_reenqueue encapsulates the logic for canceling a job and re-adding it to the queue. Ensure that this method is invoked correctly in all required places.


175-175: Ensure Request object is used correctly.

The method __init__ in DatasetExportManager now accepts a Request object instead of HttpRequest, which aligns with the rest framework's request handling.

tests/python/rest_api/test_projects.py (27)

189-189: Class renamed to TestGetPostProjectBackup.

The class TestGetProjectBackup has been renamed to TestGetPostProjectBackup to reflect the inclusion of both GET and POST operations for project backups.


198-214: Ensure api_version parameter is used correctly in _test_can_get_project_backup.

The method _test_can_get_project_backup has been updated to include api_version as a parameter, which allows for testing different API versions.


215-224: Ensure api_version parameter is used correctly in _test_cannot_get_project_backup.

The method _test_cannot_get_project_backup has been updated to include api_version as a parameter, which allows for testing different API versions.


225-228: Ensure api_version parameter is used correctly in test_admin_can_get_project_backup.

The method test_admin_can_get_project_backup has been updated to include api_version as a parameter, which allows for testing different API versions.


231-243: Ensure api_version parameter is used correctly in test_user_cannot_get_project_backup.

The method test_user_cannot_get_project_backup has been updated to include api_version as a parameter, which allows for testing different API versions.


246-262: Ensure api_version parameter is used correctly in test_org_worker_cannot_get_project_backup.

The method test_org_worker_cannot_get_project_backup has been updated to include api_version as a parameter, which allows for testing different API versions.


265-283: Ensure api_version parameter is used correctly in test_org_worker_can_get_project_backup.

The method test_org_worker_can_get_project_backup has been updated to include api_version as a parameter, which allows for testing different API versions.


286-300: Ensure api_version parameter is used correctly in test_org_supervisor_can_get_project_backup.

The method test_org_supervisor_can_get_project_backup has been updated to include api_version as a parameter, which allows for testing different API versions.


303-323: Ensure api_version parameter is used correctly in test_org_supervisor_cannot_get_project_backup.

The method test_org_supervisor_cannot_get_project_backup has been updated to include api_version as a parameter, which allows for testing different API versions.


326-344: Ensure api_version parameter is used correctly in test_org_maintainer_can_get_project_backup.

The method test_org_maintainer_can_get_project_backup has been updated to include api_version as a parameter, which allows for testing different API versions.


347-361: Ensure api_version parameter is used correctly in test_org_owner_can_get_project_backup.

The method test_org_owner_can_get_project_backup has been updated to include api_version as a parameter, which allows for testing different API versions.


363-373: Ensure api_version parameter is used correctly in test_can_get_backup_project_when_some_tasks_have_no_data.

The method test_can_get_backup_project_when_some_tasks_have_no_data has been updated to include api_version as a parameter, which allows for testing different API versions.


Line range hint 375-390:
Ensure api_version parameter is used correctly in test_can_get_backup_project_when_all_tasks_have_no_data.

The method test_can_get_backup_project_when_all_tasks_have_no_data has been updated to include api_version as a parameter, which allows for testing different API versions.


392-396: Ensure api_version parameter is used correctly in test_can_get_backup_for_empty_project.

The method test_can_get_backup_for_empty_project has been updated to include api_version as a parameter, which allows for testing different API versions.


397-416: Ensure api_version parameter is used correctly in test_admin_can_get_project_backup_and_create_project_by_backup.

The method test_admin_can_get_project_backup_and_create_project_by_backup has been updated to include api_version as a parameter, which allows for testing different API versions.


617-629: Ensure api_version parameter is used correctly in _test_export_dataset.

The method _test_export_dataset has been updated to include api_version as a parameter, which allows for testing different API versions.


630-640: Ensure api_version parameter is used correctly in _test_export_annotations.

The method _test_export_annotations has been updated to include api_version as a parameter, which allows for testing different API versions.


676-686: Ensure api_version parameter is used correctly in test_can_import_dataset_in_org.

The method test_can_import_dataset_in_org has been updated to include api_version as a parameter, which allows for testing different API versions.


Line range hint 706-729:
Ensure api_version parameter is used correctly in test_can_export_and_import_dataset_with_skeletons.

The method test_can_export_and_import_dataset_with_skeletons has been updated to include api_version as a parameter, which allows for testing different API versions.


739-754: Ensure api_version parameter is used correctly in test_can_import_export_dataset_with_some_format.

The method test_can_import_export_dataset_with_some_format has been updated to include api_version as a parameter, which allows for testing different API versions.


Line range hint 765-810:
Ensure api_version parameter is used correctly in test_exported_project_dataset_structure.

The method test_exported_project_dataset_structure has been updated to include api_version as a parameter, which allows for testing different API versions.


Line range hint 817-836:
Ensure api_version parameter is used correctly in test_can_import_export_annotations_with_rotation.

The method test_can_import_export_annotations_with_rotation has been updated to include api_version as a parameter, which allows for testing different API versions.


849-862: Ensure api_version parameter is used correctly in test_can_export_dataset_with_skeleton_labels_with_spaces.

The method test_can_export_dataset_with_skeleton_labels_with_spaces has been updated to include api_version as a parameter, which allows for testing different API versions.


864-873: Ensure api_version parameter is used correctly in test_can_export_dataset_for_empty_project.

The method test_can_export_dataset_for_empty_project has been updated to include api_version as a parameter, which allows for testing different API versions.


875-891: Ensure api_version parameter is used correctly in test_can_export_project_dataset_when_some_tasks_have_no_data.

The method test_can_export_project_dataset_when_some_tasks_have_no_data has been updated to include api_version as a parameter, which allows for testing different API versions.


Line range hint 893-914:
Ensure api_version parameter is used correctly in test_can_export_project_dataset_when_all_tasks_have_no_data.

The method test_can_export_project_dataset_when_all_tasks_have_no_data has been updated to include api_version as a parameter, which allows for testing different API versions.


Line range hint 916-942:
Ensure api_version parameter is used correctly in test_can_export_and_import_dataset_after_deleting_related_storage.

The method test_can_export_and_import_dataset_after_deleting_related_storage has been updated to include api_version as a parameter, which allows for testing different API versions.

tests/python/rest_api/test_jobs.py (9)

1331-1333: LGTM!

The setup function correctly initializes the tasks attribute.


1335-1338: LGTM!

The is_local_download function correctly determines if a job is a local download based on the target_storage attribute.


1340-1350: LGTM!

The _test_export_dataset function correctly tests the export of a job dataset and performs appropriate assertions.


1352-1362: LGTM!

The _test_export_annotations function correctly tests the export of job annotations and performs appropriate assertions.


1364-1369: LGTM!

The test_can_export_dataset function correctly tests the export of a dataset for an admin user with appropriate parameters.


1371-1384: LGTM!

The test_non_admin_can_export_dataset function correctly tests the export of a dataset for non-admin users with appropriate parameters.


1386-1400: LGTM!

The test_non_admin_can_export_annotations function correctly tests the export of annotations for non-admin users with appropriate parameters.


Line range hint 1402-1450:
LGTM!

The test_exported_job_dataset_structure function correctly tests the structure of the exported job dataset with comprehensive checks.


Line range hint 1451-1499:
LGTM!

The test_export_job_among_several_jobs_in_task function correctly tests the export of a job dataset among several jobs in a task with comprehensive checks.

tests/python/rest_api/test_tasks.py (7)

659-669: LGTM!

The _test_can_export_dataset method is well-structured and handles dataset export validation correctly.


671-679: LGTM!

The test_can_export_task_dataset method is updated correctly to use _test_can_export_dataset and include the api_version parameter.


681-696: LGTM!

The test_can_export_task_with_several_jobs method is updated correctly to use _test_can_export_dataset and include the api_version parameter.


Line range hint 698-794:
LGTM!

The test_can_export_task_to_coco_format method is updated correctly to use _test_can_export_dataset and include the api_version parameter.


Line range hint 805-826:
LGTM!

The test_can_download_task_with_special_chars_in_name method is updated correctly to include the api_version parameter and use _test_can_export_dataset.


Line range hint 828-848:
LGTM!

The test_export_dataset_after_deleting_related_cloud_storage method is updated correctly to include the api_version parameter and use _test_can_export_dataset.


2278-2287: LGTM!

The test_can_export_backup_with_both_api_versions method is well-structured and tests the backup export functionality correctly across different API versions.

cvat/apps/engine/views.py (3)

83-83: LGTM! The resource field is correctly added to the fields list.

This ensures that the resource field is included in the serialized output.


3277-3277: LGTM! The resource field is correctly mapped in lookup_fields.

This ensures that the resource field can be queried based on the incoming request parameters.


3295-3295: LGTM! The resource field is correctly added to the schema definition.

This ensures that the resource field is validated against specific predefined values.

cvat/schema.yml (2)

4580-4587: Addition of 'resource' filter field is approved.

The new 'resource' filter field with enumerated values ('project', 'task', 'job') enhances the API's filtering capabilities.


4544-4544: Update to the filter fields list is approved.

The inclusion of the 'resource' field in the filter fields list ensures consistency and informs users of the new filtering option.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 41d2f04 and f82b732.

Files selected for processing (11)
  • cvat/apps/engine/background.py (16 hunks)
  • cvat/apps/engine/filters.py (4 hunks)
  • cvat/apps/engine/mixins.py (1 hunks)
  • cvat/apps/engine/views.py (4 hunks)
  • cvat/schema.yml (2 hunks)
  • tests/python/rest_api/test_jobs.py (7 hunks)
  • tests/python/rest_api/test_projects.py (17 hunks)
  • tests/python/rest_api/test_requests.py (1 hunks)
  • tests/python/rest_api/test_tasks.py (9 hunks)
  • tests/python/rest_api/utils.py (2 hunks)
  • tests/python/shared/fixtures/data.py (2 hunks)
Files skipped from review as they are similar to previous changes (5)
  • cvat/apps/engine/filters.py
  • cvat/apps/engine/mixins.py
  • cvat/apps/engine/views.py
  • cvat/schema.yml
  • tests/python/rest_api/test_requests.py
Additional comments not posted (80)
tests/python/shared/fixtures/data.py (2)

93-98: LGTM!

The fixture correctly wraps the filter_assets function for filtering project resources.


101-106: LGTM!

The fixture correctly wraps the filter_assets function for filtering task resources.

tests/python/rest_api/utils.py (10)

Line range hint 25-83:
LGTM!

The function is well-structured and includes parameters for handling forbidden requests and waiting for results. The logic for checking the response status and handling different scenarios is implemented correctly.


85-156: LGTM!

The function is well-structured and includes parameters for handling forbidden requests and waiting for results. The logic for retrieving the background request ID and checking the status is implemented correctly.


158-187: LGTM!

The function correctly handles both versions of the export API based on the api_version parameter. The logic for selecting the appropriate endpoint and calling the corresponding export function is implemented correctly.


189-193: LGTM!

The function correctly wraps the export_dataset function for exporting project datasets.


195-198: LGTM!

The function correctly wraps the export_dataset function for exporting task datasets.


200-203: LGTM!

The function correctly wraps the export_dataset function for exporting job datasets.


205-221: LGTM!

The function correctly handles both versions of the export API based on the api_version parameter. The logic for selecting the appropriate endpoint and calling the corresponding export function is implemented correctly.


223-226: LGTM!

The function correctly wraps the export_backup function for exporting project backups.


228-231: LGTM!

The function correctly wraps the export_backup function for exporting task backups.


233-277: LGTM!

The function is well-structured and includes parameters for handling forbidden requests and waiting for results. The logic for retrieving the background request ID and checking the status is implemented correctly.

cvat/apps/engine/background.py (5)

85-85: LGTM!

The function correctly handles the case where rq_job is None. The logic for managing job statuses and handling different scenarios is implemented correctly.


88-104: LGTM!

The function correctly handles the case where rq_job is None. The logic for managing job statuses and handling different scenarios is implemented correctly.


147-155: LGTM!

The function encapsulates the logic for canceling a job and re-enqueueing it, making the code cleaner and easier to maintain.


Line range hint 174-185:
LGTM!

The method correctly initializes the class with the new Request object. The logic for handling query parameters and location configuration is implemented correctly.


Line range hint 505-591:
LGTM!

The method correctly handles the case where rq_job is None. The logic for managing job statuses and handling different scenarios is implemented correctly.

tests/python/rest_api/test_projects.py (47)

198-214: Method _test_can_get_project_backup: Ensure correct handling of local and cloud storage

The method _test_can_get_project_backup checks if a project backup can be obtained. Ensure that the handling of local and cloud storage is correct.


215-224: Method _test_cannot_get_project_backup: Ensure ForbiddenException is correctly raised

The method _test_cannot_get_project_backup ensures that a forbidden exception is raised when a user cannot get a project backup. Ensure that the ForbiddenException is correctly raised and handled.


225-228: Method test_admin_can_get_project_backup: Ensure proper parameterization

The method test_admin_can_get_project_backup tests if an admin can get a project backup for both API versions. Ensure that the parameterization is correct and covers both API versions.


231-243: Method test_user_cannot_get_project_backup: Ensure correct exclusion of admin users

The method test_user_cannot_get_project_backup tests if a non-admin user cannot get a project backup. Ensure that admin users are correctly excluded.


246-262: Method test_org_worker_cannot_get_project_backup: Ensure correct exclusion of project staff

The method test_org_worker_cannot_get_project_backup tests if an org worker who is not project staff cannot get a project backup. Ensure that project staff are correctly excluded.


265-283: Method test_org_worker_can_get_project_backup: Ensure correct inclusion of project staff

The method test_org_worker_can_get_project_backup tests if an org worker who is project staff can get a project backup. Ensure that project staff are correctly included.


286-300: Method test_org_supervisor_can_get_project_backup: Ensure correct inclusion of project staff

The method test_org_supervisor_can_get_project_backup tests if an org supervisor who is project staff can get a project backup. Ensure that project staff are correctly included.


303-323: Method test_org_supervisor_cannot_get_project_backup: Ensure correct exclusion of project staff

The method test_org_supervisor_cannot_get_project_backup tests if an org supervisor who is not project staff cannot get a project backup. Ensure that project staff are correctly excluded.


326-344: Method test_org_maintainer_can_get_project_backup: Ensure correct inclusion of project staff

The method test_org_maintainer_can_get_project_backup tests if an org maintainer who is not project staff can get a project backup. Ensure that project staff are correctly included.


347-361: Method test_org_owner_can_get_project_backup: Ensure correct inclusion of project staff

The method test_org_owner_can_get_project_backup tests if an org owner who is not project staff can get a project backup. Ensure that project staff are correctly included.


363-373: Method test_can_get_backup_project_when_some_tasks_have_no_data: Ensure proper handling of projects with some empty tasks

The method test_can_get_backup_project_when_some_tasks_have_no_data tests if a project backup can be obtained when some tasks have no data. Ensure that the handling of such projects is correct.


Line range hint 375-390:
Method test_can_get_backup_project_when_all_tasks_have_no_data: Ensure proper handling of projects with all empty tasks

The method test_can_get_backup_project_when_all_tasks_have_no_data tests if a project backup can be obtained when all tasks have no data. Ensure that the handling of such projects is correct.


392-395: Method test_can_get_backup_for_empty_project: Ensure proper handling of empty projects

The method test_can_get_backup_for_empty_project tests if a project backup can be obtained for an empty project. Ensure that the handling of empty projects is correct.


397-416: Method test_admin_can_get_project_backup_and_create_project_by_backup: Ensure proper backup and restore functionality

The method test_admin_can_get_project_backup_and_create_project_by_backup tests if an admin can get a project backup and create a project from the backup. Ensure that the backup and restore functionality is correct.


610-613: Setup method: Ensure proper initialization of projects

The setup method initializes the projects for the test class. Ensure that the projects are correctly initialized.


617-629: Method _test_export_dataset: Ensure correct export of datasets

The method _test_export_dataset tests the export of datasets. Ensure that the datasets are correctly exported and that local and cloud storage are correctly handled.


630-640: Method _test_export_annotations: Ensure correct export of annotations

The method _test_export_annotations tests the export of annotations. Ensure that the annotations are correctly exported and that local and cloud storage are correctly handled.


676-686: Method test_can_import_dataset_in_org: Ensure proper import of datasets in organization

The method test_can_import_dataset_in_org tests the import of datasets in an organization for both API versions. Ensure that the import functionality is correct.


Line range hint 706-731:
Method test_can_export_and_import_dataset_with_skeletons: Ensure proper export and import of datasets with skeletons

The method test_can_export_and_import_dataset_with_skeletons tests the export and import of datasets with skeletons for both API versions. Ensure that the export and import functionality is correct.


739-755: Method test_can_import_export_dataset_with_some_format: Ensure proper import and export of datasets with specific formats

The method test_can_import_export_dataset_with_some_format tests the import and export of datasets with specific formats for both API versions. Ensure that the import and export functionality is correct.


Line range hint 764-811:
Method test_exported_project_dataset_structure: Ensure correct structure of exported project datasets

The method test_exported_project_dataset_structure tests the structure of exported project datasets for both API versions. Ensure that the structure of the exported datasets is correct.


Line range hint 815-847:
Method test_can_import_export_annotations_with_rotation: Ensure proper import and export of annotations with rotation

The method test_can_import_export_annotations_with_rotation tests the import and export of annotations with rotation for both API versions. Ensure that the import and export functionality is correct.


848-861: Method test_can_export_dataset_with_skeleton_labels_with_spaces: Ensure proper export of datasets with skeleton labels containing spaces

The method test_can_export_dataset_with_skeleton_labels_with_spaces tests the export of datasets with skeleton labels containing spaces for both API versions. Ensure that the export functionality is correct.


862-870: Method test_can_export_dataset_for_empty_project: Ensure proper export of datasets for empty projects

The method test_can_export_dataset_for_empty_project tests the export of datasets for empty projects for both API versions. Ensure that the export functionality is correct.


873-888: Method test_can_export_project_dataset_when_some_tasks_have_no_data: Ensure proper export of datasets when some tasks have no data

The method test_can_export_project_dataset_when_some_tasks_have_no_data tests the export of datasets when some tasks have no data for both API versions. Ensure that the export functionality is correct.


Line range hint 891-911:
Method test_can_export_project_dataset_when_all_tasks_have_no_data: Ensure proper export of datasets when all tasks have no data

The method test_can_export_project_dataset_when_all_tasks_have_no_data tests the export of datasets when all tasks have no data for both API versions. Ensure that the export functionality is correct.


Line range hint 913-937:
Method test_can_export_and_import_dataset_after_deleting_related_storage: Ensure proper handling of cloud storage deletion

The method test_can_export_and_import_dataset_after_deleting_related_storage tests the export and import of datasets after deleting related cloud storage for both API versions. Ensure that the handling of cloud storage deletion is correct.


Line range hint 970-988:
Method test_creates_subfolders_for_subsets_on_export: Ensure proper creation of subfolders for subsets on export

The method test_creates_subfolders_for_subsets_on_export tests the creation of subfolders for subsets on export for both API versions. Ensure that the subfolders are correctly created.


Line range hint 1042-1046:
Method test_can_delete_label: Ensure proper deletion of labels

The method test_can_delete_label tests if a label can be deleted from a project. Ensure that the label deletion functionality is correct.


Line range hint 1048-1059:
Method test_can_delete_skeleton_label: Ensure proper deletion of skeleton labels

The method test_can_delete_skeleton_label tests if a skeleton label can be deleted from a project. Ensure that the skeleton label deletion functionality is correct.


Line range hint 1061-1071:
Method test_can_rename_label: Ensure proper renaming of labels

The method test_can_rename_label tests if a label can be renamed in a project. Ensure that the label renaming functionality is correct.


Line range hint 1073-1082:
Method test_cannot_rename_label_to_duplicate_name: Ensure proper handling of duplicate label names

The method test_cannot_rename_label_to_duplicate_name tests if a label cannot be renamed to a duplicate name in a project. Ensure that the handling of duplicate label names is correct.


Line range hint 1084-1091:
Method test_cannot_add_foreign_label: Ensure proper handling of foreign labels

The method test_cannot_add_foreign_label tests if a foreign label cannot be added to a project. Ensure that the handling of foreign labels is correct.


Line range hint 1093-1100:
Method test_admin_can_add_label: Ensure proper addition of labels by admin

The method test_admin_can_add_label tests if an admin can add a label to a project. Ensure that the label addition functionality is correct.


Line range hint 1102-1118:
Method test_non_project_staff_privileged_org_members_can_add_label: Ensure proper addition of labels by non-project staff privileged org members

The method test_non_project_staff_privileged_org_members_can_add_label tests if non-project staff privileged org members can add a label to a project. Ensure that the label addition functionality is correct.


Line range hint 1120-1131:
Method test_non_project_staff_org_members_cannot_add_label: Ensure proper restriction of label addition by non-project staff org members

The method test_non_project_staff_org_members_cannot_add_label tests if non-project staff org members cannot add a label to a project. Ensure that the restriction of label addition is correct.


Line range hint 1133-1147:
Method test_project_staff_org_members_can_add_label: Ensure proper addition of labels by project staff org members

The method test_project_staff_org_members_can_add_label tests if project staff org members can add a label to a project. Ensure that the label addition functionality is correct.


Line range hint 1149-1165:
Method test_admin_can_add_skeleton: Ensure proper addition of skeleton labels by admin

The method test_admin_can_add_skeleton tests if an admin can add a skeleton label to a project. Ensure that the skeleton label addition functionality is correct.


Line range hint 1197-1204:
Method _test_response_200: Ensure proper retrieval of project previews

The method _test_response_200 tests if a project preview can be retrieved successfully. Ensure that the project preview retrieval functionality is correct.


Line range hint 1206-1212:
Method _test_response_403: Ensure proper handling of forbidden project previews

The method _test_response_403 tests if a forbidden response is correctly handled when retrieving a project preview. Ensure that the forbidden response handling is correct.


Line range hint 1214-1220:
Method _test_response_404: Ensure proper handling of not found project previews

The method _test_response_404 tests if a not found response is correctly handled when retrieving a project preview. Ensure that the not found response handling is correct.


Line range hint 1223-1231:
Method test_project_preview_admin_accessibility: Ensure proper access to project previews by admin

The method test_project_preview_admin_accessibility tests if an admin can access project previews. Ensure that the project preview access by admin is correct.


Line range hint 1234-1244:
Method test_project_preview_owner_accessibility: Ensure proper access to project previews by project owner or assignee

The method test_project_preview_owner_accessibility tests if a project owner or assignee can access project previews. Ensure that the project preview access by project owner or assignee is correct.


Line range hint 1246-1256:
Method test_project_preview_not_found: Ensure proper handling of not found project previews

The method test_project_preview_not_found tests if a not found response is correctly handled when retrieving a project preview. Ensure that the not found response handling is correct.


Line range hint 1258-1265:
Method test_user_cannot_see_project_preview: Ensure proper restriction of project preview access by non-admin users

The method test_user_cannot_see_project_preview tests if non-admin users cannot access project previews. Ensure that the restriction of project preview access is correct.


Line range hint 1267-1276:
**Method test_if_supervisor_or_worker_cannot_see_project_preview: Ensure proper


189-189: Class Renaming: Ensure consistency

The class TestGetProjectBackup has been renamed to TestGetPostProjectBackup. Ensure that this new class name is consistently used across the codebase.

Verification successful

Class Renaming Verified: Consistent Usage

The class TestGetProjectBackup has been successfully renamed to TestGetPostProjectBackup, and the new class name is consistently used across the codebase.

  • No occurrences of the old class name TestGetProjectBackup were found.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the new class name `TestGetPostProjectBackup` in the codebase.

# Test: Search for the class usage. Expect: Only occurrences of the new class name.
rg --type python -A 5 $'class TestGetPostProjectBackup'

Length of output: 87


Script:

#!/bin/bash
# Description: Verify the usage of the new class name `TestGetPostProjectBackup` in the codebase.

# Test: Search for the class usage. Expect: Only occurrences of the new class name.
rg --type py -A 5 $'class TestGetPostProjectBackup'

Length of output: 418


Script:

#!/bin/bash
# Description: Verify that the old class name `TestGetProjectBackup` is no longer used in the codebase.

# Test: Search for any occurrences of the old class name. Expect: No occurrences of the old class name.
rg --type py $'TestGetProjectBackup'

Length of output: 35

tests/python/rest_api/test_jobs.py (9)

1331-1333: LGTM!

The setup function correctly initializes the tasks attribute.


1335-1338: LGTM!

The is_local_download function correctly determines if a job is a local download.


1340-1350: LGTM!

The _test_export_dataset function correctly handles the export of a job dataset and includes appropriate assertions.


1352-1362: LGTM!

The _test_export_annotations function correctly handles the export of job annotations and includes appropriate assertions.


1364-1373: LGTM!

The test_can_export_dataset function correctly tests the export of a dataset for both API versions.


1374-1387: LGTM!

The test_non_admin_can_export_dataset function correctly tests the export of a dataset by a non-admin user for both API versions.


1389-1403: LGTM!

The test_non_admin_can_export_annotations function correctly tests the export of annotations by a non-admin user for both API versions.


Line range hint 1405-1453:
LGTM!

The test_exported_job_dataset_structure function correctly tests the structure of the exported job dataset for different annotation formats and API versions.


Line range hint 1454-1502:
LGTM!

The test_export_job_among_several_jobs_in_task function correctly tests the export of a job dataset among several jobs in a task for different annotation formats and API versions.

tests/python/rest_api/test_tasks.py (7)

659-669: LGTM!

The function _test_can_export_dataset is well-structured and includes appropriate assertions to validate the dataset export.


671-679: LGTM!

The function test_can_export_task_dataset is correctly updated to handle different API versions and includes appropriate checks based on the task's storage.


681-696: LGTM!

The function test_can_export_task_with_several_jobs is correctly updated to handle different API versions and dataset formats, with appropriate checks based on the task's storage.


Line range hint 698-805:
LGTM!

The function test_can_export_task_to_coco_format is correctly updated to handle different API versions and includes appropriate checks based on the task's storage. The annotations update and validation are handled correctly.


Line range hint 807-824:
LGTM!

The function test_can_download_task_with_special_chars_in_name is correctly updated to handle different API versions and includes appropriate checks for special characters in the task name.


Line range hint 826-846:
LGTM!

The function test_export_dataset_after_deleting_related_cloud_storage is correctly updated to handle different API versions and includes appropriate checks for dataset export after deleting related cloud storage.


2289-2298: LGTM!

The function test_can_export_backup_with_both_api_versions is well-structured and includes appropriate assertions to validate the backup export for different API versions.

Comment on lines 65 to 90
def filter_assets(resources: Iterable, **kwargs):
filtered_resources = []
exclude_prefix = "exclude_"

for resource in resources:
for key, value in kwargs.items():
if exclude := key.startswith(exclude_prefix):
key = key[len(exclude_prefix) :]

op = operator.ne if exclude else operator.eq
cur_value, rest = resource, key
while rest:
field_and_rest = rest.split("__", maxsplit=1)
if 2 == len(field_and_rest):
field, rest = field_and_rest
else:
field, rest = field_and_rest[0], None
cur_value = cur_value[field]
# e.g. task has null target_storage
if not cur_value:
break

if not rest and op(cur_value, value) or rest and op == operator.ne:
filtered_resources.append(resource)

return filtered_resources
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle potential issue with nested field access when cur_value is None.

If cur_value is None before accessing nested fields, it may lead to a TypeError. Consider adding a check to handle this case.

-  while rest:
+  while rest and cur_value:
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
def filter_assets(resources: Iterable, **kwargs):
filtered_resources = []
exclude_prefix = "exclude_"
for resource in resources:
for key, value in kwargs.items():
if exclude := key.startswith(exclude_prefix):
key = key[len(exclude_prefix) :]
op = operator.ne if exclude else operator.eq
cur_value, rest = resource, key
while rest:
field_and_rest = rest.split("__", maxsplit=1)
if 2 == len(field_and_rest):
field, rest = field_and_rest
else:
field, rest = field_and_rest[0], None
cur_value = cur_value[field]
# e.g. task has null target_storage
if not cur_value:
break
if not rest and op(cur_value, value) or rest and op == operator.ne:
filtered_resources.append(resource)
return filtered_resources
def filter_assets(resources: Iterable, **kwargs):
filtered_resources = []
exclude_prefix = "exclude_"
for resource in resources:
for key, value in kwargs.items():
if exclude := key.startswith(exclude_prefix):
key = key[len(exclude_prefix) :]
op = operator.ne if exclude else operator.eq
cur_value, rest = resource, key
while rest and cur_value:
field_and_rest = rest.split("__", maxsplit=1)
if 2 == len(field_and_rest):
field, rest = field_and_rest
else:
field, rest = field_and_rest[0], None
cur_value = cur_value[field]
# e.g. task has null target_storage
if not cur_value:
break
if not rest and op(cur_value, value) or rest and op == operator.ne:
filtered_resources.append(resource)
return filtered_resources

@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2024

Codecov Report

Attention: Patch coverage is 69.64286% with 34 lines in your changes missing coverage. Please review.

Project coverage is 83.57%. Comparing base (3cd4c39) to head (57198fa).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8216      +/-   ##
===========================================
+ Coverage    83.40%   83.57%   +0.17%     
===========================================
  Files          393      393              
  Lines        41579    41608      +29     
  Branches      3843     3843              
===========================================
+ Hits         34678    34774      +96     
+ Misses        6901     6834      -67     
Components Coverage Δ
cvat-ui 79.64% <ø> (-0.06%) ⬇️
cvat-server 87.05% <69.64%> (+0.37%) ⬆️

@Marishka17
Copy link
Contributor Author

Marishka17 commented Aug 13, 2024

@zhiltsov-max, @SpecLad
I've added using locks when handling and setting up RQ jobs to eliminate race conditions and make behavior more predictable (since in the current implementation we can modify rq job (delete/cancel/create a new one with the same id)). There is a comparison of 3 cases (other cases were not compared):

Before fix:

  1. [API v1] finished|failed X1 job, action=download
    request 1: deletes X1, returns results file or exception details
    request 2: deletes* X1 (idempotent action), returns results file or exception details

  2. [API v1] finished X1 job, no action==download, result is outdated (request timestamp from meta < last updated time)
    request 1:

    • X1.cancel() -> enqueues X1 dependents, removes X1 from FinishedJobRegistry, addes X1 to CancelledJobRegistry
    • X1.delete() -> deletes X1 from the CancelledJobRegistry and Redis,
    • then creates a new X1

    request 2 with small delay: deletes X1 from CancelledJobRegistry, creates new X1* job, but that X1* will depend on X1 from 1st request, because X1 was placed to export queue or to StartedJobRegistry. That means that X1 will be present in 2 registries (started and deferred), will be "dependent on itself" and will be executed twice.

  3. [API v2] finished|failed X1 job
    request 1: deletes X1, creates new one, returns 202
    request 2: similar to the situation described above: X1 job exists in started and deferred registries, is executed twice

After fix:

  1. [API v1] finished|failed X1 job, action=download
    request 1: deletes X1, returns results file or exception details
    request 2: return 400, Unknown export request id

  2. [API v1] finished X1 job, no action==download, result is outdated (request timestamp from meta < last updated time)
    request 1: the same as listed above (before fix, case 2, request 1)
    request 2 with small delay: returns a response based on updated rq job status.

  3. [API v2] finished|failed X1 job
    request 1: deletes X1, creates new one, returns 202
    request 2: return 409

Copy link
Contributor

@SpecLad SpecLad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a rebase, but otherwise LGTM.

Copy link

sonarcloud bot commented Aug 20, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
3.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@Marishka17 Marishka17 merged commit 3fdb032 into develop Aug 21, 2024
32 of 33 checks passed
@Marishka17 Marishka17 deleted the mk/rest_api_tests branch August 21, 2024 08:13
@cvat-bot cvat-bot bot mentioned this pull request Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants