support nested structures #776

bpleshakov · 2020-03-25T14:37:46Z

Fixes #769

Description of the Change

Checklist

PR only contains one change (considered splitting up PR)
unit-test added
documentation updated
CHANGELOG.md updated (only for user relevant changes)
author name in AUTHORS

bpleshakov · 2020-03-25T14:39:31Z

@sliverc Hi. I made PR we discussed here #769
Please link that issue with the pull request.

UPD:
Coverage check shows that there is quite a lot of uncovered code that I didn't change and that is not quite related to this pr. What should we do with this issue?

UPD 2:
I haven't found doc section that describes settings available in JSONAPISettings. Where I am supposed to update doc?

Update my fork

# Conflicts: # rest_framework_json_api/renderers.py

codecov · 2020-03-25T16:04:00Z

Codecov Report

Merging #776 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #776      +/-   ##
==========================================
+ Coverage   97.32%   97.36%   +0.03%     
==========================================
  Files          56       56              
  Lines        2920     2962      +42     
==========================================
+ Hits         2842     2884      +42     
  Misses         78       78

Impacted Files	Coverage Δ
rest_framework_json_api/settings.py	`100.00% <ø> (ø)`
example/tests/unit/test_renderers.py	`100.00% <100.00%> (ø)`
rest_framework_json_api/renderers.py	`87.46% <100.00%> (+0.40%)`	⬆️

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 1318610...deec195. Read the comment docs.

auvipy

what about increasing the coverage? & are you sure this is compliant with 1.0 or 1.1 specs of json-api?

bpleshakov · 2020-03-26T09:13:33Z

@auvipy yes it is complaint with 1.0 json-api spec.
Please, read #769

UPD:
I have done everything I could to increase coverage of this pr.
Any other uncovered code is strictly unrelated to this pr and has to be covered in other prs.

auvipy

can you check the build failures?

bpleshakov · 2020-03-31T20:55:11Z

@auvipy Yeah, I have checked. I really messed up.

Btw, don't merge this pr, I found out that there is an issue with error object generation, which I will resolve on Thursday.

sliverc

Thanks for working on this. See my inline comments. For error object object generation when looking through the code it is not totally obvious what causes this but hope you can tackle that issue as well.

example/tests/test_rendering_strategies.py

pytest.ini

rest_framework_json_api/settings.py

rest_framework_json_api/renderers.py

f json_api settings f pytest.ini

…hat DeprecationWarning raises whenever nested serializer is used without correct settings

sliverc

Thanks for your updates. The PR is shaping up nicely - good work so far! There are some comments I have added though.

example/tests/unit/test_renderers.py

rest_framework_json_api/serializers.py

example/tests/test_nested_errors.py

rest_framework_json_api/serializers.py

sliverc · 2020-04-17T18:35:10Z

rest_framework_json_api/utils.py

    # handle all errors thrown from serializers
    else:
        for field, error in response.data.items():
            field = format_value(field)
            pointer = '/data/attributes/{}'.format(field)
            # see if they passed a dictionary to ValidationError manually
+            # The bit tricky problem is here. It is may be nested drf thing in format
+            # name: error_object, or it may be custom error thrown by user. I guess,
+            # if it is drf error, dict will always have single key


The comment is not quite clear what is happening here. Can you elaborate on this issue?

For the record, all of this needed to put the correct pointer into an error object.

Well, the issue here is that the error object may be any valid dict with any string as a key and any object as a value.

Let consider two examples. First one is validator method in identity serializer
As you can see users can specify any dict whatever they want as a detailed message, and old behavior would be "pass it as is, do nothing". By the way, I am not entirely sure that old behavior is correct and we still should pass this object as is. At least we have to append pointer here if it is possible.

The second example is second level attribute exception
In this case, an error object will be

{ "body": ["This field is required"] }

where the body is a field name. Moreover, if we consider a more deep nested structure, the error object will be more complicated with nested dicts and lists. This is just how DRF validation works and passes errors.
But I am quite sure if we get a single dict DRF error it will be always a dict with a single key consists of field name and either error message, dict or list depending on the serializer field type.

Since we don't know from where the error object come out neither we don't know serializer structure at this point it is a tricky question how we can separate custom error object defined by users from that errors generated by DRF itself. I assumed this heuristic with a length of the dict is the simplest solution, even though it doesn't feel entirely correct.

