upsertWithWhere feature support in juggler DAO [2.x]

[Amir-61]

• Implements updateOrCreateWithWhere

Backport from #1001

[Amir-61]

@bajtos @superkhau I just simply cherry-picked @mountain1234585's PR to 2.x and lots of their added test cases for upsertWithWhere fail in different connectors... I'm not sure why they fail in 2.x while they all passed on master branch; do you have any idea?

[Amir-61]

@slnode test please

[bajtos]

@Amir-61 are those connectors implementing the new method connector.upsertWithWhere? If they are not, then the tests are expected to fail in the new upsertWithWhere tests added in this patch.

[bajtos]

The CI build failure only confirms my guess.
http://ci.strongloop.com/job/loopback-connector-mongodb/1557/

Test Result (70 failures / +70)
mongodb imported features manipulation upsertWithWhere.updates specific instances when PK is not an auto-generated id
Persistence hooks PersistedModel.upsertWithWhere.triggers hooks in the correct order on update
Persistence hooks PersistedModel.upsertWithWhere.triggers hooks only once
Persistence hooks PersistedModel.upsertWithWhere.triggers `before save` hook on update
Persistence hooks PersistedModel.upsertWithWhere.triggers `before save` hook on create
Persistence hooks PersistedModel.upsertWithWhere.triggers `persist` hook on create
Persistence hooks PersistedModel.upsertWithWhere.triggers `loaded` hook on update
mongodb imported features manipulation upsertWithWhere.updates specific instances when PK is not an auto-generated id
Persistence hooks PersistedModel.upsertWithWhere.triggers hooks in the correct order on update
Persistence hooks PersistedModel.upsertWithWhere.triggers hooks only once
(...)

[bajtos]

I think we need to come up with a mechanism allowing connectors to choose which parts of the shared test suite they want to run. I think the easiest solution is to modify juggler to conditionally skip upsertWitWhere tests if the connector does not implement connector.upsertWithWhere.

[Amir-61]

@slnode test please

[Amir-61]

@bajtos @superkhau

@Amir-61 are those connectors implementing the new method connector.upsertWithWhere? If they are not, then the tests are expected to fail in the new upsertWithWhere tests added in this patch.

I'm not very clear why the failure is expected. If upsertWithWhere is not implemented then it should use non atomic version which uses find, create and updateAttributes which are all implemented in all connectors:

      self.find({ where: ctx.query.where }, opts, function(err, instances) {
        if (err) return cb(err);
        var modelsLength = instances.length;
        if (modelsLength === 0)  {
          self.create(data, options, cb);
        } else if (modelsLength === 1) {
          var modelInst = instances[0];
          modelInst.updateAttributes(data, options, cb);
        } else {
          process.nextTick(function() {
            var error = new Error('There are multiple instances found.' +
                'Upsert Operation will not be performed!');
            error.statusCode = 400;
            cb(error);
          });
        }
      });

I think we need to come up with a mechanism allowing connectors to choose which parts of the shared test suite they want to run. I think the easiest solution is to modify juggler to conditionally skip upsertWitWhere tests if the connector does not implement connector.upsertWithWhere.

This made sense for replaceOrCreate where we need to skip tests for connectors which do not implement replaceById because non-atomic implementation was using replaceById under hood:

      Model.findOne({ where: ctx.query.where }, opts, function(err, found) {
        if (err) return cb(err);
        if (!isOriginalQuery) {
          // The custom query returned from a hook may hide the fact that
          // there is already a model with `id` value `data[idName(Model)]`
          var pkName = idName(Model);
          delete data[pkName];
          if (found) id = found[pkName];
        }
        if (found) {
          self.replaceById(id, data, options, cb);
        } else {
          Model = self.lookupModel(data);
          var obj = new Model(data);
          obj.save(options, cb);
        }
      });

Apart from that tests for other methods fail which are definitely irrelevant like delete, updateAll,deleteById, destroyAll , findOrCreate...

[bajtos]

