Azure / azure-iot-pcs-remote-monitoring-dotnet

Azure IoT .NET solution for Remote Monitoring
MIT License
162 stars 95 forks source link

Remove dependency on obsolete RegistryManager.GetDevicesAsync method #152

Closed murdockcrc closed 5 years ago

murdockcrc commented 5 years ago

Type of issue

Description

IoTHubManager.Services depends on an obsolete method GetDevicesAsync(int). I propose to open a PR to remove this dependency, and to refactor the affected tests on class DeviceTests.cs

I have a fork which already fixes this, give me a heads up and I can submit a PR to this project.

ppathan commented 5 years ago

@murdockcrc Thanks for your contribution. Please submit a PR and we will take a look.

murdockcrc commented 5 years ago

@ppathan I have noticed that the DeviceServiceModel returns the device's authentication keys by populating the property Authentication.

Since removing the method GetDevicesAsync means we don't access the devices in bulk from the registry any longer (and instead rely in the twin), I can populate most of the properties except the device's authentication keys.

Is it strictly necessary to retrieve the devices' keys in bulk? If yes, I am not sure what is the most performant way to retrieve the keys in bulk, since the twin does not include them. I know that you display the keys in the device's detail pane, could it be that we want to ask for the keys when the user clicks on a specific device in the Devices screen, or what approach would you suggest to follow here?

elvinm commented 5 years ago

The right way to move forward is that for bulk query calls we simply wouldn't retrieve the Authentication details because 1) They aren't needed in this use case (they are needed only when retrieving details for a particular device i.e. GetAsync(string id). 2) There wouldn't be a way to do it that performs optimally each device in the query result set would need a subsequent call to GetDeviceAsync.

murdockcrc commented 5 years ago

@elvinm , yes, we would need to query every deviceId individually, which will not be performant for large batches of devices. Removing the Authentication information would technically be a breaking change, since perhaps somebody already integrated with that API call and depends on that data. However, if you don't have anything against this approach, I will modify the response so that DeviceServiceModel.Authentication includes an empty instance of AuthenticationMechanismServiceModel.

murdockcrc commented 5 years ago

Opened PR

ppathan commented 5 years ago

Fixed