[WIP] Mk/validation api small fixes #8816
Conversation
…nfigured for a task
…s when a task does not have a validation layout associated with
|
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 Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe changes involve modifications to the CVAT application, focusing on enhancing the handling of validation layouts within tasks and jobs. Key updates include the introduction of a Changes
Poem
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 (
|
|
@zhiltsov-max, I figured out when reviewing your PRs that PATCH /api/tasks/id/validation_layout returns |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
cvat/apps/engine/migrations/0087_alter_validationlayout_task_data.py(1 hunks)cvat/apps/engine/models.py(3 hunks)cvat/apps/engine/serializers.py(3 hunks)cvat/apps/engine/views.py(4 hunks)cvat/schema.yml(4 hunks)
🔇 Additional comments (11)
cvat/apps/engine/models.py (3)
263-269: LGTM: Addition of ReverseNullableOneToOneDescriptor
The implementation correctly handles nullable reverse one-to-one relationships by returning None when the related object does not exist.
270-272: LGTM: Addition of NullableOneToOneField
The custom field appropriately sets the related_accessor_class to handle nullable relationships.
313-313: LGTM: Updated type hint for validation_layout
The type hint is updated to ValidationLayout | None, accurately reflecting that the field may be None.
cvat/schema.yml (2)
2667-2668: LGTM! Good addition of standard HTTP status codes for the job validation layout endpoint.
The addition of 204 and 400 response codes improves the API error handling and follows REST best practices:
- 204: Clearly indicates when no validation layout exists
- 400: Properly handles invalid requests
Also applies to: 2712-2713
6154-6155: LGTM! Good addition of standard HTTP status codes for the task validation layout endpoint.
The addition of 204 and 400 response codes maintains consistency with the job endpoint and follows REST best practices:
- 204: Clearly indicates when no validation layout exists
- 400: Properly handles invalid requests
Also applies to: 6210-6211
cvat/apps/engine/views.py (6)
1749-1749: LGTM
Adding a 204 No Content response for cases when no validation layout is associated with a task appropriately reflects possible API responses.
1757-1757: LGTM
Including a 400 Bad Request response in the OpenAPI schema enhances API documentation and clarifies potential error responses.
1788-1789: LGTM
Returning 204 No Content when no validation layout exists is appropriate and aligns with RESTful practices.
2352-2352: LGTM
Adding a 204 No Content response for cases when no validation layout is associated with a job appropriately reflects possible API responses.
2360-2360: LGTM
Including a 400 Bad Request response in the OpenAPI schema enhances API documentation and clarifies potential error responses.
2396-2401: Ensure correct handling of empty content in response
The condition if content: determines whether to return 200 OK or 204 No Content. Verify that response_serializer.data accurately reflects the presence of a validation layout. Consider explicitly checking if validation_layout is None for clarity and to prevent unexpected behavior.
| # TODO: probably it should take validation_layout instead of task | ||
| request_serializer = TaskValidationLayoutWriteSerializer(db_task, data=request.data) | ||
| request_serializer.is_valid(raise_exception=True) | ||
| validation_layout = request_serializer.save().data.validation_layout | ||
| updated_task = request_serializer.save() | ||
| validation_layout = updated_task.data.validation_layout | ||
| else: |
There was a problem hiding this comment.
Address the TODO comment and pass validation_layout to the serializer
The TODO comment suggests that the serializer should take validation_layout instead of db_task. Passing the correct instance ensures proper serialization and updates. Please update the code to pass validation_layout as the instance. If validation_layout is None, handle its creation accordingly.
Apply this diff to fix the issue:
if request.method == "PATCH":
- # TODO: probably it should take validation_layout instead of task
- request_serializer = TaskValidationLayoutWriteSerializer(db_task, data=request.data)
+ validation_layout = db_task.data.validation_layout
+ if validation_layout is None:
+ validation_layout = ValidationLayout(data=db_task.data)
+ request_serializer = TaskValidationLayoutWriteSerializer(validation_layout, data=request.data)
request_serializer.is_valid(raise_exception=True)
- updated_task = request_serializer.save()
- validation_layout = updated_task.data.validation_layout
+ validation_layout = request_serializer.save()
else:
validation_layout = db_task.data.validation_layout📝 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.
| # TODO: probably it should take validation_layout instead of task | |
| request_serializer = TaskValidationLayoutWriteSerializer(db_task, data=request.data) | |
| request_serializer.is_valid(raise_exception=True) | |
| validation_layout = request_serializer.save().data.validation_layout | |
| updated_task = request_serializer.save() | |
| validation_layout = updated_task.data.validation_layout | |
| else: | |
| validation_layout = db_task.data.validation_layout | |
| if validation_layout is None: | |
| validation_layout = ValidationLayout(data=db_task.data) | |
| request_serializer = TaskValidationLayoutWriteSerializer(validation_layout, data=request.data) | |
| request_serializer.is_valid(raise_exception=True) | |
| validation_layout = request_serializer.save() | |
| else: |
|
@coderabbitai, stop |
|
@Marishka17: Understood, I will pause the automatic reviews for this pull request. (_/) ✅ Actions performedReviews paused. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8816 +/- ##
===========================================
- Coverage 73.92% 73.91% -0.02%
===========================================
Files 409 408 -1
Lines 43957 43902 -55
Branches 3986 3986
===========================================
- Hits 32497 32451 -46
+ Misses 11460 11451 -9
|
|
@SpecLad, Do you foresee any possible issues with modifying the logic of the |
Yes. Neither Plus, it seems like there's a much simpler, non-hacky solution here: define a property on |
Marishka17
changed the title
It is not possible to define the property with the same name and use it because Django prioritizes model field attributes over custom properties when both have the same name. However, it may be probably fixed by changing the |
I didn't mean a property with the same name. I just meant a helper property, side by side with the one generated by Django. |
But is it not a bit odd that there will be 2 options for accessing the same data? |
Sure. I think it's just the price you pay when you provide convenience functionality. |
|
/check |
|
✔️ All checks completed successfully |
|
Motivation and context
How has this been tested?
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
Bug Fixes
Documentation
Refactor