Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Combination of many=True and a dotted source doesn't allow a default #7550

Closed
5 of 6 tasks
stephenfin opened this issue Sep 26, 2020 · 0 comments
Closed
5 of 6 tasks

Comments

@stephenfin
Copy link
Contributor

stephenfin commented Sep 26, 2020

Checklist

  • I have verified that that issue exists against the master branch of Django REST framework.
  • I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
  • This is not a usage question. (Those should be directed to the discussion group instead.)
  • This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
  • I have reduced the issue to the simplest possible case.
  • I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

Consider a model with a many-many relationship to itself via a through model.

class FooModel(models.Model):
    text = models.CharField(max_length=100)
    bar = models.ForeignKey(
        'BarModel', null=True, blank=True, on_delete=models.SET_NULL,
        related_name='foos', related_query_name='foo')


class BarModel(models.Model):
    pass

We can attempt to serialize this using something like the following.

        class _FooSerializer(serializers.ModelSerializer):
            class Meta:
                model = FooModel
                fields = ('id', 'text')

        class FooSerializer(serializers.ModelSerializer):
            other_foos = _FooSerializer(source='bar.foos', many=True)

            class Meta:
                model = FooModel
                fields = ('id', 'other_foos')

(NOTE: I haven't tested this yet. My case is a lot more complicated so I'll try reduce that is this reproducer isn't valid)

Expected behavior

This is intended to flatten the default output from:

{
    "id": 1,
    "bar": {
        "foos": [
            {
                "id": 2,
                "text": "abc"
            }
        ]
    }
}

to

{
    "id": 1,
    "bar": [
        {
            "id": 2,
            "text": "abc"
        }
    ]
}

Actual behavior

We get an exception:

Got AttributeError when attempting to get a value for field `related` on serializer `PatchDetailSerializer`.
The serializer field might be named incorrectly and not match any attribute or key on the `Patch` instance.
Original exception text was: 'NoneType' object has no attribute 'patches'.

As discussed in #5489, you need to have a default value if you wish to use dotted notation with a nullable value.

class FooSerializer(serializers.ModelSerializer):
    bar = _FooSerializer(source='bar.foos', default=None)

    class Meta:
        fields = ('id', 'bar')

This fixes things for empty case. However, we also need to specify many=True to correct the not-empty case otherwise we get the following error:

'RelatedManager' object has no attribute 'pk'

If we do that, we now get:

'NoneType' object has no attribute 'patches'

for the empty case 😞

stephenfin added a commit to stephenfin/django-rest-framework that referenced this issue Oct 3, 2020
The docs note:

  When serializing fields with dotted notation, it may be necessary to
  provide a `default` value if any object is not present or is empty
  during attribute traversal.

However, this doesn't work for fields with 'many=True'. When using
these, the default is simply ignored.

The solution is simple: do in 'ManyRelatedField' what we were already
doing for 'Field', namely, catch possible 'AttributeError' and
'KeyError' exceptions and return the default if there is one set.

Signed-off-by: Stephen Finucane <[email protected]>
Closes: encode#7550
stephenfin added a commit to stephenfin/django-rest-framework that referenced this issue Oct 3, 2020
Signed-off-by: Stephen Finucane <[email protected]>
stephenfin added a commit to stephenfin/django-rest-framework that referenced this issue Oct 3, 2020
Signed-off-by: Stephen Finucane <[email protected]>
stephenfin added a commit to getpatchwork/patchwork that referenced this issue Dec 13, 2020
This wasn't writeable for reasons I haven't been able to figure out.
However, it's not actually needed: the 'PatchSerializer' can do the job
just fine, given enough information. This exposes a bug in DRF, which
has been reported upstream [1]. While we wait for that fix, or some
variant of it, to be merged, we must monkey patch the library.

[1] encode/django-rest-framework#7550
[2] encode/django-rest-framework#7574

Signed-off-by: Stephen Finucane <[email protected]>
Reported-by: Ralf Ramsauer <[email protected]>
Closes: #379
Cc: Daniel Axtens <[email protected]>
Cc: Rohit Sarkar <[email protected]>
stephenfin added a commit to getpatchwork/patchwork that referenced this issue Dec 13, 2020
This wasn't writeable for reasons I haven't been able to figure out.
However, it's not actually needed: the 'PatchSerializer' can do the job
just fine, given enough information. This exposes a bug in DRF, which
has been reported upstream [1]. While we wait for that fix, or some
variant of it, to be merged, we must monkey patch the library.

Conflicts:
	patchwork/api/patch.py

NOTE(stephenfin): Conflicts are due to the absence of commit d3d4f9f
("Add Django 3.0 support") which we do not want to backport here.

[1] encode/django-rest-framework#7550
[2] encode/django-rest-framework#7574

Signed-off-by: Stephen Finucane <[email protected]>
Reported-by: Ralf Ramsauer <[email protected]>
Closes: #379
Cc: Daniel Axtens <[email protected]>
Cc: Rohit Sarkar <[email protected]>
(cherry picked from commit fe07f30)
@encode encode locked and limited conversation to collaborators Mar 3, 2021
tomchristie pushed a commit that referenced this issue Jun 8, 2022
* Handle unset fields with 'many=True'

The docs note:

  When serializing fields with dotted notation, it may be necessary to
  provide a `default` value if any object is not present or is empty
  during attribute traversal.

However, this doesn't work for fields with 'many=True'. When using
these, the default is simply ignored.

The solution is simple: do in 'ManyRelatedField' what we were already
doing for 'Field', namely, catch possible 'AttributeError' and
'KeyError' exceptions and return the default if there is one set.

Signed-off-by: Stephen Finucane <[email protected]>
Closes: #7550

* Add test cases for #7550

Signed-off-by: Stephen Finucane <[email protected]>
sigvef pushed a commit to sigvef/django-rest-framework that referenced this issue Dec 3, 2022
* Handle unset fields with 'many=True'

The docs note:

  When serializing fields with dotted notation, it may be necessary to
  provide a `default` value if any object is not present or is empty
  during attribute traversal.

However, this doesn't work for fields with 'many=True'. When using
these, the default is simply ignored.

The solution is simple: do in 'ManyRelatedField' what we were already
doing for 'Field', namely, catch possible 'AttributeError' and
'KeyError' exceptions and return the default if there is one set.

Signed-off-by: Stephen Finucane <[email protected]>
Closes: encode#7550

* Add test cases for encode#7550

Signed-off-by: Stephen Finucane <[email protected]>

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants