Creating a reported property called `update` breaks the twin update function

[peolivei2]

According to SDK samples. to update a reported property, you should call the twin.properties.reported.update function. However, if I create a reported property called update, this overwrites the function and twin updates no longer work. See repro below:

const Mqtt = require('azure-iot-device-mqtt').Mqtt;
const Client = require('azure-iot-device').Client;
const setTimeout = require('timers').setTimeout;

const connectionStr = '<redacted>';

const client = Client.fromConnectionString(connectionStr, Mqtt);

client.open(err => {
    if (err) {
        console.log('[LOG] Open failed');
        return;
    }

    console.log('[LOG] Client open. Getting twin...');

    client.getTwin((twinErr, deviceTwin) => {
        console.log('[LOG] getTwin callback', twinErr, deviceTwin);

        deviceTwin.properties.reported.update({ update: 'abc' }, function(err) {
            if (err) {
                throw err;
            };

            console.log('[LOG] Twin state reported');

            deviceTwin.properties.reported.update({ someprop: 'abc' }, function(err) {
                if (err) {
                    console.log('[LOG] Error updating reported property');
                    throw err;
                };

                console.log('[LOG] Twin state reported');
            });
        });
    });

    setTimeout(() => {
        client.close(() => {
            console.log('[LOG] Client closed. Getting twin...');
        });
    }, 10000);
});

Which outputs:

[LOG] Client open. Getting twin...
[LOG] getTwin callback null Twin {
  properties:
   { reported: { update: [Function: update], '$version': 1 },
     desired: { fanSpeed: [Object], '$version': 1 } } }
[LOG] Twin state reported
C:\Users\peolivei\Downloads\sdktst\tst.js:27
            deviceTwin.properties.reported.update({ someprop: 'abc' }, function(err) {
                                           ^

TypeError: deviceTwin.properties.reported.update is not a function
    at C:\Users\peolivei\Downloads\sdktst\tst.js:27:44
    at C:\Users\peolivei\Downloads\sdktst\node_modules\azure-iot-common\lib\retry_operation.js:54:21
    at C:\Users\peolivei\Downloads\sdktst\node_modules\azure-iot-device\lib\twin.js:122:21
    at MqttTwinClient._onResponseMessage (C:\Users\peolivei\Downloads\sdktst\node_modules\azure-iot-device-mqtt\lib\mqtt_twin_client.js:251:17)
    at MqttTwinClient._onMqttMessage (C:\Users\peolivei\Downloads\sdktst\node_modules\azure-iot-device-mqtt\lib\mqtt_twin_client.js:214:18)    at MqttBase.emit (events.js:194:15)
    at C:\Users\peolivei\Downloads\sdktst\node_modules\azure-iot-mqtt-base\lib\mqtt_base.js:248:23
    at process._tickCallback (internal/process/next_tick.js:61:11)

[pierreca]

hah. that's a bit of an embarrassing situation. we could:

• prevent you from creating such a property on the client side and keep the current API
• add a new API that would allow you to support that and maybe deprecate the current API

suggestions?

[peolivei2]

According to the docs, $ is not allowed in twin property names (https://docs.microsoft.com/en-us/azure/iot-hub/iot-hub-devguide-device-twins). Maybe we can duplicate the update function as $update and keep both, so we don't break people who already use update and want to use the latest version of the SDK. That way can both support this and don't break current applications.

[YoDaMa]

Commenting adding context to this:

• After discussions, there are issues that could be foreseen by duplicating the update function as $update. It would have to be something that would would definitely be a new feature, so at least a minor version bump.
• The way of changing the device twin in this way anyways might be falling behind. With the public preview of Digital Twins, using that API would be the preferred use-case for manipulating the device twin.
• Disallowing update as a twin property is not the solution, since that would change the device twin spec.

---

Some code analysis:

The present functionality of adding the update property to the device twin will mean that the code to update will be overwritten if someone creates an update property in the twin, which is within the spec.

from twin.ts

  private _clearCachedProperties(): void {
    const self = this;
    this.properties = {
      reported : {
        update : function(state: any, done: (err?: null) => void): void {
          self._updateReportedProperties(state, done);
        }
      },
      desired : {
      }
    };
  }

I think the best solution that we could do is in the future, particularly with Digital Twins / PnP on the way, will be to not touch this api even though it's broken, and rely on the new API. In the future, we should never implement an API like this, that uses functions on the JSON object itself, since the object is mutable and thus could be updated in any way that follows the twins spec.

A future API change in DT/PnP would look something like this:

client.updateReportedProperties(updateObject, callback)

[jebrando]

@peolivei2 First of all thanks for bringing this up. We discussed this issue and I would agree that it is highly unfortunate naming that we have implemented, but now with this being release it would be a breaking change so changing this is a nonstarter. We are going to close this issue and if you have additional concerns please ping me and we can continue to talk about it.

[az-iot-builder-01]

@peolivei2, @YoDaMa, @jebrando, thank you for your contribution to our open-sourced project! Please help us improve by filling out this 2-minute customer satisfaction survey