Loopback: Request: Return a Promise for PersistedModel.create() batch mode.

[jbcpollak]

PersistedModel.create() doesn't return a Promise in batch-create model. See https://github.com/strongloop/loopback/issues/418#issuecomment-74691763 for details.

If you try, you'll get an error like this:

TypeError: XXX.app.models.YYY.create(...).then is not a function
    at /.../zzz.js:42:6

It would be good to find a solution so this mode can support promises as well. If it cannot be done, I would suggest breaking this mode out into a separate batchCreate() method so the lack of Promise support can be documented.

[jannyHou]

hey @jbcpollak, thanks for your suggestion, when you test batch-create, are you trying to create multiple instances like this test case?
https://github.com/strongloop/loopback-datasource-juggler/blob/master/test/manipulation.test.js#L218-L224

[jannyHou]

@jbcpollak could you confirm the scenario that you are testing? From my understanding batch create is like:

module.exports = function(app) {
  var myUser = app.models.myuser;

  // sample data
  var mySamples = [{
    "username": "me",
    "password": "hah",
    "email": "[email protected]"
  }, {
    "username": "mee",
    "password": "hah",
    "email": "[email protected]"
  }];

  // batch create
  myUser.create(mySamples)
    .then(function(sample) {
        console.log(sample);
    });
};

But it works fine for me.

[jbcpollak]

@jannyHou sorry for the delay, it took a while to get a good usecase.

