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

in NestedUpdateMixin would be better to delete_reverse before update_or_create_reverse #50

Open
dz0 opened this issue Sep 7, 2018 · 7 comments

Comments

@dz0
Copy link

dz0 commented Sep 7, 2018

I think, if we have unique validator (let's say for field x),
and before request we had some related items
{"id":1, "x": 10},
{"id":2, "x": 5}

and in request
{
"id":42,
"stuf":"bar",
"related": [ {"id":2, "x": 10} ]
}

here we delete id=1, but the other "takes over" his value. (ps.: not tested)

so maybe switch the lines?

        self.update_or_create_reverse_relations(instance, reverse_relations)
        self.delete_reverse_relations_if_need(instance, reverse_relations)
@isasiluispy
Copy link

totally agree, this is a duplicated of my issue #49 , take a look
I am using the way you are saying (switching the lines) and works perfectly.

@ruscoder
Copy link
Member

@isasiluispy @dz0 Hello! Thanks for the contribution. It really makes sense. I'll prepare a fix soon.

@ruscoder
Copy link
Member

I've found that your solution breaks some important tests.

@ruscoder
Copy link
Member

ruscoder commented Sep 14, 2018

I've fixed another bug in delete method.

Now, only one test is broken when I switch update and delete:
WritableNestedModelSerializerTest.test_update_reverse_one_to_one_without_pk

This test covers functionality from this PR #36

@isasiluispy
Copy link

@ruscoder will this bug be fixed ? I think it's not wotking properly yet, for unique_together constraints
like this example #49 I still have to swicth the lines to get it working :(

@ruscoder
Copy link
Member

ruscoder commented Oct 23, 2018

@isasiluispy Your suggested fix brakes functionality from #36. I don't know how we can fix it saving backward compatibility for the moment.

As for me, I don't understand the worth of #36, but a few people asked for this feature.
If you want, you can make a PR with fix for #36 and for this issue.

@claytondaley
Copy link
Contributor

claytondaley commented Nov 15, 2018

To solve this problem, the logic in #36 needs to be moved into its own utility function and update changed to:

        self.materialize_one_to_one_pk(instance, reverse_relations)  # code from #36 relocated here
        self.delete_reverse_relations_if_need(instance, reverse_relations)
        self.update_or_create_reverse_relations(instance, reverse_relations)

Hopefully that helps someone who's motivated. I can't help ATM because I'm just here to evaluate the project as a replacement for our DIY logic.

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

4 participants