replaceOrCreate should create new instance if id provided #54

jannyHou · 2016-11-08T20:44:55Z

Connect to https://github.com/strongloop-internal/scrum-loopback#991

Scenario:
Without fix in this pr

MyModel.replaceOrCreate({
  id: 'myid', // id provided 
  name: 'myname',
}, function(err, result){...});

will fail if the instance doesn't exist in database.
The current code doesn't check instance exist before do the getRevision() , which cannot run against an nonexistent instance.

Details see: https://github.com/strongloop-internal/scrum-loopback/issues/991#issuecomment-256077756

jannyHou · 2016-11-08T21:20:12Z

The change comparison looks messy..
Let me explain a little bit what I changed in this pr:
I just moved the logic if(id && dataExist){...}else{...} into callback of function self.count(), so now the logic turns to:

self.count({condition_to_check_instance_exist}, function(err, cnt) {
  dataExist = cnt === 1 ? true : false;
  if (id && dataExist) {
    // Do Replace, make sure getRevision won't run on an nonexistent instance
  } else {
    // Do Create
  }
})

@Amir-61 @bajtos PTAL. Thanks :)

Amir-61

@jannyHou Reviewed. Also please squash all your commits as one single commit.

Amir-61 · 2016-11-08T22:03:45Z

test/cloudant.test.js

+        verifyCreatedData(product);
+      });
+
+      function verifyCreatedData(data) {


I would chose a better function name; how about verifyReturnedCallbackData?

Amir-61 · 2016-11-08T22:04:59Z

test/cloudant.test.js

+        data.price.should.be.equal(100);
+
+        // Verify DB data
+        verifyDBData(data.id);


I believe the method name is/should be descriptive enough so the comment is unnecessary in this case...

Amir-61 · 2016-11-08T22:06:05Z

test/cloudant.test.js

+        verifyDBData(data.id);
+      };
+
+      function verifyDBData(id) {


Nitpick: method name is good but I may still choose verifyPersistedData... I leave it to you...Please feel free to ignore this suggestion...

Amir-61 · 2016-11-08T22:09:31Z

test/cloudant.test.js

@@ -68,6 +68,36 @@ describe('cloudant connector', function() {
          });
        };
      });
+    it('should create new instance if id provided', function(done) {


Feel free to ignore since in this file other tests use the same pattern; however, in general we use imperative mood, we do not start test names with should. Please see: https://github.com/strongloop/loopback.io/blob/gh-pages/pages/en/contrib/style-guide.md#test-naming

You may use 'Primary key' instead of id; also you could add a test when the Primary key is not called id as well...

Amir-61 · 2016-11-08T22:13:35Z

lib/cloudant.js

+    if (err) return cb(err);
+    dataExist = cnt === 1 ? true : false;
+
+    if (id && dataExist === true) {


How about:

if (id && dataExist) { ... }

Amir-61 · 2016-11-08T22:16:52Z

lib/cloudant.js

-          cb(err, self.fromDB(model, mo, doc), {isNewInstance: !rev});
+          mo.db.get(id, function(err, doc) {
+            if (err) return cb(err);
+            cb(err, self.fromDB(model, mo, doc), {isNewInstance: !rev});


I think in this branch rev always exists and !rev would always be false, right? In this case wouldn't it be more clear if you pass {isNewInstance: false} instead? Is my understanding right here? Correct me if I'm wrong.

Amir-61 · 2016-11-08T22:23:30Z

lib/cloudant.js

-      self._insert(model, data, function(err) {
+
+  // Check if data exist first to avoid
+  // running getCurrentRevision() again an nonexistent instance


Check if data exists to avoid running 'getCurrentRevision()' against new data.

Amir-61 · 2016-11-08T22:24:27Z

lib/cloudant.js

+  // Check if data exist first to avoid
+  // running getCurrentRevision() again an nonexistent instance
+  var where = {}; where[idName] = id;
+  var dataExist;


var where = {}; where[idName] = id; var dataExist;

Amir-61 · 2016-11-08T22:41:53Z

lib/cloudant.js

-    });
-  }
+    };
+  });


I'm not that much familiar with cloudant; is this the only way to do it in cloudant? Isn't there any atomic method to use? I trust your judgment in this regard...

bajtos · 2016-11-09T12:18:16Z

If I understand this change correctly, then each replaceOrCreate operation is making three database calls now: count, getRevision, replaceById or create. That creates a long window where another request may come in and interfere with our operation in progress :( Is this implementation any better than using the naive algorithm from juggler (findById + replaceById or create)? If my understanding is correct, then neither of these implementations is truly atomic.

When I step back a bit and look at the problem from a high level perspective, then I think it should be acceptable to require callers of patch/replace operations to provide _rev in the data, as that's the way how CouchDB/Cloudant allows clients to detect and resolve conflicts. With this requirement in place, the implementation in this connector should be much simpler - no need to call getRevision, just call _insert directly.

Thoughts?

jannyHou · 2016-11-14T18:15:21Z

@bajtos

to require callers of patch/replace operations to provide _rev in the data

A block of this idea is, user cannot get rev by find() or findById() since method fromDB deletes it:
https://github.com/strongloop/loopback-connector-cloudant/blob/master/lib/cloudant.js#L126-L127

My suggestion is adding a config option includeRev or simply rev when calling fromDB():

if true, returns rev of data, don't delete it
if false, delete it as our current implementation. And make default as false

Since getCurrentRevision() is also applied in other functions, I opened a separate issue to discuss it: #55

On my second thought, I think

That creates a long window where another request may come in and interfere with our operation in progress :(

would not be a problem in cloudant, see cloudant doc for transactions.
So ideally with a well designed app, it won't work in a way that different users frequently send request against one single doc.

jannyHou · 2016-12-08T17:06:05Z

Closing this pr since we agree on #55

Add test case

e582eb3

jannyHou added the bug label Nov 8, 2016

jannyHou self-assigned this Nov 8, 2016

jannyHou added the #review label Nov 8, 2016

Check id existence before do getRevision

c49fa79

jannyHou force-pushed the fix/create-with-id branch from e73e738 to c49fa79 Compare November 8, 2016 21:13

jannyHou assigned Amir-61 and bajtos Nov 8, 2016

Amir-61 reviewed Nov 8, 2016

View reviewed changes

jannyHou mentioned this pull request Nov 14, 2016

Keep ._rev property on returned instance of model #55

Closed

Amir-61 removed their assignment Nov 25, 2016

bajtos removed their assignment Dec 1, 2016

jannyHou closed this Dec 8, 2016

jannyHou removed the #review label Dec 8, 2016

jannyHou deleted the fix/create-with-id branch February 1, 2017 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replaceOrCreate should create new instance if id provided #54

replaceOrCreate should create new instance if id provided #54

jannyHou commented Nov 8, 2016 •

edited

Loading

jannyHou commented Nov 8, 2016 •

edited

Loading

Amir-61 left a comment •

edited

Loading

Amir-61 Nov 8, 2016

Amir-61 Nov 8, 2016

Amir-61 Nov 8, 2016

Amir-61 Nov 8, 2016

Amir-61 Nov 8, 2016

Amir-61 Nov 8, 2016

Amir-61 Nov 8, 2016

Amir-61 Nov 8, 2016

Amir-61 Nov 8, 2016

Amir-61 Nov 8, 2016

bajtos commented Nov 9, 2016

jannyHou commented Nov 14, 2016 •

edited

Loading

jannyHou commented Dec 8, 2016

replaceOrCreate should create new instance if id provided #54

replaceOrCreate should create new instance if id provided #54

Conversation

jannyHou commented Nov 8, 2016 • edited Loading

jannyHou commented Nov 8, 2016 • edited Loading

Amir-61 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Nov 9, 2016

jannyHou commented Nov 14, 2016 • edited Loading

jannyHou commented Dec 8, 2016

jannyHou commented Nov 8, 2016 •

edited

Loading

jannyHou commented Nov 8, 2016 •

edited

Loading

Amir-61 left a comment •

edited

Loading

jannyHou commented Nov 14, 2016 •

edited

Loading