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

UniqueValidator raises exception for nested objects #2403

Closed
gregorjerse opened this issue Jan 12, 2015 · 14 comments
Closed

UniqueValidator raises exception for nested objects #2403

gregorjerse opened this issue Jan 12, 2015 · 14 comments

Comments

@gregorjerse
Copy link

gregorjerse commented Jan 12, 2015

I have a model Service which has contains a ForeignKey to atSifrantStoritev_model.

class Service(RemarkModel):
    code = ForeignKey(tSifrantStoritev_model, to_field='SiSto', related_name='services', verbose_name=_(u'The code of the given service.'), help_text=_(u'The unique code given to the given service, usually by health insurance companies.'))

class tSifrantStoritev_model(models.Model):
    class Meta:
        verbose_name = _(u"tSifSto_mo")
    # Unique added by Gregor for index
    # SiSto is referenced from Racuni.models.Service as Foreign key
    SiSto = models.CharField(max_length=1000, blank=True, unique=True) 

Using ModelViewSet for Service andatSifrantStoritev_model models the update for the object of type Service fails with the folloving data.

dict: {'code': {'SiSto': [u'This field must be unique.']}}

I traced the source of the exception to the function

    def exclude_current_instance(self, queryset):
        """
        If an instance is being updated, then do not include
        that instance itself as a uniqueness conflict.
        """
        if self.instance is not None:
            return queryset.exclude(pk=self.instance.pk)
        return queryset

in the validators.py file. The method fails to remove a currenttSifrantStoritev_model object since self.instance equals to None. Could this be a bug in Django Rest validation system or am I missing something?

@tomchristie
Copy link
Member

tomchristie commented Jan 13, 2015

It's a constraint that needs to be properly documented, yes.
Writable nested seriailizers cannot automatically deal correctly with updates which have uniqueness constraints on one of the nested instances, as these no general rule for determining what if a corresponding object already exists or what it's id is (It might be implied by the parent object, it might be a writable id field in the nested object, it might be a writeable URL in the nested object, etc...)

The best thing to do would be to instead write any validation in the top level serializer class, in the .validate() method.

If you wanted to give your serializers as an example I might be able to demonstrate how you'd do that.

Shame that there's nothing more we can really do about this, but writable nested serializers are inherently awkward cases.

@Ernest0x
Copy link
Contributor

Ernest0x commented Apr 22, 2015

@tomchristie
I think that the exclude_current_instance() method should become smarter and also exclude from the queryset the primary key if that is provided as a writable field in the nested serializer's data, even if instance is not set on the serializer. Since the primary key (usually 'id') is provided in the data, it is safe to exclude it from the queryset because:

  • If an object with that primary key already exists, the operation is an update.
  • If an object with that primary key does not exist, excluding it from the queryset makes no difference and everything will work as expected.

That said, I am having difficulty to implement this myself becaue it seems that not enough context (about the nested serializer's data) is passed to the UniqueValidator. Field validators are only passed the value for the field, but, as in this case, it may also be useful to get passed the full data passed to the serializer containing the field. If I am missing some important information here, could you please give some hint?

So, what is your opinion on this?

@Ernest0x
Copy link
Contributor

Ernest0x commented Apr 23, 2015

Good news for everyone dealing with writable nested model serializers and interested in bypassing the restriction with UniqueValidator. Based on the concept on my previous post, I have come up with the following workaround, which is overriding the model serializer's to_internal_value() method.

    def to_internal_value(self, data):
        if 'id' in data and 'id' in self.fields:
            try:
                obj_id = self.fields['id'].to_internal_value(data['id'])
            except ValidationError as exc:
                raise ValidationError({'id': exc.detail})
            for field in self.fields.values():
                for validator in field.validators:
                    if type(validator) == UniqueValidator:
                        # Exclude id from queryset for checking uniqueness
                        validator.queryset = validator.queryset.exclude(id=obj_id)
        return super(WritableNestedModelSerializer, self).to_internal_value(data)

(ValidationError must be imported from rest_framework.serializers and UniqueValidator from rest_framework.validators)

It is assumed that the model serializer has a writable 'id' field for the model's primary key field, which is also named 'id' (as usual with django models). In my case, all models have a field named 'id' as their primary key, so the above works perfectly for me, but it's not difficult to generalize this for whatever the primary key field is named both in the model and the serializer.

@tomchristie
Copy link
Member

tomchristie commented May 1, 2015

So, what is your opinion on this?

Not had time to properly review, but marking as 'Needs design decision' to keep it visible.

@tanuka72
Copy link

tanuka72 commented Jul 21, 2015

I think I'm encountering this issue. (I'm a newbie to DRF, so not very sure...)

