Skip to content

Commit

Permalink
Stop using JObject.Parse for json string in TwinCollection (Azure#3113)
Browse files Browse the repository at this point in the history
  • Loading branch information
@brycewang-microsoft
brycewang-microsoft authored Feb 16, 2023
1 parent 1380560 commit 9ba754e
Show file tree
Hide file tree
Showing 10 changed files with 142 additions and 85 deletions.
1 change: 1 addition & 0 deletions common/src/service/HttpClientHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public HttpClientHelper(
_defaultErrorMapping = defaultErrorMapping;
_defaultOperationTimeout = timeout;

// Specify the JsonSerializerSettings. Check JsonSerializerSettingsInitializer for more details.
JsonConvert.DefaultSettings = JsonSerializerSettingsInitializer.GetJsonSerializerSettingsDelegate();

// We need two types of HttpClients, one with our default operation timeout, and one without. The one without will rely on
Expand Down
59 changes: 10 additions & 49 deletions e2e/test/iothub/twin/TwinE2ETests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Azure.Devices.Client;
Expand All @@ -12,7 +11,6 @@
using Microsoft.Azure.Devices.Shared;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;

namespace Microsoft.Azure.Devices.E2ETests.Twins
{
Expand All @@ -37,15 +35,16 @@ public class TwinE2ETests : E2EMsTestBase
}
};

// ISO 8601 date-formatted string with trailing zeros in the microseconds portion.
// This is to verify the Newtonsoft.Json known issue is worked around in the SDK.
// See https://github.com/JamesNK/Newtonsoft.Json/issues/1511 for more details about the known issue.
private const string DateTimeValue = "2023-01-31T10:37:08.4599400";

private static readonly TestDateTime s_dateTimeProperty = new()
{
Iso8601String = new DateTimeOffset(638107582284599400, TimeSpan.FromHours(1)).ToString("o", CultureInfo.InvariantCulture)
Iso8601String = DateTimeValue,
};

// ISO 8601 date time string with trailing zeros in the microseconds portion. This is to verify the Newtonsoft.Json known issue is worked around in the SDK.
// See https://github.com/JamesNK/Newtonsoft.Json/issues/1511 for more details about the known issue.
private static readonly string s_iso8601DateTimeString = "2023-01-31T10:37:08.4599400+01:00";

[TestMethod]
[Timeout(TestTimeoutMilliseconds)]
public async Task Twin_DeviceSetsReportedPropertyAndGetsItBack_Mqtt()
Expand Down Expand Up @@ -326,6 +325,8 @@ await Twin_ServiceSetsDesiredPropertyAndDeviceReceivesItOnNextGetAsync(
.ConfigureAwait(false);
}

// This is mainly for testing serialization/deserialization behavior which is independent of the
// transport protocol used, so we are not covering cases for other protocols than Mqtt here.
[TestMethod]
[Timeout(TestTimeoutMilliseconds)]
public async Task Twin_ServiceSetsDesiredPropertyAndDeviceReceivesItOnNextGet_DateTimeProperties_Mqtt()
Expand All @@ -335,33 +336,6 @@ await Twin_ServiceSetsDesiredPropertyAndDeviceReceivesItOnNextGetDateTimePropert
.ConfigureAwait(false);
}

[TestMethod]
[Timeout(TestTimeoutMilliseconds)]
public async Task Twin_ServiceSetsDesiredPropertyAndDeviceReceivesItOnNextGet_DateTimeProperties_MqttWs()
{
await Twin_ServiceSetsDesiredPropertyAndDeviceReceivesItOnNextGetDateTimePropertiesAsync(
Client.TransportType.Mqtt_WebSocket_Only)
.ConfigureAwait(false);
}

[TestMethod]
[Timeout(TestTimeoutMilliseconds)]
public async Task Twin_ServiceSetsDesiredPropertyAndDeviceReceivesItOnNextGet_DateTimeProperties_Amqp()
{
await Twin_ServiceSetsDesiredPropertyAndDeviceReceivesItOnNextGetDateTimePropertiesAsync(
Client.TransportType.Amqp_Tcp_Only)
.ConfigureAwait(false);
}

