From f45552f8f8ea2d4094c78cb60caee3c62f9d6c1e Mon Sep 17 00:00:00 2001 From: Stefan Wojcik Date: Mon, 24 Jun 2019 15:44:35 +0200 Subject: [PATCH 1/5] Drop support for positional arguments when instantiating a document For example, if you had the following class: ``` class Person(Document): name = StringField() age = IntField() ``` You could instantiate an object of such class by doing one of the following: 1. `new_person = Person('Tom', 30)` 2. `new_person = Person('Tom', age=30)` 3. `new_person = Person(name='Tom', age=30)` From now on, only option (3) is allowed. Supporting positional arguments may sound like a reasonable idea in this heavily simplified example, but in real life it's almost never what you want (especially if you use inheritance in your document definitions) and it may lead to ugly bugs. We should not rely on the *order* of fields to match a given value to a given name. This also helps us simplify the code e.g. by dropping the confusing (and undocumented) `BaseDocument._auto_id_field` attribute. --- docs/changelog.rst | 2 + mongoengine/base/document.py | 16 ++---- mongoengine/base/metaclasses.py | 57 +++++++++++++------ tests/document/instance.py | 68 +++++++++++------------ tests/fields/test_lazy_reference_field.py | 8 +-- 5 files changed, 82 insertions(+), 69 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index e82cc124..6a56325e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -6,6 +6,8 @@ Changelog Development =========== - (Fill this out as you fix issues and develop your features). +- BREAKING CHANGE: Drop support for positional arguments when instantiating a document. #? + - From now on keyword arguments (e.g. `Doc(field_name=value)`) are required. Changes in 0.18.1 ================= diff --git a/mongoengine/base/document.py b/mongoengine/base/document.py index 057258f5..047d50a4 100644 --- a/mongoengine/base/document.py +++ b/mongoengine/base/document.py @@ -60,18 +60,10 @@ class BaseDocument(object): self._created = True if args: - # Combine positional arguments with named arguments. - # We only want named arguments. - field = iter(self._fields_ordered) - # If its an automatic id field then skip to the first defined field - if getattr(self, '_auto_id_field', False): - next(field) - for value in args: - name = next(field) - if name in values: - raise TypeError( - 'Multiple values for keyword argument "%s"' % name) - values[name] = value + raise TypeError( + 'Instantiating a document with positional arguments is not ' + 'supported. Please use `field_name=value` keyword arguments.' + ) __auto_convert = values.pop('__auto_convert', True) diff --git a/mongoengine/base/metaclasses.py b/mongoengine/base/metaclasses.py index 6f507eaa..c5363b09 100644 --- a/mongoengine/base/metaclasses.py +++ b/mongoengine/base/metaclasses.py @@ -385,30 +385,35 @@ class TopLevelDocumentMetaclass(DocumentMetaclass): new_class._meta['id_field'] = field_name new_class.id = field - # Set primary key if not defined by the document - new_class._auto_id_field = getattr(parent_doc_cls, - '_auto_id_field', False) + # If the document doesn't explicitly define a primary key field, create + # one. Make it an ObjectIdField and give it a non-clashing name ("id" + # by default, but can be different if that one's taken). if not new_class._meta.get('id_field'): - # After 0.10, find not existing names, instead of overwriting id_name, id_db_name = mcs.get_auto_id_names(new_class) - new_class._auto_id_field = True 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 + + # Prepend the ID field to _fields_ordered (so that it's *always* + # the first field). new_class._fields_ordered = (id_name, ) + new_class._fields_ordered - # Merge in exceptions with parent hierarchy + # Merge in exceptions with parent hierarchy. exceptions_to_merge = (DoesNotExist, MultipleObjectsReturned) module = attrs.get('__module__') for exc in exceptions_to_merge: name = exc.__name__ - parents = tuple(getattr(base, name) for base in flattened_bases - if hasattr(base, name)) or (exc,) - # Create new exception and set to new_class + parents = tuple( + getattr(base, name) + for base in flattened_bases + if hasattr(base, name) + ) or (exc,) + + # Create a new exception and set it as an attribute on the new + # class. exception = type(name, parents, {'__module__': module}) setattr(new_class, name, exception) @@ -416,17 +421,35 @@ class TopLevelDocumentMetaclass(DocumentMetaclass): @classmethod def get_auto_id_names(mcs, new_class): + """Find a name for the automatic ID field for the given new class. + + Return a two-element tuple where the first item is the field name (i.e. + the attribute name on the object) and the second element is the DB + field name (i.e. the name of the key stored in MongoDB). + + Defaults to ('id', '_id'), or generates a non-clashing name in the form + of ('auto_id_X', '_auto_id_X') if the default name is already taken. + """ id_name, id_db_name = ('id', '_id') - if id_name not in new_class._fields and \ - id_db_name not in (v.db_field for v in new_class._fields.values()): + existing_fields = new_class._fields + existing_db_fields = (v.db_field for v in new_class._fields.values()) + if ( + id_name not in existing_fields and + id_db_name not in existing_db_fields + ): return id_name, id_db_name - id_basename, id_db_basename, i = 'auto_id', '_auto_id', 0 - while id_name in new_class._fields or \ - id_db_name in (v.db_field for v in new_class._fields.values()): + + id_basename, id_db_basename, i = ('auto_id', '_auto_id', 0) + while True: 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 + if ( + id_name not in existing_fields and + id_db_name not in existing_db_fields + ): + return id_name, id_db_name + else: + i += 1 class MetaDict(dict): diff --git a/tests/document/instance.py b/tests/document/instance.py index 02617b67..4be8aa45 100644 --- a/tests/document/instance.py +++ b/tests/document/instance.py @@ -3130,48 +3130,44 @@ class InstanceTest(MongoDBTestCase): self.assertEqual(classic_doc._data, dict_doc._data) def test_positional_creation(self): - """Ensure that document may be created using positional arguments.""" - person = self.Person("Test User", 42) - self.assertEqual(person.name, "Test User") - self.assertEqual(person.age, 42) + """Document cannot be instantiated using positional arguments.""" + with self.assertRaises(TypeError) as e: + person = self.Person("Test User", 42) + expected_msg = ( + 'Instantiating a document with positional arguments is not ' + 'supported. Please use `field_name=value` keyword arguments.' + ) + self.assertEqual(e.exception.message, expected_msg) def test_mixed_creation(self): - """Ensure that document may be created using mixed arguments.""" - person = self.Person("Test User", age=42) - self.assertEqual(person.name, "Test User") - self.assertEqual(person.age, 42) + """Document cannot be instantiated using mixed arguments.""" + with self.assertRaises(TypeError) as e: + person = self.Person("Test User", age=42) + expected_msg = ( + 'Instantiating a document with positional arguments is not ' + 'supported. Please use `field_name=value` keyword arguments.' + ) + self.assertEqual(e.exception.message, expected_msg) def test_positional_creation_embedded(self): - """Ensure that embedded document may be created using positional - arguments. - """ - job = self.Job("Test Job", 4) - self.assertEqual(job.name, "Test Job") - self.assertEqual(job.years, 4) + """Embedded document cannot be created using positional arguments.""" + with self.assertRaises(TypeError) as e: + job = self.Job("Test Job", 4) + expected_msg = ( + 'Instantiating a document with positional arguments is not ' + 'supported. Please use `field_name=value` keyword arguments.' + ) + self.assertEqual(e.exception.message, expected_msg) def test_mixed_creation_embedded(self): - """Ensure that embedded document may be created using mixed - arguments. - """ - job = self.Job("Test Job", years=4) - self.assertEqual(job.name, "Test Job") - self.assertEqual(job.years, 4) - - def test_mixed_creation_dynamic(self): - """Ensure that document may be created using mixed arguments.""" - class Person(DynamicDocument): - name = StringField() - - person = Person("Test User", age=42) - self.assertEqual(person.name, "Test User") - self.assertEqual(person.age, 42) - - def test_bad_mixed_creation(self): - """Ensure that document gives correct error when duplicating - arguments. - """ - with self.assertRaises(TypeError): - return self.Person("Test User", 42, name="Bad User") + """Embedded document cannot be created using mixed arguments.""" + with self.assertRaises(TypeError) as e: + job = self.Job("Test Job", years=4) + expected_msg = ( + 'Instantiating a document with positional arguments is not ' + 'supported. Please use `field_name=value` keyword arguments.' + ) + self.assertEqual(e.exception.message, expected_msg) def test_data_contains_id_field(self): """Ensure that asking for _data returns 'id'.""" diff --git a/tests/fields/test_lazy_reference_field.py b/tests/fields/test_lazy_reference_field.py index b10506e7..1d6e6e79 100644 --- a/tests/fields/test_lazy_reference_field.py +++ b/tests/fields/test_lazy_reference_field.py @@ -303,8 +303,8 @@ class TestLazyReferenceField(MongoDBTestCase): Animal.drop_collection() Ocurrence.drop_collection() - animal1 = Animal('doggo').save() - animal2 = Animal('cheeta').save() + animal1 = Animal(name='doggo').save() + animal2 = Animal(name='cheeta').save() def check_fields_type(occ): self.assertIsInstance(occ.direct, LazyReference) @@ -542,8 +542,8 @@ class TestGenericLazyReferenceField(MongoDBTestCase): Animal.drop_collection() Ocurrence.drop_collection() - animal1 = Animal('doggo').save() - animal2 = Animal('cheeta').save() + animal1 = Animal(name='doggo').save() + animal2 = Animal(name='cheeta').save() def check_fields_type(occ): self.assertIsInstance(occ.direct, LazyReference) From 8e69008699d20268956b6c9f3b2406e5d110c6d8 Mon Sep 17 00:00:00 2001 From: Stefan Wojcik Date: Mon, 24 Jun 2019 16:00:21 +0200 Subject: [PATCH 2/5] Fill in the PR # in the changelog [ci skip] --- docs/changelog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 6a56325e..3754b010 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -6,7 +6,7 @@ Changelog Development =========== - (Fill this out as you fix issues and develop your features). -- BREAKING CHANGE: Drop support for positional arguments when instantiating a document. #? +- BREAKING CHANGE: Drop support for positional arguments when instantiating a document. #2103 - From now on keyword arguments (e.g. `Doc(field_name=value)`) are required. Changes in 0.18.1 From b661afba0158befeaa8abc95f8c7184ede5aeeac Mon Sep 17 00:00:00 2001 From: Stefan Wojcik Date: Tue, 25 Jun 2019 11:34:31 +0200 Subject: [PATCH 3/5] Use set comprehensions for existing_fields & existing_db_fields --- 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 c5363b09..44b74509 100644 --- a/mongoengine/base/metaclasses.py +++ b/mongoengine/base/metaclasses.py @@ -431,8 +431,8 @@ class TopLevelDocumentMetaclass(DocumentMetaclass): of ('auto_id_X', '_auto_id_X') if the default name is already taken. """ id_name, id_db_name = ('id', '_id') - existing_fields = new_class._fields - existing_db_fields = (v.db_field for v in new_class._fields.values()) + existing_fields = {field_name for field_name in new_class._fields} + existing_db_fields = {v.db_field for v in new_class._fields.values()} if ( id_name not in existing_fields and id_db_name not in existing_db_fields From 0578cdb62ef762454941e872ca026ada5a01e5f9 Mon Sep 17 00:00:00 2001 From: Stefan Wojcik Date: Tue, 25 Jun 2019 11:41:27 +0200 Subject: [PATCH 4/5] Cleaner loop using itertools.count() --- mongoengine/base/metaclasses.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/mongoengine/base/metaclasses.py b/mongoengine/base/metaclasses.py index 44b74509..c3ced5bb 100644 --- a/mongoengine/base/metaclasses.py +++ b/mongoengine/base/metaclasses.py @@ -1,3 +1,4 @@ +import itertools import warnings import six @@ -440,7 +441,7 @@ class TopLevelDocumentMetaclass(DocumentMetaclass): return id_name, id_db_name id_basename, id_db_basename, i = ('auto_id', '_auto_id', 0) - while True: + for i in itertools.count(): id_name = '{0}_{1}'.format(id_basename, i) id_db_name = '{0}_{1}'.format(id_db_basename, i) if ( @@ -448,8 +449,6 @@ class TopLevelDocumentMetaclass(DocumentMetaclass): id_db_name not in existing_db_fields ): return id_name, id_db_name - else: - i += 1 class MetaDict(dict): From e57d834a0d9752dc92497d08b7fb3648212686b8 Mon Sep 17 00:00:00 2001 From: Stefan Wojcik Date: Tue, 25 Jun 2019 12:41:59 +0200 Subject: [PATCH 5/5] Fix automated tests for py3 --- tests/document/instance.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/document/instance.py b/tests/document/instance.py index 4be8aa45..06f65076 100644 --- a/tests/document/instance.py +++ b/tests/document/instance.py @@ -3137,7 +3137,7 @@ class InstanceTest(MongoDBTestCase): 'Instantiating a document with positional arguments is not ' 'supported. Please use `field_name=value` keyword arguments.' ) - self.assertEqual(e.exception.message, expected_msg) + self.assertEqual(str(e.exception), expected_msg) def test_mixed_creation(self): """Document cannot be instantiated using mixed arguments.""" @@ -3147,7 +3147,7 @@ class InstanceTest(MongoDBTestCase): 'Instantiating a document with positional arguments is not ' 'supported. Please use `field_name=value` keyword arguments.' ) - self.assertEqual(e.exception.message, expected_msg) + self.assertEqual(str(e.exception), expected_msg) def test_positional_creation_embedded(self): """Embedded document cannot be created using positional arguments.""" @@ -3157,7 +3157,7 @@ class InstanceTest(MongoDBTestCase): 'Instantiating a document with positional arguments is not ' 'supported. Please use `field_name=value` keyword arguments.' ) - self.assertEqual(e.exception.message, expected_msg) + self.assertEqual(str(e.exception), expected_msg) def test_mixed_creation_embedded(self): """Embedded document cannot be created using mixed arguments.""" @@ -3167,7 +3167,7 @@ class InstanceTest(MongoDBTestCase): 'Instantiating a document with positional arguments is not ' 'supported. Please use `field_name=value` keyword arguments.' ) - self.assertEqual(e.exception.message, expected_msg) + self.assertEqual(str(e.exception), expected_msg) def test_data_contains_id_field(self): """Ensure that asking for _data returns 'id'."""