Simplify PayloadConvention to enable non-JSON payloads

[drwill-ms]

No description provided.

[patilsnr]

optimistic sign off

[rido-min]

1. As suggested in Support Binary Payloads in V2 #2977 the convention should be an interface (or abstract class) to serialized from/to bytes, there is no need to encode any string. Which encoding should I use when using Protobuf?
2. The serializer should be configured at operation (Telemetry, Property or Command) level. If I want to use AVRO for Telemetry, then I cannot use Twins or Commands.
3. Have consistent API surface using Generics instead of object, providing an overload in TelemetryMessage to use the same T

[drwill-ms]

1. As suggested in Support Binary Payloads in V2 #2977 the convention should be an interface (or abstract class) to serialized from/to bytes

The base class is an abstract class.

there is no need to encode any string. Which encoding should I use when using Protobuf?

I'll change it to a string. It is used when sending telemetry to set the content encoding property.

2. The serializer should be configured at operation (Telemetry, Property or Command) level. If I want to use AVRO for Telemetry, then I cannot use Twins or Commands.

We don't have the design sufficiently complicated to enable someone to change their convention per hub primitive nor per operation. While we can see someone maybe wanting to do that, we don't see it as mainstream enough to warrant the cost to implement/maintain nor the added complexity to the API.

3. Have consistent API surface using Generics instead of object, providing an overload in TelemetryMessage to use the same T

In JSON, serialization doesn't require the type to serialize. All classes inherit from System.Object, so that is sufficient. If you see a need to do that for binary conventions, let us know.

[rido-min]

1. The comment is not about interface vs abstract class, is about the signatures, there is no need to serialize from/to string, only byte[]. Content Encoding should be optional, only used by string-based serializers.
2. I got it, then I would limit the usage of custom PayloadSerializers for just Telemetry, and always use JSONUtf8 for DirectMethods and Twins.
3. Using T will enable the compiler to catch if the type used in new TelemetryMessage matches the type registered in the PayloadSerializer

Why is it called convention? I dont see any convention here, just a way to provide custom serializers.
Maybe rename PayloadConvention to TelemetryPayloadSerializer

[drwill-ms]

The comment is not about interface vs abstract class, is about the signatures, there is no need to serialize from/to string, only byte[]. Content Encoding should be optional, only used by string-based serializers.

Okay, I didn't know what part of your sentence the key point was, then. If your only complaint is that there is a method that takes a string and returns a type specified by the user, I found that is required for getting a type from a value on a twin. The payload convention class is used for that too and must be JSON. When we're performing twin operations, the client is configurable to customize how that is deserialized.

I got it, then I would limit the usage of custom PayloadSerializers for just Telemetry, and always use JSONUtf8 for DirectMethods and Twins.

What? Why? We can use it for telemetry, direct methods, C2D, and twins. They all have an opportunity de/serialize a custom payload.

Using T will enable the compiler to catch if the type used in new TelemetryMessage matches the type registered in the PayloadSerializer

I don't understand. This isn't required for JSON serialization. The type passed by the user is a real type, but the method doesn't need to know it, though. That's why JsonConvert.SerializeObject() isn't a generic.

However, I accept it might matter for binary serialization. Do you know that it does?

Why is it called convention? I dont see any convention here, just a way to provide custom serializers.

A service and device pass payloads back and forth in a serialized state by a convention they choose. The Hub dictates JSON for Twins and promotes JSON for everything else.

Maybe rename PayloadConvention to TelemetryPayloadSerializer

It isn't just for telemetry.

[drwill-ms]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[abhipsaMisra]

The changes overall look good to me. I have some small questions here and there, but nothing major.

My apologies again on the delay it took getting to this PR. 😑

[drwill-ms]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[drwill-ms]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).