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

many=True, allow_null=True allows null instead of the list, not nulls in the list. #2766

Merged
merged 3 commits into from Jul 31, 2015

Conversation

delinhabit
Copy link
Contributor

@delinhabit delinhabit commented Mar 25, 2015

EDIT from @tomchristie

Summary of this:

children = ChildSerializer(many=True, allow_null=True)

Will allow null instead of a list, but not null items in the list.
If you want to treat null items in the list as valid, you'll need to instead do this:

children = ListSerializer(child=ChildSerializer(allow_null=True))

This fixes #2673

I tried to keep the test case minimal, but at the same to show a relevant use case. I agree that it might be done differently (suing a different API design), but that will work only for new projects. For existing projects that need to be migrated to DRF >= 3, it's a pain not to be able to just pass allow_null=True as it was done in DRF 2.x. The good part about this is that the fix is a single line of code changed.

serializer = self.get_serializer(input_data)

assert not serializer.is_valid()
assert set(serializer.errors) == {'nested'}
Copy link
Member

@kevin-brown kevin-brown Mar 26, 2015

Choose a reason for hiding this comment

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

Just a heads up, set syntax {items} was added in Python 2.7.

You should be able to just assert "nested" in serializer.errors instead.

Copy link
Contributor Author

@delinhabit delinhabit Mar 26, 2015

Choose a reason for hiding this comment

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

Yeah. I totally forgot about the py26 compatibility. Thanks for the heads up.

Your suggestions should work, but I wanted to ensure that it's only that error, and not that it's one of the errors.

@thedrow
Copy link
Contributor

thedrow commented Jun 23, 2015

LGTM.

@tomchristie
Copy link
Member

tomchristie commented Jun 23, 2015

I'm happy with the code change at this point, but I'd like to see simpler test cases. They seem over complex at the moment.

(Apologies for the brief review)

@delinhabit
Copy link
Contributor Author

delinhabit commented Jun 24, 2015

@tomchristie, @thedrow Thanks for taking the time to review this.

I'm happy to change the test cases. I'll think about how should I do that, but if you have any concrete suggestions / advises please let me know.

amount = serializers.DecimalField(max_digits=6, decimal_places=2)

class NestedSerializer(serializers.Serializer):
foo = serializers.CharField()
Copy link
Member

@tomchristie tomchristie Jun 24, 2015

Choose a reason for hiding this comment

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

Could we just have a single NestedSerializer with a field named example, and use the same nested serializer for both the allow_null and not_allow_null cases?

@delinhabit delinhabit force-pushed the allow-null-list-serializer branch from 719089f to bbd44ae Compare Jul 25, 2015
@delinhabit
Copy link
Contributor Author

delinhabit commented Jul 25, 2015

@tomchristie Rebased with master to fix the conflicts and addressed the CR comments. Thanks for your suggestions.

(Sorry it took so long)

expected_errors = {'not_allow_null': [serializer.error_messages['null']]}
assert serializer.errors == expected_errors

def test_run_the_field_validation_even_if_the_field_is_null(self):
Copy link
Member

@tomchristie tomchristie Jul 27, 2015

Choose a reason for hiding this comment

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

I understand the other two cases, but the motivation of this one is unclear to me.
Could you run me through an example of why this behavior is desired?
Alternatively, we could simply drop this test?

Copy link
Contributor Author

@delinhabit delinhabit Jul 27, 2015

Choose a reason for hiding this comment

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

This test is exactly why I raised the problem in the first place. The idea is that if allow_null=True, then null is a valid value for the field so the validation mechanism should still call validate_field.

There can be use cases when you want to permit a null for a field only for one type of authenticated user/key and not for the other. Thus we need to run the validation method for the field in order to be able to have that functionality.

@tomchristie
Copy link
Member

tomchristie commented Jul 27, 2015

Raising discussion here: https://groups.google.com/forum/#!searchin/django-rest-framework/allow_null$20list/django-rest-framework/R5bhJtMLRLs/K9m-AEqV3tEJ

Still can't say I'm sure on what behavior we'd expect.

@tomchristie tomchristie added this to the 3.2.0 Release milestone Jul 31, 2015
@tomchristie tomchristie changed the title Test case for using allow_null with many=True and a fix for it Modify subtle ChildSerializer(many=True, allow_null=True) behavior. Jul 31, 2015
tomchristie added a commit that referenced this pull request Jul 31, 2015
Modify subtle ChildSerializer(many=True, allow_null=True) behavior.
@tomchristie tomchristie merged commit a543fae into encode:master Jul 31, 2015
1 check passed
@tomchristie tomchristie changed the title Modify subtle ChildSerializer(many=True, allow_null=True) behavior. many=True, allow_null=True allows null instead of the list, not nulls in the list. Jul 31, 2015
@tomchristie tomchristie mentioned this pull request Jul 31, 2015
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants