Fix no_dereference thread safetyness + force ComplexField's field arg to be a BaseField instance + make max_length a keyword only arg on ListField

Author: bagerard

docs/changelog.rst

@@ -8,6 +8,9 @@ Development
 ===========
 - (Fill this out as you fix issues and develop your features).
 - Switch tox to use pytest instead of legacy `python setup.py test` #2804
+- improve ReferenceField wrong usage detection
+- BREAKING CHANGE: max_length in ListField is now keyword only on ListField signature
+- BREAKING CHANGE: Force `field` argument of ListField/DictField to be a field instance (e.g ListField(StringField()) instead of ListField(StringField)
 
 Changes in 0.28.2
 =================

mongoengine/base/document.py

@@ -806,10 +806,15 @@ def _from_son(cls, son, _auto_dereference=True, created=False):
 
         fields = cls._fields
         if not _auto_dereference:
+            # if auto_deref is turned off, we copy the fields so
+            # we can mutate the auto_dereference of the fields
             fields = copy.deepcopy(fields)
 
+        # Apply field-name / db-field conversion
         for field_name, field in fields.items():
-            field._auto_dereference = _auto_dereference
+            field.set_auto_dereferencing(
+                _auto_dereference
+            )  # align the field's auto-dereferencing with the document's
             if field.db_field in data:
                 value = data[field.db_field]
                 try:

mongoengine/base/fields.py

@@ -1,4 +1,5 @@
 import operator
+import threading
 import weakref
 
 import pymongo
@@ -16,6 +17,20 @@
 __all__ = ("BaseField", "ComplexBaseField", "ObjectIdField", "GeoJsonBaseField")
 
 
+class _no_dereference_for_field:
+    """Context manager for temporarily disabling a Field's auto-dereferencing
+    (meant to be used from no_dereference context manager)"""
+
+    def __init__(self, field):
+        self.field = field
+
+    def __enter__(self):
+        self.field._incr_no_dereference_context()
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        self.field._decr_no_dereference_context()
+
+
 class BaseField:
     """A base class for fields in a MongoDB document. Instances of this class
     may be added to subclasses of `Document` to define a document's schema.
@@ -24,7 +39,7 @@ class BaseField:
     name = None  # set in TopLevelDocumentMetaclass
     _geo_index = False
     _auto_gen = False  # Call `generate` to generate a value
-    _auto_dereference = True
+    _thread_local_storage = threading.local()
 
     # These track each time a Field instance is created. Used to retain order.
     # The auto_creation_counter is used for fields that MongoEngine implicitly
@@ -85,6 +100,8 @@ def __init__(
         self.sparse = sparse
         self._owner_document = None
 
+        self.__auto_dereference = True
+
         # Make sure db_field is a string (if it's explicitly defined).
         if self.db_field is not None and not isinstance(self.db_field, str):
             raise TypeError("db_field should be a string.")
@@ -120,6 +137,33 @@ def __init__(
             self.creation_counter = BaseField.creation_counter
             BaseField.creation_counter += 1
 
+    def set_auto_dereferencing(self, value):
+        self.__auto_dereference = value
+
+    @property
+    def _no_dereference_context_local(self):
+        if not hasattr(self._thread_local_storage, "no_dereference_context"):
+            self._thread_local_storage.no_dereference_context = 0
+        return self._thread_local_storage.no_dereference_context
+
+    @property
+    def _no_dereference_context_is_set(self):
+        return self._no_dereference_context_local > 0
+
+    def _incr_no_dereference_context(self):
+        self._thread_local_storage.no_dereference_context = (
+            self._no_dereference_context_local + 1
+        )
+
+    def _decr_no_dereference_context(self):
+        self._thread_local_storage.no_dereference_context = (
+            self._no_dereference_context_local - 1
+        )
+
+    @property
+    def _auto_dereference(self):
+        return self.__auto_dereference and not self._no_dereference_context_is_set
+
     def __get__(self, instance, owner):
         """Descriptor for retrieving a value from a field in a document."""
         if instance is None:
@@ -268,6 +312,10 @@ class ComplexBaseField(BaseField):
     """
 
     def __init__(self, field=None, **kwargs):
+        if field is not None and not isinstance(field, BaseField):
+            raise TypeError(
+                f"field argument must be a Field instance (e.g {self.__class__.__name__}(StringField()))"
+            )
         self.field = field
         super().__init__(**kwargs)
 
@@ -375,7 +423,7 @@ def to_python(self, value):
                 return value
 
         if self.field:
-            self.field._auto_dereference = self._auto_dereference
+            self.field.set_auto_dereferencing(self._auto_dereference)
             value_dict = {
                 key: self.field.to_python(item) for key, item in value.items()
             }

mongoengine/context_managers.py

@@ -1,9 +1,11 @@
+import contextlib
 import threading
 from contextlib import contextmanager
 
 from pymongo.read_concern import ReadConcern
 from pymongo.write_concern import WriteConcern
 
+from mongoengine.base.fields import _no_dereference_for_field
 from mongoengine.common import _import_class
 from mongoengine.connection import DEFAULT_CONNECTION_NAME, get_db
 from mongoengine.pymongo_support import count_documents
@@ -22,6 +24,7 @@
 
 class MyThreadLocals(threading.local):
     def __init__(self):
+        # {DocCls: count} keeping track of classes with an active no_dereference context
         self.no_dereferencing_class = {}
 
 
@@ -126,14 +129,55 @@ def __exit__(self, t, value, traceback):
         self.cls._get_collection_name = self.ori_get_collection_name
 
 
-class no_dereference:
+@contextlib.contextmanager
+def no_dereference(cls):
     """no_dereference context manager.
 
     Turns off all dereferencing in Documents for the duration of the context
     manager::
 
         with no_dereference(Group):
-            Group.objects.find()
+            Group.objects()
+    """
+    try:
+        cls = cls
+
+        ReferenceField = _import_class("ReferenceField")
+        GenericReferenceField = _import_class("GenericReferenceField")
+        ComplexBaseField = _import_class("ComplexBaseField")
+
+        deref_fields = [
+            field
+            for name, field in cls._fields.items()
+            if isinstance(
+                field, (ReferenceField, GenericReferenceField, ComplexBaseField)
+            )
+        ]
+        no_deref_for_fields_contexts = [
+            _no_dereference_for_field(field) for field in deref_fields
+        ]
+
+        _register_no_dereferencing_for_class(cls)
+
+        # ExitStack is just a fancy way of nesting multiple context managers into 1
+        with contextlib.ExitStack() as stack:
+            for mgr in no_deref_for_fields_contexts:
+                stack.enter_context(mgr)
+
+            yield None
+
+    finally:
+        _unregister_no_dereferencing_for_class(cls)
+
+
+class no_dereference2:
+    """no_dereference context manager.
+
+    Turns off all dereferencing in Documents for the duration of the context
+    manager::
+
+        with no_dereference(Group):
+            Group.objects()
     """
 
     def __init__(self, cls):
@@ -148,24 +192,33 @@ def __init__(self, cls):
         ComplexBaseField = _import_class("ComplexBaseField")
 
         self.deref_fields = [
-            k
-            for k, v in self.cls._fields.items()
-            if isinstance(v, (ReferenceField, GenericReferenceField, ComplexBaseField))
+            field
+            for name, field in self.cls._fields.items()
+            if isinstance(
+                field, (ReferenceField, GenericReferenceField, ComplexBaseField)
+            )
+        ]
+        self.no_deref_for_fields_contexts = [
+            _no_dereference_for_field(field) for field in self.deref_fields
         ]
 
     def __enter__(self):
         """Change the objects default and _auto_dereference values."""
         _register_no_dereferencing_for_class(self.cls)
 
-        for field in self.deref_fields:
-            self.cls._fields[field]._auto_dereference = False
+        for ndff_context in self.no_deref_for_fields_contexts:
+            ndff_context.__enter__()
+        # for field in self.deref_fields:
+        #     self.cls._fields[field]._auto_dereference = False
 
     def __exit__(self, t, value, traceback):
         """Reset the default and _auto_dereference values."""
         _unregister_no_dereferencing_for_class(self.cls)
 
-        for field in self.deref_fields:
-            self.cls._fields[field]._auto_dereference = True
+        for ndff_context in self.no_deref_for_fields_contexts:
+            ndff_context.__exit__(t, value, traceback)
+        # for field in self.deref_fields:
+        #     self.cls._fields[field]._auto_dereference = True    # should set initial values back
 
 
 class no_sub_classes:
@@ -180,7 +233,7 @@ class no_sub_classes:
     def __init__(self, cls):
         """Construct the no_sub_classes context manager.
 
-        :param cls: the class to turn querying sub classes on
+        :param cls: the class to turn querying subclasses on
         """
         self.cls = cls
         self.cls_initial_subclasses = None

mongoengine/fields.py

@@ -6,6 +6,7 @@
 import socket
 import time
 import uuid
+from inspect import isclass
 from io import BytesIO
 from operator import itemgetter
 
@@ -707,7 +708,6 @@ class EmbeddedDocumentField(BaseField):
     """
 
     def __init__(self, document_type, **kwargs):
-        # XXX ValidationError raised outside of the "validate" method.
         if not (
             isinstance(document_type, str)
             or issubclass(document_type, EmbeddedDocument)
@@ -910,9 +910,9 @@ class ListField(ComplexBaseField):
         Required means it cannot be empty - as the default for ListFields is []
     """
 
-    def __init__(self, field=None, max_length=None, **kwargs):
+    def __init__(self, field=None, *, max_length=None, **kwargs):
         self.max_length = max_length
-        kwargs.setdefault("default", lambda: [])
+        kwargs.setdefault("default", list)
         super().__init__(field=field, **kwargs)
 
     def __get__(self, instance, owner):
@@ -1035,10 +1035,9 @@ class DictField(ComplexBaseField):
     """
 
     def __init__(self, field=None, *args, **kwargs):
-        self._auto_dereference = False
-
-        kwargs.setdefault("default", lambda: {})
+        kwargs.setdefault("default", dict)
         super().__init__(*args, field=field, **kwargs)
+        self.set_auto_dereferencing(False)
 
     def validate(self, value):
         """Make sure that a list of valid fields is being used."""
@@ -1151,8 +1150,9 @@ def __init__(
             :class:`~pymongo.dbref.DBRef`, regardless of the value of `dbref`.
         """
         # XXX ValidationError raised outside of the "validate" method.
-        if not isinstance(document_type, str) and not issubclass(
-            document_type, Document
+        if not (
+            isinstance(document_type, str)
+            or (isclass(document_type) and issubclass(document_type, Document))
         ):
             self.error(
                 "Argument to ReferenceField constructor must be a "

python-mongoengine.spec

@@ -0,0 +1,10 @@
+import pytest
+
+from mongoengine.base import ComplexBaseField
+from tests.utils import MongoDBTestCase
+
+
+class TestComplexBaseField(MongoDBTestCase):
+    def test_field_validation(self):
+        with pytest.raises(TypeError, match="field argument must be a Field instance"):
+            ComplexBaseField("test")