Scille / umongo

sync/async MongoDB ODM, yes.
MIT License
448 stars 63 forks source link

Update on Dict object results in non-concurrent safe Mongo update #348

Open Bernie opened 3 years ago

Bernie commented 3 years ago

If one intends to update a Dict object on a umongo Model by adding or removing an entry in the model, it should be done by a setting/unsetting the Dict elements to avoid issues related to concurrent document updates.

However, umongo instead treats the dict naively, simply setting the entire dict as a whole:

from umongo import Document, fields
from umongo.frameworks import MongoMockInstance
from mongomock import MongoClient
instance = MongoMockInstance(MongoClient().test)

@instance.register
class TestWithDict(Document):
    model_dict = fields.DictField(fields.StringField(), fields.StringField())

test = TestWithDict(model_dict={})
test.commit()

test = TestWithDict.find_one()

test.model_dict['my_key'] = 'my_val'
test.to_mongo(update=True)

The end result of the update query is:

{'$set': {'model_dict': {'my_key': 'my_val'}}}

Which unfortunately is not concurrent safe. If another process had made an update on the dict between this fetch and update, the other update would be erased.

A safer update operation would be:

{'$set': {'model_dict.my_key': 'my_val'}}

This type of update enables a safe merging of the model's view with what is existing in the DB even when an outside update was made in between the fetch and update, and should be preferred.

Neustradamus commented 3 years ago

@Bernie: I search your email address, can you publish a comment here with and remove after? Thanks in advance.

lafrech commented 3 years ago

@Bernie, I'm not sure we can even try to support this.

The issue is the same with embedded documents and lists, isn't it?

Only the first document level generates '$set' and such. For all nested levels, the whole substructure is erased.

Not saying it is a good thing, but that's how it is right now, the Dict behaviour is not an exception, and changing this would be a huge change. Besides, I'm not sure concurrent safety can be achieved. Even if we managed to yield a correct atomic change, there would be corner cases where one change makes the other obsolete (like if the first change removes the whole substructure).

Neustradamus commented 3 years ago

@Bernie: I am sorry to talk here but can you look your ksmbd 425 ticket?

It is solved or not?

Can you reply to the author?

Have you tested the 3.4.0 release?

Thanks in advance.