Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions mongoengine/document.py
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ def save(
_refs=None,
save_condition=None,
signal_kwargs=None,
upsert=None,
Copy link
Collaborator

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 to force_insert=False)

Copy link
Author

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.

**kwargs,
):
"""Save the :class:`~mongoengine.Document` to the database. If the
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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):
Copy link
Collaborator

Choose a reason for hiding this comment

The 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().
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

Choose a reason for hiding this comment

The 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Author

Choose a reason for hiding this comment

The 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"
)
Expand Down
47 changes: 47 additions & 0 deletions tests/document/test_instance.py
Original file line number Diff line number Diff line change
Expand Up @@ -1457,6 +1457,53 @@ 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

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.
Depending on how we resolved some of the comments I made in this PR, we'll probably need a few more tests as well

def test_can_save_if_not_included(self):
class EmbeddedDoc(EmbeddedDocument):
pass
Expand Down