[TestMethod]
[Timeout(TestTimeoutMilliseconds)]
public async Task Twin_ServiceSetsDesiredPropertyAndDeviceReceivesItOnNextGet_DateTimeProperties_AmqpWs()
{
await Twin_ServiceSetsDesiredPropertyAndDeviceReceivesItOnNextGetDateTimePropertiesAsync(
Client.TransportType.Amqp_WebSocket_Only)
.ConfigureAwait(false);
}

[TestMethod]
[Timeout(TestTimeoutMilliseconds)]
public async Task Twin_DeviceSetsReportedPropertyAndServiceReceivesIt_Mqtt()
Expand Down Expand Up @@ -713,24 +687,11 @@ private async Task Twin_ServiceSetsDesiredPropertyAndDeviceReceivesItOnNextGetDa

var twinPatch = new Twin();

// Here is an example to create a TwinCollection with json object if the users use date-formatted
// string and don't want to drop the trailing zeros in the microseconds portion while parsing the
// json properties. The Newtonsoft.Json serializer settings added in SDK apply to serial-/deserialization
// during the client operations. Therefore, to ensure the datetime has the trailing zeros and this
// test can verify the returned peoperties not dropping them, manually set up "DateParseHandling.None"
// while creating a new TwinCollection object as the twin property.
JObject jobjectProperty = JsonConvert.DeserializeObject<JObject>(
jsonString,
new JsonSerializerSettings
{
DateParseHandling = DateParseHandling.None
});

twinPatch.Properties.Desired = new TwinCollection(jobjectProperty, null);
twinPatch.Properties.Desired = new TwinCollection(jsonString);
await registryManager.UpdateTwinAsync(testDevice.Id, twinPatch, "*").ConfigureAwait(false);

Twin deviceTwin = await deviceClient.GetTwinAsync().ConfigureAwait(false);
Assert.AreEqual<string>(deviceTwin.Properties.Desired["Iso8601String"].ToString(), s_iso8601DateTimeString);
Assert.AreEqual<string>(deviceTwin.Properties.Desired["Iso8601String"].ToString(), DateTimeValue);

