Skip to content

Conversation

rpatterson
Copy link

Description

The documentation says about a field's required argument:

Setting this to False also allows the object attribute or dictionary key to be omitted from output when serializing the instance.

But this isn't the case for Django ORM model instances because they always return at least the field default so there's never an AttributeError. This means that DRF's logic for omitting fields is never triggered and all model fields are always present in the JSON representation regardless of the field's required argument.

This PR fixes that by conditionally checking against the model field's default when the instance is a Django ORM model instance only when DRF's omit logic would take effect and forcing an AttributeError in those cases. There's full test coverage for all the code this PR touches and there are no tox errors that I didn't also see when runagainst encode/master.

http://www.django-rest-framework.org/api-guide/fields/#required

@lovelydinosaur
Copy link
Contributor

"May be omitted" isn't the same as "Should be omitted".

I'm not convinced we want this behavioral change, as is.
The exclusion of "pk: None" in some of those examples isn't necessarily what our users would expect.

@rpatterson
Copy link
Author

Ok, how about if it were triggered (via exc_on_model_default) only if a serializer Meta flag, whose default is set by a setting, were set to True?

@rpkilby
Copy link
Contributor

rpkilby commented Oct 4, 2017

@rpatterson - have you tested your project against the current master branch? #5375 made changes to the behavior of nested attribute access.

@rpatterson
Copy link
Author

rpatterson commented Oct 4, 2017 via email

@lovelydinosaur
Copy link
Contributor

only if a serializer Meta flag, whose default is set by a setting, were set to True?

It’s not really complexity I’d like to see us us add, no.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants