Unify error handling with the cloud storage #5389
Conversation
|
/check |
| raise NotFound('The resource {} not found. It may have been deleted.'.format(self.name)) | ||
| elif storage_status == Status.AVAILABLE: | ||
| raise | ||
| raise ValidationError(str(ex)) |
Check warning
Code scanning / CodeQL
Information exposure through an exception
| raise NotFound("The file '{}' not found on the cloud storage '{}'".format(key, self.name)) | ||
| elif file_status == Status.FORBIDDEN: | ||
| raise PermissionDenied("Access to the file '{}' on the '{}' cloud storage is denied".format(key, self.name)) | ||
| raise ValidationError(str(ex)) |
Check warning
Code scanning / CodeQL
Information exposure through an exception
| except (ValidationError, PermissionDenied, NotFound) as ex: | ||
| msg = str(ex) if not isinstance(ex, ValidationError) else \ | ||
| '\n'.join([str(d) for d in ex.detail]) | ||
| return Response(data=msg, status=ex.status_code) |
Check warning
Code scanning / CodeQL
Information exposure through an exception
|
✔️ All checks completed successfully |
| except (ValidationError, PermissionDenied, NotFound) as ex: | ||
| msg = str(ex) if not isinstance(ex, ValidationError) else \ | ||
| '\n'.join([str(d) for d in ex.detail]) | ||
| return Response(data=msg, status=ex.status_code) |
Check warning
Code scanning / CodeQL
Information exposure through an exception
| except (ValidationError, PermissionDenied, NotFound) as ex: | ||
| msg = str(ex) if not isinstance(ex, ValidationError) else \ | ||
| '\n'.join([str(d) for d in ex.detail]) | ||
| return Response(data=msg, status=ex.status_code) |
Check warning
Code scanning / CodeQL
Information exposure through an exception
|
/check |
|
✔️ All checks completed successfully |
| slogger.cloud_storage[pk].info(msg) | ||
| return Response(data=msg, status=status.HTTP_404_NOT_FOUND) | ||
| return Response(data=msg, status=ex.status_code) |
Check warning
Code scanning / CodeQL
Information exposure through an exception
| msg = str(ex) if not isinstance(ex, ValidationError) else \ | ||
| '\n'.join([str(d) for d in ex.detail]) | ||
| slogger.cloud_storage[pk].info(msg) | ||
| return Response(data=msg, status=ex.status_code) |
Check warning
Code scanning / CodeQL
Information exposure through an exception
cvat/apps/engine/utils.py
Outdated
| # [ErrorDetail(string="...", code=\'invalid\')]\n' | ||
| parsed_msg = msg.split('string=')[1].split(', code=')[0].strip("\"") | ||
| elif 'PermissionDenied' in msg: | ||
| # msg like 'rest_framework.exceptions.PermissionDenied: ... \n' |
There was a problem hiding this comment.
I will prefer to have a more general solution. For example,
if msg.startswith('rest_framework.exceptions.'):
parsed_msg = msg.split(':')[1].strip()| @@ -110,17 +110,34 @@ def parse_specific_attributes(specific_attributes): | |||
| } if parsed_specific_attributes else dict() | |||
|
|
|||
|
|
|||
| def parse_exception_message(msg): | |||
| parsed_msg = msg | |||
There was a problem hiding this comment.
Probably need to handle a case when msg is empty by a reason. Please resolve a conflict and see the case.
There was a problem hiding this comment.
Mostly looks good to me, but I have few questions about these changes. Also agree with Nikita's comments
P.S.: also have question that not related with this PR but related with the CloudStorageViewSet, why somewhere we use HTTPResponse and somewhere Response? Is it possible to use only one "response" class?
Added a general solution for checking bucket and file status instead of checking in all places. Exception information has become more user-friendly.
Motivation and context
Closes issue 34
Added a general solution for checking bucket and file status instead of checking in all places. Exception information has become more user-friendly.
How has this been tested?
Added test
Checklist
developbranch- [ ] I have updated the documentation accordingly- [ ] I have increased versions of npm packages if it is necessary (cvat-canvas,cvat-core, cvat-data and cvat-ui)
License
Feel free to contact the maintainers if that's a concern.