[Bug Report] DirectMethodResponse payload json string is sent as escaped string

[emilm]

Context

• SDK version used: 2.0.3 (Please include the version of the SDK used)

Description of the issue

In version 1.x of the SDK, DirectMethodResponse sent a json string as-is. I require my dates to be formatted differently, and can't use the standard serializer.

Code sample exhibiting the issue

this.client.subscribeToMethods(this::onMethodCalled, this);
private DirectMethodResponse onMethodCalled(String methodName, DirectMethodPayload methodData, Object context) {
return new DirectMethodResponse(METHOD_SUCCESS,  serializeToString(myObject));
}

Result in iot hub:

{
    "status": 200,
    "payload": "[{\"date\":\"2022-08-02T14:07:11.000Z\",\"macAddress\":\"xxx\",\"ipAddress\":\"192.168.66.39\",\"clientId\":\"xxx\"},{\"date\":\"2022-08-02T14:07:21.000Z\",\"macAddress\":\"xxx\",\"ipAddress\":\"192.168.66.26\",\"clientId\":\"xxxxx\"}]"
}

The standard serializer returns the date like this if I don't serialize myself. Not sure how well that works with newtonsoft which I use in the receiving end. This is a breaking change, I think. Or can I somehow control the date serialization? Do I have to put decorators from now on or something?

"date": {
                "year": 2022,
                "month": 7,
                "dayOfMonth": 2,
                "hourOfDay": 13,
                "minute": 37,
                "second": 10
            },

[brycewang-microsoft]

Hi @emilm,

We re-worked the payload type of direct methods for both device and service a few months before. And you are right, this is a breaking change, so we brought this since our Java SDK v2.0.0. FYI, the related PR is here.

As you noticed, before this change, DirectMethodResponse on the sender side took the payload in the type of (Json) String only which would result in some weird double encoded payload on the receiver side. After this change, DirectMethodResponse on the sender side simply takes the payload in generic type Object, and the payload on receiving is in type of com.google.gson.JsonElement and can be obtained by 3 different getters getPayloadAsJsonElement(), getPayloadAsJsonString() and getPayload(Class<T> clazz).

We are using Gson as the serialization/deserialization library in our SDK. The getter getPayloadAsJsonString() on the receiver can be used if you wish to deserialize to a specific type using a deserialization library of your choice.

For samples of method payload in different data types, please refer to our E2E tests here.

[emilm]

Thanks @brycewang-microsoft . Yes my concern is the sending end. It increases the payload size with the standard serialization of date for example. Maybe add an option to inject your own GSON instance ? I have added type adapters on a GSON instance I use.

If getPayloadAsJsonString() is 'as is', but setPayload(String) in DirectMethodResponse escapes the json with quotes etc, you might have an inconsistency in how get / set is behaving, sort of.

Constructor in DirectMethodResponse takes object, but setPayload takes a String, but they both set the same property internally. But it gives me the (false) impression that the string won't be modified, but the Object will be serialized.

I did a workaround and serialized to JsonElement with JsonTreeWriter, and pass that as the object in the DirectMethodResponse constructor. That should work right? Seems to , at least for me.

I guess getPayload(Class<T> clazz) should work as long as I don't use the specialized date serialization in the other end. Maybe on the safe side I should do my own deserialization with getPayloadAsJsonString()

I followed this document - https://github.com/Azure/azure-iot-sdk-java/blob/main/SDK%20v2%20migration%20guide.md - maybe add a note about this there?

[brycewang-microsoft]

Hi @emilm, I hope I understand your use case correctly and share my answers as following:

It increases the payload size with the standard serialization of date for example. Maybe add an option to inject your own GSON instance ? I have added type adapters on a GSON instance I use.

After our changes, actually you don't need manual serialization of your payload object while calling DirectMethodResponse constructor on the sender side. The SDK will do serialization internally while creating the request. So in your use case, now you can do things like:

return new DirectMethodResponse(METHOD_SUCCESS,  myObject);

Meanwhile, on the receiver side, you can use the getter getPayload(Class<T> clazz) with the class of myObject as parameter. A sample can be found here.

The pros of doing so is to keep the consistency of payload type on both sender and receiver side. Moreover, once receiving the payload, if the user really wants to get the payload in "Json string" which can be deserialized with their own choice of library, they can use the getter getPayloadAsJsonString() instead.

On the other hand, if the user manually do serialization on the sender side, the payload will be actually double-serialized as the SDK will do it one more time. Therefore, on the receiving side, the user should call getPayload(String.class) to get a "Json string" payload; otherwise, if using getPayloadAsJsonString(), this would cause "double encoded payload" as mentioned before.

