Updates the `MessageSerializer` to store the json value as the actual json object in the `data` field.Update data field serialization #193

GarrettBeatty · 2025-03-19T14:12:11Z

Issue #, if available: #168

Description of changes:

Updates the MessageSerializer to store the json value as the actual json object in the data field.
Add datacontenttype field to message

Before

{
    "version": "0",
    "id": "359472c2-c66b-00df-5976-cef04c383a15",
    "detail-type": "Payload",
    "source": "/aws/messaging",
    "account": "147997163238",
    "time": "2025-03-14T18:40:58Z",
    "region": "us-east-1",
    "resources": [],
    "detail": {
        "id": "fdc50c90-f747-4794-ae91-d39894cc6442",
        "source": "/aws/messaging",
        "specversion": "1.0",
        "type": "Payload",
        "time": "2025-03-14T18:40:56.3356898+00:00",
        "data": "{\"Id\":\"8b939477-089c-49ce-ae90-49e9817bd593\",\"Name\":\"Test\",\"SomeValue\":42}"
    }

After

{
    "version": "0",
    "id": "ce7c8a25-da87-879d-100c-874adbaf9c51",
    "detail-type": "Payload",
    "source": "/aws/messaging",
    "account": "147997163238",
    "time": "2025-03-14T19:03:10Z",
    "region": "us-east-1",
    "resources": [],
    "detail": {
        "id": "0ef8cf5e-938a-4dc8-9b62-c6ccb3344bc7",
        "source": "/aws/messaging",
        "specversion": "1.0",
        "type": "Payload",
        "time": "2025-03-14T19:03:08.4022404+00:00",
        "datacontenttype": "application/json",
        "data": {
            "Id": "0172a7de-3757-4740-a771-4aca5f05d67c",
            "Name": "Test",
            "SomeValue": 42
        }
    }
}

Testing

I followed the steps in Not properly formatting the data element of the MessageEnvelope #168 (comment) to validate the before and after

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

philasmar · 2025-03-19T20:23:44Z

.autover/changes/56a61b2c-4302-4077-9dc6-3fc3bc60a998.json

+  "Projects": [
+    {
+      "Name": "AWS.Messaging",
+      "Type": "Patch",


I think this should be a Minor because there is a significant change in behavior. I would call that out as a breaking change.

philasmar · 2025-03-19T20:24:48Z

src/AWS.Messaging/MessageEnvelope.cs

+    /// The data content type.
+    /// </summary>
+    [JsonPropertyName("datacontenttype")]
+    public string? DataContentType { get; set; } = null!;


why do you need to explicitly set it to null!? The default for string? is null.

philasmar · 2025-03-19T20:26:55Z

src/AWS.Messaging/Serialization/IMessageSerializer.cs


    /// <summary>
-    /// Deserializes the raw string message into the .NET type.
+    /// Deserializes a JsonElement message into the specified .NET type.


add a doc ref for JsonElement

fixed this and other comments in latest revision

philasmar · 2025-03-19T20:27:01Z

src/AWS.Messaging/Serialization/IMessageSerializer.cs

-    /// <param name="message">The string message that will be deserialized.</param>
-    /// <param name="deserializedType">The .NET type that represents the deserialized message.</param>
-    object Deserialize(string message, Type deserializedType);
+    /// <param name="message">The JsonElement containing the message to be deserialized.</param>


philasmar · 2025-03-19T20:29:55Z

src/AWS.Messaging/Serialization/IMessageSerializer.cs

-    {
-        return (T)Deserialize(message, typeof(T));
-    }
+    string DataContentType { get; }


Isn't this always "application/json"? How else are we initializing this to a diff val?

for our message serializer it will always be application/json.

philasmar · 2025-03-19T20:30:07Z

src/AWS.Messaging/Serialization/MessageSerializer.cs

-    /// <inheritdoc/>
-    /// <exception cref="FailedToDeserializeApplicationMessageException"></exception>
+    /// <summary>
+    /// Deserializes a JsonElement message into the specified type.


add doc ref to JsonElement

philasmar · 2025-03-19T20:30:12Z

src/AWS.Messaging/Serialization/MessageSerializer.cs

+    /// <summary>
+    /// Deserializes a JsonElement message into the specified type.
+    /// </summary>
+    /// <param name="message">The JsonElement containing the message to deserialize.</param>


normj · 2025-03-20T22:56:02Z

src/AWS.Messaging/Serialization/EnvelopeSerializer.cs


            // Deserialize the message content using the custom serializer
-            var dataContent = JsonPropertyHelper.GetRequiredProperty(root, "data", element => element.GetString()!);
+            var dataContent = JsonPropertyHelper.GetRequiredProperty(root, "data", element => element);


Can we make this resilient to support the data being either JSON or escaped JSON? I know we are in preview and can make breaking changes but I'm concerned how we are going to roll out this change. If we can make it resilient at least on the reading side that can help.

Hmm. Is there a reason we would want to support both? The correct behavior per the cloudspec is the new logic and feel like since we are still in preview this is the time to make the breaking change. I feel it would complicate the code to support both

src/AWS.Messaging/Serialization/IMessageSerializer.cs

GarrettBeatty · 2025-03-28T15:28:23Z

src/AWS.Messaging/Serialization/EnvelopeSerializer.cs

                ["time"] = envelope.TimeStamp,
-                ["data"] = _messageSerializer.Serialize(message)
+                ["datacontenttype"] = _messageSerializer.DataContentType,
+                ["data"] = JsonNode.Parse(_messageSerializer.Serialize(message)) // parse the string to get the value as the actual json node.


this makes the assumption that the user's implementation of message serializer will always return json, which i think is fine for now ( unless are we expecting customers to want to do xml serialization or other things)? Otherwise we have to add more logic in EnvelopeSerializer to handle the case where its not json type for serializing/deserializing like

if isDataContentJson(datacontenttype) data = JsonNode.Parse(_messageSerializer.Serialize(message)) else data = _messageSerializer.Serialize(message) private isDataContentJson() { // some way to determine if its json }

and then in deserialization we would have to do the reverse. If we are fine with this logic being inside EnvelopeSerializer then i can add it.

also @normj responding to your other comment here #193 (comment) - having a flag to turn on and off for parsing as well would be a similar implementation. so wondering how strongly you feel about that.

I don't think we should assume the content return is JSON. Should we have the MessageSerializer's serialize method return back a results type that includes the serialized message and the content type? Then the envelople serializer can look at the content type and decide what to do based on the content type?

return back a results type that includes the serialized message and the content type? Then the envelope serializer can look at the content type and decide what to do based on the content type?

im fine with this. i think thats the best way we are going to figure out. i will update the code

I've updated it in the latest revision to have a check in EnvelopeSerializer. And i retested both scenarios manually.

Retested with application/json - data gets stored as the actual json object

Retested with application/plain as the data content type, the data gets stored as the string value. i manually edited our messageserializer and changed the datacontenttype to application/plain to test

application/json

{ "version": "0", "id": "a4579b24-b741-cc2e-df7d-eec8f890da27", "detail-type": "Payload", "source": "/aws/messaging", "account": "147997163238", "time": "2025-03-31T17:33:28Z", "region": "us-east-1", "resources": [], "detail": { "id": "b08c718c-e63d-4547-9790-030025689103", "source": "/aws/messaging", "specversion": "1.0", "type": "Payload", "time": "2025-03-31T17:33:27.1657903+00:00", "datacontenttype": "application/json", "data": { "Id": "036f7898-3e03-403a-b4f6-f18227310330", "Name": "Test", "SomeValue": 42 } } }

application/plain

{ "version": "0", "id": "80f21c60-9797-d928-d0e1-af78e0f7dcba", "detail-type": "Payload", "source": "/aws/messaging", "account": "147997163238", "time": "2025-03-31T17:39:49Z", "region": "us-east-1", "resources": [], "detail": { "id": "0695f334-bb82-4f03-be12-154cb892b172", "source": "/aws/messaging", "specversion": "1.0", "type": "Payload", "time": "2025-03-31T17:39:47.7147488+00:00", "datacontenttype": "application/plain", "data": "{\"Id\":\"4a6f1d73-289d-4632-88b0-a79318961a25\",\"Name\":\"Test\",\"SomeValue\":42}" } }

normj · 2025-03-31T22:30:02Z

src/AWS.Messaging/Serialization/EnvelopeSerializer.cs


+            if (IsJsonContentType(envelope.DataContentType))
+            {
+                blob["data"] = JsonNode.Parse(_messageSerializer.Serialize(message));


I was thinking instead of having a global DataContentType for the IMessageSerializer. The Serialize method would return back the content type. That way you could implement different message content type based on the message type.

I think that would mean the MessageEnvelope<T> from CreateEnvelopeAsync would leave DataContentType null since it is unknown at this point. Then in SerializeAsync have something like this:

MessageSerializerResults serializationResults = _messageSerializer.Serialize(message); var blob = new JsonObject { ["id"] = envelope.Id, ["source"] = envelope.Source?.ToString(), ["specversion"] = envelope.Version, ["type"] = envelope.MessageTypeIdentifier, ["time"] = envelope.TimeStamp, ["datacontenttype"] = serializationResults.ContentType }; if (IsJsonContentType(serializationResults.ContentType)) { blob["data"] = JsonNode.Parse(serializationResults.Data); } else { blob["data"] = serializationResults.Data; } class MessageSerializerResults { public string ContentType {get;set;} public string Data {get;set;} }

ah okay i see what you mean ! Yes i like that better. I will update

updated in last revision to have that

GarrettBeatty · 2025-04-01T15:27:15Z

test/AWS.Messaging.UnitTests/SerializationTests/EnvelopeSerializerTests.cs

-            Source = new Uri("/backend/service", UriKind.Relative),
-            Version = "1.0",
-            MessageTypeIdentifier = "xml",
-            TimeStamp = _testdate,


removed these because testing plain/text was doing same thing

GarrettBeatty marked this pull request as ready for review March 19, 2025 14:56

GarrettBeatty requested review from normj and philasmar and removed request for normj March 19, 2025 14:57

philasmar requested changes Mar 19, 2025

View reviewed changes

normj requested changes Mar 20, 2025

View reviewed changes

GarrettBeatty force-pushed the refactorrebase branch from d522cf6 to e56fc52 Compare March 25, 2025 21:11

Base automatically changed from refactorrebase to v4sdk-development March 26, 2025 22:44

GarrettBeatty marked this pull request as draft March 28, 2025 15:23

GarrettBeatty force-pushed the datacontent4 branch from 1ab1168 to 2bc86b2 Compare March 28, 2025 15:26

GarrettBeatty commented Mar 28, 2025

View reviewed changes

Parse data as the actual json object

f9da426

GarrettBeatty force-pushed the datacontent4 branch from 2bc86b2 to f9da426 Compare March 28, 2025 15:34

GarrettBeatty marked this pull request as ready for review March 28, 2025 16:15

GarrettBeatty requested review from normj and philasmar March 28, 2025 16:44

philasmar approved these changes Mar 28, 2025

View reviewed changes

GarrettBeatty added 2 commits March 31, 2025 13:31

PR comment

52ae468

fix version

6c785fc

normj requested changes Mar 31, 2025

View reviewed changes

GarrettBeatty added 2 commits April 1, 2025 11:19

Use MessageSerializerResult

e8a817d

add test

b26b9dd

GarrettBeatty commented Apr 1, 2025

View reviewed changes

GarrettBeatty requested a review from normj April 1, 2025 15:28

update doc

72416ba

normj approved these changes Apr 2, 2025

View reviewed changes

GarrettBeatty merged commit cb4df73 into v4sdk-development Apr 2, 2025
5 checks passed

GarrettBeatty deleted the datacontent4 branch April 2, 2025 17:39

Updates the MessageSerializer to store the json value as the actual json object in the data field.Update data field serialization #193

Updates the MessageSerializer to store the json value as the actual json object in the data field.Update data field serialization #193

Uh oh!

Conversation

GarrettBeatty commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GarrettBeatty Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GarrettBeatty Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GarrettBeatty Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Updates the `MessageSerializer` to store the json value as the actual json object in the `data` field.Update data field serialization #193

Updates the `MessageSerializer` to store the json value as the actual json object in the `data` field.Update data field serialization #193

GarrettBeatty commented Mar 19, 2025 •

edited

Loading

GarrettBeatty Mar 19, 2025 •

edited

Loading

GarrettBeatty Mar 28, 2025 •

edited

Loading

GarrettBeatty Mar 31, 2025 •

edited

Loading