added a feature to save object data in order #1497

userlocalhost · 2017-03-01T09:35:42Z

I'm so sorry that I closed #1494 because I remove remote branch of previous PR by mistake.

I corrected the things that you mentioned. Cloud you please review this patch, @wojcikstefan?

…trieving data in order

changed followings. - added optional parameter `container_class` which enables to choose intermediate class at encoding Python data, instead of additional field class. - removed OrderedDocument class because the equivalent feature could be implemented by the outside of Mongoengine.

…e implementation might be in ordered, but other one is not)

wojcikstefan · 2017-03-06T02:48:47Z

This looks good @userlocalhost! I'd still love to expand the tests to:

See if the document is actually correctly stored in the database. You can do it by using raw PyMongo to query the collection with CodecOptions(document_class=OrderedDict).
See if the order is maintained after doc.reload().

userlocalhost · 2017-03-07T09:18:41Z

Thank you again for your review. I got it!

But I think it takes a little time to get them done, because I realized that the field type is transformed to the dict internally after calling doc.reload(). By this transformation, the order collapses.
I think I have to solve it, so please give me a time to do it.

Previous dereference implementation re-contains data as `dict` except for the predicted type. But the OrderedDict is not predicted, so the its data would be converted `dict` implicitly. As the result, the order of stored data get wrong. And this patch prevents it.

This checks DBRef conversion using DynamicField with the ordering guarantee.

…Field This tests DynamicField dereference with ordering guarantee.

userlocalhost · 2017-03-08T15:18:09Z

I realized that the field type is transformed to the dict internally after calling doc.reload(). By this transformation, the order collapses.

I solved this problem by adding a condition in dereference processing (f2fe58c).
And I added another test for it (9cd3dcd).

Then, I wrote additional tests on the basis of your advice, @wojcikstefan.

userlocalhost · 2017-03-23T06:35:26Z

If there is no problem, could you please merge this?

userlocalhost · 2017-04-04T01:18:12Z

If some problems existed, I would correct them. So would you please review this, @thedrow?

…ata in order

userlocalhost · 2017-04-06T01:26:37Z

Thank you very much for your review, @thedrow!
I corrected the points that you mentioned.

wojcikstefan · 2017-04-07T14:46:41Z

Thanks for looking at it @thedrow and sorry for stalling @userlocalhost, have been swamped with other responsibilities lately. I'll double-check this PR soon just in case.

userlocalhost · 2017-04-07T14:56:30Z

Thank you for merging, @thedrow!

And thank you for your reply, @wojcikstefan.
If there was something which you're worrying about, I would also correct it.

This thing broke after MongoEngine/mongoengine#1497 was merged.

wojcikstefan

This PR was merged prematurely. My bad for not being around and pushing back on both the implementation and the feature itself...

Having thought about it some more, I don't think it makes sense to support ordering in only one particular field type. We should either support it in the entirety of MongoEngine or not at all. I'll revert this PR now. Thanks for all the work anyway @userlocalhost !

