Handle error consistently on the backend using custom drf error handler #909

silentninja · 2021-12-20T22:40:42Z

Fixes #623

This PR introduces custom Api Exception and custom Error Handler to handle non ApiException.

Custom Api Exception - Exception are captured and reraised with equivalent ApiException subclass which contains the error code, reusable logic for parsing the exception. It is responsible for converting the exception into a proper exception body according to the spec.
Custom error handler - Used as a last resort, it captures any exceptions that do not conform to the api spec and use a default map to get the equivalent ApiException, if an equivalent ApiException does not exist, it converts it to a UnclassifiedException(error code: 49XX), with the message as the string representation of the exception that is thrown.

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
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.

…at is thrown.

`previews` api route uses the context manager to capture error and convert it to proper error response

…exception as first argument similar to when calling default parser

kgodey

@silentninja Please see code comments.

I'm also confused about why we're not using DRF's custom exception handler. https://www.django-rest-framework.org/api-guide/exceptions/. You mentioned in the PR description that you'd have to do it on a per-view basis, but I'm not sure why that's the case. You can create an exception handler in a single place and then update our settings file to use it as the default exception handler. This seems cleanest to me - we can then use standard Python/DRF exceptions without as much boilerplate.

See this Stack Overflow answer for some more discussion on this topic.

kgodey · 2021-12-21T23:20:11Z

mathesar/errors.py

 class InvalidTableError(Exception):
    pass
