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
Fixed #18556 - Allowed RelatedManager.add() to execute 1 query where possible. #4319
Conversation
6139a79
to
6e8e3af
Compare
|
||
The ``add()`` method executes one query regardless of the number | ||
of objects being added, (unless the objects have not yet been saved | ||
to the database in which case that is done). In prior versions, it |
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.
I'd put the opening parenthesis before "in which case" instead of directly after the comma.
768fc00
to
4e1cc7f
Compare
@loic, tests are passing, but not sure the changes in GFK are entirely correct/sufficient. Some more tests might be needed. Could you check? |
@@ -36,7 +36,7 @@ Related objects reference | |||
In this example, the methods below will be available both on | |||
``topping.pizza_set`` and on ``pizza.toppings``. | |||
|
|||
.. method:: add(obj1, [obj2, ...]) | |||
.. method:: add(obj1, [obj2, ..., bulk=False]) |
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.
Isn't it bulk=True
?
6aa875a
to
9b4469d
Compare
7340a82
to
947efb8
Compare
05e0e5e
to
4c6043b
Compare
7a71d70
to
8d0fc18
Compare
setattr(obj, self.object_id_field_name, self.pk_val) | ||
obj.save() | ||
if obj._state.adding or obj._state.db != db: | ||
raise ValueError("%r instance isn't saved, but must be." % obj) |
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.
This exception message is different from that in related.py
though the logic/intention surrounding it seems to be the same. Is this intentional?
(FWIW, I find the message in related.py
to be clearer)
0da24ae
to
9324643
Compare
setattr(obj, self.object_id_field_name, self.pk_val) | ||
|
||
if bulk: | ||
ids = [] |
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.
Nitpick but pks
maybe?
… possible. Thanks Loic Bistuer for review.
Test failure seems unrelated. Left a couple of nitpick comments, otherwise PR looks great, I'm happy to merge it afterwards. |
Merged in adc0c4f. Sorry I took so long to give a final review ;) |
https://code.djangoproject.com/ticket/18556