I have a model Person that contains a OneToOneField pointing to User model. I am trying to use nested model serializers to create/retrieve/update them. I am able to perform GET/POST operations using the browsable APIs, but when I try to update an existing Person object using PUT, I encounter the error:
HTTP 400 Bad Request
"user": {
"username": [
"This field must be unique."
]
}

What are my options at this point, if indeed this is the same issue?

@Ernest0x
Copy link
Contributor

Ernest0x commented Jul 21, 2015

What are my options at this point, if indeed this is the same issue?

You can read my post above for a workaround. Keep in mind that my workaround requires that you include 'id' as a writable field in the serializer.

@tanuka72
Copy link

tanuka72 commented Jul 21, 2015

Since I'm using ModelSerializer, the UserSerializer that I've generated from User model has id field as read-only by default. I haven't figured out how to make the id field writable.
So when I include your code snippet from above, I can see that it does not execute beyond the clause
if 'id' in data and 'id' in self.fields:

However, would it be safe to expose the id field of Django's User object model as a writable field??

@tomchristie
Copy link
Member

tomchristie commented Jul 21, 2015

@tanuka72 I'd consider dropping ModelSerializer and instead using an explict serializer class - that way you can ensure that all the fields and validators are exactly as you want them, not as are generated by default. Drop into manage.py shell and print repr(ExistingModelSerializer()) to see the serializer class that's been generated as a starting point for that.

@tanuka72
Copy link

tanuka72 commented Jul 21, 2015

Thanks, will try that.

@Ernest0x
Copy link
Contributor

Ernest0x commented Jul 21, 2015

I haven't figured out how to make the id field writable.

Put an "extra_kwargs" attribute in the Meta class of your ModelSerializer subclass like this:

class Meta:
    model = MyModel
    extra_kwargs = {'id': {'read_only': False, 'required': False}}

@tomchristie
Copy link
Member

tomchristie commented Jul 21, 2015

That'll also work yes. Either way around it's a case over overriding the default field that's generated.

@tanuka72
Copy link

tanuka72 commented Jul 21, 2015

@Ernest0x Yes, your workaround works for me now.
However, I'll spend some time doing as @tomchristie suggested and use explicit serializer classes to see if that approach is better.

@tomchristie I really like the concept of writable nested model serializers as a concise way of presenting/updating related models in one shot. I'm trying to mimic what I've done before by combining multiple ModelForms in a single template. What would be the most elegant and DRY way to do that using DRF?

@tanuka72
Copy link

tanuka72 commented Jul 24, 2015

@tomchristie I replaced my ModelSerializers with explicit serializers (nested), and am facing a different issue now. Rather than confuse this thread, I've posted it on StackOverflow:
http://stackoverflow.com/questions/31582562/django-rest-framework-put-post-fails-in-case-of-blank-optional-fields-in-nested

Am I doing something wrong, or encountering a known problem?

jamaalscarlett added a commit to jamaalscarlett/django-push-notifications that referenced this issue Jan 21, 2016
When the serializer is nested the unique validator on registration_id
does not behave correctly.  This is a known issue with DRF
see: encode/django-rest-framework#2403

Added missing allow_null to device_id field

Fixes jazzband#270
jamaalscarlett added a commit to jamaalscarlett/django-push-notifications that referenced this issue Jan 21, 2016
When the serializer is nested the unique validator on registration_id
does not behave correctly.  This is a known issue with DRF
see: encode/django-rest-framework#2403

Added missing allow_null to device_id field

Fixes jazzband#270
jamaalscarlett added a commit to jamaalscarlett/django-push-notifications that referenced this issue Jan 21, 2016
When the serializer is nested the unique validator on registration_id
does not behave correctly.  This is a known issue with DRF
see: encode/django-rest-framework#2403

Added missing allow_null to device_id field

Fixes jazzband#270
jamaalscarlett added a commit to jamaalscarlett/django-push-notifications that referenced this issue Jan 22, 2016
When the serializer is nested the unique validator on registration_id
does not behave correctly.  This is a known issue with DRF
see: encode/django-rest-framework#2403

Added missing allow_null to device_id field

Fixes jazzband#270
@tomchristie tomchristie added this to the 3.4.0 Release milestone Jun 13, 2016
@tomchristie
Copy link
Member

tomchristie commented Jun 13, 2016

Closing as duplicated issue - Also tracking this as #2996.

marktucker-django pushed a commit to marktucker-django/Django-Push-Notifications that referenced this issue Jun 4, 2022
When the serializer is nested the unique validator on registration_id
does not behave correctly.  This is a known issue with DRF
see: encode/django-rest-framework#2403

Added missing allow_null to device_id field

Fixes #270
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants