ViewDefinition.sync_many silences and ignores conflicts`

[GoogleCodeExporter]

If a conflict in a design doc is introduced by starting two processes at the 
same time that make calls similar to:

x.py:
couchdb.design.ViewDefinition('foo', 'xxx', '...', reduce=None).sync(db)

y.py:
couchdb.design.ViewDefinition('foo', 'yyy', '...', reduce=None).sync(db)

Only one of xxx or yyy will be present in the design doc (ssuming the race 
condition triggers).

http://code.google.com/p/couchdb-python/source/browse/couchdb/design.py line 
200 has: 

        db.update(docs)

Which doesn't check the return for success.  It should have something like:

for (success, _id, _rev) in db.update(docs):
    if not success:
        raise _rev # it's actually an exception

Original issue reported on code.google.com by wickedg...@gmail.com on 25 May 2011 at 8:25

[GoogleCodeExporter]

Should db.update be atomic then? 

Original comment by kxepal on 25 May 2011 at 9:23

[GoogleCodeExporter]

It's not a matter of atomicity or not, it's that conflict errors are silently 
discarded.  Application authors cannot tell if their view update succeeded or 
not, and there's no guarantee that even if it exists *now* that some later view 
sync from another process won't overwrite it.  Ex:

time proc command
00   x.py started
00   y.py started
01   x.py GET _design/foo  # rev == 1-a
01   y.py GET _design/foo  # rev == 1-a
02   x.py POST _design/foo # added view xxx, rev == 2-a
03   x.py GET _design/foo  # rev == 2-a, xxx is present
04   y.py POST _design/foo # added view yyy but xxx not present from rev 1-a, 
rev == 2-b
05   y.py GET _design/foo  # rev == 2-b, yyy is present
06   x.py GET _design/foo/xxx # error

Original comment by wickedg...@gmail.com on 25 May 2011 at 5:22

[GoogleCodeExporter]

> It's not a matter of atomicity or not, it's that conflict errors are silently 
discarded.
sync_many could change many design documents by single operation, so this is 
matters to keep design system consistency.

> 04   y.py POST _design/foo # added view yyy but xxx not present from rev 1-a, 
rev == 2-b
this wouldn't be, because of ResourceConflict exception.

Anyway, I would agree that missing feedback about syncing of view functions is 
bad.

Original comment by kxepal on 25 May 2011 at 5:50

[GoogleCodeExporter]

Ahh, yes, you are correct - the change would be rejected, not put into 
conflict.  Sorry about the mistake.  :)

Original comment by wickedg...@gmail.com on 25 May 2011 at 7:40

[GoogleCodeExporter]

Attached patch to return db.update result and tests for it.

> Ahh, yes, you are correct - the change would be rejected, not put into 
conflict.
I could clearly recall that `all_or_nothing` option rejects commit changes for 
conflicts or other errors, but seems that behavior was changed - now it ignores 
conflicts making it useless in this situation, because you would have to 
request each synced ddoc to check it for _conflict revisions.

Original comment by kxepal on 27 May 2011 at 1:05

Attachments:

• design.patch

[GoogleCodeExporter]

The changes to couchdb.design look great. Given CouchDB's guarantees, I don't 
think there's anything else it can do other than return the result of 
Database.update() and let the caller check for problems. I guess we could also 
log a warning or something?

The test seems a little over-complicated ;-). All that really needs testing is 
that sync/sync_many now returns a list of results. Database.update() already 
has tests that cover conflicts. In any case, the test is unreliable as it 
depends on the execution order of the threads.

Also, the patch causes doctest failures.

FYI, the current _bulk_docs behaviour has existed since 0.9 and, IIRC, that was 
when all_or_nothing was added too.

Original comment by matt.goo...@gmail.com on 28 May 2011 at 10:02

[GoogleCodeExporter]

Hmm, seems I've to reread Guide to CouchDB in fact on coming release(: 

> The test seems a little over-complicated ;-).
Just to ensure that ViewDefinition.sync_many returns right value, not some kind 
of list type one(;

> Also, the patch causes doctest failures.

Actually, doctests are always fails for me for different reasons: json module 
naming conflict, unicode errors and others, so I haven't paid them too much 
attention.

Ok, I'll fix all of this later at the evening.

Original comment by kxepal on 31 May 2011 at 12:02

[GoogleCodeExporter]

Any update on this?  I just got bitten by this again.  It doesn't seem like you 
can use sync_many for anything production when it silently discards errors.  :/

Original comment by wickedg...@gmail.com on 30 Nov 2011 at 2:11

[GoogleCodeExporter]

> Ok, I'll fix all of this later at the evening.
Ooops, that was too long evening, sorry and thanks for reminding(:

Fixed patch attached.

Original comment by kxepal on 30 Nov 2011 at 11:56

Attachments:

• design_447.patch

[GoogleCodeExporter]

Awesome, thanks!  Looking forward to 0.9.  :)

Original comment by wickedg...@gmail.com on 30 Nov 2011 at 6:04

[GoogleCodeExporter]

I've committed this with minor simplification to docstring and commit message.

Alexander, please add your email address to the committer field in the future. 
:)

Original comment by djc.ochtman on 20 Sep 2012 at 8:13

• Changed state: Fixed