I'm not very clear why the failure is expected. If upsertWithWhere is not implemented then it should use non atomic version which uses find, create and updateAttributes which are all implemented in all connectors.

Ah, of course. In that case I don't know what else may be the cause. Did you try to reproduce this on your local machine? Both MongoDB and MySQL connectors are easy to set up IIRC.

[mountain1234585]

@Amir-61 I tried to set up mongodb connector (v1.15.2) locally, and put my code changes to the datasource-juggler. I noticed,the persistence hooks tests were failing. I also observed, the way the updateOrCreate test cases are written in 2.x ( I did a npm install in mongodb) is slightly different in 3.x. I tried to change my "upsertWithWhere" tests based on the way they are written in 2.x.
And got all the tests passing, except the one upsertWithWhere.triggers hooks only once. I couldn't find out the reason why only that test is failing , but rest other are passing now on my local setup.

Also, I am not able to see any console.log statements at the prompt where I am running the tests. Is there a way to view them ?
I have sent you the test file over email, for a quick run.

Please find below the results:
PersistedModel.upsertWithWhere
✓ triggers hooks in the correct order on create
✓ triggers hooks in the correct order on update
✓ triggers access hook on create
✓ triggers access hook on update
- triggers hooks only once
✓ applies updates from access hook when found
✓ applies updates from access hook when not found
✓ triggers before save hook on update
✓ triggers before save hook on create
✓ applies updates from before save hook on update
✓ applies updates from before save hook on create
✓ validates model after before save hook on update
✓ validates model after before save hook on create
✓ triggers persist hook on create
✓ triggers persist hook on update
✓ triggers loaded hook on create
✓ triggers loaded hook on update
✓ emits error when loaded hook fails
✓ triggers after save hook on update
✓ triggers after save hook on create

[Amir-61]

@mountain1234585
I diged into the issue and figured out operation hooks failures of your PR; I raised PR#1061 to solve these failures on 2.x; The PR needs to be reviewed by @bajtos and landed so then your backporting PR should go on top of that; I wish you had not squashed your changes with the original commit and had it in separate commits instead, because probably we do not need your operation hook changes any more after landing PR#1061; I’m not sure if that solves all the problems or not, but I think it should solve operation hooks failures; however, please note I see lots of other failures for other methods like delete,deleteById, deleteAll/destroyAll, findOrCreate… so your backporting PR may still fail; you need to wait for the PR#1061 to get reviewed and landed and then do the backporting on top of that and see the result.

[mountain1234585]

@Amir-61 I have not made any commits on 1001 after you landed them on 3.x. I was trying to change the tests on my local setup and run them.

[Amir-61]

@Amir-61 I have not made any commits on 1001 after you landed them on 3.x. I was trying to change the tests on my local setup and run them.

Oh ok good thanks!

[Amir-61]

@bajtos @mountain1234585 @raymondfeng

So I landed the implementation of notify for find method with PR#1061 and rebased @mountain1234585's implementation of upsertWithWhere on top of 2.x. and still many tests fail!

I have absolutely no idea why tests on master branch passed and they are still passing! They should absolutely fail! I cloned this backporting PR on my local machine for testing on mongoDB; the following test was one of the failures:

      it('triggers `loaded` hook on update', function(done) {
        TestModel.observe('loaded', ctxRecorder.recordAndNext());

        TestModel.upsertWithWhere({ id: existingInstance.id },
          { id: existingInstance.id, name: 'updated name' },
          function(err, instance) {
            if (err) return done(err);
            console.log('>>>TestModel:', ctxRecorder.records);
            ctxRecorder.records.should.eql(aCtxForModel(TestModel, {
              data: {
                id: existingInstance.id,
                name: 'updated name',
              },
              isNewInstance: false,
            }));
            done();
          });
      });

Failure message:

      + expected - actual

         }
         "hookState": {
           "test": true
         }
      +  "isNewInstance": false
         "options": {}
       }

