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

Fixes unexpected deletion of related objects. #97

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gibsonbailey
Copy link

@gibsonbailey gibsonbailey commented Dec 24, 2019

When updating an instance with foreign key relations, the objects linking to the instance should not be deleted. Instead, the foreign key should simply be removed from the objects to the instance. This commit checks if the related field is a foreign key, and, if so, it unlinks the object. If the field cannot be null, the object still gets deleted, as it does in the current state of the master branch.

…king to the instance should not be deleted. Instead, the foreign key should simply be removed from the objects to the instance. This commit checks if the related field is a foreign key, and, if so, it unlinks the object. If the field cannot be null, the object gets deleted.
@codecov-io
Copy link

codecov-io commented Dec 24, 2019

Codecov Report

Merging #97 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
+ Coverage   98.59%   98.61%   +0.01%     
==========================================
  Files           3        3              
  Lines         213      216       +3     
==========================================
+ Hits          210      213       +3     
  Misses          3        3
Impacted Files Coverage Δ
drf_writable_nested/mixins.py 98.52% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87a1476...771a4f7. Read the comment docs.

@ruscoder
Copy link
Member

@gibsonbailey Thanks for the contribution. Could you please add a failing test which is fixed with your changes?

@gibsonbailey
Copy link
Author

gibsonbailey commented Dec 26, 2019

Yes, thanks for the consideration.

…gn key, i.e. s, are not deleted when they are removed from a profile.
@gibsonbailey
Copy link
Author

@ruscoder Test has been added. Thanks Vadim!

@ruscoder
Copy link
Member

@gibsonbailey Thanks for the tests.
I'm not sure that this logic should work in this way.
Let's consider how Django works with null=True and on_delete=CASCADE.
https://docs.djangoproject.com/en/2.2/ref/models/fields/#django.db.models.ForeignKey.on_delete

With CASCADE, Django deletes the instance if the related instance is deleted. To avoid this you should pass SET_NULL/SET_DEFAULT.

So, I think the logic should be changed to use on_delete param instead of null

@ruscoder
Copy link
Member

My comment is still actual. So I don't close it and don't accept for now.

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

Successfully merging this pull request may close these issues.

3 participants