Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to force update a doc when there's conflict? #5832

Closed
simpzan opened this issue Nov 2, 2016 · 20 comments

Comments

@simpzan
Copy link
Contributor

commented Nov 2, 2016

Issue

How to update a doc based on a non-leaf revision? namely I want to create a new conflict in the doc.
The reason I want to do this is that I don't want to resolve the conflict when writing on one client, I want to resolve the conflict later when I want to.
Thanks!

Info

  • Environment: (browser)
  • Platform: (Chrome)
  • Adapter: (IndexedDB)
  • Server: (CouchDB)

Reproduce

N/A

@daleharvey

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

We dont have a huge amount of docs on this since its generally not something people want to do (it can get dangerous pretty quickly) but you can use new_edits: false with db.bulkDocs to create conflicts, you can find examples around the tests and some on the web.

What is your use case?

@simpzan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2016

I am developing a note web app which supports both phone and desktop. It supports syncing while editing the note. and the editing note is periodically saved to local pouchdb. So It is possible that the data in pouchdb and memory are out of sync, namely note in db has modifies from server and note in memory has modifies from user.

At this time if I save the note in memory to db based on the latest revision in db, the modifies from server will be hidden. The ideal solution is to generate a conflict and resolve later when convenient, because it is really hard to resolve the conflict on the phone.

Though I can also save the memory note as a new doc in db, I have to get a way to know which 2 notes are in conflicts.

one doc with a conflict revision vs 2 docs with some info indicating they are conflict notes, I think the first is much better.

@simpzan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2016

I found in new_edits: false with db.bulkDocs method, I have to generate a new revision myself. In my use case, I want pouchdb generate a new revision for me.

@simpzan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2016

In this doc about conflict https://pouchdb.com/guides/conflicts.html, it says that there are 2 types of conflicts, immediate conflict and eventual conflict. with eventual conflict I can resolve it at any time I want. It's great to have this freedom.

but with immediate conflict, I don't have this freedom, I have to resolve it right away. I think it should not be hard to provide this freedom for immediate conflict as well, like add a force option to put operation. What is the consideration not to do it?

@daleharvey

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

I found in new_edits: false with db.bulkDocs method, I have to generate a
new revision myself. In my use case, I want pouchdb generate a new revision for me.

So we do not currently support this, however the rev generated is just a random string so should be easy enough to generate, here is where we do it https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-adapter-utils/src/parseDoc.js#L93

like add a force option to put operation. What is the consideration not to do it?

So like 5 years ago we had a discussion around not only supporting this but making it the default mode of operation in CouchDB, I think this would be a good addition to pouchdb + couchdb, happy to help out if you fancy making a patch, it should not be too complicated, it would be an edit to https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-adapter-utils/src/updateDoc.js#L35 to support it

@simpzan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2016

@daleharvey Thank you so much for the information and the support.

I will try to add the feature, but before I can make contribution, I need to know the overall structure of the codebase, Is there any doc to get started? Thanks again!

@daleharvey

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

https://github.com/pouchdb/pouchdb/blob/master/CONTRIBUTING.md is our guide for contributors, we dont have a structure doc, but the basics are, the main part of pouchdb is kept in https://github.com/pouchdb/pouchdb/tree/master/packages/node_modules/pouchdb-core, when you call a function in pouchdb (like db.allDocs) then it goes to https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-core/src/adapter.js, this handles parsing opts and some common things, it then calls out to whichever adapter you are using (indexeddb, leveldb etc) which are stored as packages/node_modules/pouchdb-adapter-x these adapters then deal with the actual storage implementation, but they may also call out to various shared utils (like updateDoc.js I linked earlier)

@daleharvey

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

For this patch, the first thing to do is write a test, that can go in https://github.com/pouchdb/pouchdb/blob/master/tests/integration/test.bulk_docs.js and can look something like

var origDoc;
db.bulkDocs({_id: 'doc'}).then(function(_origDoc) { 
  origDoc = _origDoc;
  return db.put({_id: 'doc', _rev: origDoc.rev, update:1}, {allow_conflict: true});
}).then(function() { 
  return db.put({_id: 'doc', _rev: origDoc.rev, update:2}, {allow_conflict: true});
}).then(function() { 
  // Check stuff is ok
});
@daleharvey

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

The next thing, its worth noting that .put / .post even .delete are all helper functions, everything gets written via bulkDocs, in the idb adapter that is https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-adapter-idb/src/index.js#L306 which calls https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-adapter-idb/src/bulkDocs.js and eventually makes it way to https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-adapter-utils/src/updateDoc.js#L10 which is what deals with all doc updates, its there we detect a conflict, so in that code it needs to know if the allow_conflicts param has been set (similiar to new_edits) and if so allow the conflict to be created

@simpzan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2016

simpzan@5cf632d

@daleharvey Thanks for your help, I successfully implemented the force update feature.

But I found a new issue with this implementation, therev in the returned result is the winning revision of the doc, instead I want the revision id of the new generated revision.

If there's already a conflict in the doc before update, after the force update I have no easy way to find out which revision is actually the one I just generated.

