-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
adding upsert option to save #2532
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -332,6 +332,7 @@ def save( | |
_refs=None, | ||
save_condition=None, | ||
signal_kwargs=None, | ||
upsert=None, | ||
**kwargs, | ||
): | ||
"""Save the :class:`~mongoengine.Document` to the database. If the | ||
|
@@ -361,6 +362,8 @@ def save( | |
Raises :class:`OperationError` if the conditions are not satisfied | ||
:param signal_kwargs: (optional) kwargs dictionary to be passed to | ||
the signal calls. | ||
:param upsert: (optional) explicitly forces upsert to value if it is an | ||
update | ||
|
||
.. versionchanged:: 0.5 | ||
In existing documents it only saves changed fields using | ||
|
@@ -407,7 +410,7 @@ def save( | |
object_id = self._save_create(doc, force_insert, write_concern) | ||
else: | ||
object_id, created = self._save_update( | ||
doc, save_condition, write_concern | ||
doc, save_condition, write_concern, upsert | ||
) | ||
|
||
if cascade is None: | ||
|
@@ -505,11 +508,15 @@ 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same remark here, default should be True |
||
"""Update an existing document. | ||
|
||
Helper method, should only be used inside save(). | ||
""" | ||
if upsert and (save_condition is not None): | ||
raise ValueError( | ||
"Updating with a save_condition implies upsert is False or None but upsert is True" | ||
) | ||
collection = self._get_collection() | ||
object_id = doc["_id"] | ||
created = False | ||
|
@@ -524,12 +531,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that we silently ignore it if What do you think @erdenezul ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
raise SaveConditionError( | ||
"Race condition preventing document update detected" | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1457,6 +1457,65 @@ def test_inserts_if_you_set_the_pk(self): | |
|
||
assert 2 == self.Person.objects.count() | ||
|
||
def test_save_upsert_false_doesnt_insert_when_deleted(self): | ||
class Person(Document): | ||
name = StringField() | ||
|
||
Person.drop_collection() | ||
|
||
p1 = Person(name="Wilson Snr") | ||
p1.save() | ||
p2 = Person.objects().first() | ||
p1.delete() | ||
p2.name = " Bob Snr" | ||
p2.save(upsert=False) | ||
|
||
assert Person.objects.count() == 0 | ||
|
||
def test_save_upsert_true_inserts_when_deleted(self): | ||
class Person(Document): | ||
name = StringField() | ||
|
||
Person.drop_collection() | ||
|
||
p1 = Person(name="Wilson Snr") | ||
p1.save() | ||
p2 = Person.objects().first() | ||
p1.delete() | ||
p2.name = "Bob Snr" | ||
p2.save(upsert=True) | ||
|
||
assert Person.objects.count() == 1 | ||
|
||
def test_save_upsert_null_inserts_when_deleted(self): | ||
# probably want to remove this as this is bad but preserved for backwards compatibility | ||
# see https://github.com/MongoEngine/mongoengine/issues/564 | ||
class Person(Document): | ||
name = StringField() | ||
|
||
Person.drop_collection() | ||
|
||
p1 = Person(name="Wilson Snr") | ||
p1.save() | ||
p2 = Person.objects().first() | ||
p1.delete() | ||
p2.name = "Bob Snr" | ||
p2.save(upsert=None) # default if you dont pass it | ||
|
||
assert Person.objects.count() == 1 | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we should have 1 (perhaps more) test that ensures how providing both save_condition & upsert would behave. |
||
def test_save_upsert_raises_value_error_when_upsert_and_save_condition_set(self): | ||
class Person(Document): | ||
name = StringField() | ||
|
||
Person.drop_collection() | ||
|
||
p1 = Person(name="Wilson Snr") | ||
p1.save() | ||
p1.name = "Bob Snr" | ||
with pytest.raises(ValueError): | ||
p1.save(save_condition={}, upsert=True) | ||
|
||
def test_can_save_if_not_included(self): | ||
class EmbeddedDoc(EmbeddedDocument): | ||
pass | ||
|
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.
The default is True so I believe we should make that clear on the api, thus
upsert=True
(similarly toforce_insert=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.
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.