Unique together validation fails with default value

tomchuk commented 4 years ago

Checklist

[x] I have verified that that issue exists against the master branch of Django REST framework.
[x] I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
[x] This is not a usage question. (Those should be directed to the discussion group instead.)
[x] 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.)
[x] I have reduced the issue to the simplest possible case.
[x] 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 commented 3 years ago

I'm not sure, but does this fix or help? https://github.com/encode/django-rest-framework/pull/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Here it is: https://github.com/encode/django-rest-framework/pull/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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[bot] commented 2 years ago

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 commented 2 years ago

I think this is still current.

ashupednekar commented 1 year ago

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 commented 1 year ago

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 https://github.com/encode/django-rest-framework/pull/8130#pullrequestreview-1189278903

stale[bot] commented 1 year ago

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 commented 1 year ago

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?

encode / django-rest-framework