Another related case about the behavior of rev in result object, If there's a doc with a conflict revision, and I deleted the winning revision, the rev in the result object of the deletion operation is the formally conflict revision, instead of the new generated deletion revision.

so it seems like rev is always the winning revision of the doc, Is it possible to add another field to tell the caller the new generated revision id? What is your idea? Thanks!

@willholley

This comment has been minimized.

Copy link
Member

commented Nov 6, 2016

If there's a doc with a conflict revision, and I deleted the winning revision, the rev in the result object of the deletion operation is the formally conflict revision, instead of the new generated deletion revision.

This sounds like a bug - if you can add a minimal test case, that would be super helpful, else I'll try and investigate this week.

@simpzan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2016

Yes, I can provide a test case.

What is the expected behavior on this case?

Sent from my iPhone

On Nov 7, 2016, at 01:24, Will Holley notifications@github.com wrote:

If there's a doc with a conflict revision, and I deleted the winning revision, the rev in the result object of the deletion operation is the formally conflict revision, instead of the new generated deletion revision.

This sounds like a bug - if you can add a minimal test case, that would be super helpful, else I'll try and investigate this week.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@simpzan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2016

@willholley this is the test case,

    it.only('remove a doc with conflict will return rev of the conflict revision', function(done) {
      // given
      var docs = [
        {
          _id: 'fubar',
          _rev: '1-a1',
          _revisions: { start: 1, ids: [ 'a1' ] }
        }, {
          _id: 'fubar',
          _rev: '2-a2',
          _revisions: { start: 2, ids: [ 'a2', 'a1' ] }
        }, {
          _id: 'fubar',
          _rev: '2-b2',
          _revisions: { start: 2, ids: [ 'b2', 'a1' ] }
        }
      ];
      var db = new PouchDB(dbs.name);
      db.bulkDocs({ docs: docs, new_edits: false }).then(function () {
        return db.get('fubar', {conflicts: true});
      })
      // when
      .then(function (doc) {
        console.log('doc with conflict', doc)
        return db.remove(doc);
      })
      // then
      .then(function (result) {
        console.log('remove result', result)
        done();
      }).catch(done);
    })

and this is the output of the test case on node.js environment. apparently there're diff between pouchdb leveldb version and couchdb.

  test.bulk_docs.js-local
doc with conflict { _id: 'fubar', _rev: '2-b2', _conflicts: [ '2-a2' ] }
remove result { ok: true, id: 'fubar', rev: '2-a2' }
    ✓ remove a doc with conflict will return rev of the conflict revision

  test.bulk_docs.js-http
doc with conflict { _id: 'fubar', _rev: '2-b2', _conflicts: [ '2-a2' ] }
remove result { ok: true,
  id: 'fubar',
  rev: '3-5f6ce785557543b6780177c7806d346b' }
    ✓ remove a doc with conflict will return rev of the conflict revision (77ms)

@willholley can you tell me what exactly is 'rev' field in the result object? Is it always the winning rev id or always the new generated rev id? Thanks!

@willholley

This comment has been minimized.

Copy link
Member

commented Nov 7, 2016

Thanks @simpzan - the CouchDB behaviour is correct.

On 7 Nov 2016 7:09 a.m., "Samuel" notifications@github.com wrote:

@willholley https://github.com/willholley this is the test case,

it.only('remove a doc with conflict will return rev of the conflict revision', function(done) {
  // given
  var docs = [
    {
      _id: 'fubar',
      _rev: '1-a1',
      _revisions: { start: 1, ids: [ 'a1' ] }
    }, {
      _id: 'fubar',
      _rev: '2-a2',
      _revisions: { start: 2, ids: [ 'a2', 'a1' ] }
    }, {
      _id: 'fubar',
      _rev: '2-b2',
      _revisions: { start: 2, ids: [ 'b2', 'a1' ] }
    }
  ];
  var db = new PouchDB(dbs.name);
  db.bulkDocs({ docs: docs, new_edits: false }).then(function () {
    return db.get('fubar', {conflicts: true});
  })
  // when
  .then(function (doc) {
    console.log('doc with conflict', doc)
    return db.remove(doc);
  })
  // then
  .then(function (result) {
    console.log('remove result', result)
    done();
  }).catch(done);
})

and this is the output of the test case on node.js environment. apparently
there're diff between pouchdb leveldb version and couchdb.

test.bulk_docs.js-local
doc with conflict { _id: 'fubar', _rev: '2-b2', _conflicts: [ '2-a2' ] }
remove result { ok: true, id: 'fubar', rev: '2-a2' }
✓ remove a doc with conflict will return rev of the conflict revision

test.bulk_docs.js-http
doc with conflict { _id: 'fubar', _rev: '2-b2', _conflicts: [ '2-a2' ] }
remove result { ok: true,
id: 'fubar',
rev: '3-5f6ce785557543b6780177c7806d346b' }
✓ remove a doc with conflict will return rev of the conflict revision (77ms)

@willholley https://github.com/willholley can you tell me what exactly
is 'rev' field in the result object? Is it always the winning rev id or
always the new generated rev id? Thanks!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5832 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAF9-2MBNoHkyrLoNTO5LDJTUXVtnqyVks5q7s6ZgaJpZM4KnDXu
.

