-
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
Add kwargs into insert method #2169 #2170
base: master
Are you sure you want to change the base?
Add kwargs into insert method #2169 #2170
Conversation
@@ -345,7 +350,7 @@ def insert( | |||
insert_func = collection.insert_one | |||
|
|||
try: | |||
inserted_result = insert_func(raw) | |||
inserted_result = insert_func(raw, **kwargs) | |||
ids = ( | |||
[inserted_result.inserted_id] | |||
if return_one |
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 see a potential bug related to the following code (that is a bit below in this method):
# Apply inserted_ids to documents
for doc, doc_id in zip(docs, ids):
doc.pk = doc_id
In case you provide ordered=False and it raises an error (e.g if you try to insert 2 times a document with same pk), that piece of code won't be reached, leaving the mongoengine documents without id's. Instead of relying on zip
to get the corresponding id, we could rely on the raw
dict that actually gets updated with the id by pymongo (see this post). Now since it will raise an error anyway, perhaps it could be acceptable to not set the id on the Documents instances. What do you think?
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.
btw in case of successful unordered insert, I'm also unsure if we could rely on zip , as I don't know if we can rely on the order of inserted_result.inserted_ids
Closes #2169