MongoEngine / mongoengine

A Python Object-Document-Mapper for working with MongoDB
http://mongoengine.org
MIT License
4.24k stars 1.23k forks source link

Problem handling dots in DictField keys #2539

Open MarSoft opened 3 years ago

MarSoft commented 3 years ago

Consider the following code:

class TestModel(db.Document):
    test = db.DictField()

obj = TestModel()
obj.test['key.with.dot'] = 'abc'
obj.save()

I think this should work, because MongoDB allows dots in dict key names as of version 3.6. But in fact I get the following error:

Traceback (most recent call last):
  File "test.py", line 8, in <module>
    obj.save()
  File "mongoengine/document.py", line 450, in save
    self._clear_changed_fields()
  File "mongoengine/base/document.py", line 547, in _clear_changed_fields
    field_name = data._reverse_db_field_map.get(part, part)
AttributeError: 'NoneType' object has no attribute '_reverse_db_field_map'

After some digging I found that _clear_changed_fields takes list of string keys from _get_changed_fields() where each key is a dot-delimited path into the object. Obviously this doesn't play well with keys containing dots.

I think it would be safer to store keys as tuples rather than dot-delimited strings, but it will probably involve too many changes?

Tested with MongoEngine==0.23.1 and python 3.9.5.

kunal1gupta commented 3 years ago

I think it's more of a python issue. We don't face the same issue when defining the field as an Embedded doc with its own class. Python is not able to interpret dicts using dots, whereas Mongodb, which is closer to JS is. If you are able to define the structure of the DictField as an EmbeddedDoc, might make it easier

bagerard commented 3 years ago

@MarSoft is right, MongoEngine tracks the fields that are being changed through _changed_fields using a "dot-delimited" convention to describe the path.

I'm not sure why the "dot-delimited" convention was chosen in the first place in _changed_fields, a tuple indeed sound like a better approach. I know it's useful for embedded fields and for list fields.

I don't think it would be such a big work to do the change but there might be edge cases that I'm not considering here

princess-entrapta commented 1 year ago

Is this issue still in 0.25.0 ? We use mongoengine in production and we have the same error 'Nonetype' object has no attribute '_reverse_db_field_map' but we're not sure at all where it could come from.

I do not reproduce OP bug by testing locally. In what conditions is this crash occurring ?

rghotra commented 1 year ago

Seems to only be an issue with object.save() - object.update(field=updatedField) works

princess-entrapta commented 1 year ago

We in the end had a problem with dots if field keys as well