adding upsert option to save #2532
Conversation
| @@ -332,6 +332,7 @@ def save( | |||
| _refs=None, | |||
| save_condition=None, | |||
| signal_kwargs=None, | |||
| upsert=None, | |||
There was a problem hiding this comment.
The default is True so I believe we should make that clear on the api, thus upsert=True (similarly to force_insert=False)
There was a problem hiding this comment.
The problem is that is not True. It only defaults to true IF we do not send a save condition. I'm not sure how to better fix that because there are 3 states.
| @@ -505,7 +508,7 @@ def _integrate_shard_key(self, doc, select_dict): | |||
|
|
|||
| return select_dict | |||
|
|
|||
| def _save_update(self, doc, save_condition, write_concern): | |||
| def _save_update(self, doc, save_condition, write_concern, upsert=None): | |||
There was a problem hiding this comment.
same remark here, default should be True
| @@ -524,12 +527,13 @@ def _save_update(self, doc, save_condition, write_concern): | |||
|
|
|||
| update_doc = self._get_update_doc() | |||
| if update_doc: | |||
| upsert = save_condition is None | |||
| if upsert is None: | |||
| upsert = save_condition is None | |||
There was a problem hiding this comment.
save_condition=something and upsert=True are contradictory, perhaps we should raise an exception whenever it enters that condition. If we don't do this, the alternative is to mention which one will prevail on the docstring (and in my opinion save_condition should prevail).
There was a problem hiding this comment.
Agree, I am going to add a guard at the top asserting that upsert = True and save_condition cannot be set
| with set_write_concern(collection, write_concern) as wc_collection: | ||
| last_error = wc_collection.update_one( | ||
| select_dict, update_doc, upsert=upsert | ||
| ).raw_result | ||
| if not upsert and last_error["n"] == 0: | ||
| if save_condition is not None and last_error["n"] == 0: |
There was a problem hiding this comment.
The fact that we silently ignore it if upsert=False is not entirely fixing (#564), I'm a bit hesitating to make it raise an exception because I believe people impacted by #564 will prefer to be aware of the problem
What do you think @erdenezul ?
There was a problem hiding this comment.
Maybe just adding to the doc string explaining the behavior of upsert=False and that it will silently fail? You do return if an object was created or not at the end of the method.
| p2.save(upsert=None) # default if you dont pass it | ||
|
|
||
| assert Person.objects.count() == 1 | ||
|
|
There was a problem hiding this comment.
I believe we should have 1 (perhaps more) test that ensures how providing both save_condition & upsert would behave.
Depending on how we resolved some of the comments I made in this PR, we'll probably need a few more tests as well
|
Thanks for your feedback @bagerard. |
|
I made the small changes raising exceptions but got no response on the True vs None because of the three states a month ago. Any updates/thoughts? |
|
Thought i would check in again on this... any response? |
|
It has been ~3 months and closing in on a year since my original pull request, Thought I would check in again on this... any response? |
|
just remembered this PR, thought i would check in... |
I believe this addresses #564 without breaking backwards compatibility. Allowing you to explicitly choose not to upsert so you don't create bad documents in your database.
This may not be the correct solve long term as save as it is can literally break your database if you delete the backing object between when you loaded this one but it does maintain backwards compatibility in case someone is using that "feature".
Includes tests as well, my first pull request here so let me know if I need to do anything else.