WebThingsIO / gateway-addon-python

Python bindings for developing add-ons for WebThings Gateway
Mozilla Public License 2.0
16 stars 10 forks source link

Python and Node addon library have different behavior when setProperty failed. #17

Closed sogaani closed 6 years ago

sogaani commented 6 years ago

I'd prefer to return fails message like action. Because, I want to use setProperty for checking that the device is online.

I figured out this issue on google actions App review. They have a test case which checks that the google actions App return the device state.

The test case is run like so:

  1. Pair device(s) to Google Home
  2. Ask, “OK Google, is the on?”
    • Expected response if device is on: “The is on.”
    • Expected response if device is off: “The is off.”
  3. Remove the device from power (meaning that we simply unplug the test device from all power sources, without removing the device’s pairing information)
  4. After a delay, ask “OK Google, is the on?” Expected response within 30 minutes – 1 hour: “It looks like the isn’t available right now.”

Thing does not have any state like online or unplugged. So, I'm using this issue check the device is online.

mrstegeman commented 6 years ago

gateway-addon-python assumes that setting a property value won't fail. Maybe that's invalid, though. The relevant bits of code are:

sogaani commented 6 years ago

Related wot script spec https://w3c.github.io/wot-scripting-api/#the-writeproperty-method If we follow wot spec, maybe python addon should raise exception on failing setProperty?

mrstegeman commented 6 years ago

We don't really follow that spec at all, so I wouldn't go by that. However, I'm not opposed to wrapping the set_value() call in a try/except. It would match the node library, then.

What are you suggesting we do on reject/exception, though? The node library sends back a property change notification.

sogaani commented 6 years ago

My suggestion is that If we catch reject/exception, addon-proxy send a error message "setProperty is failed" to the Gateway, and the Gateway return a error response for 'PUT /things/:thingId/:properties/:propertyName'.

mrstegeman commented 6 years ago

18 will give us the same behavior as the node library. We can build on that to send back an error message.

I kind of like having the property change notification, that way the UI gets updated back to the current value.

sogaani commented 6 years ago

18 looks good!

I kind of like having the property change notification, that way the UI gets updated back to the current value.

Does this comment mean that send propertyChange message on websocket and response errors on REST API when setProperty via REST API is failed?

mrstegeman commented 6 years ago

I think it would be appropriate to send back the current value as part of an error message in both cases, rather than having a property notification AND an error message. Unfortunately, our WoT spec doesn't yet define what to do when errors occur.