Using operator names in field names when updating

[starsirius]

Re: #949, #843

Question 1

Similar to using operator names in field names when querying, it also raises exceptions when updating.

from mongoengine import *
from mongoengine.queryset import transform

class Shoes(EmbeddedDocument):
    size = FloatField()     # `size` is in MATCH_OPERATORS
    brand = StringField()   # `brand` is not

class Human(Document):
    age = IntField()
    type = StringField()    # `type` is in MATCH_OPERATORS
    shoes = EmbeddedDocumentField(Shoes)

print transform.update(Human, type='introversion')  # Raises an "IndexError: list index out of range" error
print transform.update(Human, shoes__brand='NIKE')  # Correct. {'$set': {'shoes.brand': 'NIKE'}}
print transform.update(Human, shoes__size=10.5)  # Raises an "AttributeError: 'float' object has no attribute 'get'" error

I believe it is due to the same issue above. We can do similar trailing __ tricks as the PR, something like

transform.update(Human, type__='introversion')
transform.update(Human, shoes__size__=10.5)

BUT, more importantly, I wonder if we ever need operators, when updating, other than UPDATE_OPERATORS (e.g. type and size in the above case). See Question 2 below. Or maybe I misunderstood anything.

Question 2: operators other than UPDATE_OPERATOR when updating

Given the document classes above, I wonder the following result would be desired.

print transform.update(Human, age__gt=10)  # {'$set': {'age': {'$gt': 10}}}

[MRigal]

Hi @starsirius
Very valid points! Thanks for reporting them!
Would you like to submit a PR with some before failing tests, then passing and some fix. Eventually raising an "InvalidQueryError" for the case of Question 2

[starsirius]

@MRigal Sure. I will do that. To confirm, for question 1, we don't even need the trailing __ trick for updates, because the operator will only be at the beginning and everything else are field names. Is my understanding correct?

[MRigal]

I was trying to think to a counter-proof, but I can't find any. So I would skip it if it doesn't make the code harder to understand.

What may make it harder is that we go through a "transform" method which is common to both methods. I'm not sure what's the best method on a first sight, if it should be decomposed into two methods, one common to both with an argument about the requested operation and another specific to the more complex operations, or if a simple if is enough without making the code too complex.

Let's see with your proposal that I will review with pleasure

[starsirius]

@MRigal Sorry that I couldn't get a chance to work on this in the past week. 😛

I realized that sometimes we do need the match operator at the end when doing an update, e.g. pull and push. This operation is totally valid:

transform.update(Post, pull__comments__vote__lt=1)
# {'$pull': {'comments': {'vote': {'$lt': 1}}}}

And we do have a test for it. So, we can't simply ignore the match operator.

We could follow the same pattern we use in transform.query that adds a trailing __ to indicate it's a field name instead of a match operator. It will also be a breaking change tho, and I wonder how you think. Thanks!

[MRigal]

Hi @starsirius
Thanks for coming back on that point. I didn't had the time to look deeper into the code and investigate the consequences. So yes, I guess you are right. I wonder if it is worth separating the case of pull and push, but else, definitely, giving the ability to update fields named like operator is very good.
While I've looked at the query transform code, I thought that there might be quite a bit of code that could be simplified and/or put in common between query() and update().
If you feel inspired in your PR, please do so.
And I don't think it is such a breaking change, it is just giving more possibilities to the user. And since we have the behaviour for querying, I would almost consider it to be a bugfix. In any case I would include it in our next minor release.
Thanks!

[yanheven]

for q1, any patch commited?