Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add convenience method for acknowledging writable property update requests #2157

Merged
merged 10 commits into from
Aug 31, 2021
Merged

Add convenience method for acknowledging writable property update requests #2157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dbc664f
feat(iot-device): Add convenience method for acknowledging writable p…
abhipsaMisra Aug 25, 2021
c67d2b0
comments
abhipsaMisra Aug 26, 2021
08b913c
update API doc
abhipsaMisra Aug 30, 2021
5cdb7f7
simplify writable client property
abhipsaMisra Aug 30, 2021
0b0618d
Revert "simplify writable client property"
abhipsaMisra Aug 30, 2021
1367cd8
update tests
abhipsaMisra Aug 30, 2021
6f4f9ee
minor
abhipsaMisra Aug 30, 2021
997b331
refactor out common code
abhipsaMisra Aug 30, 2021
2ecd6af
simplify usings
abhipsaMisra Aug 30, 2021
7cc63d2
fix
abhipsaMisra Aug 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions configure_tls_protocol_version_and_ciphers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ For more information, check out this article about [best practices with .NET and
Also follow the instructions on how to [enable and disable ciphers].

[.NET Framework 4.5.1 is no longer supported]: https://devblogs.microsoft.com/dotnet/support-ending-for-the-net-framework-4-4-5-and-4-5-1/
[TLS registry settings]: https://docs.microsoft.com/en-us/windows-server/security/tls/tls-registry-settings
[best practices with .NEt and TLS]: https://docs.microsoft.com/en-us/dotnet/framework/network-programming/tls
[enable and disable ciphers]: https://support.microsoft.com/en-us/help/245030/how-to-restrict-the-use-of-certain-cryptographic-algorithms-and-protoc
[TLS registry settings]: https://docs.microsoft.com/windows-server/security/tls/tls-registry-settings
[best practices with .NEt and TLS]: https://docs.microsoft.com/dotnet/framework/network-programming/tls
[enable and disable ciphers]: https://support.microsoft.com/help/245030/how-to-restrict-the-use-of-certain-cryptographic-algorithms-and-protoc

### Linux Instructions

Expand Down
112 changes: 112 additions & 0 deletions e2e/test/iothub/properties/PropertiesE2ETests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,42 @@ await Properties_ServiceSetsWritablePropertyAndDeviceReceivesEventAsync(
.ConfigureAwait(false);
}

[LoggedTestMethod]
public async Task Properties_ServiceSetsWritablePropertyAndDeviceReceivesEventAndResponds_Mqtt()
Copy link
Contributor

@drwill-ms drwill-ms Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use a data-driven test method instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are run sequentially, so I wanted to avoid the increase in test run time

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How long do the tests take to run? Is the cost to readability and supportability worth the time gain?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a substantial increase. I remember the test run time decreased from 3+hours to about 1.5hours when we introduced class-level parallel execution. I'd say restricting parallel execution for each set of tests might bring up the test time closer to 2 hours.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that it could sometimes dramatically increase the time. I'm more curious about this instance, for these tests.

I think we could have a rule that says, if the test takes longer than 10 seconds to run then split it out, otherwise use data-driven. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Yes, we could do that, I don't mind.
Afaik NUnit doesn't have the same limitation (parameterized tests can be executed in parallel), so moving our existing tests to NUnit be a good long term change to have.

{
await Properties_ServiceSetsWritablePropertyAndDeviceReceivesEventAndRespondsAsync(
Client.TransportType.Mqtt_Tcp_Only,
Guid.NewGuid().ToString())
.ConfigureAwait(false);
}

[LoggedTestMethod]
public async Task Properties_ServiceSetsWritablePropertyAndDeviceReceivesEventAndResponds_MqttWs()
{
await Properties_ServiceSetsWritablePropertyAndDeviceReceivesEventAndRespondsAsync(
Client.TransportType.Mqtt_WebSocket_Only,
Guid.NewGuid().ToString())
.ConfigureAwait(false);
}

[LoggedTestMethod]
public async Task Properties_ServiceSetsWritablePropertyMapAndDeviceReceivesEventAndResponds_Mqtt()
{
await Properties_ServiceSetsWritablePropertyAndDeviceReceivesEventAndRespondsAsync(
Client.TransportType.Mqtt_Tcp_Only,
s_mapOfPropertyValues)
.ConfigureAwait(false);
}

[LoggedTestMethod]
public async Task Properties_ServiceSetsWritablePropertyMapAndDeviceReceivesEventAndResponds_MqttWs()
{
await Properties_ServiceSetsWritablePropertyAndDeviceReceivesEventAndRespondsAsync(
Client.TransportType.Mqtt_WebSocket_Only,
s_mapOfPropertyValues)
.ConfigureAwait(false);
}

[LoggedTestMethod]
public async Task Properties_ServiceSetsWritablePropertyAndDeviceReceivesItOnNextGet_Mqtt()
{
Expand Down Expand Up @@ -305,6 +341,82 @@ await Task
await deviceClient.CloseAsync().ConfigureAwait(false);
}

private async Task Properties_ServiceSetsWritablePropertyAndDeviceReceivesEventAndRespondsAsync<T>(Client.TransportType transport, T propValue)
{
using var cts = new CancellationTokenSource(s_maxWaitTimeForCallback);
string propName = Guid.NewGuid().ToString();

Logger.Trace($"{nameof(Properties_ServiceSetsWritablePropertyAndDeviceReceivesEventAsync)}: name={propName}, value={propValue}");

TestDevice testDevice = await TestDevice.GetTestDeviceAsync(Logger, _devicePrefix).ConfigureAwait(false);
using var deviceClient = DeviceClient.CreateFromConnectionString(testDevice.ConnectionString, transport);

var writablePropertyCallbackSemaphore = new SemaphoreSlim(0, 1);
await deviceClient
.SubscribeToWritablePropertyUpdateRequestsAsync(
async (writableProperties, userContext) =>
{
try
{
bool isPropertyPresent = writableProperties.TryGetValue(propName, out T propertyFromCollection);

isPropertyPresent.Should().BeTrue();
propertyFromCollection.Should().BeEquivalentTo(propValue);
userContext.Should().BeNull();

var writablePropertyAcks = new ClientPropertyCollection();
foreach (KeyValuePair<string, object> writableProperty in writableProperties)
{
if (writableProperty.Value is WritableClientProperty writableClientProperty)
{
writablePropertyAcks.Add(writableProperty.Key, writableClientProperty.AcknowledgeWith(CommonClientResponseCodes.OK));
}
}

await deviceClient.UpdateClientPropertiesAsync(writablePropertyAcks).ConfigureAwait(false);
}
finally
{
writablePropertyCallbackSemaphore.Release();
}
},
null,
cts.Token)
.ConfigureAwait(false);

using var testDeviceCallbackHandler = new TestDeviceCallbackHandler(deviceClient, testDevice, Logger);

await Task
.WhenAll(
RegistryManagerUpdateWritablePropertyAsync(testDevice.Id, propName, propValue),
writablePropertyCallbackSemaphore.WaitAsync(cts.Token))
.ConfigureAwait(false);

// Validate the updated properties from the device-client
ClientProperties clientProperties = await deviceClient.GetClientPropertiesAsync().ConfigureAwait(false);

// Validate that the writable property update request was received
bool isWritablePropertyRequestPresent = clientProperties.WritablePropertyRequests.TryGetValue(propName, out T writablePropertyRequest);
isWritablePropertyRequestPresent.Should().BeTrue();
writablePropertyRequest.Should().BeEquivalentTo(propValue);

// Validate that the writable property update request was acknowledged

bool isWritablePropertyAckPresent = clientProperties.ReportedFromClient.TryGetValue(propName, out IWritablePropertyResponse writablePropertyAck);
isWritablePropertyAckPresent.Should().BeTrue();
// TryGetValue doesn't have nested deserialization, so we'll have to deserialize the retrieved value
deviceClient.PayloadConvention.PayloadSerializer.ConvertFromObject<T>(writablePropertyAck.Value).Should().BeEquivalentTo(propValue);

bool isWritablePropertyAckPresentSpecific = clientProperties.ReportedFromClient.TryGetValue(propName, out NewtonsoftJsonWritablePropertyResponse writablePropertyAckNewtonSoft);
isWritablePropertyAckPresentSpecific.Should().BeTrue();
// TryGetValue doesn't have nested deserialization, so we'll have to deserialize the retrieved value
deviceClient.PayloadConvention.PayloadSerializer.ConvertFromObject<T>(writablePropertyAckNewtonSoft.Value).Should().BeEquivalentTo(propValue);

bool isWritablePropertyAckPresentAsValue = clientProperties.ReportedFromClient.TryGetValue(propName, out T writablePropertyAckValue);
isWritablePropertyAckPresentAsValue.Should().BeTrue();
writablePropertyAckValue.Should().BeEquivalentTo(propValue);
}

private async Task Properties_ServiceSetsWritablePropertyAndDeviceReceivesItOnNextGetAsync(Client.TransportType transport)
{
string propName = Guid.NewGuid().ToString();
Expand Down
118 changes: 118 additions & 0 deletions e2e/test/iothub/properties/PropertiesWithComponentsE2ETests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,42 @@ await PropertiesWithComponents_ClientHandlesRejectionInvalidPropertyNameAsync(
.ConfigureAwait(false);
}

[LoggedTestMethod]
public async Task PropertiesWithComponents_ServiceSetsWritablePropertyAndDeviceReceivesEventAndResponds_Mqtt()
{
await PropertiesWithComponents_ServiceSetsWritablePropertyAndDeviceReceivesEventAndRespondsAsync(
Client.TransportType.Mqtt_Tcp_Only,
Guid.NewGuid().ToString())
.ConfigureAwait(false);
}

[LoggedTestMethod]
public async Task PropertiesWithComponents_ServiceSetsWritablePropertyAndDeviceReceivesEventAndResponds_MqttWs()
{
await PropertiesWithComponents_ServiceSetsWritablePropertyAndDeviceReceivesEventAndRespondsAsync(
Client.TransportType.Mqtt_WebSocket_Only,
Guid.NewGuid().ToString())
.ConfigureAwait(false);
}

[LoggedTestMethod]
public async Task PropertiesWithComponents_ServiceSetsWritablePropertyMapAndDeviceReceivesEventAndResponds_Mqtt()
{
await PropertiesWithComponents_ServiceSetsWritablePropertyAndDeviceReceivesEventAndRespondsAsync(
Client.TransportType.Mqtt_Tcp_Only,
s_mapOfPropertyValues)
.ConfigureAwait(false);
}

[LoggedTestMethod]
public async Task PropertiesWithComponents_ServiceSetsWritablePropertyMapAndDeviceReceivesEventAndResponds_MqttWs()
{
await PropertiesWithComponents_ServiceSetsWritablePropertyAndDeviceReceivesEventAndRespondsAsync(
Client.TransportType.Mqtt_WebSocket_Only,
s_mapOfPropertyValues)
.ConfigureAwait(false);
}

private async Task PropertiesWithComponents_DeviceSetsPropertyAndGetsItBackSingleDeviceAsync(Client.TransportType transport)
{
TestDevice testDevice = await TestDevice.GetTestDeviceAsync(Logger, _devicePrefix).ConfigureAwait(false);
Expand Down Expand Up @@ -312,6 +348,88 @@ await Task
await deviceClient.CloseAsync().ConfigureAwait(false);
}

private async Task PropertiesWithComponents_ServiceSetsWritablePropertyAndDeviceReceivesEventAndRespondsAsync<T>(Client.TransportType transport, T propValue)
{
using var cts = new CancellationTokenSource(s_maxWaitTimeForCallback);
string propName = Guid.NewGuid().ToString();

Logger.Trace($"{nameof(PropertiesWithComponents_ServiceSetsWritablePropertyAndDeviceReceivesEventAndRespondsAsync)}: name={propName}, value={propValue}");

TestDevice testDevice = await TestDevice.GetTestDeviceAsync(Logger, _devicePrefix).ConfigureAwait(false);
using var deviceClient = DeviceClient.CreateFromConnectionString(testDevice.ConnectionString, transport);

var writablePropertyCallbackSemaphore = new SemaphoreSlim(0, 1);
await deviceClient
.SubscribeToWritablePropertyUpdateRequestsAsync(
async (writableProperties, userContext) =>
{
try
{
bool isPropertyPresent = writableProperties.TryGetValue(ComponentName, propName, out T propertyFromCollection);

isPropertyPresent.Should().BeTrue();
propertyFromCollection.Should().BeEquivalentTo(propValue);
userContext.Should().BeNull();

var writablePropertyAcks = new ClientPropertyCollection();
foreach (KeyValuePair<string, object> writableProperty in writableProperties)
{
if (writableProperty.Value is IDictionary<string, object> componentProperties)
{
foreach (KeyValuePair<string, object> componentProperty in componentProperties)
{
if (componentProperty.Value is WritableClientProperty writableClientProperty)
{
writablePropertyAcks.AddComponentProperty(writableProperty.Key, componentProperty.Key, writableClientProperty.AcknowledgeWith(CommonClientResponseCodes.OK));
}
}
}
}

await deviceClient.UpdateClientPropertiesAsync(writablePropertyAcks).ConfigureAwait(false);
}
finally
{
writablePropertyCallbackSemaphore.Release();
}
},
null,
cts.Token)
.ConfigureAwait(false);

using var testDeviceCallbackHandler = new TestDeviceCallbackHandler(deviceClient, testDevice, Logger);

await Task
.WhenAll(
RegistryManagerUpdateWritablePropertyAsync(testDevice.Id, ComponentName, propName, propValue),
writablePropertyCallbackSemaphore.WaitAsync(cts.Token))
.ConfigureAwait(false);

// Validate the updated properties from the device-client
ClientProperties clientProperties = await deviceClient.GetClientPropertiesAsync().ConfigureAwait(false);

// Validate that the writable property update request was received
bool isWritablePropertyRequestPresent = clientProperties.WritablePropertyRequests.TryGetValue(ComponentName, propName, out T writablePropertyRequest);
isWritablePropertyRequestPresent.Should().BeTrue();
writablePropertyRequest.Should().BeEquivalentTo(propValue);

// Validate that the writable property update request was acknowledged

bool isWritablePropertyAckPresent = clientProperties.ReportedFromClient.TryGetValue(ComponentName, propName, out IWritablePropertyResponse writablePropertyAck);
isWritablePropertyAckPresent.Should().BeTrue();
// TryGetValue doesn't have nested deserialization, so we'll have to deserialize the retrieved value
deviceClient.PayloadConvention.PayloadSerializer.ConvertFromObject<T>(writablePropertyAck.Value).Should().BeEquivalentTo(propValue);

bool isWritablePropertyAckPresentSpecific = clientProperties.ReportedFromClient.TryGetValue(ComponentName, propName, out NewtonsoftJsonWritablePropertyResponse writablePropertyAckNewtonSoft);
isWritablePropertyAckPresentSpecific.Should().BeTrue();
// TryGetValue doesn't have nested deserialization, so we'll have to deserialize the retrieved value
deviceClient.PayloadConvention.PayloadSerializer.ConvertFromObject<T>(writablePropertyAckNewtonSoft.Value).Should().BeEquivalentTo(propValue);

bool isWritablePropertyAckPresentAsValue = clientProperties.ReportedFromClient.TryGetValue(ComponentName, propName, out T writablePropertyAckValue);
isWritablePropertyAckPresentAsValue.Should().BeTrue();
writablePropertyAckValue.Should().BeEquivalentTo(propValue);
}

private async Task PropertiesWithComponents_ServiceSetsWritablePropertyAndDeviceReceivesItOnNextGetAsync(Client.TransportType transport)
{
string propName = Guid.NewGuid().ToString();
Expand Down
15 changes: 10 additions & 5 deletions iothub/device/devdoc/Convention-based operations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ public abstract class PayloadCollection : IEnumerable, IEnumerable<KeyValuePair<
public IEnumerator<KeyValuePair<string, object>> GetEnumerator();
public virtual byte[] GetPayloadObjectBytes();
public virtual string GetSerializedString();
protected void SetCollection(PayloadCollection payloadCollection);
IEnumerator System.Collections.IEnumerable.GetEnumerator();
public bool TryGetValue<T>(string key, out T value);
}
Expand Down Expand Up @@ -127,15 +126,16 @@ public Task<ClientPropertiesUpdateResponse> UpdateClientPropertiesAsync(ClientPr
/// <param name="callback">The global call back to handle all writable property updates.</param>
/// <param name="userContext">Generic parameter to be interpreted by the client code.</param>
/// <param name="cancellationToken">A cancellation token to cancel the operation.</param>
public Task SubscribeToWritablePropertiesEventAsync(Func<ClientPropertyCollection, object, Task> callback, object userContext, CancellationToken cancellationToken = default);
public Task SubscribeToWritablePropertyUpdateRequestsAsync(Func<ClientPropertyCollection, object, Task> callback, object userContext, CancellationToken cancellationToken = default);
```

#### All related types

```csharp
public class ClientProperties : ClientPropertyCollection {
public class ClientProperties {
public ClientProperties();
public ClientPropertyCollection Writable { get; private set; }
public ClientPropertyCollection ReportedFromClient { get; private set; }
public ClientPropertyCollection WritablePropertyRequests { get; private set; }
}

public class ClientPropertyCollection : PayloadCollection {
Expand All @@ -153,6 +153,12 @@ public class ClientPropertyCollection : PayloadCollection {
public virtual bool TryGetValue<T>(string componentName, string propertyName, out T propertyValue);
}

public class WritableClientProperty {
public object Value { get; internal set; }
public long Version { get; internal set; }
public IWritablePropertyResponse AcknowledgeWith(int statusCode, string description = null);
}

public interface IWritablePropertyResponse {
int AckCode { get; set; }
string AckDescription { get; set; }
Expand All @@ -169,7 +175,6 @@ public sealed class NewtonsoftJsonWritablePropertyResponse : IWritablePropertyRe
}

public class ClientPropertiesUpdateResponse {
public ClientPropertiesUpdateResponse();
public string RequestId { get; internal set; }
public long Version { get; internal set; }
}
Expand Down
4 changes: 4 additions & 0 deletions iothub/device/src/ClientPropertiesUpdateResponse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ namespace Microsoft.Azure.Devices.Client
/// </summary>
public class ClientPropertiesUpdateResponse
{
internal ClientPropertiesUpdateResponse()
{
}

/// <summary>
/// The request Id that is associated with the <see cref="InternalClient.UpdateClientPropertiesAsync(ClientPropertyCollection, System.Threading.CancellationToken)"/> operation.
/// </summary>
Expand Down
Loading