-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
Fixed #30382 -- Allowed specifying parent classes in force_insert of Model.save(). #16830
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
Conversation
I have started the review of this work but I'm having issues reproducing the original problem as described in the ticket. I have commented in there to hopefully obtain further details from the reporter. |
@nessita there might be another possible issue #34549, check my comment for more information on how to reproduce. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added my 2¢
As mentioned in the ticket, while this work does generate the expected queries for: ChildModel(id=1).save(force_insert=True) For this code, an extra query is now executed: parent = ParentModel.objects.create(id=1)
ChildModel.objects.create(parentmodel_ptr=parent) In
With this PR:
|
39c5161
to
c8d3cf0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tweaks @Akash-Kumar-Sen!
I left some suggestions for improvements, happy to discuss them with you.
As for the new feature itself, it would be great to get some feedback from other members of the community. I believe that allowing an issubclass
predicate of parents to be passed to force_insert
is an elegant solution to the problem as explained on the mailing list but I've never actually needed this feature myself so it'd be great to have at least someone else's feedback.
Maybe we can get the original reporter of the issue to chime in here.
Thank you for the suggestions @charettes ! Updated the PR accordingly. Getting feedback from other members of the community would be great. |
@charettes Do you want to review it again? If not, I can try to push it forward. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @Akash-Kumar-Sen!
The validation seem sane logic wise but I think the messaging could be improved.
@felixxm I'm not sure what the planed sequencing is here in terms of merging but I think that we should make sure diamond MTI insertion with a default pk works in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Akash-Kumar-Sen Thanks for updates 👍 I left more comments.
e182ac5
to
6bed3f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Akash-Kumar-Sen Thanks 👍 I pushed edits to docs and reorganized tests.
@charettes Thanks for all reviews 🥇
6b71fff
to
be0f91a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a few suggestion to the docs, otherwise the change LGTM
@charettes Thanks 👍 I pushed edits to docs. |
Fix for issue#30382
Approach :