From 1862bcf86709c7e58379ee5c5a0baa52b0347d78 Mon Sep 17 00:00:00 2001 From: mrigal Date: Thu, 16 Apr 2015 14:33:16 +0200 Subject: [PATCH 1/7] added test for abstract document without pk creation and adapted behaviour --- mongoengine/base/document.py | 2 +- mongoengine/document.py | 2 ++ tests/document/inheritance.py | 11 +++++++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/mongoengine/base/document.py b/mongoengine/base/document.py index 5707992a..7142a3de 100644 --- a/mongoengine/base/document.py +++ b/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) diff --git a/mongoengine/document.py b/mongoengine/document.py index eedd01d2..01b69fb2 100644 --- a/mongoengine/document.py +++ b/mongoengine/document.py @@ -152,6 +152,8 @@ class Document(BaseDocument): """ def fget(self): + if not 'id_field' in self._meta: + return None return getattr(self, self._meta['id_field']) def fset(self, value): diff --git a/tests/document/inheritance.py b/tests/document/inheritance.py index e8347054..afa3ccea 100644 --- a/tests/document/inheritance.py +++ b/tests/document/inheritance.py @@ -307,6 +307,17 @@ class InheritanceTest(unittest.TestCase): doc = Animal(name='dog') self.assertFalse('_cls' in doc.to_mongo()) + 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'twe created a new error type + self.assertRaises(KeyError, lambda: setattr(bkk, 'pk', 1)) + def test_allow_inheritance_embedded_document(self): """Ensure embedded documents respect inheritance """ From 53fbc165ba84b0314d2c10f16bf9425bbba44fa2 Mon Sep 17 00:00:00 2001 From: mrigal Date: Thu, 16 Apr 2015 14:37:46 +0200 Subject: [PATCH 2/7] added content of PR #688 with a test to proove it is a bit right --- mongoengine/base/metaclasses.py | 14 ++++++++++---- tests/document/inheritance.py | 17 +++++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/mongoengine/base/metaclasses.py b/mongoengine/base/metaclasses.py index 8a25ff3d..6fdbe19c 100644 --- a/mongoengine/base/metaclasses.py +++ b/mongoengine/base/metaclasses.py @@ -390,10 +390,16 @@ class TopLevelDocumentMetaclass(DocumentMetaclass): new_class._fields['id'] = ObjectIdField(db_field='_id') new_class._fields['id'].name = 'id' new_class.id = new_class._fields['id'] - - # Prepend id field to _fields_ordered - if 'id' in new_class._fields and 'id' not in new_class._fields_ordered: - new_class._fields_ordered = ('id', ) + new_class._fields_ordered + new_class._db_field_map['id'] = '_id' + new_class._reverse_db_field_map['_id'] = 'id' + if 'id' in new_class._fields_ordered: + # An existing id field will be overwritten anyway, so remove it + loc = new_class._fields_ordered.index('id') + new_class._fields_ordered = new_class._fields_ordered[:loc] + \ + new_class._fields_ordered[loc+1:] + else: + # Prepend id field to _fields_ordered + new_class._fields_ordered = ('id', ) + new_class._fields_ordered # Merge in exceptions with parent hierarchy exceptions_to_merge = (DoesNotExist, MultipleObjectsReturned) diff --git a/tests/document/inheritance.py b/tests/document/inheritance.py index afa3ccea..e408e611 100644 --- a/tests/document/inheritance.py +++ b/tests/document/inheritance.py @@ -307,6 +307,23 @@ 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() + country = 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], 'id') + def test_abstract_document_creation_does_not_fail(self): class City(Document): From 051cd744adbe9a00ee3e3630167bfd80bc431589 Mon Sep 17 00:00:00 2001 From: mrigal Date: Thu, 16 Apr 2015 14:38:30 +0200 Subject: [PATCH 3/7] added another test to proove we still do not handle all cases well --- tests/document/inheritance.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/document/inheritance.py b/tests/document/inheritance.py index e408e611..cbc0d0b7 100644 --- a/tests/document/inheritance.py +++ b/tests/document/inheritance.py @@ -324,6 +324,24 @@ class InheritanceTest(unittest.TestCase): self.assertEqual(len(berlin._fields_ordered), 4) self.assertEqual(berlin._fields_ordered[0], '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() + country = 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), 5) + self.assertEqual(berlin._fields_ordered[0], 'id') + def test_abstract_document_creation_does_not_fail(self): class City(Document): From 2e963023368372bb975bd671124686b20db822a6 Mon Sep 17 00:00:00 2001 From: mrigal Date: Thu, 16 Apr 2015 14:53:28 +0200 Subject: [PATCH 4/7] not in fix --- mongoengine/document.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mongoengine/document.py b/mongoengine/document.py index 01b69fb2..7b05ef8c 100644 --- a/mongoengine/document.py +++ b/mongoengine/document.py @@ -152,7 +152,7 @@ class Document(BaseDocument): """ def fget(self): - if not 'id_field' in self._meta: + if 'id_field' not in self._meta: return None return getattr(self, self._meta['id_field']) From 915849b2ce7706dbf091207f92ff7d9e37291823 Mon Sep 17 00:00:00 2001 From: Matthieu Rigal Date: Thu, 11 Jun 2015 16:39:20 +0200 Subject: [PATCH 5/7] Implemented method to auto-generate non-collisioning auto_id names --- docs/changelog.rst | 1 + docs/guide/defining-documents.rst | 2 +- mongoengine/base/metaclasses.py | 38 +++++++++++++++++++------------ tests/document/inheritance.py | 29 ++++++++++++++++++----- tests/queryset/queryset.py | 2 -- 5 files changed, 49 insertions(+), 23 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 65e236ba..dfa9c7a6 100644 --- a/docs/changelog.rst +++ b/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 ================ diff --git a/docs/guide/defining-documents.rst b/docs/guide/defining-documents.rst index d1448158..4572d30b 100644 --- a/docs/guide/defining-documents.rst +++ b/docs/guide/defining-documents.rst @@ -310,7 +310,7 @@ Dealing with deletion of referred documents By default, MongoDB doesn't check the integrity of your data, so deleting documents that other documents still hold references to will lead to consistency issues. Mongoengine's :class:`ReferenceField` adds some functionality to -safeguard against these kinds of database integrity problems, providing each +safeguard against these kinds of database integrit2y problems, providing each reference with a delete rule specification. A delete rule is specified by supplying the :attr:`reverse_delete_rule` attributes on the :class:`ReferenceField` definition, like this:: diff --git a/mongoengine/base/metaclasses.py b/mongoengine/base/metaclasses.py index 6fdbe19c..f608940d 100644 --- a/mongoengine/base/metaclasses.py +++ b/mongoengine/base/metaclasses.py @@ -385,21 +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._fields['id'] = ObjectIdField(db_field='_id') - new_class._fields['id'].name = 'id' - new_class.id = new_class._fields['id'] - new_class._db_field_map['id'] = '_id' - new_class._reverse_db_field_map['_id'] = 'id' - if 'id' in new_class._fields_ordered: - # An existing id field will be overwritten anyway, so remove it - loc = new_class._fields_ordered.index('id') - new_class._fields_ordered = new_class._fields_ordered[:loc] + \ - new_class._fields_ordered[loc+1:] - else: - # Prepend id field to _fields_ordered - new_class._fields_ordered = ('id', ) + new_class._fields_ordered + new_class._meta['id_field'] = id_name + new_class._fields[id_name] = ObjectIdField(db_field=id_db_name) + new_class._fields[id_name].name = id_name + new_class.id = new_class._fields[id_name] + new_class._db_field_map[id_name] = id_db_name + new_class._reverse_db_field_map[id_db_name] = id_name + # Prepend id field to _fields_ordered + new_class._fields_ordered = (id_name, ) + new_class._fields_ordered # Merge in exceptions with parent hierarchy exceptions_to_merge = (DoesNotExist, MultipleObjectsReturned) @@ -414,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 = '{}_{}'.format(id_basename, i) + id_db_name = '{}_{}'.format(id_db_basename, i) + i += 1 + return id_name, id_db_name + + class MetaDict(dict): diff --git a/tests/document/inheritance.py b/tests/document/inheritance.py index cbc0d0b7..7673a103 100644 --- a/tests/document/inheritance.py +++ b/tests/document/inheritance.py @@ -316,14 +316,30 @@ class InheritanceTest(unittest.TestCase): class EuropeanCity(City): name = StringField() - country = 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(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): @@ -334,13 +350,14 @@ class InheritanceTest(unittest.TestCase): class EuropeanCity(City): name = StringField() - country = 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), 5) - self.assertEqual(berlin._fields_ordered[0], 'id') + 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): @@ -350,7 +367,7 @@ class InheritanceTest(unittest.TestCase): 'allow_inheritance': False} bkk = City(continent='asia') self.assertEqual(None, bkk.pk) - # TODO: expected error? Shouldn'twe created a new error type + # 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): diff --git a/tests/queryset/queryset.py b/tests/queryset/queryset.py index 0965e40f..83fb52df 100644 --- a/tests/queryset/queryset.py +++ b/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() From 810819861354914a217a58e6f7e9c772dca4549a Mon Sep 17 00:00:00 2001 From: Matthieu Rigal Date: Thu, 11 Jun 2015 22:38:05 +0200 Subject: [PATCH 6/7] corrected formatting for Python 2.6 compatibility --- mongoengine/base/metaclasses.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mongoengine/base/metaclasses.py b/mongoengine/base/metaclasses.py index f608940d..16e6bd29 100644 --- a/mongoengine/base/metaclasses.py +++ b/mongoengine/base/metaclasses.py @@ -418,8 +418,8 @@ class TopLevelDocumentMetaclass(DocumentMetaclass): 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 = '{}_{}'.format(id_basename, i) - id_db_name = '{}_{}'.format(id_db_basename, i) + 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 From e1da83a8f6e835478e2e039cc241529c2384d2f2 Mon Sep 17 00:00:00 2001 From: Matthieu Rigal Date: Fri, 12 Jun 2015 09:12:19 +0200 Subject: [PATCH 7/7] Cosmetic --- docs/guide/defining-documents.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/guide/defining-documents.rst b/docs/guide/defining-documents.rst index 4572d30b..d1448158 100644 --- a/docs/guide/defining-documents.rst +++ b/docs/guide/defining-documents.rst @@ -310,7 +310,7 @@ Dealing with deletion of referred documents By default, MongoDB doesn't check the integrity of your data, so deleting documents that other documents still hold references to will lead to consistency issues. Mongoengine's :class:`ReferenceField` adds some functionality to -safeguard against these kinds of database integrit2y problems, providing each +safeguard against these kinds of database integrity problems, providing each reference with a delete rule specification. A delete rule is specified by supplying the :attr:`reverse_delete_rule` attributes on the :class:`ReferenceField` definition, like this::