The test is not checking if connector.upsertWithWhere exists or not; for non-atomic implementation for upsertWithWhere for the mentioned test, it should use loaded Operation Hooks from updateAttributes where there is no isNewInstance property. Please see here, whereas for atomic implementation isNewInstance property is provided; please see here; hence, I have absolutly no idea why tests on master passed! It may make sense to revert back the changes from master too.

[Amir-61]

@bajtos @raymondfeng

FYI: I just tested mongodb with the latest commit of juggler on master branch and the same test fails locally, but it passes on CI (?) Is it possible that connectors do not get the latest commit of loopback-datasource-juggler on CI for master branch?

[bajtos]

FYI: I just tested mongodb with the latest commit of juggler on master branch and the same test fails locally, but it passes on CI (?) Is it possible that connectors do not get the latest commit of loopback-datasource-juggler on CI for master branch?

I believe the connectors are getting the latest version that matches the specification in their devDependencies. Since most connectors depend on juggler@2.x, they install the latest version from the 2.x branch (not from master).

I was actually thinking about this recently, I think we should create a new protected branch in each connector that will track master but change the dev-dependency to juggler@3.x (pre-release). That way at least the CIS system will run connector tests against 3.x/master version of juggler.

[Amir-61]

@bajtos

I believe the connectors are getting the latest version that matches the specification in their devDependencies. Since most connectors depend on juggler@2.x, they install the latest version from the 2.x branch (not from master).

So thats definitely the problem! What is the point of running connectors as dependencies while they are not pulling changes from master for loopback-datasource-juggler then? They send PRs in juggler against master branch and connectors (dependencies) are not pulling those changes from loopback-datasource-juggler; right now PR#1001 is landed and apparently that breaks all connectors, but we did not notice since CI for connectors were pulling juggler@2.x; now I have no idea how to solve this; should their PR#1001 be reverted back or how can they solve their patch PR#1001 while connectors on CI are not pulling the changes from master for loopback-datasource-juggler?

[Amir-61]

@slnode test please

[Amir-61]

@mountain1234585 Our CI was faulty; although your PR#1001 was breaking dependencies (connectors) with tons of failures, CI was not reporting them on master branch, because it was not picking the changes on master branch for dependencies; I fixed your test cases now in 248f772 ; also I implemented notify for find method in 2.x with PR#1061 to unblock your work from landing.

The commit needs to be forward-ported to master branch as well.

@bajtos @superkhau Please review.

Edit: Please don't get overwhelmed with changes in test/manipulation.test.js I just moved the describe('upsertWithWhere', function() {...} to the end of the file.

[bajtos]

This is changing the intent of the test. Before, the query did not match the existing instance, I presume a new record was created? Now the existing record is updated. Is that ok?

[Amir-61]

Please note for non-atomic implementation id change in data object does not get ignored; for more info please see: https://github.com/strongloop/loopback-datasource-juggler/blob/72200ce935367cffc7c7df3b6cffc625acb8192a/lib/dao.js#L3020-L3033

[Amir-61]

@slnode test please

[Amir-61]

@slnode test please

[Amir-61]

@bajtos Thanks for the review! I applied your feedback.

[bajtos]

@Amir-61 please capture the information from #1052 (comment) in a comment in the test source code. I don't have any more comments.

[Amir-61]

@bajtos @superkhau,

I want to land this PR with the fixes; however, it seems a KV test fails in loopback-datasource-juggler/node=0.10,os=macos and that stops CI from testing connectors; please see the log here any thought about this failure?

1) KeyValue-Memory connector KeyValue API ttl existing key with expire before expiration time returns ttl - Promise API:
     Error: Cannot get TTL for unknown key "a-key"
      at lib/connectors/kv-memory.js:148:17

Also I forward-port the tests fixes to master; please review this PR: #1073

[bajtos]

it seems a KV test fails in loopback-datasource-juggler/node=0.10,os=macos and that stops CI from testing connectors;

@superkhau could you PTAL? you were fixing KV-related CI failures recently.

[Amir-61]

Fixed the failures for this PR and landed to both master and loopbakc-datasource-juggler@2.x.