fixed server for duplicate attribute names #7890
Conversation
WalkthroughThe recent changes enhance the CVAT application by introducing a validation mechanism to prevent the creation of duplicate attribute names within the same label. This improvement ensures data integrity by checking for duplicate attribute names when updating or creating labels, leading to better user feedback and error handling. Changes
Assessment against linked issues
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 Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (3)
cvat/apps/engine/views.py (3)
Line range hint
44-44: Remove unused import statement.- import cvat.apps.dataset_manager.views # pylint: disable=unused-import
Line range hint
1967-1967: Remove unnecessary f-string formatting.- return Response("{} is an invalid directory".format(directory_param), + return Response(f"{directory_param} is an invalid directory",
Line range hint
2721-2721: Remove unnecessary f-string formatting.- return Response("{}".format(directory_param), + return Response(directory_param,
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/apps/engine/views.py (1 hunks)
Additional Context Used
Ruff (3)
cvat/apps/engine/views.py (3)
44-44:
cvat.apps.dataset_manager.viewsimported but unused
1967-1967: f-string without any placeholders
2721-2721: f-string without any placeholders
Additional comments not posted (1)
cvat/apps/engine/views.py (1)
2285-2292: LGTM! The validation logic for duplicate attribute names is correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- changelog.d/20240514_201903_krishavrajsingh_duplicateAttribute.md (1 hunks)
Additional Context Used
LanguageTool (1)
changelog.d/20240514_201903_krishavrajsingh_duplicateAttribute.md (1)
Near line 3: You might be missing the article “the” here.
Context: ### Fixed - server checks attributes with duplicate names ...
changelog.d/20240514_201903_krishavrajsingh_duplicateAttribute.md
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- changelog.d/20240514_201903_krishavrajsingh_duplicateAttribute.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- changelog.d/20240514_201903_krishavrajsingh_duplicateAttribute.md
cvat/apps/engine/views.py
Outdated
| @@ -2282,7 +2282,14 @@ def perform_update(self, serializer): | |||
| "Sublabels cannot be modified this way. " | |||
| "Please send a PATCH request with updated parent label data instead.", | |||
| code=status.HTTP_400_BAD_REQUEST) | |||
|
|
|||
| attributes = self.request.data.get('attributes') | |||
There was a problem hiding this comment.
We need not only run this validation when updating a Label, but also when creating it.
Please, move this logic to the LabelSerializer (there are methods update_label and create_labels).
These methods are called from ProjectWriteSerializer and TaskWriteSerializer when labels updated.
| @@ -0,0 +1,4 @@ | |||
| ### Fixed | |||
|
|
|||
| the server checks attributes with duplicate names and throws ValidationError if found | |||
There was a problem hiding this comment.
| the server checks attributes with duplicate names and throws ValidationError if found | |
| - REST API allowed to create several attributes with the same name within one label |
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (1)
cvat/apps/engine/serializers.py (1)
Line range hint
718-718: Remove unnecessary f-string.- f'label:update Skeleton id:{db_skeleton.id} for label_id:{db_label.id}' + 'label:update Skeleton id:{} for label_id:{}'.format(db_skeleton.id, db_label.id)
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- changelog.d/20240514_201903_krishavrajsingh_duplicateAttribute.md (1 hunks)
- cvat/apps/engine/serializers.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- changelog.d/20240514_201903_krishavrajsingh_duplicateAttribute.md
Additional Context Used
Ruff (2)
cvat/apps/engine/serializers.py (2)
718-718: f-string without any placeholders
1153-1153: f-string without any placeholders
cvat/apps/engine/serializers.py
Outdated
| encountered_names = set() | ||
| for attribute in attributes: | ||
| attr_name = attribute.get('name') | ||
| if attr_name in encountered_names: | ||
| raise exceptions.ValidationError('attribute with same name exists') | ||
| else: | ||
| encountered_names.add(attr_name) |
There was a problem hiding this comment.
Validation logic for duplicate attribute names looks good. However, the error message can be more descriptive.
- raise exceptions.ValidationError('attribute with same name exists')
+ raise exceptions.ValidationError(f"Duplicate attribute name '{attr_name}' found.")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.
| encountered_names = set() | |
| for attribute in attributes: | |
| attr_name = attribute.get('name') | |
| if attr_name in encountered_names: | |
| raise exceptions.ValidationError('attribute with same name exists') | |
| else: | |
| encountered_names.add(attr_name) | |
| encountered_names = set() | |
| for attribute in attributes: | |
| attr_name = attribute.get('name') | |
| if attr_name in encountered_names: | |
| raise exceptions.ValidationError(f"Duplicate attribute name '{attr_name}' found.") | |
| else: | |
| encountered_names.add(attr_name) |
cvat/apps/engine/serializers.py
Outdated
| encountered_names = set() | ||
| for attribute in attributes: | ||
| attr_name = attribute.get('name') | ||
| if attr_name in encountered_names: | ||
| raise exceptions.ValidationError('attribute with same name exists') | ||
| else: | ||
| encountered_names.add(attr_name) |
There was a problem hiding this comment.
Validation logic for duplicate attribute names looks good. However, the error message can be more descriptive.
- raise exceptions.ValidationError('attribute with same name exists')
+ raise exceptions.ValidationError(f"Duplicate attribute name '{attr_name}' found.")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.
| encountered_names = set() | |
| for attribute in attributes: | |
| attr_name = attribute.get('name') | |
| if attr_name in encountered_names: | |
| raise exceptions.ValidationError('attribute with same name exists') | |
| else: | |
| encountered_names.add(attr_name) | |
| encountered_names = set() | |
| for attribute in attributes: | |
| attr_name = attribute.get('name') | |
| if attr_name in encountered_names: | |
| raise exceptions.ValidationError(f"Duplicate attribute name '{attr_name}' found.") | |
| else: | |
| encountered_names.add(attr_name) |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/apps/engine/serializers.py (3 hunks)
Additional Context Used
Ruff (2)
cvat/apps/engine/serializers.py (2)
717-717: f-string without any placeholders
1152-1152: f-string without any placeholders
Additional comments not posted (3)
cvat/apps/engine/serializers.py (3)
324-333: Thecheck_attribute_names_uniquemethod is a good addition to ensure attribute names are unique. This will help prevent errors related to duplicate attribute names.
349-349: The call tocheck_attribute_names_uniquein theupdate_labelmethod ensures that attribute names are validated for uniqueness before updating a label. This is a necessary validation step.
466-466: The call tocheck_attribute_names_uniquein thecreate_labelsmethod ensures that attribute names are validated for uniqueness before creating labels. This is a necessary validation step.
|
Great, thanks for the contribution! |
Fixes #3113
Checked the array of attributes for duplicate attribute names and thrown Validation error when found
testing video
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
ValidationErrorif duplicate attribute names are detected.