Loopback: Method updateAttributes of PersistedModel is not returning Promise

[htmlauthor]

Steps to reproduce

1. Clone and run https://github.com/htmlauthor/loopback-sandbox/commit/c4fefa6cf32584d0248453309337cbf4ac637070
2. Open /explorer
3. Under model "thing" find endpoint PATCH /things/testUpdateAttributes/{idToUpdate}
4. Use 1 for idToUpdate and some payload in "updatedData", click "Try it out!"
5. View console where application is running you will see this output:

Entering testUpdateAttributes
shouldBePromise undefined
Leaving testUpdateAttributes
updatedThing:
thing {__cachedRelations: Object, __data: Object, __dataSource: undefined, __strict: false, __persisted: true, …}

Link to reproduction sandbox

https://github.com/htmlauthor/loopback-sandbox

Expected result

As you can see "Leaving testUpdateAttributes" was out before "updatedThing: " which means await shouldBePromise; statement is ignored and not waited for. Also shouldBePromise value is undefined where it should be Promise if updateAttributes would return Promise.

Additional information

$ node -e 'console.log(process.platform, process.arch, process.versions.node)'
darwin x64 8.9.4

$ npm ls --prod --depth 0 | grep loopback
[email protected] /Users/justinaskabasinskas/Projects/loopback-sandbox
├── [email protected]
├── [email protected]
├── [email protected]

[bajtos]

I am cross-posting the relevant remote method here:

  Thing.testUpdateAttributes = async (idToUpdate, dataToUpdate, callback) => {
    console.log('Entering testUpdateAttributes');
    let thing = await app.models.thing.findById(idToUpdate);
    const shouldBePromise = thing.updateAttributes(dataToUpdate, (error, updatedThing) => {
      if (error) {
        console.error(error);
      } else {
        console.log('updatedThing: ', updatedThing);
      }
    });

    console.log('shouldBePromise', shouldBePromise);

    await shouldBePromise;

    console.log('Leaving testUpdateAttributes');
};

[bajtos]

TL;DR: Please don't mix promises and callbacks. Pick one async flow control style and stick to it.

Longer version:

LoopBack methods do not return a promise when a callback argument was provided. Only when there was no callback argument, we create an internal callback that's resolving/rejecting a new Promise object.

Conceptually:

PersistedModel.prototype.updateAttributes = function(data, callback) {
  if (!callback) {
    callback = createPromiseCallback();
  }

  this.dataSource.connector.patchAttributes(this.id, data, callback);

  return callback.promise;
}

Below is a fixed version of your remote method:

  Thing.testUpdateAttributes = async (idToUpdate, dataToUpdate) => {
      console.log('Entering testUpdateAttributes');
      const thing = await app.models.thing.findById(idToUpdate);
      try {
        const shouldBePromise = thing.updateAttributes(dataToUpdate);
        console.log('shouldBePromise', shouldBePromise);
        const updatedThing = await shouldBePromise;
        console.log('updatedThing: ', updatedThing);
      } catch (error) {
        console.error(error);
      }
      console.log('Leaving testUpdateAttributes');
    };

The steps to reproduce now print the following test to console (I am running on Node.js 10.1.0):

Entering testUpdateAttributes
shouldBePromise Promise {
  _bitField: 0,
  _fulfillmentHandler0: undefined,
  _rejectionHandler0: undefined,
  _promise0: undefined,
  _receiver0: undefined }
updatedThing:  { name: 'foobar',
  size: 0,
  creationTime: 2018-05-02T17:43:18.168Z,
  id: 1 }
Leaving testUpdateAttributes

I cannot tell what's your original intention. If I was writing a similar method myself, I would write it as follows:

  Thing.testUpdateAttributes = async(idToUpdate, dataToUpdate) => {
    const thing = await app.models.thing.findById(idToUpdate);
    const updatedThing = await thing.updateAttributes(dataToUpdate);
    console.log('updatedThing: ', updatedThing);
    return updatedThing;
  };

Two things to notice:

• There is no need to handle errors ourselves, just pass them back to LoopBack. By default, it will print the error to console and return a safe error response back to the client. In callback style, this is achieved by forwarding the error via return callback(error).
• Update/patch methods usually return some information back. Your current implementation returns 204 No Content. To change the HTTP response body from a remote async method, just return the object you want to send back.

[bajtos]

I am closing this issue as answered.