await deviceClient.CloseAsync().ConfigureAwait(false);
await registryManager.CloseAsync().ConfigureAwait(false);
Expand Down
1 change: 1 addition & 0 deletions iothub/device/src/InternalClient.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ public InternalClient(
if (Logging.IsEnabled)
Logging.Enter(this, transportSettings, pipelineBuilder, nameof(InternalClient) + "_ctor");

// Specify the JsonSerializerSettings. Check JsonSerializerSettingsInitializer for more details.
JsonConvert.DefaultSettings = JsonSerializerSettingsInitializer.GetJsonSerializerSettingsDelegate();

TlsVersions.Instance.SetLegacyAcceptableVersions();
Expand Down
1 change: 0 additions & 1 deletion iothub/device/src/Transport/Amqp/AmqpUnit.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Azure.Devices.Client.Exceptions;
using Microsoft.Azure.Devices.Client.Extensions;
using Microsoft.Azure.Devices.Client.Transport.Amqp;
using Microsoft.Azure.Devices.Shared;

Expand Down
71 changes: 38 additions & 33 deletions iothub/device/tests/Transport/Mqtt/MqttTransportHandlerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,17 @@ public class MqttTransportHandlerTests
{
private const string DummyConnectionString = "HostName=127.0.0.1;SharedAccessKeyName=AllAccessKey;DeviceId=FakeDevice;SharedAccessKey=dGVzdFN0cmluZzE=";
private const string DummyModuleConnectionString = "HostName=127.0.0.1;SharedAccessKeyName=AllAccessKey;DeviceId=FakeDevice;ModuleId=FakeModule;SharedAccessKey=dGVzdFN0cmluZzE=";
private const string fakeMethodResponseBody = "{ \"foo\" : \"bar\" }";
private const string methodPostTopicFilter = "$iothub/methods/POST/#";
private const string twinPatchDesiredTopicFilter = "$iothub/twin/PATCH/properties/desired/#";
private const string twinPatchReportedTopicPrefix = "$iothub/twin/PATCH/properties/reported/";
private const string twinGetTopicPrefix = "$iothub/twin/GET/?$rid=";
private const int statusSuccess = 200;
private const int statusFailure = 400;
private const string fakeResponseId = "fakeResponseId";
private static readonly TimeSpan ReceiveTimeoutBuffer = TimeSpan.FromSeconds(5);
private const string FakeMethodResponseBody = "{ \"foo\" : \"bar\" }";
private const string MethodPostTopicFilter = "$iothub/methods/POST/#";
private const string TwinPatchDesiredTopicFilter = "$iothub/twin/PATCH/properties/desired/#";
private const string TwinPatchReportedTopicPrefix = "$iothub/twin/PATCH/properties/reported/";
private const string TwinGetTopicPrefix = "$iothub/twin/GET/?$rid=";
private const string TwinJsonString1 = "{\"foo\":\"bar\"}";
private const string TwinJsonString2 = "{\"Iso8601String\":\"2023-01-31T10:37:08.4599400\"}";
private const int StatusSuccess = 200;
private const int StatusFailure = 400;
private const string FakeResponseId = "FakeResponseId";
private static readonly TimeSpan s_receiveTimeoutBuffer = TimeSpan.FromSeconds(5);
private delegate bool MessageMatcher(Message msg);

[TestMethod]
Expand Down Expand Up @@ -149,7 +151,7 @@ public async Task MqttTransportHandlerEnableMethodsAsyncSubscribesSuccessfully()
// assert
await channel
.Received()
.WriteAsync(Arg.Is<SubscribePacket>(msg => msg.Requests[0].TopicFilter.Equals(methodPostTopicFilter)))
.WriteAsync(Arg.Is<SubscribePacket>(msg => msg.Requests[0].TopicFilter.Equals(MethodPostTopicFilter)))
.ConfigureAwait(false);
}

Expand All @@ -162,7 +164,7 @@ public async Task MqttTransportHandlerEnableMethodsAsyncSubscribeTimesOut()
IChannel channel;
var transport = CreateTransportHandlerWithMockChannel(out channel);
channel
.WriteAsync(Arg.Is<SubscribePacket>(msg => msg.Requests[0].TopicFilter.Equals(methodPostTopicFilter)))
.WriteAsync(Arg.Is<SubscribePacket>(msg => msg.Requests[0].TopicFilter.Equals(MethodPostTopicFilter)))
.Returns(x => { throw new TimeoutException(); });

// act & assert
Expand All @@ -188,7 +190,7 @@ public async Task MqttTransportHandlerDisableMethodsAsyncUnsubscribesSuccessfull
// assert
await channel
.Received()
.WriteAsync(Arg.Is<UnsubscribePacket>(msg => System.Linq.Enumerable.ElementAt(msg.TopicFilters, 0).Equals(methodPostTopicFilter))).ConfigureAwait(false);
.WriteAsync(Arg.Is<UnsubscribePacket>(msg => System.Linq.Enumerable.ElementAt(msg.TopicFilters, 0).Equals(MethodPostTopicFilter))).ConfigureAwait(false);
}

// Tests_SRS_CSHARP_MQTT_TRANSPORT_28_003: `DisableMethodsAsync` shall return failure if the unsubscription fails.
Expand All @@ -199,7 +201,7 @@ public async Task MqttTransportHandlerDisablemethodsAsyncUnsubscribeTimesOut()
// arrange
var transport = CreateTransportHandlerWithMockChannel(out IChannel channel);
channel
.WriteAsync(Arg.Is<SubscribePacket>(msg => msg.Requests[0].TopicFilter.Equals(methodPostTopicFilter)))
.WriteAsync(Arg.Is<SubscribePacket>(msg => msg.Requests[0].TopicFilter.Equals(MethodPostTopicFilter)))
.Returns(x => { throw new TimeoutException(); });

