Drop support for positional arguments when instantiating a document

[wojcikstefan]

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.

[bagerard]

LGTM, great idea to get rid of this. I didn't know it was even possible to use positional args but its terribly error prone

[bagerard]

I wasn't aware of this auto_id feature but it does not looks like its working correctly

Currently no test seems to be testing the raw structure when auto_id is triggered

I would expect an _auto_id_0 field in the raw mongo document, am I missing something?

[wojcikstefan]

Ah, I missed this :( I have checked that instantiating the document creates correct fields, i.e. in your example:

In [18]: MyDoc._fields
Out[18]:
{'an_int': <mongoengine.fields.IntField at 0x109e058d0>,
 'auto_id_0': <mongoengine.base.fields.ObjectIdField at 0x109e05350>,
 'id': <mongoengine.fields.IntField at 0x109e05550>}

However, I did not check persisting a document with such a field structure... Technically there's an automated test for it here:

mongoengine/tests/document/inheritance.py

Lines 388 to 405 in b1002dd

	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)

That said, it only works because berlin.id isn't set. This feature is indeed broken and AFAICT, it has always been broken :(

---

That might be a good thing after all... It means that most likely nobody is using this (overly magical and half-implemented) feature and we can gut it. I'll do that in a follow-up PR, providing an explanation of how this feature came to be, why it doesn't work correctly (hint: it all started with #961), and why we don't really need it.

For now, I'll keep the docstring/readability improvements related to that feature, so that it's easier to understand its behavior (even if that behavior is broken).

[thomasst]

Nothing to add from my side, @bagerard has good points.

[amcgregor]

Has always been broken.

It means that most likely nobody is using this (overly magical and half-implemented) feature and we can gut it.

Hard to not feel called out by the removal of a contributed feature used extensively across several codebases at work. (Next up: scalar()?) This removal is making upgrading quite a chore, requiring the addition of an explicit constructor in a number of locations.

Which means my primary work application now has its own base Document class its own models inherit from to correct… things. As a stop-gap to migration to another DAO. If you're going to preserve the order of field declaration, there must be some reason, right? It's useful.

This sample is "a quick hack", but I have written a new generic constructor — ignoring duplicate checks, it's three lines, even under MongoEngine. Here's the order preservation bit, from the declarative layer's metaclass. (A little lower down, it also promotes __annotations__ from the fields up to the now containing class. So… type hinting dataclass functionality as icing.)

class LoginHistory(DynamicDocument):
	meta = dict(
			collection = "AuthHistory",
			allow_inheritance = False,
			indexes = [dict(fields=['expires'], expireAfterSeconds=0)]
		)
	
	def __init__(self, user, success, location, **kw):
		# Forced to do this, now.
		super(LoginHistory, self).__init__(user=user, success=success, location=location, **kw)
	
	user = ReferenceField(User, reverse_delete_rule=CASCADE)
	success = BooleanField()
	location = IPAddressField()
	sso = StringField(default=None)
	expires = DateTimeField(default=lambda: datetime.utcnow() + timedelta(days=30))

In the "multiple inheritance" potential edge case, my declarative abstraction tracks field creation order. class C(A, B) will override fields from A that match B, and will explicitly preserve original order (from A) on these overrides. (Unless decorator-adjusted: it is possible to explicitly control the order. Also, yes, broad DynamicDocument use, because having a dynamic data store explode when encountering an "unexpected" field is… un-good.)