umongo icon indicating copy to clipboard operation
umongo copied to clipboard

Referring to fields with a string in `find` does not detect typos in field names.

Open talwrii opened this issue 4 years ago • 5 comments

Using strings to refer to fields makes it possible to make typos:

class Test(Document):
    field = StrField()

Test.find({"filed": 1})

Could this be avoided in some way?

One approach might be to have some of queryset like in django. I imagine it might make more sense to make this lightweight initially, and have it generate the query dictionary rather than supporting all of mongo's operations (given that combined operations like find_one_and_update can be a core part of using mongo).

This might look something like:

Test.find(q(Test.field == 1))

(This probably wants to support some sort of composition (Test.field == 2 & Test.other_field == 1), which is what requires q: Queryset -> dict. If everything were functional this could be avoided (e.g. and_(Test.field == 2, Test.field == 3)).

talwrii avatar Apr 15 '20 09:04 talwrii

with some inspection of the class, we can use this to get a list of fields:

def Type_Obj(UM_Class):
    fields = UM_Class.schema.fields.keys()
    class __TYPE_CLASS():
        pass
    __TYPE_CLASS.__name__ = f'DB_TYPE_{UM_Class.__name__}'    
    for field in fields:
        setattr(__TYPE_CLASS, field, field)
    
    return __TYPE_CLASS()

Then you can do this:

class Test(Document):
    field = StrField()

t = Type_Obj(Test)
Test.find({t.field: 1})

This way should be much cleaner and flexible. umongo probably should include it as a standard way

shaozi avatar Apr 15 '20 17:04 shaozi

That is simpler.

One idea to avoid typing, would be to put this behaviour in DocumentImplementation itself, so we would have

class Test(Document):
    field = StrField()

Test.find({Test.field: 1})

Though to avoid collision we might need

Test.find({Test.fields.field: 1})

The fields property could be set in build_document_from_template on BaseBuilder. I think this is one line of code :).

For now, I'm going to adapt your code and use it like:

def field(cls, name):
    "Ensure that a field exists at runtime"
    if name not in cls.schema.fields:
        raise KeyError(name)
    return name

Test.find({field(Test, 'field'): 1}, 

talwrii avatar Apr 15 '20 18:04 talwrii

Thank you both for your thoughts about this.

I think the idea of the author of the lib (@touilleMan) was to make it simple, as opposed to mongoengine, more raw but much easier to maintain. I'm a bit reluctant to introduce a query object.

However, the rawness has shown its limits, already: #185, #169, https://github.com/Scille/umongo/issues/99#issuecomment-286040344.

If you come up with a change that keeps things simple but still offers more flexibility or security, I'm open to the idea. It's a cost/benefit balance.

BTW, you might have noticed I released 3.0 beta versions. I'd be very much interested by your feedback. Please try it if you have the time. And please mention the issues you think should belong to the 3.0 milestone. We're close to a stable release, but it's still time to cram a few other changes into it.

lafrech avatar Apr 18 '20 20:04 lafrech

One way to make sure schema is valid can be making use of this feature in MongoDB:

https://www.mongodb.com/blog/post/mongodb-36-json-schema-validation-expressive-query-syntax

shaozi avatar Apr 20 '20 17:04 shaozi

I don't intend to introduce a complex query feature in the short term.

However, I'm open to a feature allowing to pick field names from the document object to catch typos, if someone comes up with a nice design.

lafrech avatar Jan 04 '21 14:01 lafrech