I also see that the related PR where you wanted to use ordering (StackStorm/st2#3353 (comment)) hasn't been accepted and the maintainers have other ideas on how to solve the problem (including lists of dicts, which would already work in MongoEngine)

wojcikstefan · 2017-05-08T01:14:10Z

mongoengine/fields.py

+    def __init__(self, container_class=dict, *args, **kwargs):
+        self._container_cls = container_class
+        if not issubclass(self._container_cls, Mapping):
+            self.error('The class that is specified in `container_class` parameter '


This shouldn't throw a ValidationError, which is reserved to invalid data, not an invalid field definition.

wojcikstefan · 2017-05-08T01:15:08Z

tests/fields/fields.py

+        else:
+            db_doc = pymongo_db.doc.find_one(as_class=OrderedDict)
+
+        self.assertEqual(','.join(doc.ordered_data.keys()), 'd,c,b,a')


This is an invalid test - it should verify the db_doc here, not the doc.

wojcikstefan · 2017-05-08T01:15:32Z

tests/fields/fields.py

+        self.assertEqual(','.join(doc.ordered_data.keys()), 'd,c,b,a')
+
+    def test_dynamicfield_with_wrong_container_class(self):
+        with self.assertRaises(ValidationError):


Again, ValidationError is a wrong error type.

wojcikstefan · 2017-05-08T01:17:06Z

tests/fields/fields.py

+        # This is because 'codec_options' is supported on pymongo3 or later
+        if IS_PYMONGO_3:
+            class OrderedDocument(Document):
+                my_metaclass = TopLevelDocumentMetaclass


You shouldn't have to override the metaclass here. I expect you did this because you ran into issues when subclassing the OrderedDocument. That's because the class isn't abstract and it doesn't allow inheritance. These two lines should be replaced with meta = {'abstract': True}.

wojcikstefan · 2017-05-08T01:18:18Z

tests/fields/fields.py

+            class DocWithInvalidField:
+                data = DynamicField(container_class=list)
+
+    def test_dynamicfield_with_wrong_container_class_and_reload_docuemnt(self):


This function name is incorrect - the container class is not wrong in this test.

wojcikstefan · 2017-05-08T01:18:36Z

tests/test_dereference.py

+        if IS_PYMONGO_3:
+            class OrderedDocument(Document):
+                my_metaclass = TopLevelDocumentMetaclass
+                __metaclass__ = TopLevelDocumentMetaclass


ditto - should be an abstract class instead.

userlocalhost added 5 commits February 28, 2017 03:34

added OrderedDynamicField class to store data in the defined order be…

6810953

…cause of MongoEngine#203

added OrderedDocument class to decode BSON data to OrderedDict for re…

84a8f1e

…trieving data in order

added test for OrderedDynamicField and OrderedDocument

e32a977

removed a checking of dict order because this order is not cared (som…

6aaf9ba

…e implementation might be in ordered, but other one is not)

userlocalhost force-pushed the feature/order_guarantee branch 2 times, most recently from e73ae55 to 5957dc7 Compare March 1, 2017 09:39

userlocalhost added 3 commits March 8, 2017 14:35

Added a test for the change of the condition in DeReference processing

9cd3dcd

This checks DBRef conversion using DynamicField with the ordering guarantee.

This is Additional tests for the container_class parameter of Dynamic…

ffbb2c9

…Field This tests DynamicField dereference with ordering guarantee.

userlocalhost changed the title ~~added a feature to save and load object data in order~~ added a feature to save object data in order Mar 24, 2017

userlocalhost mentioned this pull request Mar 24, 2017

Process config attributes / options in the order defined in the config schema when using "pack config" CLI command StackStorm/st2#3053

Open

Refactored the implementation of DynamicField extension for storing d…

d8b238d

…ata in order

thedrow merged commit 972ac73 into MongoEngine:master Apr 7, 2017

apolkosnik added a commit to apolkosnik/crits that referenced this pull request Apr 7, 2017

This change makes CRITs work on Mongoengine 0.12

b684c69

This thing broke after MongoEngine/mongoengine#1497 was merged.

apolkosnik mentioned this pull request Apr 7, 2017

This change makes CRITs work on Mongoengine 0.12 crits/crits#894

Merged

wojcikstefan mentioned this pull request May 8, 2017

Issue with wrapped DictField #1542

Closed

wojcikstefan reviewed May 8, 2017

View reviewed changes

wojcikstefan added a commit that referenced this pull request May 8, 2017

revert #1497

009059d

added a feature to save object data in order #1497

added a feature to save object data in order #1497

Uh oh!

Conversation

userlocalhost commented Mar 1, 2017

Uh oh!

wojcikstefan commented Mar 6, 2017

Uh oh!

userlocalhost commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

userlocalhost commented Mar 8, 2017

Uh oh!

userlocalhost commented Mar 23, 2017

Uh oh!

userlocalhost commented Apr 4, 2017

Uh oh!

userlocalhost commented Apr 6, 2017

Uh oh!

wojcikstefan commented Apr 7, 2017

Uh oh!

userlocalhost commented Apr 7, 2017

Uh oh!

wojcikstefan left a comment

Choose a reason for hiding this comment

Uh oh!

wojcikstefan May 8, 2017

Choose a reason for hiding this comment

Uh oh!

wojcikstefan May 8, 2017

Choose a reason for hiding this comment

Uh oh!

wojcikstefan May 8, 2017

Choose a reason for hiding this comment

Uh oh!

wojcikstefan May 8, 2017

Choose a reason for hiding this comment

Uh oh!

wojcikstefan May 8, 2017

Choose a reason for hiding this comment

Uh oh!

wojcikstefan May 8, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

userlocalhost commented Mar 7, 2017 •

edited

Loading