Unique together validation fails with default value

[tomchuk]

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

See test case in PR

Expected behavior

Serializer validates

Actual behavior

Serializer does not validate

I haven't dug into the internals too much, but it appears what happens is that when validating the unique_together constraint on a model where one of the fields has a default value that is not passed in as data to the serializer, the default value is substituted for the purposes of validation, even in the case where that field is available from an instance.

[john-parton]

I'm not sure, but does this fix or help? #8002

Currently on master, the default value for a model field doesn't get copied to the field on the serializer, so how could it do the appropriate validation?

[john-parton]

I looked into it, and the defaults actually doesn't have anything to do with, I don't think.

If you modify your test and remove the default kwargs, it still fails, but with a different related error. That way it says that field1 and field2 are required, even though they're already set on the instance.

I think it has to do with the fact that the validators are passed only the data arguments. It doesn't look at anything on the passed instance.

If you look at UniqueTogetherValidator you'll see that it's passed attrs to validate. If you go back to ModelSerializer.is_valid, you'll see that it passes only initial_data and not the instance.

[tomchuk]

Thanks for following up @john-parton - it seems the test case is still failing when run against your branch

$ git config --get remote.origin.url
https://github.com/john-parton/django-rest-framework.git
$ git branch
* bugfix/pass-defaults-in-modelserializer
  master
$ pytest -k Issue7489Test -q
F                                                                                       [100%]
========================================== FAILURES ===========================================
____ Issue7489Test.test_model_serializer_substitutes_default_on_unique_together_validation ____
tests/test_model_serializer.py:1359: in test_model_serializer_substitutes_default_on_unique_together_validation
    self.assertTrue(serializer.is_valid(), serializer.errors)
E   AssertionError: False is not true : {'non_field_errors': [ErrorDetail(string='The fields field1, field2 must make a unique set.', code='unique')]}
=================================== short test summary info ===================================
FAILED tests/test_model_serializer.py::Issue7489Test::test_model_serializer_substitutes_default_on_unique_together_validation
1 failed, 1404 deselected in 5.62s

[john-parton]

Yeah, see my previous comment from 3 minutes ago. I have a patch that fixes, I think, but it's a little finicky. I'll see if I can get it stable and submit a PR.

[john-parton]

Here it is: #8004

This might cause some breakage, because previously there could be a dirty value in the database, and so long as you didn't try to touch it, it wouldn't cause re-validation.

For instance, suppose you have a validator on some_bad_field that prohibits "bad_value", and another unrelated field 'foo'.

# OK
instance = MyModel.objects.create(some_bad_field='bad_value')

# Old way didn't complain
serializer = MyModelSerializer(instance=instance, data={'foo': 'bar'})

# New way will re-validate ALL the fields, including some_bad_field, and fail
serializer = MyModelSerializer(instance=instance, data={'foo': 'bar'})

Whether that's desirable behavior or not should probably be discussed.

[tomchuk]

Certainly fixes my issue, thanks! I started poking around at alternatives, but it seems to fix this elsewhere would require some pretty invasive changes. Perhaps this change would be more palatable if it were configurable via something like Serializer.Meta.validate_instance_data.

+            if self.instance and getattr(self.Meta, 'validate_instance_data', False) is True:
+                validation_data = self.to_representation(self.instance)
+                validation_data.update(self.initial_data)
+            else:
+                validation_data = self.initial_data
             try:
-                self._validated_data = self.run_validation(self.initial_data)
+                self._validated_data = self.run_validation(validation_data)

[john-parton]

Ah yeah, making it opt-in makes sense. Probably opting in at the SETTINGS level would make a little more sense? Having to remember to do this for each model seems like a pain point. You probably want all your serializes to have consistent behavior one way or the other.

I'll see if I can update my PR

[john-parton]

@tomchuk I updated my PR so that the behavior is opt-in, using either a setting VALIDATE_ENTIRE_INSTANCE or a Meta attribute, Meta.validate_entire_instance.

You might want to go weigh in there.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[peterthomassen]

I think this is still current.

[ashupednekar]

I've faced this recenty, would like to go all in to figure out what's causing this and hopefully fix...

Can I take this up?

[auvipy]

I've faced this recenty, would like to go all in to figure out what's causing this and hopefully fix...

Can I take this up?

this is what is remaining #8130 (review)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[peterthomassen]

I've faced this recenty, would like to go all in to figure out what's causing this and hopefully fix...

Are you still in?