I have something similar to what you showed, but it does not work (I'm using ES6 syntax, but that shouldn't be the problem):

    let jobs = [
        {
            "id": "f5b4cbd0-4639-4736-a35c-610bc76b11b3",
            "externalId": "30715796",
            "lines": [
                {
                    // proprietary
                },
                {
                    // proprietary
                }
            ]
        }
    ]

    let Job = app.models.Job;
    let jobsP = Job.create(jobs);

    jobsP.then(jobs => {
        console.log('num jobs' + jobs.length);
        };

And I see:

TypeError: jobsP.then is not a function
    at assertAllocation (test/server/jobTest.spec.js:83:8)
    at Context.<anonymous> (test/server/jobTest.spec.js:116:5)
    at wrappedPromise.propagateAslWrapper [as __asl_wrapper] (node_modules/async-listener/index.js:415:23)
    at node_modules/async-listener/index.js:450:32

Here are my loopback version specifications in package.json:

    "loopback": "^2.27.0",
    "loopback-boot": "^2.17.0",
    "loopback-component-explorer": "^2.4.0",
    "loopback-connector-remote": "^1.2.0",
    "loopback-connector-rest": "^1.11.0",
    "loopback-datasource-juggler": "^2.45.2",

[jannyHou]

@jbcpollak thanks for your code, to be honest I am not very familiar with ES6 syntax.... I find some explanation about combining loopback and es6: https://groups.google.com/forum/#!topic/loopbackjs/x6ApjcsbQ30
Does this help?

[jannyHou]

@jbcpollak The code still works for me:

module.exports = function(app) {
  "use strict";
  let Job = app.models.myuser;

  let jobs = [{
   "username": "me",
   "password": "hah",
   "email": "[email protected]"
  }, {
   "username": "mee",
   "password": "hah",
   "email": "[email protected]"
  }];

  //let Job = app.models.Job;
  let jobsP = Job.create(jobs);

  jobsP.then(jobs => {
    console.log('num jobs' + jobs.length);
  });
};

Did you try to declare "use strict";?

Result:

$ node .
Web server listening at: http://localhost:3000
Browse your REST API at http://localhost:3000/explorer
num jobs2

[bajtos]

PersistedModel.create() doesn't return a Promise in batch-create model.

This is intentional. In the batch mode, the callback is invoked with two arrays - list of errors in the first argument, list of instances created in the second argument. In other words, batch create can partially succeed/fail at at same time. This is difficult to map into promises, where an operation either succeeds or fails completely.

The recommended solution is to call Promise.all, here is a code snippet to illustrate the idea:

Promise.all(jobsP.map(Job.create)).then(jobs => {
  console.log('num jobs' + jobs.length);
});

See the current implementation for more details.

[zhaojunmo]

Thank you, @bajtos , this problem confused me for a long time.
However, we have a new problem here: Why does @jannyHou 's code work (which should not) ?
Thanks again for your time.

[jbcpollak]

Why not make batch create mode use Promise.all() itself?

It seems reasonable that when in batch create mode create would call

return Promise.all(jobsP.map(Job.create));

On Fri, Apr 29, 2016, 5:00 AM Max Mok [email protected] wrote:

Thank you, @bajtos https://github.com/bajtos , this problem confused me
for a long time.
However, we have a new problem here: Why does @jannyHou
https://github.com/jannyHou 's code work (which should not) ?
Thanks again for your time.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/strongloop/loopback/issues/2164#issuecomment-215663314

[bajtos]

Why not make batch create mode use Promise.all() itself?

Let's move that discussion to https://github.com/strongloop/loopback/issues/1275 please.

However, we have a new problem here: Why does @jannyHou 's code work (which should not) ?
Thanks again for your time.

Ha, this is interesting. I believe this happens when the datasource is not connected yet, see this code:

  var connectionPromise = stillConnecting(this.getDataSource(), this, arguments);
  if (connectionPromise) {
    return connectionPromise;
  }

When I introduce setTimeout delay into @jannyHou's code, it stops working and report same error as you are observing in your app.

Obviously, stillConnecting is not clever enough to determine that bulk create should not return promise and thus it creates a dummy callback backed by a promise that is then returned from create.

I think we need to change that code into something like following:

  var connectionPromise = stillConnecting(this.getDataSource(), this, arguments);
  if (connectionPromise) {
    return Array.isArray(data) ? 
      undefined : // bulk create does not support promise mode, thus don't return any promise
      connectionPromise;
  }

@jbcpollak would you like to submit a pull request with that fix? Just add a unit-test to verify the new behaviour.

[doublemarked]

This is intentional. In the batch mode, the callback is invoked with two arrays - list of errors in the first argument, list of instances created in the second argument. In other words, batch create can partially succeed/fail at at same time. This is difficult to map into promises, where an operation either succeeds or fails completely.

@bajtos, if I can guess at your rationale here - because there is no transaction covering the entire batch, the create function should return partial success/failure to accurately inform the caller of what has occurred in the datastore.

FYI: I discovered today that batch creates via relation (like hasMany) do not appear to respect this. For instance, modelInstance.relatedstuff.create([a,b,c]) will return a promise that resolves into the array of created instances. I have not tested a failure, but glancing at the code it seems that a partial failure will result in a promise rejection.

I don't know what to suggest. I came to this thread annoyed that batch create doesn't return a promise, but I understand the trouble w/ partial success and can't think of a better solution. Perhaps the batch creates via relation should also require a callback.

[bajtos]

because there is no transaction covering the entire batch, the create function should return partial success/failure to accurately inform the caller of what has occurred in the datastore.

Yes, that would be ideal!

Perhaps the batch creates via relation should also require a callback.

I agree with your proposal. Unfortunately it is a breaking change IMO, thus it has to wait until the next major version :( Let's open a new issue to keep track of it: https://github.com/strongloop/loopback-datasource-juggler/issues/1217

Considering that it was our intentional decision to not return a promise from a batch create() call, I am closing this issue as rejected.

[bajtos]

I am closing this issue as rejected.

Let me correct that - the request for a promise-enabled bulk create is a valid one, let's discuss it in https://github.com/strongloop/loopback/issues/1275.

[BoLaMN]

I believe this should be handled at the connector as most drivers allow for inserting an array in one database call but instead the dao layer does one by one

[bajtos]

FYI: https://github.com/strongloop/loopback-datasource-juggler/pull/1380 modified create() function to return Promise in batch mode too.