The other possible solutions are:

Overwrite serializer's validation process and extend the validation error class so we can put the correct pointer at the moment we get an error. I assume this is the most correct way to deal with these problems. However, it will take too much effort and I am not able to do that in the nearest future.

The spec states:

An error object MAY have the following members:

id: a unique identifier for this particular occurrence of the problem.
links: a links object containing the following members:
about: a link that leads to further details about this particular occurrence of the problem.
status: the HTTP status code applicable to this problem, expressed as a string value.
code: an application-specific error code, expressed as a string value.
title: a short, human-readable summary of the problem that SHOULD NOT change from occurrence to occurrence of the problem, except for purposes of localization.
detail: a human-readable explanation specific to this occurrence of the problem. Like title, this field’s value can be localized.
source: an object containing references to the source of the error, optionally including any of the following members:
pointer: a JSON Pointer [RFC6901] to the associated entity in the request document [e.g. "/data" for a primary data object, or "/data/attributes/title" for a specific attribute].
parameter: a string indicating which URI query parameter caused the error.
meta: a meta object containing non-standard meta-information about the error.

We can come up with another heuristic to check that if dict contains any subset of these fields. However it doesn't protect us in two problems: first, these fields may overlap with serializer fields and second, users may pass as a dict key whatever they want which may be neither serializer field or spec stated field.

I am not sure what we should do with this comment, I can expand it with detailed explanation copy-pasted from here with todo inserted or leave it as is with the issue reported.
I would prefer to report this to the doc as a known issue and merge this pr as is.

After a night with an idea of overwriting the validation process, I don't think it entirely solves this problem. We would still get all of this problems in case a user uses plain drf serializers

From a JSON API specification perspective the nested json is actually just a value and it suggests that an error is reported on an JSON API attribute level.
As we are having some issues concerning this would it not be better that we simply report an error on the attribute level and not on the nested value? I have a feeling this would also follow the spec closer although it doesn't seem to forbid to do more nested errors.

@sliverc
I am not sure.
In my opinion, JSON:API spec is quite strict about what exactly pointer attribute must contain

pointer: a JSON Pointer [RFC6901] to the associated entity in the request document [e.g. "/data" for a primary data object, or "/data/attributes/title" for a specific attribute].

So if we have an error somewhere deep in a nested structure, but we report it as it would happen in top-level attribute, it just doesn't feel correct and seems to be out of spec.
Moreover, it is not clear what exactly we have to put into the detail field. We aren't seem permitted to put here JSON object, detail has to be string, as far as I understand. If we will try to extract this string from the depth of the error object, we would fail into the same problem.

By the way, I want to know should we inject the source object into the custom error object or not? I think It would be nice if we persist one if the error object doesn't have it already.

great that we have access to the serializer. In case of injecting of the source object. Yes that would be good, but only if it doesn't already exist. It is kind of blowing up this PR though. Our main issue at this point is error handling and the rest would be mergeable in this PR. So what do you think if we move the part of error handling into a second PR and merge this PR? In the second PR injecting source could also be part.

We won't release before this second PR would have been landed.

that sounds reasonable

I agree with that.

@sapiosexual Could you extract the error handling part from this PR into a new then? Thanks.

example/tests/test_errors.py

sliverc

Great. Thanks.

auvipy

great job

support nested structures

314ea5a

bpleshakov and others added 4 commits March 25, 2020 17:43

Merge pull request #1 from django-json-api/master

01e399d

Update my fork

Merge remote-tracking branch 'origin/master' into patch-3

aa0d7b9

# Conflicts: # rest_framework_json_api/renderers.py

f code style

49c479e

f ignore deprecation warning

0b099c2

Boris Pleshakov added 2 commits March 26, 2020 02:12

f cover settings file with test

5ddcf69

f add changelog

5a9f1e5

auvipy suggested changes Mar 26, 2020

View reviewed changes

Boris Pleshakov added 3 commits March 26, 2020 14:19

+ tests for relations strategy

db139e0

f codestyle

cf31527

f included

387500a

auvipy suggested changes Mar 31, 2020

View reviewed changes

f wrong check

9071b4f

bpleshakov changed the title ~~support nested structures~~ WIP: support nested structures Mar 31, 2020

sliverc requested changes Apr 6, 2020