// act & assert
Expand Down Expand Up @@ -293,12 +295,12 @@ public async Task MqttTransportHandler_DisableEventReceiveAsync_UnsubscribeTimes
public async Task MqttTransportHandler_SendMethodResponseAsync_SendsMessage()
{
// arrange
var responseBytes = Encoding.UTF8.GetBytes(fakeMethodResponseBody);
var responseBytes = Encoding.UTF8.GetBytes(FakeMethodResponseBody);
var transport = CreateTransportHandlerWithMockChannel(out IChannel channel);
var response = new MethodResponseInternal(responseBytes, fakeResponseId, statusSuccess);
var response = new MethodResponseInternal(responseBytes, FakeResponseId, StatusSuccess);
MessageMatcher matches = (msg) =>
{
return StringComparer.InvariantCulture.Equals(msg.MqttTopicName, $"$iothub/methods/res/{statusSuccess}/?$rid={fakeResponseId}");
return StringComparer.InvariantCulture.Equals(msg.MqttTopicName, $"$iothub/methods/res/{StatusSuccess}/?$rid={FakeResponseId}");
};

// act
Expand Down Expand Up @@ -329,7 +331,7 @@ public async Task MqttTransportHandlerEnableTwinPatchAsyncSubscribes()
// assert
await channel
.Received()
.WriteAsync(Arg.Is<SubscribePacket>(msg => msg.Requests[0].TopicFilter.Equals(twinPatchDesiredTopicFilter))).ConfigureAwait(false);
.WriteAsync(Arg.Is<SubscribePacket>(msg => msg.Requests[0].TopicFilter.Equals(TwinPatchDesiredTopicFilter))).ConfigureAwait(false);
}

// Tests_SRS_CSHARP_MQTT_TRANSPORT_18_012: `EnableTwinPatchAsync` shall return failure if the subscription request fails.
Expand All @@ -340,7 +342,7 @@ public async Task MqttTransportHandlerEnableTwinPatchAsyncTimesOut()
// arrange
var transport = CreateTransportHandlerWithMockChannel(out IChannel channel);
channel
.WriteAsync(Arg.Is<SubscribePacket>(msg => msg.Requests[0].TopicFilter.Equals(twinPatchDesiredTopicFilter)))
.WriteAsync(Arg.Is<SubscribePacket>(msg => msg.Requests[0].TopicFilter.Equals(TwinPatchDesiredTopicFilter)))
.Returns(x => { throw new TimeoutException(); });

// act & assert
Expand All @@ -357,19 +359,21 @@ public async Task MqttTransportHandlerEnableTwinPatchAsyncTimesOut()
// Tests_SRS_CSHARP_MQTT_TRANSPORT_18_034: `SendTwinGetAsync` shall shall open the transport if this method is called when the transport is not open.
// Tests_SRS_CSHARP_MQTT_TRANSPORT_18_021: If the response contains a success code, `SendTwinGetAsync` shall return success to the caller
[TestMethod]
public async Task MqttTransportHandlerSendTwinGetAsyncHappyPath()
[DataRow("foo", TwinJsonString1)]
[DataRow("Iso8601String", TwinJsonString2)]
public async Task MqttTransportHandlerSendTwinGetAsyncHappyPath(string propName, string twinJson)
{
// arrange
var transport = CreateTransportHandlerWithMockChannel(out IChannel channel);
var twin = new Twin();
twin.Properties.Desired["foo"] = "bar";
twin.Properties.Desired = new TwinCollection(twinJson);
var twinByteStream = System.Text.Encoding.UTF8.GetBytes(JsonConvert.SerializeObject(twin.Properties));
channel
.WriteAndFlushAsync(Arg.Is<Message>(msg => msg.MqttTopicName.StartsWith(twinGetTopicPrefix)))
.WriteAndFlushAsync(Arg.Is<Message>(msg => msg.MqttTopicName.StartsWith(TwinGetTopicPrefix)))
.Returns(msg =>
{
var response = new Message(twinByteStream);
response.MqttTopicName = GetResponseTopic(msg.Arg<Message>().MqttTopicName, statusSuccess);
response.MqttTopicName = GetResponseTopic(msg.Arg<Message>().MqttTopicName, StatusSuccess);
transport.OnMessageReceived(response);
return TaskHelpers.CompletedTask;
});
Expand All @@ -379,7 +383,7 @@ public async Task MqttTransportHandlerSendTwinGetAsyncHappyPath()
var twinReturned = await transport.SendTwinGetAsync(CancellationToken.None).ConfigureAwait(false);

// assert
Assert.AreEqual<string>(twin.Properties.Desired["foo"].ToString(), twinReturned.Properties.Desired["foo"].ToString());
Assert.AreEqual<string>(twin.Properties.Desired[propName].ToString(), twinReturned.Properties.Desired[propName].ToString());
}

// Tests_SRS_CSHARP_MQTT_TRANSPORT_18_019: If the response is failed, `SendTwinGetAsync` shall return that failure to the caller.
Expand All @@ -389,11 +393,11 @@ public async Task MqttTransportHandlerSendTwinGetAsyncReturnsFailure()
// arrange
var transport = CreateTransportHandlerWithMockChannel(out IChannel channel);

