From 915849b2ce7706dbf091207f92ff7d9e37291823 Mon Sep 17 00:00:00 2001 From: Matthieu Rigal Date: Thu, 11 Jun 2015 16:39:20 +0200 Subject: [PATCH] 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()