Skip to content

Conversation

@re1ro
Copy link
Contributor

@re1ro re1ro commented Apr 29, 2015

In case we have document with SortedListField in DB and we want to update order by changing ordering field
SortedListField.to_mongo returns correct sorted list
But BaseDocument._delta generates wrong set_data trying to update only specific fields at specific indexes without realizing that list order is changed and those indexes are incorrect
To fix that we make BaseDocument._get_changed_fields aware of SortedListField with _ordering behavior.

Related test cases are added

Cheers

Review on Reviewable

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.10% when pulling 31f6067 on rma4ok:master into 42ba4a5 on MongoEngine:master.

@re1ro
Copy link
Contributor Author

re1ro commented Apr 29, 2015

I would also love if you guys apply this fix to 0.8.x branch and release 0.8.8 with that fix along with PyMongo fixes.
Thank you!

@emmanuel-isaac
Copy link

+1

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 91.68% when pulling 31f6067 on rma4ok:master into 42ba4a5 on MongoEngine:master.

@MRigal
Copy link
Member

MRigal commented Apr 29, 2015

I seriously doubt we will release another 0.8.x version... many things have been fixed with 0.9, even if some breaking changes were introduced. Which issue would prevent you from using 0.9+ ?

@MRigal
Copy link
Member

MRigal commented Apr 29, 2015

Else, very good job ! You may add your name to the contributors and a line in the changelog. Good to merge for me 👍

@re1ro
Copy link
Contributor Author

re1ro commented Apr 29, 2015

@MRigal the only big problem for me is 0.9 fails with FieldDoesNotExist if I have any data in my doc that is not described in my model.
Like in mongo:

{name: 'Bob', age: 22}

Model:

class User(Document):
    name = StringField()

I get

  File "mongoengine/queryset/base.py", line 309, in first
    result = queryset[0]
  File "mongoengine/queryset/base.py", line 161, in __getitem__
    _auto_dereference=self._auto_dereference, only_fields=self.only_fields)
  File "mongoengine/base/document.py", line 709, in _from_son
    obj = cls(__auto_convert=False, _created=created, __only_fields=only_fields, **data)
  File "mongoengine/base/document.py", line 81, in __init__
    raise FieldDoesNotExist(msg)

So 0.9 requires me to clean up my data in mongo every time I change field names in model

@MRigal
Copy link
Member

MRigal commented Apr 29, 2015

This won't stay a problem much longer for you, as we are gonna merge #957 which will re-allow old behaviour for the next version

@re1ro
Copy link
Contributor Author

re1ro commented Apr 29, 2015

@MRigal cool! I also want to check updating SortedListField of StringFields or IntFields and include here if something needs to be fixed.
I'll update you within an hour

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.10% when pulling 3e409ad on rma4ok:master into 42ba4a5 on MongoEngine:master.

@re1ro
Copy link
Contributor Author

re1ro commented Apr 29, 2015

@MRigal SortedListField for primitives gets handled perfectly. Feel free to merge
Thank you for blazing fast reaction!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.46%) to 91.23% when pulling 3e409ad on rma4ok:master into 42ba4a5 on MongoEngine:master.

@MRigal
Copy link
Member

MRigal commented Apr 29, 2015

Strange failure, I've restarted the problematic job

@re1ro
Copy link
Contributor Author

re1ro commented Apr 29, 2015

@MRigal Yeah, I've changed only docs. Seems to be good now

@MRigal MRigal merged commit 3e409ad into MongoEngine:master May 6, 2015
MRigal added a commit that referenced this pull request May 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants