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

Error structure core components #986

Merged
merged 13 commits into from
Jan 21, 2022
Merged

Conversation

silentninja
Copy link
Contributor

@silentninja silentninja commented Jan 19, 2022

Fixes #983

It adds the necessary components required for

  1. Custom error handler for capturing exceptions
  2. Error structure based on NamedTuple
  3. Unique Enum that contains the list of error codes
  4. Logic to call respective parsers to convert non ApiException to proper ApiException

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the master branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

1. To capture all exception and convert it to proper api error response
2. To check if error response are correct
@silentninja silentninja requested a review from a team January 19, 2022 17:41
@codecov-commenter
Copy link

codecov-commenter commented Jan 19, 2022

Codecov Report

Merging #986 (8052b4e) into master (0c0d30f) will decrease coverage by 0.83%.
The diff coverage is 17.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #986      +/-   ##
==========================================
- Coverage   93.30%   92.46%   -0.84%     
==========================================
  Files          86       89       +3     
  Lines        3165     3200      +35     
==========================================
+ Hits         2953     2959       +6     
- Misses        212      241      +29     
Flag Coverage Δ
pytest-backend 92.46% <17.14%> (-0.84%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mathesar/api/exceptions/error_codes.py 0.00% <0.00%> (ø)
mathesar/api/exceptions/exceptions.py 0.00% <0.00%> (ø)
mathesar/exception_handlers.py 54.54% <54.54%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c0d30f...8052b4e. Read the comment docs.

Copy link
Contributor

@kgodey kgodey left a comment

Choose a reason for hiding this comment

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

Some more style issues.

mathesar/exception_handlers.py Outdated Show resolved Hide resolved
mathesar/exception_handlers.py Outdated Show resolved Hide resolved
mathesar/exception_handlers.py Outdated Show resolved Hide resolved
…re already caught.

Add base Exception and Error classes which can be extended upon if needed
@silentninja silentninja marked this pull request as draft January 19, 2022 20:18
@github-actions github-actions bot requested a review from kgodey January 20, 2022 01:38
Copy link
Contributor

@kgodey kgodey left a comment

Choose a reason for hiding this comment

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

This looks fine to me. It's hard to think about the structure without example exceptions implemented to think through, but we can figure that out in later PRs.

I added some minor comments, please fix and merge yourself, no need for me to re-review.


from django.utils.encoding import force_str
from rest_framework import status
from rest_framework.exceptions import APIException as DRFApiException
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be DRFAPIException or something like OriginalAPIException if the concatenated acronyms are hard to read, it's confusing to mix API and Api

self.status_code = status_code


class GenericApiError(APIException):
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be API instead of Api


@unique
class ErrorCodes(Enum):
NonClassifiedError = 4999
Copy link
Contributor

Choose a reason for hiding this comment

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

I think UnknownError might be a bit clearer for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more like saying we are able to get the detail of the error but haven't specified any error code for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe UnspecifiedCodeError, then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my choice at first but felt like error code hasn't been classified yet made sense, UnspecifiedCodeError seems like error code has not been specified and there is an error related to that, but I think I will go with UnknownError seems neutral.

return APIException(exc, ErrorCodes.NonClassifiedError.value)


class APIException(DRFApiException):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call ours MathesarAPIException? It might be confusing to overload the original name for contributors who are used to working with DRF.

Rename `GenericApiError` to `GenericAPIError`
@silentninja silentninja requested a review from a team January 20, 2022 20:06
Copy link
Contributor

@mathemancer mathemancer left a comment

Choose a reason for hiding this comment

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

I think it makes sense. Like @kgodey , I'd need to see it in action to provide more feedback.

@kgodey kgodey merged commit d3263a3 into master Jan 21, 2022
@kgodey kgodey deleted the error-structure_core_components branch January 21, 2022 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Basic components to assist with error handling and adhere exceptions to the Api spec
4 participants