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

WritableNestedModelSerializer and unique_together #49

Open
isasiluis28 opened this issue Aug 29, 2018 · 12 comments
Open

WritableNestedModelSerializer and unique_together #49

isasiluis28 opened this issue Aug 29, 2018 · 12 comments

Comments

@isasiluis28
Copy link

WritableNestedModelSerializer won't work when there is a foreign key relation with a unique constraint con the child model.
screen shot 2018-08-29 at 9 15 11 am
screen shot 2018-08-29 at 9 17 57 am

I already tried the mixin UniqueFieldsMixin, but it does not work either.

@isasiluis28 isasiluis28 changed the title WritableNestedModelSerializer on update of nested serializer with many=True and unique constraint WritableNestedModelSerializer and unique_together Aug 29, 2018
@ir4y
Copy link
Member

ir4y commented Aug 30, 2018

Hi @isasiluis28
Thank you for the contribution.

Can you add a test which reproduces the bug?

@isasiluis28
Copy link
Author

Sure @ir4y
Suppose we add a new register via POST like this
screen shot 2018-08-30 at 8 49 13 am

This is the object we have created
screen shot 2018-08-30 at 8 51 46 am

Then if we try to update creating a new object for producto_detalles (without the ID field, this is because we want to delete the other objects), but with same ID of veterinaria that is already there before the update (which by the way is the one with a constraint)
screen shot 2018-08-30 at 8 57 23 am

Then it raises an IntegrityError
screen shot 2018-08-30 at 8 58 57 am

I think the problem is that is trying to create the new reverse relations without deleting the previous ones, so at that point we still have the previous objects, and when it tries to create the new relations it will raise an IntegrityError beacause of the existance of an object that already satisfies the constraint.
I think that the algorithm has to remove first the relations where the IDs are not found anymore, then create the new reverse relations.

Looking at the code that handles the updates in NestedUpdateMixin I found that first tries to create or update the new relations, then deletes the relations that are no longer needed, doesn't it have to be the other way around? meaning first delete the relations, then create the new ones?
screen shot 2018-08-30 at 9 14 42 am

@ir4y
Copy link
Member

ir4y commented Aug 30, 2018

I mean fork drf-writable-nested and add the test which will fail. TDD style.
Reproduce your issue in code.

@ruscoder
Copy link
Member

ruscoder commented Sep 3, 2018

Hello @isasiluis28! Thank you for the contribution.
UniqueFieldsMixin cannot work with unique_together validator for the moment.

But I think it is possible to support this validator.

@ayushin
Copy link

ayushin commented May 9, 2019

Is there any progress on this?

@skamensky
Copy link

@ruscoder , any advice on how to implement this? I'd give it a go.

@samomar
Copy link

samomar commented Dec 16, 2019

UniqueFieldsMixin does not work with unique_together for me

I have to send an empty list that deletes the items first then send the new items

I guess the way it works by default, it deletes the previous nested items and creates a new ones, we should await and confirm the items are deleted before sending in new items otherwise it'll raise an IntegrityError

@samomar
Copy link

samomar commented Dec 16, 2019

The only way I can get this to work is by passing an ID or making one of my unique fields a primary key.

@Telemaco019
Copy link

Is there any progress? Is unique_together validator going to be supported in one of the next releases?

@FranzForstmayr
Copy link

@ruscoder
Can you give a hint what's necessary to get this issue fixed?

@JohnMulligan
Copy link

I hit a wall on this as well.

@ruscoder
Copy link
Member

Hi. Unfortunately, I haven't used drf-writable-nested for more than 6 years and I can't give a hint on how to implement it properly without diving into the implementation, but according to the discussion above there's a workaround suggested in #50 that has one limitation, but it might be not relevant for someone who affected by this issue.

So, I can recommend something like

class CustomWritableNestedModelSerializer(WritableNestedModelSerializer):
    def update(self, instance, validated_data):
        relations, reverse_relations = self._extract_relations(validated_data)

        # Create or update direct relations (foreign key, one-to-one)
        self.update_or_create_direct_relations(
            validated_data,
            relations,
        )

        # Update instance
        instance = super(NestedUpdateMixin, self).update(
            instance,
            validated_data,
        )
        self.delete_reverse_relations_if_need(instance, reverse_relations)
        self.update_or_create_reverse_relations(instance, reverse_relations)
        instance.refresh_from_db()
        return instance

that is actually copy-paste of original update with changed order of invocation of delete_reverse_relations_if_need and update_or_create_reverse_relations (according to the suggestions from #50).

In general, if someone would like to solve a conflict between #50 and #36 it might be merged into the main branch.

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

No branches or pull requests

9 participants