Add convenience method for acknowledging writable property update requests

[abhipsaMisra]

Fix for #2065 , #2097 , #2100

As highlighted in the above feature requests, constructing the correct IWritablePropertyResponse for writable property update requests was not easily discoverable.
This PR aims to make the flow more convenient.

• On subscribing to SubscribeToWritablePropertyUpdateRequestsAsync, a client application will receive the writable property update requests as a collection of WritableClientProperty values.
• WritableClientProperty has a convenience method to "ack" these property update requests.
• The "ack'ed" response is formatted in the format as required by IoT plug and play.

You can now do:

await deviceClient
    .SubscribeToWritablePropertyUpdateRequestsAsync(
        async (writableProperties, userContext) =>
        {
            var writablePropertyAcks = new ClientPropertyCollection();
            foreach (KeyValuePair<string, object> writableProperty in writableProperties)
            {
                if (writableProperty.Value is WritableClientProperty rootLevelWritableClientProperty)
                {
                    string rootLevelPropertyName = writableProperty.Key;
                    writablePropertyAcks.Add(rootLevelPropertyName, rootLevelWritableClientProperty.AcknowledgeWith(CommonClientResponseCodes.OK));
                }
                else if (writableProperty.Value is IDictionary<string, object> componentProperties)
                {
                    string componentName = writableProperty.Key;
                    foreach (KeyValuePair<string, object> componentProperty in componentProperties)
                    {
                        if (componentProperty.Value is WritableClientProperty componentLevelwritableClientProperty)
                        {
                            string componentLevelPropertyName = componentProperty.Key;
                            writablePropertyAcks.AddComponentProperty(componentName, componentLevelPropertyName, componentLevelwritableClientProperty.AcknowledgeWith(CommonClientResponseCodes.OK));
                        }
                    }
                }
            }

            await deviceClient.UpdateClientPropertiesAsync(writablePropertyAcks).ConfigureAwait(false);
        },
        null,
        cts.Token)
    .ConfigureAwait(false);

Note: In earlier discussions we had contemplated updating:

public Task SubscribeToWritablePropertyUpdateRequestsAsync(Func<ClientPropertyCollection, object, Task> callback, object userContext, CancellationToken cancellationToken = default);

to

public Task RespondToWritablePropertyUpdateRequestsAsync(Func<ClientPropertyCollection, object, Task<ClientPropertyCollection>> callback, object userContext, CancellationToken cancellationToken = default);

However, we decided against doing this because this would associate each writable property request with a response (reported property update) API call. Instead, we are formatting the response that the user can then add to a ClientPropertyCollection and issue the update call themselves.

[abhipsaMisra]

For writable property update responses reported from the client, you should be able to retrieve the property as:

• generic IWritablePropertyResponse
• your implemented IWritablePropertyResponse
• the .Value part of the IWritablePropertyResponse

[jamdavi]

Seeing as how people might see WritableClientProperty as a potential value. Do we also want to support that scenario?

The reasoning is, in the SubscribeTo... methods we return a collection of these types and customers might believe this is an acceptable way to address the property.

In the case where they get the full twin doc people might want to compare.

I imagine someone doing this.

var props = client.GetClientProperties();
props.TryGetValue("reportedTemperature", out WritableClientProperty thelastrequest);
props.TryGetValue("reportedTemperature", out double mylastresponse);

if (thelastrequest.Value != mylastresponse)
{
    // 205 is a "Reset Content" code
    mylastrequest.AcknowledgeWith(205, "Values were not in sync");
}

[abhipsaMisra]

That's a valid concern, esp for the reset scenario that you've described above.
My concern is that since WritableClientProperty is something we create (as opposed to an IWritablePropertyResponse that service mandates the response to be in), I'm not sure if we should be returning that for a GetClientProperties() flow, which is supposed to give you the current state of the client's properties. In my mind, updating the response of Subscribe... flow was ok since that gave you the writable props, in an ack-able format, since you were expected to ack it.
I don't have a strong argument for either case though.

[abhipsaMisra]

Thinking of this a bit more, a writable property is always meant to be ack'ed, no matter how we get it, i.e. through a request notification or via the whole "twin". So I think returning a WritableClientProperty in GetClientProperties() flow makes sense.

[abhipsaMisra]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[drwill-ms]

Why not use a data-driven test method instead?

[abhipsaMisra]

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

[drwill-ms]

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

[abhipsaMisra]

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.

[drwill-ms]

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?

[abhipsaMisra]

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.

[abhipsaMisra]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[abhipsaMisra]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[BertKleewein]

Value

This assumes that you're acknowledging the exact value that was sent by the service. What if you want to respond with a value that is slightly off? For exmple, the service sends a writable property request to set the temperature to 72.3 degrees, and the device app returns a WritablePropertyResponse that says "we set it to 72.2 degrees" because the hardware had to round down. (I assume this is a valid scenario, but I'm assuming and you know what that means :))

[abhipsaMisra]

That's fair. We've included the following doc comment in the ack function which calls out how to achieve this:

azure-iot-sdk-csharp/iothub/device/src/WritableClientProperty.cs

Lines 38 to 41 in 7cc63d2

	/// This writable property update response will contain the property value and version supplied in the writable property update request.
	/// If you would like to construct your own writable property update response with custom value and version number, you can
	/// create an instance of <see cref="PayloadSerializer.CreateWritablePropertyResponse(object, int, long, string)"/>.
	/// See <see href="https://docs.microsoft.com/azure/iot-develop/concepts-convention#writable-properties"/> for more details.