Constructor in DirectMethodResponse takes object, but setPayload takes a String, but they both set the same property internally. But it gives me the (false) impression that the string won't be modified, but the Object will be serialized.

You are right, this is misleading. I will fix it.

I did a workaround and serialized to JsonElement with JsonTreeWriter, and pass that as the object in the DirectMethodResponse constructor. That should work right? Seems to , at least for me.

I believe this has been addressed by my answer above.

I guess getPayload(Class clazz) should work as long as I don't use the specialized date serialization in the other end. Maybe on the safe side I should do my own deserialization with getPayloadAsJsonString()

To add extra explanation for getPayload(Class<T> clazz) in particular, its purpose is to give users ability to send and receive payloads both in customized data type.

maybe add a note about this there?

We did update our samples/tests to demonstrate this change, but it seems still confusing to the users and V2 migration guidance should be a better place to include those stuff. Sure thing, we will update the notes accordingly.

[emilm]

Hello @brycewang-microsoft :)

After our changes, actually you don't need manual serialization of your payload object while calling DirectMethodResponse constructor on the sender side. The SDK will do serialization internally while creating the request.

The pros of doing so is to keep the consistency of payload type on both sender and receiver side. Moreover, once receiving the payload, if the user really wants to get the payload in "Json string" which can be deserialized with their own choice of library, they can use the getter getPayloadAsJsonString() instead.

Sorry for not explaining properly. I forgot to mention that my receiver side is C# / NewtonSoft. That's why I would like to have full control over my payload. The reason is that different languages and libraries serialize / deserialize in different ways out of the box. How GSON serializes date as standard, is not how Newtonsoft deserializes the same date in the other end for instance., as you mention. But my point is that I would love to have something like setPayloadAsString so get/set have the same behavior. The problem with having forced serialization in the DirectMethodResponse is that devices with different versions of the SDK can send data that looks different , and then you have different and possible invalid data between old and new devices with different versions of the SDK that the receiving end has to handle. Having your own serialization with string as before eliminates this problem. This is the case now actually before I did the workaround with JsonTreeWriter. If i use your internal serialization I am not guaranteed the same payload across your SDK versions.

Also - sendEventAsync() in ModuleClient is sending the json string as is it looks like, while DirectMethodResponse does not. I feel like it all should behave the same way. I still would like to have both options - the ability to decide my own payload without SDK internal serialization on both Direct Methods and sending events. Right now they seem to behave differently.

The pros of doing so is to keep the consistency of payload type on both sender and receiver side. Moreover, once receiving the payload, if the user really wants to get the payload in "Json string" which can be deserialized with their own choice of library, they can use the getter getPayloadAsJsonString() instead.

I agree, but that's given both ends use Java and GSON with default settings.

To add extra explanation for getPayload(Class<T> clazz) in particular, its purpose is to give users ability to send and receive payloads both in customized data type.

That's a great feature!

[emilm]

@brycewang-microsoft any updates on this please? :)

I think I should be able to decide exactly how my payload is so I hope you will add back pure string as an option. What if I don't want JSON but some other format?

Thanks!

[brycewang-microsoft]

Hi @emilm, sorry I couldn't get back sooner!

It's interesting to see a use case involving different languages of IoT SDKs on the sender and receiver side, so I would like to reproduce this on my end and see if there's any improvements we may bring. I will update my test results here once I complete.

For different payload types across different versions of SDK, this should be expected as a breaking change while migrating from V1 to V2. It won't be expected to see huge changes further regarding payload of direct methods across the V2 versions though.

As per IoT service design, the payload for method requests and responses is a JSON document up to 128 KB( ref). When using our SDK, we do allow the users to input payload as various types and the SDK will take care of serialization/deserialization internally, but as this is a service design, the payload has to be Json format on the transfer and there's not much we can change on the client unfortunately.

[brycewang-microsoft]

Meanwhile, can you share the .NET SDK version numbers you are using with NewtonSoft on the receiver side? Also, a sample of the Json string which you are using on the sender side?

[emilm]

Thanks for your response @brycewang-microsoft !
C# server side / caller:

Microsoft.Azure.Devices, Version=1.37.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35
Newtonsoft.Json, Version=13.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed

Java client / callee side:

<dependency>
    <groupId>com.microsoft.azure.sdk.iot</groupId>
    <artifactId>iot-device-client</artifactId>
    <version>2.0.3</version>
</dependency>
<dependency>
    <groupId>com.google.code.gson</groupId>
    <artifactId>gson</artifactId>
    <version>2.9.0</version>
    <scope>compile</scope>
