update only changed fields. see issue #2 for details #45
Conversation
…ent is deepcopied
|
build is failing due to codecov |
Codecov Report
@@ Coverage Diff @@
## master #45 +/- ##
==========================================
+ Coverage 89.96% 90.10% +0.13%
==========================================
Files 43 43
Lines 6636 6718 +82
==========================================
+ Hits 5970 6053 +83
+ Misses 666 665 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
see #2 |
|
Hi, I haven't had a chance to look at this yet - but wanted to say I have seen it and it looks quite interesting. Just a little update to let you know your contribution is appreciated and seen. I hope to take a serious look at it before too much longer. |
|
Hi, thanks for working on this and for your patience. Could you add a simple test that uses I ran these changes against a large app that uses Ming (Apache Allura, which I work on). I found two issues:
class MyThing:
class __mongometa__:
...
def before_save(data):
data['mod_date'] = datetime.utcnow()We should probably add a Ming test that has this kind of behavior.
if state.original_document:
# here we do a symmetric difference to see what fields did change
fields = tuple(set((k for k, v in
doc_to_set(state.original_document)
^ doc_to_set(state.document))))
else:
fields = () |
|
state.options isn't documented |
|
I think this is a good approach to dealing with it. I get this test error, not sure why the CI tests didn't error on it: I would like to keep the save() behavior that lets a list of fields be passed in. Even though its not documented, there was a test for it and I know I have some code that uses it. I think the method could work like this. It also fixes the above error if the args/fields ends up empty. @annotate_doc_failure
def save(self, doc, *args, state=None, **kwargs):
data = self._prep_save(doc, kwargs.pop('validate', True))
if not args and state is not None and state.original_document:
args = tuple(set((k for k, v in
doc_to_set(state.original_document)
^ doc_to_set(data))))
if args:
values = dict((arg, data[arg]) for arg in args)
result = self._impl(doc).update(
dict(_id=doc._id), {'$set': values}, **fix_write_concern(kwargs))
else:
result = self._impl(doc).save(data, **fix_write_concern(kwargs))
if result and '_id' not in doc:
doc._id = result
return resultHowever, I have found another bug, a very subtle one. Here's a simple test case. If you do multiple updates within a session and one of the updates puts a field back to the original value, that is not detected as a change so it is not saved. Not sure what the right solution to this would be. Maybe class TestBasicMapping(TestCase):
...
def test_multiple_update_flushes(self):
initial_doc = self.Basic()
initial_doc.a = 1
self.session.flush()
self.session.close()
doc_updating = self.Basic.query.get(_id=initial_doc._id)
doc_updating.a = 2
self.session.flush()
doc_updating.a = 1 # back to "initial" value
doc_updating.e = 'foo' # change something else too
self.session.flush()
self.session.close()
doc_after_updates = self.Basic.query.get(_id=doc_updating._id)
assert doc_after_updates.a == 1``` |
|
Ming seems to use Isn't the state meant to recover data from a bad update "transaction"?
then I consider not updating original_document a bug that you spotted You're absolutely right about keeping args, so one could eventually bypass field detection or just skip some of them I'm unable to reproduce the error you reported, is it somehow apple related? Thank you |
|
Yea as far as I can tell it should be ok to update Regarding the error I got, I'm not sure why it happened really. But if we go with the |
…nd don't break on inserts. tests provided
|
Thank you Dave 😊 |
|
Hi again. When I ran the Allura tests with these ming changes, I got some really deep exceptions related to Here's the deepcopy errors |
that we need copy on write |
|
I will at least merge the For the rest (updating only changed fields) it certainly is annoying that we've gotten so close yet keep running into problems for certain situations. I haven't had much time to spend on this, but when I get a chance I do want to investigate the error I found where |
|
I have merged the |
No description provided.