Skip to content

Conversation

Akash-Kumar-Sen
Copy link
Contributor

@Akash-Kumar-Sen Akash-Kumar-Sen commented May 6, 2023

@nessita
Copy link
Contributor

nessita commented May 8, 2023

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.

@Akash-Kumar-Sen
Copy link
Contributor Author

@nessita there might be another possible issue #34549, check my comment for more information on how to reproduce.

Copy link
Contributor

@shangxiao shangxiao left a 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¢

@Akash-Kumar-Sen Akash-Kumar-Sen requested a review from shangxiao May 9, 2023 10:15
@nessita
Copy link
Contributor

nessita commented May 9, 2023

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 main:

1. INSERT INTO "ticket_30382_parentmodel" ("id", "name") VALUES (1, '')
2. UPDATE "ticket_30382_parentmodel" SET "name" = '' WHERE "ticket_30382_parentmodel"."id" = 1
3. INSERT INTO "ticket_30382_childmodel" ("parentmodel_ptr_id") VALUES (1)

With this PR:

1. INSERT INTO "ticket_30382_parentmodel" ("id", "name") VALUES (1, '')
2. UPDATE "ticket_30382_parentmodel" SET "name" = '' WHERE "ticket_30382_parentmodel"."id" = 1
3. SELECT 1 AS "a" FROM "ticket_30382_childmodel" WHERE "ticket_30382_childmodel"."parentmodel_ptr_id" = 1 LIMIT 1
4. INSERT INTO "ticket_30382_childmodel" ("parentmodel_ptr_id") VALUES (1)

@Akash-Kumar-Sen Akash-Kumar-Sen marked this pull request as draft May 18, 2023 16:10
@Akash-Kumar-Sen Akash-Kumar-Sen marked this pull request as ready for review May 21, 2023 06:17
Copy link
Member

@charettes charettes left a 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.

@Akash-Kumar-Sen Akash-Kumar-Sen marked this pull request as draft May 23, 2023 04:52
@Akash-Kumar-Sen
Copy link
Contributor Author

Thank you for the suggestions @charettes ! Updated the PR accordingly. Getting feedback from other members of the community would be great.

@Akash-Kumar-Sen Akash-Kumar-Sen marked this pull request as ready for review May 24, 2023 14:11
@Akash-Kumar-Sen Akash-Kumar-Sen requested a review from charettes May 24, 2023 14:12
@felixxm
Copy link
Member

felixxm commented Jun 13, 2023

@charettes Do you want to review it again? If not, I can try to push it forward.

Copy link
Member

@charettes charettes left a 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.

Copy link
Member

@felixxm felixxm left a 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.

@felixxm felixxm changed the title Fixed #30382 : Passing the force_insert flag when saving models on inherited parents Fixed #30382 -- Allowed specifying parent classes in force_insert of Model.save(). Jun 29, 2023
Copy link
Member

@felixxm felixxm left a 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 🥇

@felixxm felixxm force-pushed the ticket_30382 branch 3 times, most recently from 6b71fff to be0f91a Compare June 29, 2023 12:05
Copy link
Member

@charettes charettes left a 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

@felixxm
Copy link
Member

felixxm commented Jun 29, 2023

@charettes Thanks 👍 I pushed edits to docs.

@felixxm felixxm merged commit a40b010 into django:main Jun 30, 2023
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.

5 participants