(1 of 2) Items to discuss: metadata support

[JohnAD]

Writing a book or making a video is a great way to flush out ones knowledge of a subject; and it has the benefit of also discovering shortcomings of the product or library.

Yesterday, while filming a YouTube video covering fields in MongoEngine, two issues came up pretty strongly. One of them, ironically, was actually captured as part of the film. I'm listing them separately to help with discussion.

BTW, if curious, the video itself: MongoEngine Fields Ep 1: Overview and String Fields.

metadata

Allowing open-ended parameters to a function or class makes it much easier to accidentally write bugs that might not be caught. Or worse: caught long after the code was in production and data has been subtly corrupted.

In this particular case, I had written:

import mongoengine as me

AUTHORS = ["Stephen Hawking", "Albert", "DanTDM"]

class FamousQuote(me.Document):
    text = me.StringField()
    author_name = me.StringField(choice=AUTHORS)

Which executed without errors. The problem was that it didn't work as expected. The proper argument should have been choices not choice. The reason I did not get a TypeError is that awhile back support was added for arbitrary fields, metadata, in pull request #1051 .

While I do see the benefit of supporting meta data, I'm not sure why that particular way of doing was chosen. I can think of a few way this could have been done with had less impact but perhaps there are complications I'm not aware of.

proposed solution

Add a field to the 'meta' dictionary in Document called metadata. It would default to metadata=True to keep legacy behavior consistent.

metadata=False would turn off metadata support. Specifically, any entry found in **kwargs inside BaseField generates a TypeError .

Children of this class might need to be modified to handle the stricter rules. But most will not. For example StringField(BaseField) will not need to be changed.

Libraries flask-mongoengine and django-mongoengine should be checked against this change, just in case. Ironically, these libraries could use 'metadata=False' also if they 'absorb' their own arguments prior to calling super.

going further

(with other possibilities in the future)

Even better, add a global setting. Such as:

import mongoengine as me

me.connect("test_db")
me.defaults(metadata=False)

AUTHORS = ["Stephen Hawking", "Albert", "DanTDM"]

class FamousQuote(me.Document):
    text = me.StringField()
    author_name = me.StringField(choices=AUTHORS)

Any meta entry could have a default applied to all documents unless specifically overridden.

[wojcikstefan]

Nice find @JohnAD ! I'm not a fan of what #1051 did as well. It basically delays the time when you learn about an invalid kwarg till runtime. I wish all the additional data you might want to store on a field was encapsulated by say a meta kwarg (e.g. meta={ 'help_text': 'whatever', ... }). That way you provide extensibility without losing some helpful safeguards. You can argue that for example in Django help_text is a first-class kwarg, but a) they have a much tighter integration between their models and e.g. admin and forms, which use these kwargs, b) even then, they don't allow completely random kwargs to be defined on a field (see https://github.com/django/django/blob/7e299c0e03a52052068db5108de97cf59174bfd9/django/db/models/fields/__init__.py#L145).

That said, I think I'd rather fix it in a major release (and document a breaking change) than start adding more and more "checkboxes" to MongoEngine's configuration. Let's just agree on the best way to do it moving forward and stick to that. Otherwise, this library is going to turn into a bunch of ifs and elses with spaghetti code trying to enable and disable features as anybody sees fit (with maintainers having to support all of the combinations).

[amcgregor]

Unfortunately, use of MongoEngine itself turns application code into spaghetti if trees. Something I'm sad I missed was the unfortunate practice of wholly overriding __init__ on certain subclasses, such as ReferenceField, making my solution not evenly applied to all field types. Regardless of the merits of either approach, consistency is the one thing I look for.

Edited to add: very notably, the use of extra (as it was originally called, or meta in the above discussion) as a keyword argument, typically a dictionary (but not always) was its own source of inconsistent behaviour. It's a pseudo-protocol with no formal specification, and my solution in no way prevented (or prevents) such use; such use is a subset of the functionality provided. ;)