Loopback: Update PUT/ multiple records of a model at a time

[pktippa]

We can POST multiple records in an array to a model by and array. But when we try to update multiple records of a model at a time with an array it is getting failed with error(tested connectors are in memory db, mongodb)

[richardpringle]

Hey @pktippa, is it possible to fork a loopback-sandbox and update the package.json file to use the same dependencies that you have when you ran into the bug?

It was working fine for me on several different versions of LoopBack.

[pktippa]

hi @richardpringle, the issue is reproducible in loopback-sandbox without any changes in package.json, please have a look at https://github.com/pk-strongloop/loopback-sandbox/tree/issue-2123

[richardpringle]

Thanks @pktippa, looks like this is the line that causes the issue: https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/dao.js#L471

[richardpringle]

This looks like a duplicate of #1101 (or at least related)

[richardpringle]

@pktippa, as per @bajtos' comment, we are going to modify the code to produce a meaningful error but we will not support PUT with an array of records to update at this time.

[pktippa]

@richardpringle thanks for the update.

[richardpringle]

@bajtos,
I found that the issue is in the create method. If an array is passed to updateOrCreate dao.js#L471 causes the data to be passed directly to create (which makes sense since we don't actually support a bulk update). From there, for each duplicate entry, the create method will send an error which eventually gets concatenated into an array.

The rest-adapter error handler is not built to handle an array of errors. To change that might be difficult, since the format of the error will change based on the connector. For instance, the memory connector will pass back an array of javascript errors, while mongo will pass back and array of mongo error objects.

Please advise.

[richardpringle]

Input (both records already exist):

Category.create([
  {
    "name": "string",
    "toto": "string",
    "id": 1
  },
  {
    "name": "string",
    "toto": "string",
    "id": 2   
  }
],
function(err, inst) {
  console.log(err);
  console.log('\n');
  console.log(inst);
});

memory:

[ [Error: Duplicate entry for Category.id],
  [Error: Duplicate entry for Category.id] ]

[ { name: 'string', toto: 'string', id: 1 },
  { name: 'string', toto: 'string', id: 2 } ]

MongoDB

[ { [MongoError: E11000 duplicate key error collection: test.Category index: _id_ dup key: { : 1 }]
    name: 'MongoError',
    message: 'E11000 duplicate key error collection: test.Category index: _id_ dup key: { : 1 }',
    driver: true,
    code: 11000,
    index: 0,
    errmsg: 'E11000 duplicate key error collection: test.Category index: _id_ dup key: { : 1 }',
    getOperation: [Function],
    toJSON: [Function],
    toString: [Function] },
  { [MongoError: E11000 duplicate key error collection: test.Category index: _id_ dup key: { : 1 }]
    name: 'MongoError',
    message: 'E11000 duplicate key error collection: test.Category index: _id_ dup key: { : 1 }',
    driver: true,
    code: 11000,
    index: 0,
    errmsg: 'E11000 duplicate key error collection: test.Category index: _id_ dup key: { : 1 }',
    getOperation: [Function],
    toJSON: [Function],
    toString: [Function] } ]

[ { name: 'string', toto: 'string', id: 1 },
  { name: 'string', toto: 'string', id: 2 } ]

[bajtos]

From reading the code (lib/dao#L471-L474), I believe the original intention was to short-circuit to create when data does not contain id property.

I am proposing to implement the same behaviour for bulk-update case, even if it means that certain requests that would pass with the current implementation will start failing in the new version:

if (Array.isArray(data) {
  var isBulkCreate = data.all(function(item) { 
    var id = getIdValue(self, item);
    return id === undefined || id === null;
  });
  if (isBulkCreate) {
    return self.create(data, options, cb);
  } else {
    return cb(new Error('updateOrCreate does not support bulk mode'));
  }
}

This way the user should receive more helpful message when PUTting an array of objects with ids, while still be able to perform bulk-create through this endpoint to preserve backwards compatibility.

The rest-adapter error handler is not built to handle an array of errors. To change that might be difficult, since the format of the error will change based on the connector. For instance, the memory connector will pass back an array of javascript errors, while mongo will pass back and array of mongo error objects.

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?

In general, implementing bulk operations correctly is difficult. IMO, a bulk operation should be atomic, i.e. either all or no changes are made. That way the operation either succeeds and an array of created objects is returned back, or the operation fails and a single error can be returned back (possibly listing sub-errors for items that could not be created). The current implementation is rather suboptimal. It may create some of the records while other records are not created. As a result, it's difficult to determine what to return from the function. What is an error state and what is considered as success.

In order to get atomic batch-create and batch-update, we need to implement this feature in connectors too. Unfortunately, some databases like MongoDB do not support atomic update of multiple documents. Thus even if we do implement this, it won't work everywhere.

That's my thoughts on this issues. I am afraid I don't see any clear and obvious way how to fix the problem reported here :(

[richardpringle]

@bajtos Which branch should I make the change on?

[bajtos]

@richardpringle start with master, and once the PR is landed, back-port the fix to 2.x too.

[richardpringle]

@bajtos,

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.

[bajtos]

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.

I see. That makes my original proposal not very good, as the request can still fail with an error that is not very helpful.

I am proposing to fix error handling in strong-remoting too (in addition to changing juggler as proposed earlier).

Something along these lines can be added to defaultHandler:

  if (Array.isArray(err)) {
      var details = err.map(function(it) { 
        // drop sensitive information like stack traces
        // use the same approach as we do for a single error      
      });
      err = new Error('Failed with multiple errors, see `details` for more information.');
      err.details = details;
  }

While we are fixing this, perhaps we could broaden the condition if (typeof err === 'string') (source) to handle other cases like numbers and dates, basically anything that's not an object.

@raymondfeng @superkhau @ritch Thoughts?

@loay please take this into account while working on the production-grade error handler, to make sure it correctly handles errors that are not Error instances.

[richardpringle]

+1

Sorry I hadn't read this when I made my comments in the PR

[superkhau]

Something along these lines can be added to defaultHandler:

+1

While we are fixing this, perhaps we could broaden the condition if (typeof err === 'string') (source) to handle other cases like numbers and dates, basically anything that's not an object.

+1

[loay]

In addition to the necessity of clarifying the error message instead of getting an unknown error, I found out something which I think might be related.

when we do an upsert operation, the data argument must be an object, unlike the create operation that supports both object and array. I believe when we do an array of objects using a create operation, then we find a duplicate instance, it switches from create to upsert, where the array is not supported, hence we get an error message.

In my opinion, letting upsert support array as well as object should fix the problem.

[bajtos]

Status update: strong-remoting has been fixed to handle an array of errors in the response, see strongloop/strong-remoting#292.

The remaining work is to disallow bulk updateOrCreate in v3.0, see https://github.com/strongloop/loopback-datasource-juggler/pull/889

Once that's landed, this issue can be closed as done, at least I think so.

The feature request to properly implement bulk create/update is being discussed here: #1275

[bajtos]

strongloop/loopback-datasource-juggler#889 has been landed, I am closing this issue as done.

[lehni]

@bajtos you linked to #1275 as the issue where bulk create / update is being discussed, but the issue is only concerned with create / insert, not update. We're very much interested in being able to bulk update multiple instances. Is support for this planned in LB4?

[bajtos]

We're very much interested in being able to bulk update multiple instances. Is support for this planned in LB4?

I don't think so. So far, the plan for LB4 is to re-use existing loopback-datasource-juggler and connector implementations.

[lewie6]

@bajtos the last comment on this was a while ago, so is bulk update going to be in v4?

[bajtos]

Not in 4.0 GA, but we may add support for bulk update in later versions.