Skip to content

INTPYTHON-617 - Improve DictField to_python performance #2888

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

INTPYTHON-617 - Improve DictField to_python performance #2888

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2931801
WIP
NoahStapp Jun 23, 2025
d4619bd
INTPYTHON-617 - Improve DictField to_python performance
NoahStapp Jun 24, 2025
40aa9a5
Cleanup
NoahStapp Jun 24, 2025
e377528
Refresh CI
NoahStapp Aug 19, 2025
2586d45
Refresh CI
NoahStapp Aug 19, 2025
6cf60d2
to_python should return falsy values
NoahStapp Sep 24, 2025
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
8 changes: 8 additions & 0 deletions mongoengine/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1088,6 +1088,14 @@ def prepare_query_value(self, op, value):

return super().prepare_query_value(op, value)

def to_python(self, value):
to_python = getattr(self.field, "to_python", None)
return (
{k: to_python(v) for k, v in value.items()}
if to_python and value
else value or None
Copy link
Contributor

@neilsh neilsh Aug 18, 2025

Choose a reason for hiding this comment

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

I think this would convert a {} to None

Copy link
Author

@NoahStapp NoahStapp Aug 19, 2025

Choose a reason for hiding this comment

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

class MyModel(db.Document):
    data_dict_1 = db.DictField(required=False)

m = MyModel()
m.data_dict_1 = {}
m.save()

model = MyModel.objects.first()
print(model.data_dict_1)

Outputs {}.

Copy link
Contributor

@neilsh neilsh Sep 22, 2025

Choose a reason for hiding this comment

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

Some example test cases showing where the behavior differs:

from mongoengine import Document, DictField, IntField, ListField, MapField
from tests.utils import MongoDBTestCase


class TestDictFieldRegressions(MongoDBTestCase):

    def test_nested_empty_dict_in_list_of_dictfield_roundtrips_as_dict_regression(self):
        """Regression: master => {}, PR => None.

        Empty dict appended into ListField(DictField()) should roundtrip as {}.
        """
        class Model(Document):
            items = ListField(DictField())

        Model.drop_collection()

        doc = Model(items=[{"a": 1}]).save()
        Model.objects(id=doc.id).update(push__items={})
        doc.reload()

        assert doc.items[-1] == {}
        assert isinstance(doc.items[-1], dict)

    def test_nested_empty_dict_in_list_of_mapfield_roundtrips_as_dict_regression(self):
        """Regression: master => {}, PR => None.

        Empty dict appended into ListField(MapField(IntField())) should roundtrip as {}.
        """
        class Model(Document):
            items = ListField(MapField(IntField()))

        Model.drop_collection()

        doc = Model(items=[{"a": 1}]).save()
        Model.objects(id=doc.id).update(push__items={})
        doc.reload()

        assert doc.items[-1] == {}
        assert isinstance(doc.items[-1], dict)

    def test_top_level_dictfield_default_preserves_empty_dict(self):
        """No regression: master => {}, PR => {}."""
        class Post(Document):
            info = DictField()

        Post.drop_collection()

        Post(info={}).save()
        p = Post.objects.first()
        assert p.info == {}
        assert isinstance(p.info, dict)

    def test_top_level_dictfield_null_true_preserves_empty_dict_regression(self):
        """Regression: master => {}, PR => None.

        With null=True, empty dict should still roundtrip as {} at top level.
        """
        class PostNull(Document):
            info = DictField(null=True)

        PostNull.drop_collection()

        PostNull(info={}).save()
        pn = PostNull.objects.first()

        assert pn.info == {}
        assert isinstance(pn.info, dict)

Copy link
Contributor

@neilsh neilsh Sep 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's not as elegant, but I think this should handle the failed cases while preserving most of the performance improvement:

def to_python(self, value):
    # Fast-path for mapping of homogenous child field conversions,
    # preserving behavior (no {} -> None) while avoiding ComplexBaseField's
    # generic recursion when possible.
    if not isinstance(value, dict) or self.field is None:
        return super().to_python(value)
    elif not value:  # Keep empty dicts intact
        return value
    elif type(self.field).to_python is BaseField.to_python:  # identity
        return value
    else:
        child_to_python = self.field.to_python
        return {k: child_to_python(v) for k, v in value.items()}

Copy link
Author

@NoahStapp NoahStapp Sep 24, 2025

Choose a reason for hiding this comment

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

I believe this can be simplified:

def to_python(self, value):
    if value is None:
        return None
    to_python = getattr(self.field, "to_python", None)
    if not to_python or not value:
        return value
    return {k: to_python(v) for k, v in value.items()}

This passes all of the current test cases plus the ones you've added above.

Copy link
Contributor

@neilsh neilsh Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@NoahStapp if a DictField receives an invalid/non-dict value (e.g. a list instead of a dict), I think the one I proposed (and orig) would give a mongoengine ValidationError, while the one you proposed would give a standard error (AttributeError?)

getattr also has a bit of a performance hit, although I haven't benchmarked it

Copy link
Author

@NoahStapp NoahStapp Oct 6, 2025

Choose a reason for hiding this comment

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

Using my proposed code, I get a ValidationError when passing an invalid value to a DictField:

class Model(db.Document):
    field = db.DictField()

m = Model(field=[])
m.save()

------
mongoengine.errors.ValidationError: ValidationError (Model:None) (Only dictionaries may be used in a DictField: ['field'])

)


class MapField(DictField):
"""A field that maps a name to a specified field type. Similar to
Expand Down
Loading