drf-problems icon indicating copy to clipboard operation
drf-problems copied to clipboard

drf-problems fields overwrite Validation field info

Open jayvdb opened this issue 3 years ago • 3 comments

If there is a serializer data validation error on a field title, status or type, the validation message will be overwritten by drf-problems which forces exception meta info into those fields.

jayvdb avatar Jul 26 '20 22:07 jayvdb

It should be noted that I have no control over these fields - they are in a third-party Django app, and that app is critical, and my project is in production, so hacking about in critical third-party apps renaming their fields is not happening. I can change the front-end to suit drf-problems - but drf-problems needs to not conflict with other Django apps.

One approach used by other drf exception handlers is to move field validation errors down to a new subkey under the root.

I really appreciate that this app keeps the validation errors unmodified at the top node - that is great for compatibility, and easy adoption. But IMO copying the field validation errors to a subkey will allow clients to fix their code to use the subkey only in the areas where it is necessary because drf-problems is overwriting title/status/type at the top level.

I think there should be a config setting like DRF_PROBLEMS_CLOBBER = True enabled by default, which preserves the current behaviour, but also copies the field validations under the new subkey.

Then existing sites with clients already expecting field validation errors in the root can disable DRF_PROBLEMS_CLOBBER, and then only rely on the title/status/type on views which dont use those field name or after type checking those key values in the frontend response handler. Not pretty, but pragmatic. Some sites may have clients already using their API, and the third-party code is not under their control, or the users cant/wont be upgrading their clients quickly. In this case, drf-problems can be added in a "mostly working" state, but backwards compatibility and drf field validation takes priority.

Adding a new subkey introduces another key which could conflict with the original response, so it is worth thinking about it carefully, and possibly also worth adding a setting so sites can change it if it is causing them problems.

  • https://github.com/ivlevdenis/drf_pretty_exception_handler uses errors.

  • https://github.com/hipo/hipo-drf-exceptions uses detail. detail is also a drf core response field name, so this doesnt cause conflicts with field names. (Anyone using detail as a field name is about to hit their head against a wall a few times) Using detail would be a bit confusing, as the drf core detail values usually have a different structure (just a string normally afaik). But the client could work this out reasonably easily with type checking.

  • https://github.com/abogoyavlensky/drf-common-exceptions also uses detail.

  • https://github.com/rahulz/drf_exception_handler/blob/master/drf_exception_handler/exception/handler.py uses data. IMO this is too commonly used as a field name, often for a jsonfield.

IMO, detail seems like a sane choice, but it isnt backwards compatible, as it conflicts with existing use of detail in responses, which includes apps like allauth, rest-auth, etc, etc.

So, IMO, an improbable default key like $errors or _errors is more appropriate.

jayvdb avatar Jul 27 '20 06:07 jayvdb

Hmm, I see that errors is already being used in the fallback:

    else:
        data = dict(errors=response.data, title=problem_title,
                    status=problem_status, type=problem_type)

jayvdb avatar Jul 27 '20 12:07 jayvdb

I think there should be a config setting like DRF_PROBLEMS_CLOBBER = True enabled by default, which preserves the current behaviour, but also copies the field validations under the new subkey.

That sounds like a good idea. Since this library ensures view responses are valid RFC 7807 response, title, type, status are members of it (detail would probably be provided by the view itself in most cases). Having a global flag to control whether to follow valid format as per RFC or backwards-compatible response should definitely be up to the user.

Adding a new subkey introduces another key which could conflict with the original response, so it is worth thinking about it carefully, and possibly also worth adding a setting so sites can change it if it is causing them problems.

I think asking user for the key in config setting would suffice. Something like DRF_PROBLEMS_PROBLEM_KEY = None (default for overwriting the root node). Setting it to any non-null string would use that key for providing problem details.

shivanshs9 avatar Aug 05 '20 14:08 shivanshs9