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

Updated NestedBoundField as_form_field to handle None #3464

Closed
wants to merge 2 commits into from
Closed

Updated NestedBoundField as_form_field to handle None #3464

wants to merge 2 commits into from

Conversation

andrewdodd
Copy link

This fixes the html rendering of a nested bound field that is allowed to be null.

For example, if my API has related model serialisers, sort of like this:

class PersonSerializer(serializers.HyperlinkedModelSerializer):
    class Meta:
        model = Person
        fields = ('id', 'url', 'first_name', 'last_name')

class GunSerializer(serializers.HyperlinkedModelSerializer):
    pointing_at = PersonSerializer(many=False, allow_null=True, required=False)

    class Meta:
        model = Gun
        fields = ('id', 'url', 'brand', 'pointing_at')

The 'pointing_at' field becomes a NestedBoundField. The API works fine with the null reference in the 'json-only' form (i.e. ?format=json). However, when the HTML form tries to render, I get an error 'NoneType' object has no attribute 'items'.

This change prevents the FOR loop from attempting to access .items() on the self.value when it is None.

NB: This does not fix the issue with rendering the put-object-form correctly. Perhaps there should be a mechanism to allow the PrimaryKeyRelatedField HTML picker?

This fixes the html rendering of a nested bound field that is allowed to be null.

For example, if my API has related model serialisers, sort of like this:

class PersonSerializer(serializers.HyperlinkedModelSerializer):
    class Meta:
        model = Person
        fields = ('id', 'url', 'first_name', 'last_name')

class GunSerializer(serializers.HyperlinkedModelSerializer):
    pointing_at = PersonSerializer(many=False, allow_null=True, required=False)
    
    class Meta:
        model = Gun
        fields = ('id', 'url', 'brand', 'pointing_at')

The 'pointing_at' field becomes a NestedBoundField. The API works fine with the null reference in the 'json-only' form (i.e. ?format=json). However, when the HTML form tries to render, I get an error 'NoneType' object has no attribute 'items'.

This change prevents the FOR loop from attempting to access .items() on the self.value when it is None.

NB: This does not fix the issue with rendering the put-object-form correctly. Perhaps there should be a mechanism to allow the PrimaryKeyRelatedField HTML picker?
@Ernest0x
Copy link
Contributor

Nice. This fix makes browsable api not crash in a similar situation I came across. In my case, the exception was AttributeError: 'str' object has no attribute 'items' as self.value was an empty string ('').

# Conflicts:
#	rest_framework/utils/serializer_helpers.py
@andrewdodd
Copy link
Author

Yes, I can't see the harm in it being merged.

@xordoquy
Copy link
Collaborator

Well, I guess a test case would help to get this reviewed.

@andrewdodd
Copy link
Author

@xordoquy is there an existing set of tests for the serializer_helpers? I looked but couldn't find any (though that was a while ago). In the context of no other tests for the NestedBoundField, I reckon my change is passable as a 'code review' test.

@andrewdodd
Copy link
Author

Hmm, I've dug into the history a bit, and it seems that there have been a few attempts at fixing this.

In particular, this is related to the changes for these issues:

As far as I can tell, the current implementation ensures in the __init__ that if the passed value is None then the value is replaced with an empty dict. However, there are values that are not None, which are being passed to the NestedBoundField class that are also not dicts. Is it allowable for non-None, non-dicts to be passed?

In some ways I think my changes might have got a bit lucky, by using the 'truthy/falsy' behaviour of empty strings etc.

@Ernest0x
Copy link
Contributor

Well, since the check is already done in the init(), it should probably remain there. Just make sure it catches other cases too (e.g. empty strings), not only None. Right?
E.g., maybe anything other than dictionaries would be better converted to an empty dict:

if not issubclass(type(value), dict):
    value = {}

@andrewdodd
Copy link
Author

I'm trying to quickly re-create the situation I had when I made this fix but I can't. @Ernest0x do you have example model & serializer classes that cause this issue to occur?

In fact, I think the fixes for #3260 have resolved my problem!

@tomchristie
Copy link
Member

I think the fixes for #3260 have resolved my problem!

Closing this for now - welcome to reassess if required.

@Ernest0x
Copy link
Contributor

@andrewdodd, @tomchristie:

In my case, it is about an "inner" nested field with None value, which is inside another "outer" nested field. So, what happens is this: While rendering the outer nested field, the inner nested field is converted from None to an empty string (''). This happens here. Then, the inner nested field is rendered and since its value has been converted to '', it fails because the check in the __init__() is only for None value. So, it is not converted to {} and an AttributeError exception is raised because strings have no attribute 'items' of course. I have made pull request #3677 to handle this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants