Skip to content

Fix nullable source='*' fields #6659

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

Merged
merged 2 commits into from
May 20, 2019
Merged

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented May 9, 2019

Fixes #3451, #5635, and #6609.

Description

When a field like CustomField(source='*', allow_null=True) is attempting to validate a null value, the Field.validate_empty_values() method returns (True, None). This indicates that further validation should be skipped. However, set_value breaks, because the empty source_attrs indicates that a dictionary should be provided, but the value returned from validate_empty_values is None.

This an unusual edge case, but I'd argue it's valid. Although the given value may be null, source='*' indicates that the field is validating the whole object, not just the given value. If we decide not validating is the correct behavior, then validate_empty_values should at least return an empty dict.

Other considerations

I'm not entirely sure what the behavior should be when CustomField(required=False) or Serializer(partial=True).

@rpkilby rpkilby force-pushed the nullable-complex-fields branch from 75eb2ff to 5b809a9 Compare May 9, 2019 02:14
@rpkilby rpkilby added this to the 3.10 Release milestone May 9, 2019
# Nullable `source='*'` fields should not be skipped when its named
# field is given a null value. This is because `source='*'` means
# the field is passed the entire object, which is not null.
elif self.source == '*':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fix seems to be very simple on!! good job

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't the intention to return empty dict from validate_empty_values in this case? Otherwise I see failure in to_internal_value with error 'Invalid data. Expected a dictionary, but got NoneType.'?

@tomchristie
Copy link
Member

@rpkilby I guess I'm happy with this when you are.

@rpkilby rpkilby merged commit 1b8141a into encode:master May 20, 2019
@rpkilby rpkilby deleted the nullable-complex-fields branch May 20, 2019 21:58
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request Dec 3, 2022
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.

None and source="*" compatibility
4 participants