channel.WriteAndFlushAsync(Arg.Is<Message>(msg => msg.MqttTopicName.StartsWith(twinGetTopicPrefix)))
channel.WriteAndFlushAsync(Arg.Is<Message>(msg => msg.MqttTopicName.StartsWith(TwinGetTopicPrefix)))
.Returns(msg =>
{
var response = new Message();
response.MqttTopicName = GetResponseTopic(msg.Arg<Message>().MqttTopicName, statusFailure);
response.MqttTopicName = GetResponseTopic(msg.Arg<Message>().MqttTopicName, StatusFailure);
transport.OnMessageReceived(response);
return TaskHelpers.CompletedTask;
});
Expand Down Expand Up @@ -426,23 +430,24 @@ public async Task MqttTransportHandlerSendTwinGetAsyncTimesOut()
// Tests_SRS_CSHARP_MQTT_TRANSPORT_18_030: If the response contains a success code, `SendTwinPatchAsync` shall return success to the caller.
// Tests_SRS_CSHARP_MQTT_TRANSPORT_18_035: `SendTwinPatchAsync` shall shall open the transport if this method is called when the transport is not open.
[TestMethod]
public async Task MqttTransportHandlerSendTwinPatchAsyncHappyPath()
[DataRow(TwinJsonString1)]
[DataRow(TwinJsonString2)]
public async Task MqttTransportHandlerSendTwinPatchAsyncHappyPath(string twinJson)
{
// arrange
var transport = CreateTransportHandlerWithMockChannel(out IChannel channel);
var props = new TwinCollection();
var props = new TwinCollection(twinJson);
string receivedBody = null;
props["foo"] = "bar";
channel
.WriteAndFlushAsync(Arg.Is<Message>(msg => msg.MqttTopicName.StartsWith(twinPatchReportedTopicPrefix)))
.WriteAndFlushAsync(Arg.Is<Message>(msg => msg.MqttTopicName.StartsWith(TwinPatchReportedTopicPrefix)))
.Returns(msg =>
{
var request = msg.Arg<Message>();
StreamReader reader = new StreamReader(request.GetBodyStream(), System.Text.Encoding.UTF8);
receivedBody = reader.ReadToEnd();

var response = new Message();
response.MqttTopicName = GetResponseTopic(request.MqttTopicName, statusSuccess);
response.MqttTopicName = GetResponseTopic(request.MqttTopicName, StatusSuccess);
transport.OnMessageReceived(response);

return TaskHelpers.CompletedTask;
Expand All @@ -465,12 +470,12 @@ public async Task MqttTransportHandlerSendTwinPatchAsyncReturnsFailure()
var transport = CreateTransportHandlerWithMockChannel(out IChannel channel);
var props = new TwinCollection();
channel
.WriteAndFlushAsync(Arg.Is<Message>(msg => msg.MqttTopicName.StartsWith(twinPatchReportedTopicPrefix)))
.WriteAndFlushAsync(Arg.Is<Message>(msg => msg.MqttTopicName.StartsWith(TwinPatchReportedTopicPrefix)))
.Returns(msg =>
{
var request = msg.Arg<Message>();
var response = new Message();
response.MqttTopicName = GetResponseTopic(request.MqttTopicName, statusFailure);
response.MqttTopicName = GetResponseTopic(request.MqttTopicName, StatusFailure);
transport.OnMessageReceived(response);
return TaskHelpers.CompletedTask;
});
Expand Down Expand Up @@ -653,7 +658,7 @@ private async Task TestReceiveOperationThrowsBasedOnTimeout(TimeSpan timeSpan)
// assert
act.Should().Throw<OperationCanceledException>();
sw.Stop();
sw.Elapsed.Should().BeLessOrEqualTo(timeSpan + ReceiveTimeoutBuffer, "ReceiveAsync should throw within the timeout specified.");
sw.Elapsed.Should().BeLessOrEqualTo(timeSpan + s_receiveTimeoutBuffer, "ReceiveAsync should throw within the timeout specified.");
}

private MqttTransportHandler CreateTransportHandlerWithMockChannel(out IChannel channel, string connectionString = DummyConnectionString)
Expand Down
Loading

0 comments on commit 9ba754e

Please sign in to comment.