azure-iot-explorer icon indicating copy to clipboard operation
azure-iot-explorer copied to clipboard

[Enhancement] Sending a PnP Command needs to be blocked properly if validation fails

Open jspaith opened this issue 3 years ago • 8 comments

On IoT Explorer 0.14.3.0, Windows 10.

To Reproduce Steps to reproduce the behavior:

  1. Connect a device with the PnP sample for temperature controller. Though in practice anything with a DTDLv2 with a date/time I think should repo. Note I'm using embedded C SDK but I don't think that's issue.
  2. Navigate to the device=>IoT Plug and Play Components=>thermostat1=>Commands.
  3. Enter in the date, but only enter the month/day/year. Leave the hour/minute/second stuff blank.
  4. Click send-command. On the device, an empty JSON document will be received which isn't legal for DTDLv2 model.
    Note that if you fill out the hh/mm/ss stuff then non-empty JSON is happily sent.

Expected behavior Either IoT Explorer should complain if it expects the hh/mm/ss, or it should have some default so that valid JSON is always sent.

See below for screenshot shows my repro right before I submit.

repo-screen

jspaith avatar May 05 '21 17:05 jspaith

Thanks John.

Yes, IoT Explorer should not try to invoke any command or property if the input parameters are not validated.

However I see a different behavior, when I dont set the time, I got a timeout and no bytes are received on the device.

image

rido-min avatar May 05 '21 17:05 rido-min

@rido-min - which device SDK are you using? I know that some of them don't play well when a 0 length body JSON is sent to it.

@digimaun did a lot of investigation around this - for instance this on C side.

jspaith avatar May 06 '21 18:05 jspaith

I'm using https://mqtt.rido.dev :) this is not processing the method payload. (see the code here )

rido-min avatar May 06 '21 18:05 rido-min

Is it a SDK specific issue? I looks like Explorer is not sending any data if time is not set from Rido's comments above and this is the expected behavior to me. Please feel free to reopen if I misunderstood.

YingXue avatar Jul 26 '22 17:07 YingXue

I cannot repro, seems the dateTime picker does not allow to select without time.

rido-min avatar Jul 26 '22 19:07 rido-min

@rido-min / @YingXue - I just installed the latest IoT Explorer (0.14.10.0) and the issue still persists.

To be clear, the issue is that if in IoT Explorer with the dateTime picker I only select the date but NOT the time, then IoT Explorer will end up sending empty JSON to the device. There's no date even sent here, never mind time.

Looking at what the C SDK shows (though this shouldn't be SDK specific) on the callback. See the payloadLength below.

0:000> kc
 # Call Site
00 pnp_temperature_controller!PnP_TempControlComponent_CommandCallback
01 pnp_temperature_controller!invoke_command_callback
02 pnp_temperature_controller!IoTHubClientCore_LL_DeviceMethodComplete
03 pnp_temperature_controller!processDeviceMethodNotification
04 pnp_temperature_controller!mqttNotificationCallback
05 pnp_temperature_controller!ProcessPublishMessage
06 pnp_temperature_controller!recvCompleteCallback

0:000> ?? commandRequest
struct IOTHUB_CLIENT_COMMAND_REQUEST_TAG * 0x000000ba`624feea8
   +0x000 structVersion    : 0n1
   +0x008 componentName    : 0x0000029f`ac230130  "thermostat1"
   +0x010 commandName      : 0x0000029f`ac218edc  "getMaxMinReport"
   +0x018 payload          : (null) 
   +0x020 payloadLength    : 0

jspaith avatar Jul 26 '22 20:07 jspaith

If you can repro along steps provided, could you please re-activate? Thanks

jspaith avatar Jul 26 '22 20:07 jspaith

I see what you mean now. Since dtdl is saying it's a datetime, we do need users to enter both date and the time so the validation would pass and send the json body. There is slient validation in place, and sending empty body if validation fails is expected. We didn't implement blocking logic in this case.

YingXue avatar Jul 26 '22 20:07 YingXue

Fix rolled out: https://github.com/Azure/azure-iot-explorer/releases/tag/v0.14.14 Closing

YingXue avatar Sep 06 '22 21:09 YingXue

Just verified that the fix works as promised. Thanks @YingXue !

jspaith avatar Sep 06 '22 21:09 jspaith