Mongoengine: Document.modify() fails on sharded collections

[chromaticgliss]

Recently had a snippet like this that started failing after we sharded our collection.

DocumentExample is a mongoengine Document, with the shard key defined in its meta property as the docs suggest.

    obj_list = []
    doc_set = DocumentExample.objects.filter(processed=False)
    for obj in doc_set:
        modified = obj.modify(
            modified_date=datetime.datetime.utcnow(),
            processed=True
        )
        if modified:
            obj_list.append(obj)

This was failing with the following exception:

  File "/home/shemmy/sites/sunlight/envs/sunlight/local/lib/python2.7/site-packages/mongoengine/document.py", line 239, in modify
    updated = self._qs(**query).modify(new=True, **update)
  File "/home/shemmy/sites/sunlight/envs/sunlight/local/lib/python2.7/site-packages/mongoengine/queryset/base.py", line 579, in modify
    raise OperationError(u"Update failed (%s)" % err)
mongoengine.errors.OperationError: Update failed (exception: query for sharded findAndModify must have shardkey)

In order to fix we included the Document's _object_key property, which includes the shard key, as a query argument.

    obj_list = []
    doc_set = DocumentExample.objects.filter(processed=False)
    for obj in doc_set:
        modified = obj.modify(
            query=obj._object_key, # *** ADDED THIS ****
            modified_date=datetime.datetime.utcnow(),
            processed=True
        )
        if modified:
            obj_list.append(obj)

Perhaps I'm misunderstanding the purpose of the modify() function, but it seems like the _object_key property should be used by default instead of just the primary key here, as is the case in the save() and update() functions.

[wojcikstefan]

Hi @chromaticgliss, thank you for reporting this. It definitely looks like a bug - the shard key should be included in the modify operation by default. Do you have some capacity to submit a PR?

[chromaticgliss]

@wojcikstefan Not immediately, but perhaps in a few days I can find the time to make the fix.

[erdenezul]

@wojcikstefan @touilleMan Please take a look at my PR and advise how to write a unit test for it.

[tomchop]

I'm running into this as well. The fix seems super simple, any plans to merge it soon?

[erdenezul]

@tomchop I am merging this because I meticulously tested manually in sharded collection and no idea how to test with unittest. Moreover, in the update method, the fix was also same.

[tomchop]

Awesome, thanks a lot!

[erdenezul]

Related PR got merged and closing this.