Merge pull request #961 from MRigal/id-meta-foo

Fixes and tests for default 'id' field creation in Document metaclass

docs/changelog.rst

@@ -21,6 +21,7 @@ Changes in 0.9.X - DEV
 - Fix for issue where FileField deletion did not free space in GridFS. 
 - No_dereference() not respected on embedded docs containing reference. #517
 - Document save raise an exception if save_condition fails #1005
+- Fixes some internal _id handling issue. #961
 
 Changes in 0.9.0
 ================

mongoengine/base/document.py

@@ -184,7 +184,7 @@ class BaseDocument(object):
             self__initialised = False
         # Check if the user has created a new instance of a class
         if (self._is_document and self__initialised
-                and self__created and name == self._meta['id_field']):
+                and self__created and name == self._meta.get('id_field')):
             super(BaseDocument, self).__setattr__('_created', False)
 
         super(BaseDocument, self).__setattr__(name, value)

mongoengine/base/metaclasses.py

@@ -385,15 +385,17 @@ class TopLevelDocumentMetaclass(DocumentMetaclass):
         new_class._auto_id_field = getattr(parent_doc_cls,
                                            '_auto_id_field', False)
         if not new_class._meta.get('id_field'):
+            # After 0.10, find not existing names, instead of overwriting
+            id_name, id_db_name = cls.get_auto_id_names(new_class)
             new_class._auto_id_field = True
-            new_class._meta['id_field'] = 'id'
+            new_class._meta['id_field'] = id_name
-            new_class._fields['id'] = ObjectIdField(db_field='_id')
+            new_class._fields[id_name] = ObjectIdField(db_field=id_db_name)
-            new_class._fields['id'].name = 'id'
+            new_class._fields[id_name].name = id_name
-            new_class.id = new_class._fields['id']
+            new_class.id = new_class._fields[id_name]
-
+            new_class._db_field_map[id_name] = id_db_name
-        # Prepend id field to _fields_ordered
+            new_class._reverse_db_field_map[id_db_name] = id_name
-        if 'id' in new_class._fields and 'id' not in new_class._fields_ordered:
+            # Prepend id field to _fields_ordered
-            new_class._fields_ordered = ('id', ) + new_class._fields_ordered
+            new_class._fields_ordered = (id_name, ) + new_class._fields_ordered
 
         # Merge in exceptions with parent hierarchy
         exceptions_to_merge = (DoesNotExist, MultipleObjectsReturned)
@@ -408,6 +410,20 @@ class TopLevelDocumentMetaclass(DocumentMetaclass):
 
         return new_class
 
+    def get_auto_id_names(self):
+        id_name, id_db_name = ('id', '_id')
+        if id_name not in self._fields and \
+                id_db_name not in (v.db_field for v in self._fields.values()):
+            return id_name, id_db_name
+        id_basename, id_db_basename, i = 'auto_id', '_auto_id', 0
+        while id_name in self._fields or \
+                id_db_name in (v.db_field for v in self._fields.values()):
+            id_name = '{0}_{1}'.format(id_basename, i)
+            id_db_name = '{0}_{1}'.format(id_db_basename, i)
+            i += 1
+        return id_name, id_db_name
+
+
 
 class MetaDict(dict):
 

mongoengine/document.py

@@ -152,6 +152,8 @@ class Document(BaseDocument):
         """
 
         def fget(self):
+            if 'id_field' not in self._meta:
+                return None
             return getattr(self, self._meta['id_field'])
 
         def fset(self, value):

tests/document/inheritance.py

@@ -307,6 +307,69 @@ class InheritanceTest(unittest.TestCase):
         doc = Animal(name='dog')
         self.assertFalse('_cls' in doc.to_mongo())
 
+    def test_abstract_handle_ids_in_metaclass_properly(self):
+
+        class City(Document):
+            continent = StringField()
+            meta = {'abstract': True,
+                    'allow_inheritance': False}
+
+        class EuropeanCity(City):
+            name = StringField()
+
+        berlin = EuropeanCity(name='Berlin', continent='Europe')
+        self.assertEqual(len(berlin._db_field_map), len(berlin._fields_ordered))
+        self.assertEqual(len(berlin._reverse_db_field_map), len(berlin._fields_ordered))
+        self.assertEqual(len(berlin._fields_ordered), 3)
+        self.assertEqual(berlin._fields_ordered[0], 'id')
+
+    def test_auto_id_not_set_if_specific_in_parent_class(self):
+
+        class City(Document):
+            continent = StringField()
+            city_id = IntField(primary_key=True)
+            meta = {'abstract': True,
+                    'allow_inheritance': False}
+
+        class EuropeanCity(City):
+            name = StringField()
+
+        berlin = EuropeanCity(name='Berlin', continent='Europe')
+        self.assertEqual(len(berlin._db_field_map), len(berlin._fields_ordered))
+        self.assertEqual(len(berlin._reverse_db_field_map), len(berlin._fields_ordered))
+        self.assertEqual(len(berlin._fields_ordered), 3)
+        self.assertEqual(berlin._fields_ordered[0], 'city_id')
+
+    def test_auto_id_vs_non_pk_id_field(self):
+
+        class City(Document):
+            continent = StringField()
+            id = IntField()
+            meta = {'abstract': True,
+                    'allow_inheritance': False}
+
+        class EuropeanCity(City):
+            name = StringField()
+
+        berlin = EuropeanCity(name='Berlin', continent='Europe')
+        self.assertEqual(len(berlin._db_field_map), len(berlin._fields_ordered))
+        self.assertEqual(len(berlin._reverse_db_field_map), len(berlin._fields_ordered))
+        self.assertEqual(len(berlin._fields_ordered), 4)
+        self.assertEqual(berlin._fields_ordered[0], 'auto_id_0')
+        berlin.save()
+        self.assertEqual(berlin.pk, berlin.auto_id_0)
+
+    def test_abstract_document_creation_does_not_fail(self):
+
+        class City(Document):
+            continent = StringField()
+            meta = {'abstract': True,
+                    'allow_inheritance': False}
+        bkk = City(continent='asia')
+        self.assertEqual(None, bkk.pk)
+        # TODO: expected error? Shouldn't we create a new error type?
+        self.assertRaises(KeyError, lambda: setattr(bkk, 'pk', 1))
+
     def test_allow_inheritance_embedded_document(self):
         """Ensure embedded documents respect inheritance
         """

tests/queryset/queryset.py

@@ -3689,11 +3689,9 @@ class QuerySetTest(unittest.TestCase):
     def test_scalar(self):
 
         class Organization(Document):
-            id = ObjectIdField('_id')
             name = StringField()
 
         class User(Document):
-            id = ObjectIdField('_id')
             name = StringField()
             organization = ObjectIdField()