[SEMVER-MAJOR] Disallow bulk updateOrCreate. #889

richardpringle · 2016-04-05T04:59:41Z

BREAKING CHANGE

@bajtos, please review.

richardpringle · 2016-04-05T05:00:30Z

Also, let me know if you want me to break this up into separate commits for some of that test functionality.

superkhau · 2016-04-05T06:43:00Z

lib/dao.js

+  if (Array.isArray(data)) {
+    var isBulkCreate = data.every(function(item){
+      var id = getIdValue(self, item);
+      return id === undefined || id === null;


This can probably be simplified to:

return !getIdValue(self, item);

superkhau · 2016-04-05T07:11:31Z

@richardpringle Reviewed with some feedback for you.

richardpringle · 2016-04-05T14:10:54Z

fix for #2123

bajtos · 2016-04-06T11:19:08Z

lib/dao.js

+    if (isBulkCreate) {
+      return self.create(data, options, cb);
+    } else {
+      return cb(new Error('updateOrCreate does not support bulk mode or any array input'));


... or any array input - that's not correct, is it? We still support array input when items don't contain ids.

bajtos · 2016-04-06T11:27:03Z

fix for #2123

@richardpringle ^^^ that did not create cross-link, did you mean strongloop/loopback#2123? It's also best to put cross-links into PR description at the top (you can edit the description after the PR was created).

@superkhau @raymondfeng This change modifies the outcome of certain edge cases, e.g. when calling updateOrCreate with an array of instances where some of them have id set. See the discussion in the linked issue for more details. I think these edge cases are not valid usage of the API , thus one can argue it's not a breaking change and it's ok to back-port this change to 2.x too.

OTOH, if you think we should be more conservative and do not back-port this change, then I would vote for a more aggressive cleanup and always return cb(new Error(...)) when the input is an array.

Or perhaps we can always return an error for array input in 3.x, and in 2.x branch, we can do what's implemented here, i.e. fail only when the request is not a clear bulk create.

Thoughts?

bajtos · 2016-04-06T11:42:33Z

Cross-posting from strongloop/loopback#2123

It makes me wonder, what happens when calling bulk-create remotely via POST method and some of the instances cannot be created, e.g. because they don't pass validation or perhaps violate unique constraint/index? Will the user receive unknown error too?

They get the same "unknown error" because of the rest-adapter error handler. The err object in this case is really an array of errors so err.message is undefined.

In that case, I am leaning towards disabling array input for updateOrCreate completely and land this change in master (3.0) only.

Thoughts?

richardpringle · 2016-04-06T13:55:26Z

@superkhau suggested that we could also just make a change in the rest-adapter error handler to enable handling an array of errors.

I really do think it would be better to clean up both POST (create) and PUT (replaceOrCreate) and PATCH (updateOrCreate) and release the changes all at the same time to stay consistent, assuming that replaceOrCreate will run into the same issues.

So for now, I would suggest making the changes in strong-remoting, making them available in LoopBack 2.x and only landing some of my test cases here once I clean them up.

richardpringle · 2016-04-06T13:57:59Z

Another argument for adjusting strong-remoting is that I would expect the error you get via REST would be the same as the error on the back-end.

richardpringle · 2016-05-05T14:58:40Z

@bajtos, @superkhau, ready for another round of review

superkhau · 2016-05-05T18:22:49Z

LGTM

bajtos · 2016-05-06T13:04:52Z

test/manipulation.test.js

+      Todo = db.define('Todo', {
+        content: String,
+      });
+      done();


I think you should still call db.automigrate(done) to ensure the tables are created? Note that these tests are run by all connectors, and especially SQL connectors need to create tables before you can store any data.

The way how the code is written now, you are relying on some other tests running earlier to create a table for Todo model, possibly with a different set of properties than your Todo model has.

bajtos · 2016-05-06T15:27:43Z

LGTM, please wait to see dependant build of connectors to pass, just to be sure.

richardpringle · 2016-05-16T18:22:01Z

@slnode test please

richardpringle · 2016-05-16T19:17:47Z

dependant loopback-connector-remote is failing on all instances of node 0.10

I believe it's because of the following PR: strongloop/loopback-connector-remote#44

and more particularly https://github.com/strongloop/loopback-connector-remote/pull/44/files#r61875179

richardpringle · 2016-05-17T19:12:56Z

Need to re-trigger tests again when strongloop/loopback-connector-remote#47 lands

bajtos · 2016-05-24T11:49:48Z

@slnode test please

richardpringle · 2016-05-24T15:10:16Z

@bajtos, it looks like the expected behavior has now changed in some dependent modules (loopback-sdk-angular for example).

What's the next step, is it to go through and re-write and/or remove unit tests from the dependants?

bajtos · 2016-05-25T08:50:29Z

it looks like the expected behavior has now changed in some dependent modules (loopback-sdk-angular for example).

Ha, that just confirms that this patch is a breaking change.

What's the next step, is it to go through and re-write and/or remove unit tests from the dependants?

Could you please point me to the failing tests? Ideally, we should fix the tests to work with both LB 2.x and 3.x, but this really depends.

richardpringle · 2016-05-25T14:37:14Z

Here are the failing tests:

And below are the logs from Jenkins (link to see console on Jenkins):

Tue, 24 May 2016 12:10:35 GMT engine:ws received "42["info",{"log":"'methods', ['get', 'save', 'query', 'remove', 'delete', 'create', 'upsert', 'exists', 'findById', 'find', 'findOne', 'updateAll', 'deleteById', 'count', 'prototype$updateAttributes', 'createChangeStream', 'bind', 'updateOrCreate', 'update', 'destroyById', 'removeById', 'modelName']","type":"log"}]"
Tue, 24 May 2016 12:10:35 GMT express:router dispatching POST /setup
Tue, 24 May 2016 12:10:35 GMT engine:socket packet
Tue, 24 May 2016 12:10:35 GMT express:router query  : /setup
Tue, 24 May 2016 12:10:35 GMT express:router expressInit  : /setup
Tue, 24 May 2016 12:10:35 GMT socket.io-parser decoded 2["info",{"log":"'methods', ['get', 'save', 'query', 'remove', 'delete', 'create', 'upsert', 'exists', 'findById', 'find', 'findOne', 'updateAll', 'deleteById', 'count', 'prototype$updateAttributes', 'createChangeStream', 'bind', 'updateOrCreate', 'update', 'destroyById', 'removeById', 'modelName']","type":"log"}] as {"type":2,"nsp":"/","data":["info",{"log":"'methods', ['get', 'save', 'query', 'remove', 'delete', 'create', 'upsert', 'exists', 'findById', 'find', 'findOne', 'updateAll', 'deleteById', 'count', 'prototype$updateAttributes', 'createChangeStream', 'bind', 'updateOrCreate', 'update', 'destroyById', 'removeById', 'modelName']","type":"log"}]}
Tue, 24 May 2016 12:10:35 GMT express:router <anonymous>  : /setup
Tue, 24 May 2016 12:10:35 GMT express:router jsonParser  : /setup
Tue, 24 May 2016 12:10:35 GMT socket.io:socket got packet {"type":2,"nsp":"/","data":["info",{"log":"'methods', ['get', 'save', 'query', 'remove', 'delete', 'create', 'upsert', 'exists', 'findById', 'find', 'findOne', 'updateAll', 'deleteById', 'count', 'prototype$updateAttributes', 'createChangeStream', 'bind', 'updateOrCreate', 'update', 'destroyById', 'removeById', 'modelName']","type":"log"}]}
Tue, 24 May 2016 12:10:35 GMT body-parser:json content-type "application/json"
Tue, 24 May 2016 12:10:35 GMT socket.io:socket emitting event ["info",{"log":"'methods', ['get', 'save', 'query', 'remove', 'delete', 'create', 'upsert', 'exists', 'findById', 'find', 'findOne', 'updateAll', 'deleteById', 'count', 'prototype$updateAttributes', 'createChangeStream', 'bind', 'updateOrCreate', 'update', 'destroyById', 'removeById', 'modelName']","type":"log"}]
Tue, 24 May 2016 12:10:35 GMT body-parser:json content-encoding "identity"
Tue, 24 May 2016 12:10:35 GMT body-parser:json read body
LOG: 'methods', ['get', 'save', 'query', 'remove', 'delete', 'create', 'upsert', 'exists', 'findById', 'find', 'findOne', 'updateAll', 'deleteById', 'count', 'prototype$updateAttributes', 'createChangeStream', 'bind', 'updateOrCreate', 'update', 'destroyById', 'removeById', 'modelName']
Tue, 24 May 2016 12:10:35 GMT engine:ws received "42["result",{"id":"","description":"has all methods including aliases","suite":["services","MyModel $resource"],"success":false,"skipped":false,"time":5,"log":["expected [ Array(22) ] to be a superset of [ Array(13) ]\nassert@http://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:873:67\nhttp://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:2293:25\nhttp://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:3627:30\nhttp://localhost:9876/base/test.e2e/spec/services.spec.js?e850318c0e078721d0c01502db8db3ccb18edce7:399:47"],"assertionErrors":[{"name":"AssertionError","message":"expected [ Array(22) ] to be a superset of [ Array(13) ]","showDiff":false}]}]"
Tue, 24 May 2016 12:10:35 GMT engine:socket packet
Tue, 24 May 2016 12:10:35 GMT socket.io-parser decoded 2["result",{"id":"","description":"has all methods including aliases","suite":["services","MyModel $resource"],"success":false,"skipped":false,"time":5,"log":["expected [ Array(22) ] to be a superset of [ Array(13) ]\nassert@http://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:873:67\nhttp://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:2293:25\nhttp://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:3627:30\nhttp://localhost:9876/base/test.e2e/spec/services.spec.js?e850318c0e078721d0c01502db8db3ccb18edce7:399:47"],"assertionErrors":[{"name":"AssertionError","message":"expected [ Array(22) ] to be a superset of [ Array(13) ]","showDiff":false}]}] as {"type":2,"nsp":"/","data":["result",{"id":"","description":"has all methods including aliases","suite":["services","MyModel $resource"],"success":false,"skipped":false,"time":5,"log":["expected [ Array(22) ] to be a superset of [ Array(13) ]\nassert@http://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:873:67\nhttp://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:2293:25\nhttp://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:3627:30\nhttp://localhost:9876/base/test.e2e/spec/services.spec.js?e850318c0e078721d0c01502db8db3ccb18edce7:399:47"],"assertionErrors":[{"name":"AssertionError","message":"expected [ Array(22) ] to be a superset of [ Array(13) ]","showDiff":false}]}]}
Tue, 24 May 2016 12:10:35 GMT socket.io:socket got packet {"type":2,"nsp":"/","data":["result",{"id":"","description":"has all methods including aliases","suite":["services","MyModel $resource"],"success":false,"skipped":false,"time":5,"log":["expected [ Array(22) ] to be a superset of [ Array(13) ]\nassert@http://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:873:67\nhttp://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:2293:25\nhttp://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:3627:30\nhttp://localhost:9876/base/test.e2e/spec/services.spec.js?e850318c0e078721d0c01502db8db3ccb18edce7:399:47"],"assertionErrors":[{"name":"AssertionError","message":"expected [ Array(22) ] to be a superset of [ Array(13) ]","showDiff":false}]}]}
Tue, 24 May 2016 12:10:35 GMT socket.io:socket emitting event ["result",{"id":"","description":"has all methods including aliases","suite":["services","MyModel $resource"],"success":false,"skipped":false,"time":5,"log":["expected [ Array(22) ] to be a superset of [ Array(13) ]\nassert@http://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:873:67\nhttp://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:2293:25\nhttp://localhost:9876/base/node_modules/chai/chai.js?80a81da918cc1a355fd23c104379920817ef5b78:3627:30\nhttp://localhost:9876/base/test.e2e/spec/services.spec.js?e850318c0e078721d0c01502db8db3ccb18edce7:399:47"],"assertionErrors":[{"name":"AssertionError","message":"expected [ Array(22) ] to be a superset of [ Array(13) ]","showDiff":false}]}]
      ✖ has all methods including aliases

Tue, 24 May 2016 12:10:37 GMT socket.io-parser decoded 2["result",{"id":"","description":"creates multiple related models","suite":["services","for models with hasAndBelongsToMany relations"],"success":false,"skipped":false,"time":0,"log":["undefined is not an object (evaluating 'action.apply')\ncreateMany@http://localhost:3838/services?services::with%20authentication%20using%20built-in%20User%20model::adds%20auth-related%20methods%20to%20the%20User%20model:913:24\nhttp://localhost:9876/base/test.e2e/spec/services.spec.js?e850318c0e078721d0c01502db8db3ccb18edce7:936:49"],"assertionErrors":[]}] as {"type":2,"nsp":"/","data":["result",{"id":"","description":"creates multiple related models","suite":["services","for models with hasAndBelongsToMany relations"],"success":false,"skipped":false,"time":0,"log":["undefined is not an object (evaluating 'action.apply')\ncreateMany@http://localhost:3838/services?services::with%20authentication%20using%20built-in%20User%20model::adds%20auth-related%20methods%20to%20the%20User%20model:913:24\nhttp://localhost:9876/base/test.e2e/spec/services.spec.js?e850318c0e078721d0c01502db8db3ccb18edce7:936:49"],"assertionErrors":[]}]}
Tue, 24 May 2016 12:10:37 GMT express:router <anonymous>  : /setup
Tue, 24 May 2016 12:10:37 GMT socket.io:socket got packet {"type":2,"nsp":"/","data":["result",{"id":"","description":"creates multiple related models","suite":["services","for models with hasAndBelongsToMany relations"],"success":false,"skipped":false,"time":0,"log":["undefined is not an object (evaluating 'action.apply')\ncreateMany@http://localhost:3838/services?services::with%20authentication%20using%20built-in%20User%20model::adds%20auth-related%20methods%20to%20the%20User%20model:913:24\nhttp://localhost:9876/base/test.e2e/spec/services.spec.js?e850318c0e078721d0c01502db8db3ccb18edce7:936:49"],"assertionErrors":[]}]}
Tue, 24 May 2016 12:10:37 GMT express:router jsonParser  : /setup
Tue, 24 May 2016 12:10:37 GMT body-parser:json content-type "application/json"
Tue, 24 May 2016 12:10:37 GMT socket.io:socket emitting event ["result",{"id":"","description":"creates multiple related models","suite":["services","for models with hasAndBelongsToMany relations"],"success":false,"skipped":false,"time":0,"log":["undefined is not an object (evaluating 'action.apply')\ncreateMany@http://localhost:3838/services?services::with%20authentication%20using%20built-in%20User%20model::adds%20auth-related%20methods%20to%20the%20User%20model:913:24\nhttp://localhost:9876/base/test.e2e/spec/services.spec.js?e850318c0e078721d0c01502db8db3ccb18edce7:936:49"],"assertionErrors":[]}]
Tue, 24 May 2016 12:10:37 GMT body-parser:json content-encoding "identity"
Tue, 24 May 2016 12:10:37 GMT body-parser:json read body
      ✖ creates multiple related models

Tue, 24 May 2016 12:10:37 GMT engine:ws received "42["result",{"id":"","description":"removes all related models","suite":["services","for models with hasAndBelongsToMany relations"],"success":false,"skipped":true,"time":0,"log":[],"assertionErrors":[]}]"
Tue, 24 May 2016 12:10:37 GMT engine:socket packet
Tue, 24 May 2016 12:10:37 GMT socket.io-parser decoded 2["result",{"id":"","description":"removes all related models","suite":["services","for models with hasAndBelongsToMany relations"],"success":false,"skipped":true,"time":0,"log":[],"assertionErrors":[]}] as {"type":2,"nsp":"/","data":["result",{"id":"","description":"removes all related models","suite":["services","for models with hasAndBelongsToMany relations"],"success":false,"skipped":true,"time":0,"log":[],"assertionErrors":[]}]}
Tue, 24 May 2016 12:10:37 GMT socket.io:socket got packet {"type":2,"nsp":"/","data":["result",{"id":"","description":"removes all related models","suite":["services","for models with hasAndBelongsToMany relations"],"success":false,"skipped":true,"time":0,"log":[],"assertionErrors":[]}]}
Tue, 24 May 2016 12:10:37 GMT socket.io:socket emitting event ["result",{"id":"","description":"removes all related models","suite":["services","for models with hasAndBelongsToMany relations"],"success":false,"skipped":true,"time":0,"log":[],"assertionErrors":[]}]
      ✖ removes all related models (skipped)

FAILED TESTS:
    MyModel $resource
      ✖ can create multiple new resources
        PhantomJS 2.1.1 (Linux 0.0.0)
      undefined is not a function (evaluating 'MyModel.createMany')
      /mnt/jenkins/workspace/loopback-sdk-angular/89d66e82/test.e2e/spec/services.spec.js:331:37

      ✖ has all methods including aliases
        PhantomJS 2.1.1 (Linux 0.0.0)
      expected [ Array(22) ] to be a superset of [ Array(13) ]
      assert@/mnt/jenkins/workspace/loopback-sdk-angular/89d66e82/node_modules/chai/chai.js:873:67
      /mnt/jenkins/workspace/loopback-sdk-angular/89d66e82/node_modules/chai/chai.js:2293:25
      /mnt/jenkins/workspace/loopback-sdk-angular/89d66e82/node_modules/chai/chai.js:3627:30
      /mnt/jenkins/workspace/loopback-sdk-angular/89d66e82/test.e2e/spec/services.spec.js:399:47

      ✖ creates multiple related models
        PhantomJS 2.1.1 (Linux 0.0.0)
      undefined is not an object (evaluating 'action.apply')
      createMany@http://localhost:3838/services?services::with%20authentication%20using%20built-in%20User%20model::adds%20auth-related%20methods%20to%20the%20User%20model:913:24
      /mnt/jenkins/workspace/loopback-sdk-angular/89d66e82/test.e2e/spec/services.spec.js:936:49

richardpringle · 2016-06-06T13:43:44Z

@slnode test please

bajtos · 2016-08-04T12:30:59Z

@richardpringle what's the status of this patch?

bajtos · 2016-08-04T14:37:44Z

@richardpringle I have rebased the patch on the current master and resolved merge conflicts. Run git fetch && git reset --hard origin/PUT-bulk-update to update your local branch.

bajtos · 2016-08-05T11:33:40Z

AFAICT, the downstream CI failures are not related to this change. I am going to land it.

richardpringle added the #review label Apr 5, 2016

superkhau reviewed Apr 5, 2016
View reviewed changes

superkhau assigned superkhau and richardpringle and unassigned superkhau Apr 5, 2016

richardpringle assigned bajtos and unassigned richardpringle Apr 5, 2016

bajtos reviewed Apr 6, 2016
View reviewed changes

richardpringle force-pushed the PUT-bulk-update branch from 76e2490 to 43c9972 Compare May 5, 2016 14:50

richardpringle assigned bajtos and unassigned richardpringle May 5, 2016

bajtos reviewed May 6, 2016
View reviewed changes

bajtos assigned richardpringle and unassigned bajtos May 6, 2016

richardpringle force-pushed the PUT-bulk-update branch from 43c9972 to af1d364 Compare May 6, 2016 14:49

richardpringle force-pushed the PUT-bulk-update branch from af1d364 to b66a4a0 Compare June 2, 2016 14:40

bajtos self-assigned this Jun 14, 2016

Amir-61 added the juggler-DAO label Jul 29, 2016

bajtos force-pushed the PUT-bulk-update branch from b66a4a0 to 537c8e2 Compare August 4, 2016 14:36

Disallow bulk updateOrCreate.

0b62dd9

bajtos force-pushed the PUT-bulk-update branch from 90267f3 to 0b62dd9 Compare August 5, 2016 09:54

bajtos merged commit e3b6b78 into master Aug 5, 2016

bajtos deleted the PUT-bulk-update branch August 5, 2016 11:33

bajtos removed the #review label Aug 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SEMVER-MAJOR] Disallow bulk updateOrCreate. #889

[SEMVER-MAJOR] Disallow bulk updateOrCreate. #889

richardpringle commented Apr 5, 2016 •

edited by bajtos

Loading

richardpringle commented Apr 5, 2016

superkhau Apr 5, 2016

superkhau commented Apr 5, 2016

richardpringle commented Apr 5, 2016

bajtos Apr 6, 2016

bajtos commented Apr 6, 2016

bajtos commented Apr 6, 2016

richardpringle commented Apr 6, 2016

richardpringle commented Apr 6, 2016

richardpringle commented May 5, 2016

superkhau commented May 5, 2016

bajtos May 6, 2016

bajtos commented May 6, 2016

richardpringle commented May 16, 2016

richardpringle commented May 16, 2016 •

edited

Loading

richardpringle commented May 17, 2016

bajtos commented May 24, 2016

richardpringle commented May 24, 2016

bajtos commented May 25, 2016

richardpringle commented May 25, 2016 •

edited

Loading

richardpringle commented Jun 6, 2016

bajtos commented Aug 4, 2016

bajtos commented Aug 4, 2016

bajtos commented Aug 5, 2016

[SEMVER-MAJOR] Disallow bulk updateOrCreate. #889

[SEMVER-MAJOR] Disallow bulk updateOrCreate. #889

Conversation

richardpringle commented Apr 5, 2016 • edited by bajtos Loading

richardpringle commented Apr 5, 2016

superkhau Apr 5, 2016

Choose a reason for hiding this comment

superkhau commented Apr 5, 2016

richardpringle commented Apr 5, 2016

bajtos Apr 6, 2016

Choose a reason for hiding this comment

bajtos commented Apr 6, 2016

bajtos commented Apr 6, 2016

richardpringle commented Apr 6, 2016

richardpringle commented Apr 6, 2016

richardpringle commented May 5, 2016

superkhau commented May 5, 2016

bajtos May 6, 2016

Choose a reason for hiding this comment

bajtos commented May 6, 2016

richardpringle commented May 16, 2016

richardpringle commented May 16, 2016 • edited Loading

richardpringle commented May 17, 2016

bajtos commented May 24, 2016

richardpringle commented May 24, 2016

bajtos commented May 25, 2016

richardpringle commented May 25, 2016 • edited Loading

richardpringle commented Jun 6, 2016

bajtos commented Aug 4, 2016

bajtos commented Aug 4, 2016

bajtos commented Aug 5, 2016

richardpringle commented Apr 5, 2016 •

edited by bajtos

Loading

richardpringle commented May 16, 2016 •

edited

Loading

richardpringle commented May 25, 2016 •

edited

Loading