@willholley

This comment has been minimized.

Copy link
Member

commented Nov 7, 2016

i.e. it should return the rev that was generated from the update, not
(necessarily) the winning rev.

On 7 Nov 2016 8:21 a.m., wrote:

Thanks @simpzan - the CouchDB behaviour is correct.

On 7 Nov 2016 7:09 a.m., "Samuel" notifications@github.com wrote:

@willholley https://github.com/willholley this is the test case,

it.only('remove a doc with conflict will return rev of the conflict revision', function(done) {
  // given
  var docs = [
    {
      _id: 'fubar',
      _rev: '1-a1',
      _revisions: { start: 1, ids: [ 'a1' ] }
    }, {
      _id: 'fubar',
      _rev: '2-a2',
      _revisions: { start: 2, ids: [ 'a2', 'a1' ] }
    }, {
      _id: 'fubar',
      _rev: '2-b2',
      _revisions: { start: 2, ids: [ 'b2', 'a1' ] }
    }
  ];
  var db = new PouchDB(dbs.name);
  db.bulkDocs({ docs: docs, new_edits: false }).then(function () {
    return db.get('fubar', {conflicts: true});
  })
  // when
  .then(function (doc) {
    console.log('doc with conflict', doc)
    return db.remove(doc);
  })
  // then
  .then(function (result) {
    console.log('remove result', result)
    done();
  }).catch(done);
})

and this is the output of the test case on node.js environment.
apparently there're diff between pouchdb leveldb version and couchdb.

test.bulk_docs.js-local
doc with conflict { _id: 'fubar', _rev: '2-b2', _conflicts: [ '2-a2' ] }
remove result { ok: true, id: 'fubar', rev: '2-a2' }
✓ remove a doc with conflict will return rev of the conflict revision

test.bulk_docs.js-http
doc with conflict { _id: 'fubar', _rev: '2-b2', _conflicts: [ '2-a2' ] }
remove result { ok: true,
id: 'fubar',
rev: '3-5f6ce785557543b6780177c7806d346b' }
✓ remove a doc with conflict will return rev of the conflict revision (77ms)

@willholley https://github.com/willholley can you tell me what exactly
is 'rev' field in the result object? Is it always the winning rev id or
always the new generated rev id? Thanks!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5832 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAF9-2MBNoHkyrLoNTO5LDJTUXVtnqyVks5q7s6ZgaJpZM4KnDXu
.

@simpzan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2016

That's great! Thanks @willholley !

So when this bug fixed, I think the new force put operation should have the expected behavior as well. I can't wait to see it fixed. :)

willholley added a commit that referenced this issue Nov 7, 2016
For some adapters, document updates were returning
the winning rev in the document update metadata
rather than the revision just written. This adds
a test for the behaviour and fixes it.
willholley added a commit that referenced this issue Nov 8, 2016
For some adapters, document updates were returning
the winning rev in the document update metadata
rather than the revision just written. This adds
a test for the behaviour and fixes it.
willholley added a commit that referenced this issue Nov 8, 2016
For some adapters, document updates were returning the winning rev in the document update metadata rather than the revision just written. This adds a test for the behaviour and fixes it.
simpzan added a commit to simpzan/pouchdb that referenced this issue Nov 12, 2016
add a `force` option to `put` operation to allow update based on
non-leaf revison and generate a new conflict revision, instead of
reporting update conflict error.
@nolanlawson

This comment has been minimized.

Copy link
Member

commented Dec 18, 2016

Is this issue fixed @simpzan / @willholley or is there still stuff left to do?

nolanlawson added a commit that referenced this issue Dec 18, 2016
add a `force` option to `put` operation to allow update based on
non-leaf revison and generate a new conflict revision, instead of
reporting update conflict error.
@simpzan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2016

@nolanlawson the main issue is the new feature force update. It is still waiting to be merged.

And during the development of the new feature, I found a bug, and @willholley had already fixed it.

simpzan added a commit to simpzan/pouchdb that referenced this issue Jan 29, 2017
add a `force` option to `put` operation to allow update based on
non-leaf revison and generate a new conflict revision, instead of
reporting update conflict error.
it is implemented by generating `new_edits=false` option.
nolanlawson added a commit that referenced this issue May 17, 2017
* (#5832) - new feature: force update.
add a `force` option to `put` operation to allow update based on
non-leaf revison and generate a new conflict revision, instead of
reporting update conflict error.
it is implemented by generating `new_edits=false` option.

* optimized force update feature with method proposed by @ronag.
need not to fetch `revisions` from db, 2 revisions are enough,
namely the new generated one and the old one provided by the user.

* bugfix:explictly set radix to 10 for parseInt().

* fix an unintented change caused coverage becomes < 100%.

* replace const with var to fix PhantomJS tests failure.

* add test case of force putting on second level parent node.

* a simple note on the new `force` option for put() api.
@listepo

This comment has been minimized.

Copy link

commented Jun 15, 2017

Any news?

@simpzan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2017

@listepo I have submitted a PR implementing this feature and it has been merged, see here. dd59491

@simpzan simpzan closed this Jun 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.