</dependency>

Server serialization:

dmSettings = new JsonSerializerSettings();
            dmSettings.Formatting = Formatting.None;
            dmSettings.ContractResolver = new CamelCasePropertyNamesContractResolver();
            dmSettings.Converters.Add(new Newtonsoft.Json.Converters.StringEnumConverter());
            dmSerializer = JsonSerializer.Create(dmSettings);

Problem is that gson serializes date differently than newtonsoft. I have no way to change how your internal gson serializes datetime. So what I have to do for now is to use a JsonElement where it is "pre-serialized", if you will.

Maybe a way to reproduce could be to serialize an object with a property of DateTime on the C# caller side and deserialize to an object with the same property name with Instant datatype on the Java callee side. (Java has like 100 different DateTimes).

Gson and NewtonSoft operate differently, so again I think it would be wise to give the option to give us the possibility to serialize / deserialize ourselves.

When you have a fleet of 1000+ devices, it's hard to update with a breaking change. So it should be avoided, and could have been in this case. I avoided it by using JsonElement as mentioned. This works because then the Instant is serialized properly before it's passed to the DirectMethodResponse constructor:

var response = new DirectMethodResponse(METHOD_SUCCESS, MessageSerializerUtil.gson.toJsonTree(ret));

But in order to know this workaround you kinda have to know the inner workings of the classes using DirectMethodResponse in your framework

[brycewang-microsoft]

Thanks for reaching back!

In your workaround, you are passing the payload in type of gson.JsonElement by calling "toJsonTree()", so I am wondering if you can call "getPayloadAsJsonElement()" method on the payload you receive on the device, and then pass it as JsonElement into the constructor of DirectMethodResponse, respond back to the service. Codes on the device side will be similar to:

        public DirectMethodResponse onMethodInvoked(String methodName, DirectMethodPayload methodData, Object context)
        {
            int status = 404;
            Object payload = null;

            if (IsExpected(methodName))
            {
                status = 200;
                payload = methodData.getPayloadAsJsonElement();
            }
            
            return new DirectMethodResponse(status, payload);
        }

I used the approach above, with Java V2 SDK + Gson on the device client and .NET SDK + NewtonSoft on the service client to validate, and I could obtain payload as expected. Can you try this out and let me know if it helps?

We want SDK to take the control of serialization/deserialization, to avoid of risks on failed communicating with the hub service via all kinds of serialization/deserialization libraries of users' choice.

[brycewang-microsoft]

FYI, the issue fix in #1586 has been shipped in the latest release.

[emilm]

Sorry, I was gone for a bit, and my entire draft of the post disappeared during an update.

Did you try this with an object with DateTtime (C#) and Instant (Java) in the request? This is where the serialization differ the most! But when you get it as JsonElement it's already sort of pre-serialized. It's just JSON primitives at that point which has no idea what DateTime is (in my case it's String at that point), and will be accepted as an object to be serialized to String. That's my theory anyhow!

So what I gather from this discussion is that you have to use JSON, and you can't use other frameworks like msgpack as payloads? DMs are locked to JSON?

[brycewang-microsoft]

Hi @emilm, the workable case I tested was calling "getPayloadAsJsonElement()" or "getPayloadAsJsonString()" on the payload received on device client, and then passing the payload as-is into the DirectMethodResponse ctor and return. In such a case, the payload on the service side (.NET) was using DateTtime indeed but the payload on the device side (Java) was not using Instant since I didn't handle the payload additionally.

I also tested to call "getPayload(CustomObject.class)" where CustomObject had an Instant property, and this didn't work. I agreed with your theory that Gson may not understand how to deserialize a DateTtime object.

With that being said, "getPayloadAsJsonElement()" or "getPayloadAsJsonString()" should still be workable in such a case, even though some additional manual handling on the payload might be needed.

Also, I tested to pass payload in type of msgpack into Java and .NET SDK, and both failed. It matches the service design that the payload for method requests and responses has to be a valid JSON. So yes, DMs are "locked" to JSON.

[emilm]

Right! Thanks for testing.

Yes, the only thing that could be useful is to have corresponding set as you have on the get, so you explicitly have a separate method for the jsonElement in set and a String so people can use the serialization library of their choice, in DirectMethodResponse

I don't need it personally right now since it works to serialize the JSON object but I was thinking more for clarity perhaps!

[brycewang-microsoft]

Hi @emilm, I think this interesting and would like to track it as an enhancement further here. I am closing this one as there is nothing to fix in particular. Thanks!

[emilm]

Thanks a lot ! 👍