Bug fixed accessing BaseList with negative indices

[Neurostack]

If you __setitem__ in BaseList with a negative index and then try to save this, you will get an error like:

OperationError: Could not save document (cannot use the part (shape of signal.shape.-1) to traverse the element ({shape: [ 0 ]})).

To fix this I rectify negative list indices in BaseList _mark_as_changed as the appropriate positive index. This fixes the above error.

---

This change is

[lafrech]

Hi @Neurostack. Thank you for your contribution.

I let the maintainers with deeper knowledge of MongoEngine discuss whether this is the best solution to address this issue.

Just two questions:

• Shouldn't we use the modulo operation (%) rather than len(self)+key if key < 0 else key) ? This would write key % len(self).
• Would you mind adding a test to illustrate what doesn't work without and works with your fix?

Thanks again.

[Neurostack]

key % len(self) does seem to be a more elegant solution
here is test code showing the bug that is fixed with this commit

from mongoengine import *

connect('test')

class DocumentWithList(Document):
    mylist = ListField()

test = DocumentWithList()
test.mylist = [1,2,3]
test.save()

test.mylist[-1] = 4
test.save()

[lafrech]

OK so maybe you could add a new function around here that would look like this:

def test_ListField_with_negative_indices(self):

    class DocumentWithList(Document):
        mylist = ListField()

    test = DocumentWithList()
    test.mylist = [1,2,3]
    test.save()
    test.mylist[-1] = 4
    test.save()

[Neurostack]

It looks like the CI build fail is a result of 'test_compound_key_dictfield' failing and not this PR. You can see this is an ongoing issue e34100b. @lafrech let me know if you need anything else.

[lafrech]

It looks like the CI build fail is a result of 'test_compound_key_dictfield' failing and not this PR.

Most probably. I relaunched the build.

let me know if you need anything else

Looks fine to me.

Maybe change

def test_ListField_with_negative_indices(self):

into

def test_list_field_with_negative_indices(self):

for consistency.

@MongoEngine/mongoengine, any thoughts about this?

[touilleMan]

@lafrech sure, all code should follow pep8 ;-)

[lafrech]

I meant "what do you guys think about the whole PR ?", not about that function name.

Travis check passed, BWT.

[lafrech]

@thedrow, the idea is that when updating a list field using negative indexes like this:

test.mylist[-1] = 4

_mark_as_changed is called with key being negative:

self._instance._mark_as_changed('%s.%s' % (self._name, key))

and then when saving the doc, you get an exception.

The change makes sure that key is stored as positive index by recording the (potentially negative) index modulo the length of the list.

Indeed, in mongo shell, I can do:

db.collection.updateOne({}, {$set: {'items.1': '2'}})

but not:

db.collection.updateOne({}, {$set: {'items.-1': '2'}})

I get something like:

"errmsg" : "cannot use the part (items of items.-1) to traverse the element...

As far as I could understand, this seems like the right place to address this issue.

@Neurostack,before integration into master

• please review test function name to use underscore notation as said above
• add yourself to contributors (optional but welcome)
• add a line to changelog (optional but appreciated)

[Neurostack]

All small changes have been made and I cleaned up the test case in the process. The PR has been rebased into one commit and CI passes. Let me know if you need anything else.