+
+
+ExceptionTransformerDetail = namedtuple(


@silentninja I updated the whitespace here to make it more easily readable (in my opinion) and to match the rest of the code style. I figured updating it directly would be easier.

kgodey · 2021-12-21T23:24:13Z

mathesar/api/viewsets/tables.py

@@ -105,18 +107,21 @@ def previews(self, request, pk=None):
            raise ValidationError("Incorrect number of columns in request.")

        table_data = TableSerializer(table, context={"request": request}).data
-        try:
+        with exception_transformer({**dict.fromkeys([DataError, IntegrityError], ExceptionTransformerDetail(400,


@silentninja This function call for exception_transformer is a little hard to follow. It would be clearer if we could declare the dictionaries in a separate line and pass them here.

I also think it would be clearer to instantiate the named tuple using keyword arguments (e.g. status_code=400, etc.) so that we don't have to remember the order in which to pass arguments and it's clearer what's going on without having to look at the original function signature.

kgodey · 2021-12-21T23:26:32Z

mathesar/api/viewsets/tables.py

@@ -105,18 +107,21 @@ def previews(self, request, pk=None):
            raise ValidationError("Incorrect number of columns in request.")

        table_data = TableSerializer(table, context={"request": request}).data
-        try:
+        with exception_transformer({**dict.fromkeys([DataError, IntegrityError], ExceptionTransformerDetail(400,
+                                                                                                            4001,


How do we generate numbers for error codes and make sure they are unique?

I think it might make sense to have a mapping of exceptions and error codes somewhere central and use it here.

I will replace the error codes with Enum

kgodey · 2021-12-21T23:27:14Z

mathesar/api/viewsets/tables.py

            preview_records = table.get_preview(columns)
-        except (DataError, IntegrityError) as e:
-            if type(e.orig) == InvalidTextRepresentation or type(e.orig) == CheckViolation:


It seems like we've lost this additional if check in the refactor. I don't think all DataError and IntegrityError instances are indicative of an invalid type cast.

silentninja · 2021-12-22T01:53:29Z

You mentioned in the PR description that you'd have to do it on a per-view basis, but I'm not sure why that's the case.
It wouldn't be possible to have different behaviour for an exception based on the route. For example, the below exception handling wouldn't be possible.

except (DataError, IntegrityError) as e:
            if type(e.orig) == InvalidTextRepresentation or type(e.orig) == CheckViolation:

If we use are using a global DRF error handler, we might have to raise a custom APIException in order to assign a proper error code.
While in the case of context managers, a parser that handles
if type(e.orig) == InvalidTextRepresentation or type(e.orig) == CheckViolation: can be used, making the exception handling reusable. This does provide reusability at the cost of hiding away details, making it hard to explicitly know about the error.

I am on the fence tbh, I prefer to use drf error handler and lots of custom ApiException but since our django layer is thin, I think we might be having lots of non DRF errors.

kgodey · 2021-12-22T03:40:54Z

I think it's fine to have to raise custom APIException errors to raise a proper error code, it'll be more explicit (and generally follow the Zen of Python's guidance on errors better). Even if we do have to catch a lot of DRF errors at the serializer or viewset layer, that's fine. I think it'll be easier for new contributors to the codebase as well.

Replace context manager error handling with custom ApiExceptions

…, to help convert validation errors from serializer to proper error body Add custom exception handler to convert exceptions that don't conform the error spec

…kage Fix error handler to look for default error handlers using the class of the exception raised

silentninja · 2022-01-10T14:56:48Z

@kgodey I have replaced context managers with drf error handlers, let me know if your thoughts on this. I find this to be better as it seems to fit the codebase. @dmos62 @mathemancer I would love to hear your opinions too if you have any

silentninja · 2022-01-10T15:09:10Z

I went down the rabbit hole trying to replicate golang syle error handling using context managers as it looked more dry, but it was not verbose and made the code less explicit to the readers.

@dmos62 Based on your comment I made the error codes centralised with option for the caller to supply their own if necessary

mathesar/exceptions/exceptions.py

kgodey

@silentninja Added some feedback.

mathesar/exceptions/exceptions.py

kgodey · 2022-01-10T20:35:39Z

mathesar/exceptions/exceptions.py

+
+ExceptionBody = namedtuple('ExceptionBody',
+                           [
+                               'code',


Per the spec, this should be error_code.

Made an intentional change as drf seems to be using code rather than error_code. Moreover code seems apt as it is already inside an error object

Okay, that's fine with me, but please update the spec too.

kgodey · 2022-01-10T20:40:38Z

mathesar/exceptions/exceptions.py

+
+from mathesar.exceptions.error_codes import ErrorCodes
+
+ExceptionBody = namedtuple('ExceptionBody',


I think this will be most readable in one of these two options:

ExceptionBody = namedtuple('ExceptionBody', ['code', 'message', 'field', 'details'], defaults=[None, None])

or

ExceptionBody = namedtuple( 'ExceptionBody', ['code', 'message', 'field', 'details'], defaults=[None, None] )

kgodey · 2022-01-10T20:45:33Z

mathesar/api/viewsets/tables.py


        column_names = [col["name"] for col in columns]
        if not len(column_names) == len(set(column_names)):
-            raise ValidationError("Column names must be distinct")
+            raise CustomValidationError([ExceptionBody(ErrorCodes.DistinctColumnNameRequired.value,


This is more involved than I think is necessary. It should be possible to just say something like:

raise DistinctColumnsRequiredException(message="Column names must be distinct")

The error code should be associated with the DistinctColumnsRequiredException in the exceptions module, it shouldn't need to be defined here.

This validation logic should ideally be in the validate_columns method of the serializer, not here (see docs). Then it will automatically associate it with the columns field and we don't have to do it manually.

The error code should be associated with the DistinctColumnsRequiredException in the exceptions module, it shouldn't need to be defined here.

Yes, that is the idea, I haven't added other custom Exceptions yet as it will be dealt with in this issue. But do you think it will be better to create a new Exception for one-off validations like this? I was looking at using the message or error code to match with the relevant information, similar to the drf friendly errors library but found there are places where a different message is needed for a similar error code.

This validation logic should ideally be in the validate_columns method of the serializer, not here

I am aware of it, been itching to move it for a long it, but didn't want to pollute the PR, but it does seem like a good time to move them into serializers

Do you think it will be better to create a new Exception for one-off validations like this?

I think it will be easiest to have a single pattern to follow for tying error codes to exceptions, even for one-off exceptions.

It's fine to pass different message values to the same exception, you can do this for Python exceptions as well so it's a common pattern.

it does seem like a good time to move them into serializers

Since you're already editing that particular code, I think it's fine to do it now. You don't have to do it for all of the code in this PR.

kgodey · 2022-01-10T20:49:00Z

mathesar/exceptions/error_codes.py

@@ -0,0 +1,13 @@
+from enum import Enum, unique


I think this should all live within mathessar/api, since these are only API related exceptions.

…onse which does not conform to the api spec to proper response

dmos62 · 2022-01-12T11:31:51Z

Together with Kriti's comments, this makes sense to me.

Add MathesarErrorMixin to serializers in order to change the validation errors into correct error format that conforms to the spec

…spective Modelserializer Add MultipleDataFiles Custom exception class and its respective error code Add Duplicate table exception class and its respective error code

mathemancer · 2022-01-13T09:32:47Z

@silentninja I don't have any major suggestions to add; I'm just glad you're taking up this heroic adventure.

…own within a serializer Rename CustomValidationError to GenericValidationError Move table preview validation to serializers

…nsupportedTypeException

… used in RecordViewset Add Api error classes for InvalidTableError to be used in DataFileViewset Change status of Api exception for UnsupportedTypeException to 500 Server error by default

Raise exception if error response is not formatted correctly in debug mode

…into mathesar-623-error-consistency

Add error code for `InvalidTableError` Add `ListSerializer` field error codes

Fix Exception handler to raise Exception if a non Api error is made during debug mode

Fix PolyMorphicSerializer to change class whenever mapping happens Fix exception handler to add missing error codes Fix test cases to reflect new error structure

silentninja · 2022-01-25T08:20:13Z

Replaced by PR #1016

silentninja added 4 commits December 16, 2021 14:36

Add error handler based on context manger to capture any exception th…

126968c

…at is thrown.

Add default exception parser

64b93bb

`previews` api route uses the context manager to capture error and convert it to proper error response

Fix exception transformer custom manager to call custom parsers with …

b60bba6

…exception as first argument similar to when calling default parser

Fix lint errors

add5522

silentninja requested a review from a team December 20, 2021 22:40

github-actions bot requested review from dmos62, eito-fis, kgodey, mathemancer and pavish December 20, 2021 22:40

silentninja added 2 commits December 21, 2021 04:16

Convert Exceptions that are not specified into proper 500 error response

b8e39d1

Merge branch 'master' into mathesar-623-error-consistency

acdc83f

silentninja changed the title ~~Mathesar 623 error consistency~~ Handle error consistently on the backend using context managers Dec 20, 2021

silentninja mentioned this pull request Dec 21, 2021

Fixes #859 Return proper error response on not null violation #910

Closed

7 tasks

Updated whitespace for readability.

6512ae6

kgodey requested changes Dec 21, 2021

View reviewed changes

silentninja marked this pull request as draft December 22, 2021 01:01

silentninja self-assigned this Dec 22, 2021

silentninja added the pr-status: revision A PR awaiting follow-up work from its author after review label Dec 22, 2021

silentninja added 3 commits January 6, 2022 05:55

Add Custom Api exception classes

261c2c6

Replace context manager error handling with custom ApiExceptions

Add drf-friendly-errors library as a requirement for mathesar backend…

7a370cf

…, to help convert validation errors from serializer to proper error body Add custom exception handler to convert exceptions that don't conform the error spec

Move exception handlers and custom exceptions into the exceptions pac…

c1fee0e

…kage Fix error handler to look for default error handlers using the class of the exception raised

silentninja changed the title ~~Handle error consistently on the backend using context managers~~ Handle error consistently on the backend using custom drf error handler Jan 10, 2022

dmos62 reviewed Jan 10, 2022

View reviewed changes

mathesar/exceptions/exceptions.py Outdated Show resolved Hide resolved

kgodey requested changes Jan 10, 2022

View reviewed changes

Add default api exception converters to convert validation error resp…

d3d3cea

…onse which does not conform to the api spec to proper response

silentninja added 2 commits January 12, 2022 19:24

Move exception related files into appropriate modules

7600ef3

Add MathesarErrorMixin to serializers in order to change the validation errors into correct error format that conforms to the spec

Move validation logic when creating a table in TableViewset to its re…

e9c483d

…spective Modelserializer Add MultipleDataFiles Custom exception class and its respective error code Add Duplicate table exception class and its respective error code

silentninja added 10 commits January 14, 2022 07:09

Add ValidationError base class to handle custom validation errors thr…

4640fa0

…own within a serializer Rename CustomValidationError to GenericValidationError Move table preview validation to serializers

Fix error code for column size mismatch exception

0746f5a

Add Api error classes for IntegrityError, InvalidTypeCastException, U…

37d758d

…nsupportedTypeException

Add Api error classes for BadFilter, BadSort, BadGroupException to be…

83b7500

… used in RecordViewset Add Api error classes for InvalidTableError to be used in DataFileViewset Change status of Api exception for UnsupportedTypeException to 500 Server error by default

Change is_pretty to accept a data dictionary instead of response object

8f9e2b4

Raise exception if error response is not formatted correctly in debug mode

Merge remote-tracking branch 'origin/mathesar-623-error-consistency' …

d8546dc

…into mathesar-623-error-consistency

Fix raising a exception instance instead of class

71e8164

Add ListSerializer to MathesarErrorMessageMixin fieldmap property

c9884eb

Add error code for `InvalidTableError` Add `ListSerializer` field error codes

Fix test cases to assert based on new error structure

c83eed2

Fix Exception handler to raise Exception if a non Api error is made during debug mode

Add nested serializer support for Error Mixin

de0ba25

Fix PolyMorphicSerializer to change class whenever mapping happens Fix exception handler to add missing error codes Fix test cases to reflect new error structure

silentninja closed this Jan 23, 2022

kgodey deleted the mathesar-623-error-consistency branch January 25, 2022 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle error consistently on the backend using custom drf error handler #909

Handle error consistently on the backend using custom drf error handler #909

silentninja commented Dec 20, 2021 •

edited

Loading

kgodey left a comment

kgodey Dec 21, 2021

kgodey Dec 21, 2021

kgodey Dec 21, 2021

kgodey Dec 21, 2021

silentninja Dec 22, 2021

kgodey Dec 21, 2021

silentninja commented Dec 22, 2021 •

edited

Loading

kgodey commented Dec 22, 2021

silentninja commented Jan 10, 2022

silentninja commented Jan 10, 2022 •

edited

Loading

kgodey left a comment

kgodey Jan 10, 2022

silentninja Jan 10, 2022

kgodey Jan 10, 2022

kgodey Jan 10, 2022

kgodey Jan 10, 2022

silentninja Jan 10, 2022 •

edited

Loading

silentninja Jan 10, 2022

kgodey Jan 10, 2022

kgodey Jan 10, 2022

dmos62 commented Jan 12, 2022

mathemancer commented Jan 13, 2022

silentninja commented Jan 25, 2022


		from mathesar.exceptions.error_codes import ErrorCodes

		ExceptionBody = namedtuple('ExceptionBody',

Handle error consistently on the backend using custom drf error handler #909

Handle error consistently on the backend using custom drf error handler #909

Conversation

silentninja commented Dec 20, 2021 • edited Loading

Checklist

Developer Certificate of Origin

kgodey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silentninja commented Dec 22, 2021 • edited Loading

kgodey commented Dec 22, 2021

silentninja commented Jan 10, 2022

silentninja commented Jan 10, 2022 • edited Loading

kgodey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silentninja Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmos62 commented Jan 12, 2022

mathemancer commented Jan 13, 2022

silentninja commented Jan 25, 2022

silentninja commented Dec 20, 2021 •

edited

Loading

silentninja commented Dec 22, 2021 •

edited

Loading

silentninja commented Jan 10, 2022 •

edited

Loading

silentninja Jan 10, 2022 •

edited

Loading