fixed server to check default_attribute and values for checkbox #7889
Conversation
WalkthroughThe recent update ensures that checkbox attributes and their default values are validated to be 'true' or 'false' across different components, aiming to resolve issues related to updating labels with multiple attributes, including 'expression'. 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: 1
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: Convert f-string to regular string.- return Response("{} is an invalid directory".format(directory_param), + return Response(f"{directory_param} is an invalid directory",
Line range hint
2723-2723: Convert f-string to regular string.- return Response(data='unknown data type {}.'.format(self.type), + return Response(data=f'unknown data type {self.type}.',
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
2723-2723: f-string without any placeholders
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- changelog.d/20240514_153646_krishavrajsingh_server.md (1 hunks)
Additional Context Used
LanguageTool (1)
changelog.d/20240514_153646_krishavrajsingh_server.md (1)
Near line 1: Possible typo: you repeated a word
Context: ### Fixed - fixed server to check default_attribute and v...
There was a problem hiding this comment.
Actionable comments posted: 5
Out of diff range and nitpick comments (2)
cvat/apps/engine/views.py (2)
Line range hint
1967-1967: Convert f-string to regular string.The f-string on this line does not contain any placeholders and should be converted to a regular string.
- return Response("{} is an invalid directory".format(directory_param), + return Response(f"{directory_param} is an invalid directory",
Line range hint
2723-2723: Convert f-string to regular string.The f-string on this line does not contain any placeholders and should be converted to a regular string.
- return Response(data='unknown data type {}.'.format(self.type), + return Response(data=f'unknown data type {self.type}.',
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- changelog.d/20240514_153646_krishavrajsingh_server.md (1 hunks)
- cvat/apps/engine/views.py (1 hunks)
Additional Context Used
LanguageTool (1)
changelog.d/20240514_153646_krishavrajsingh_server.md (1)
Near line 4: You might be missing the article “the” here.
Context: ### Fixed - Server now checks default_attribute and values...
Ruff (3)
cvat/apps/engine/views.py (3)
44-44:
cvat.apps.dataset_manager.viewsimported but unused
1967-1967: f-string without any placeholders
2723-2723: f-string without any placeholders
Additional comments not posted (1)
cvat/apps/engine/views.py (1)
Line range hint
44-44: Remove unused import statement.The import statement
import cvat.apps.dataset_manager.viewsis not used anywhere in the file and can be safely removed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- changelog.d/20240514_153646_krishavrajsingh_server.md (1 hunks)
Additional Context Used
LanguageTool (1)
changelog.d/20240514_153646_krishavrajsingh_server.md (1)
Near line 3: You might be missing the article “the” here.
Context: ### Fixed - Server now checks default_attribute and values...
|
Should I move the logic to LabelSerializer?? |
cvat/apps/engine/views.py
Outdated
| attributes = self.request.data.get('attributes', []) | ||
| for attribute in attributes: | ||
| if attribute.get('input_type') == 'checkbox': | ||
| def_val = attribute.get('default_value') | ||
| values = attribute.get('values', []) | ||
| if values and values[0] not in ['true', 'false']: | ||
| raise ValidationError('checkbox values must be either true or false') | ||
| if def_val and def_val not in ['true','false']: | ||
| raise ValidationError("default_value for checkbox must be either true or false") |
There was a problem hiding this comment.
Hello,
- We also need this check when we create attributes, not only when update.
- Also code from these views not always executed. For example when we importing a task from a backup, it does not use this code (please, see:
cvat/apps/engine/backup.py:_ImporterBase:_create_labels). That is why we should move all these Validation checks to AttributeSerializer. - On the other hand, incorrect values may already be saved in the database. And in this case we need a data migration file to fix the issue.
The issue is not so trivial as it may look. What do you think?
There was a problem hiding this comment.
I will proceed with this:
[] Move the validation logic to 'AttributeSerializer'
[] Make a data migration to fix existing invalid checkbox values in database.
Thanks for your detailed feedback. If you have furthur suggestions, please let me know.
There was a problem hiding this comment.
Actionable comments posted: 4
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- changelog.d/20240514_153646_krishavrajsingh_server.md (1 hunks)
- cvat/apps/engine/migrations/0079_fix_checkbox_values.py (1 hunks)
- cvat/apps/engine/serializers.py (1 hunks)
Additional Context Used
Ruff (2)
cvat/apps/engine/serializers.py (2)
728-728: f-string without any placeholders
1163-1163: f-string without any placeholders
Markdownlint (2)
changelog.d/20240514_153646_krishavrajsingh_server.md (2)
1: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
2: null
Lists should be surrounded by blank lines
Additional comments not posted (1)
cvat/apps/engine/migrations/0079_fix_checkbox_values.py (1)
19-25: The migration setup looks correct and well-structured.
| def validate(self, attr): | ||
| if attr.get('input_type') == 'checkbox': | ||
| def_val = attr.get('default_value') | ||
| value = attr.get('values') | ||
| attr_name = attr.get('name') | ||
| if value not in ['true', 'false']: | ||
| raise serializers.ValidationError(f"Checkbox value for attribute '{attr_name}' must be either true or false") | ||
| if def_val not in ['true', 'false']: | ||
| raise serializers.ValidationError(f"Default_value of checkbox for attribute '{attr_name}' must be either true or false") | ||
| return attr |
There was a problem hiding this comment.
Ensure consistent error messages for checkbox validation.
- raise serializers.ValidationError(f"Checkbox value for attribute '{attr_name}' must be either true or false")
- raise serializers.ValidationError(f"Default_value of checkbox for attribute '{attr_name}' must be either true or false")
+ raise serializers.ValidationError(f"Checkbox value for attribute '{attr_name}' must be 'true' or 'false'")
+ raise serializers.ValidationError(f"Default value of checkbox for attribute '{attr_name}' must be 'true' or 'false'")The error messages should consistently use the format 'true' or 'false' with quotes to match the expected string values. Also, use "Default value" instead of "Default_value" for better readability.
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.
| def validate(self, attr): | |
| if attr.get('input_type') == 'checkbox': | |
| def_val = attr.get('default_value') | |
| value = attr.get('values') | |
| attr_name = attr.get('name') | |
| if value not in ['true', 'false']: | |
| raise serializers.ValidationError(f"Checkbox value for attribute '{attr_name}' must be either true or false") | |
| if def_val not in ['true', 'false']: | |
| raise serializers.ValidationError(f"Default_value of checkbox for attribute '{attr_name}' must be either true or false") | |
| return attr | |
| def validate(self, attr): | |
| if attr.get('input_type') == 'checkbox': | |
| def_val = attr.get('default_value') | |
| value = attr.get('values') | |
| attr_name = attr.get('name') | |
| if value not in ['true', 'false']: | |
| raise serializers.ValidationError(f"Checkbox value for attribute '{attr_name}' must be 'true' or 'false'") | |
| if def_val not in ['true', 'false']: | |
| raise serializers.ValidationError(f"Default value of checkbox for attribute '{attr_name}' must be 'true' or 'false'") | |
| return attr |
| @@ -0,0 +1,3 @@ | |||
| ### Fixed | |||
There was a problem hiding this comment.
Add a blank line before the heading for better readability.
+
### FixedCommittable 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.
| ### Fixed | |
| + | |
| ### Fixed |
| @@ -0,0 +1,3 @@ | |||
| ### Fixed | |||
| - Server now checks default_attribute and values for checkbox | |||
There was a problem hiding this comment.
Add a blank line before the list for better readability.
+
- Server now checks default_attribute and values for checkboxCommittable 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.
| - Server now checks default_attribute and values for checkbox | |
| - Server now checks default_attribute and values for checkbox |
| for attribute in Attribute.objects.filter(input_type='checkbox'): | ||
| if attribute.default_value not in ['true', 'false']: | ||
| attribute.default_value = 'false' | ||
| if attribute.values not in ['true', 'false']: |
There was a problem hiding this comment.
Ensure correct comparison for list values.
- if attribute.values not in ['true', 'false']:
+ if all(val not in ['true', 'false'] for val in attribute.values):This change ensures that each value in the list is checked individually against 'true' or 'false'.
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.
| if attribute.values not in ['true', 'false']: | |
| if all(val not in ['true', 'false'] for val in attribute.values): |
Fixes #2927
I checked the values of default_value and values and threw error if it wasn't true or false for checkbox
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