View reviewed changes

Boris Pleshakov added 5 commits April 10, 2020 19:08

f tests

ab9a1ae

f json_api settings f pytest.ini

f pep

65f9b15

f rewrite complex drf errors structures into JSON-API format

67442ab

f move DeprecationWarning from settings to metaclass, which ensures t…

177bfbc

…hat DeprecationWarning raises whenever nested serializer is used without correct settings

f pep8

fdc8166

bpleshakov changed the title ~~WIP: support nested structures~~ support nested structures Apr 14, 2020

auvipy approved these changes Apr 14, 2020

View reviewed changes

bpleshakov requested a review from sliverc April 14, 2020 11:00

sliverc requested changes Apr 17, 2020

View reviewed changes

Boris Pleshakov added 2 commits April 19, 2020 03:02

requested changes

72df66f

Merge master

874bd9c

f unused import

873b48e

bpleshakov requested a review from sliverc April 19, 2020 01:15

sliverc mentioned this pull request May 3, 2020

Avoid overwriting of pointer when using custom errors #399

Closed

bpleshakov force-pushed the patch-3 branch from b95bc5b to 873b48e Compare May 8, 2020 23:00

auvipy suggested changes May 8, 2020

View reviewed changes

example/tests/test_errors.py Outdated Show resolved Hide resolved

Merge branch 'master' into patch-3

82d2002

auvipy suggested changes May 13, 2020

View reviewed changes

example/tests/test_errors.py Outdated Show resolved Hide resolved

bpleshakov force-pushed the patch-3 branch from 390d99c to 82d2002 Compare May 13, 2020 14:25

f remove error handling part

f5f68d4

bpleshakov mentioned this pull request May 13, 2020

WIP: Error handling #787

Closed

5 tasks

Update CHANGELOG.md

deec195

sliverc approved these changes May 14, 2020

View reviewed changes

auvipy approved these changes May 14, 2020

View reviewed changes

sliverc merged commit 2adfb5b into django-json-api:master May 14, 2020

sliverc mentioned this pull request Jun 5, 2020

Exception handling with nested resources #438

Closed

This was referenced Aug 24, 2020

Add handling of nested errors #815

Merged

Remove global ignore of serializer warnings #816

Merged

sliverc mentioned this pull request Dec 3, 2020

inflection ignored in lookup by RelationshipView #790

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support nested structures #776

support nested structures #776

bpleshakov commented Mar 25, 2020 •

edited by sliverc

Loading

bpleshakov commented Mar 25, 2020 •

edited

Loading

codecov bot commented Mar 25, 2020 •

edited

Loading

auvipy left a comment

bpleshakov commented Mar 26, 2020 •

edited

Loading

auvipy left a comment

bpleshakov commented Mar 31, 2020

sliverc left a comment

sliverc left a comment

sliverc Apr 17, 2020

bpleshakov Apr 19, 2020 •

edited

Loading

bpleshakov Apr 19, 2020

sliverc Apr 29, 2020

bpleshakov Apr 29, 2020

bpleshakov May 8, 2020

sliverc May 11, 2020

auvipy May 11, 2020

bpleshakov May 11, 2020

sliverc May 13, 2020

sliverc left a comment

auvipy left a comment

support nested structures #776

support nested structures #776

Conversation

bpleshakov commented Mar 25, 2020 • edited by sliverc Loading

Description of the Change

Checklist

bpleshakov commented Mar 25, 2020 • edited Loading

codecov bot commented Mar 25, 2020 • edited Loading

Codecov Report

auvipy left a comment

Choose a reason for hiding this comment

bpleshakov commented Mar 26, 2020 • edited Loading

auvipy left a comment

Choose a reason for hiding this comment

bpleshakov commented Mar 31, 2020

sliverc left a comment

Choose a reason for hiding this comment

sliverc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpleshakov Apr 19, 2020 • 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

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

sliverc left a comment

Choose a reason for hiding this comment

auvipy left a comment

Choose a reason for hiding this comment

bpleshakov commented Mar 25, 2020 •

edited by sliverc

Loading

bpleshakov commented Mar 25, 2020 •

edited

Loading

codecov bot commented Mar 25, 2020 •

edited

Loading

bpleshakov commented Mar 26, 2020 •

edited

Loading

bpleshakov Apr 19, 2020 •

edited

Loading