fix: Minor tweaks to the System.Text.Json JsonEventFormatter for compliance

- Make "JSON media type detection" configurable, and default to */json and */*+json
- Default to JSON content type (including making it present in the serialized event)
- Make a "binary vs non-binary" distinction as the first part of serialization (instead of as a fallback)
- Deserialize string data values regardless of content type
- Prohibit non-string data values for non-JSON content types

Signed-off-by: Jon Skeet <jonskeet@google.com>

src/CloudNative.CloudEvents.SystemTextJson/JsonEventFormatter.cs

@@ -29,7 +29,11 @@ namespace CloudNative.CloudEvents.SystemTextJson
     /// nor "data_base64" property is populated in a structured mode message.
     /// </description></item>
     /// <item><description>
-    /// If the data content type is absent or has a media type of "application/json", the data is encoded as JSON,
+    /// If the data value is a byte array, it is serialized either directly as binary data
+    /// (for binary mode messages) or as base64 data (for structured mode messages).
+    /// </description></item>
+    /// <item><description>
+    /// Otherwise, if the data content type is absent or has a media type indicating JSON, the data is encoded as JSON,
     /// using the <see cref="JsonSerializerOptions"/> passed into the constructor, or the default options.
     /// </description></item>
     /// <item><description>
@@ -37,26 +41,32 @@ namespace CloudNative.CloudEvents.SystemTextJson
     /// the data is serialized as a string.
     /// </description></item>
     /// <item><description>
-    /// Otherwise, if the data value is a byte array, it is serialized either directly as binary data
-    /// (for binary mode messages) or as base64 data (for structured mode messages).
-    /// </description></item>
-    /// <item><description>
     /// Otherwise, the encoding operation fails.
     /// </description></item>
     /// </list>
     /// <para>
-    /// When decoding CloudEvent data, this implementation uses the following rules:
+    /// When decoding structured mode CloudEvent data, this implementation uses the following rules,
-    /// </para>
+    /// which can be modified by overriding <see cref="DecodeStructuredModeDataBase64Property(JsonElement, CloudEvent)"/>
-    /// <para>
+    /// and <see cref="DecodeStructuredModeDataProperty(JsonElement, CloudEvent)"/>.
-    /// In a structured mode message, any data is either binary data within the "data_base64" property value,
-    /// or is a JSON token as the "data" property value. Binary data is represented as a byte array.
-    /// A JSON token is decoded as a string if is just a string value and the data content type is specified
-    /// and has a media type beginning with "text/". A JSON token representing the null value always
-    /// leads to a null data result. In any other situation, the JSON token is preserved as a <see cref="JsonElement"/>
-    /// that can be used for further deserialization (e.g. to a specific CLR type). This behavior can be modified
-    /// by overriding <see cref="DecodeStructuredModeDataBase64Property(JsonElement, CloudEvent)"/> and
-    /// <see cref="DecodeStructuredModeDataProperty(JsonElement, CloudEvent)"/>.
     /// </para>
+    /// <list type="bullet">
+    /// <item><description>
+    /// If the "data_base64" property is present, its value is decoded as a byte array.
+    /// </description></item>
+    /// <item><description>
+    /// If the "data" property is present (and non-null) and the content type is absent or indicates a JSON media type,
+    /// the JSON token present in the property is preserved as a <see cref="JsonElement"/> that can be used for further
+    /// deserialization (e.g. to a specific CLR type).
+    /// </description></item>
+    /// <item><description>
+    /// If the "data" property has a string value and a non-JSON content type has been specified, the data is
+    /// deserialized as a string.
+    /// </description></item>
+    /// <item><description>
+    /// If the "data" property has a non-null, non-string value and a non-JSON content type has been specified,
+    /// the deserialization operation fails.
+    /// </description></item>
+    /// </list>
     /// <para>
     /// In a binary mode message, the data is parsed based on the content type of the message. When the content
     /// type is absent or has a media type of "application/json", the data is parsed as JSON, with the result as
@@ -310,6 +320,9 @@ namespace CloudNative.CloudEvents.SystemTextJson
             }
             else
             {
+                // If no content type has been specified, default to application/json
+                cloudEvent.DataContentType ??= JsonMediaType;
+                
                 DecodeStructuredModeDataProperty(dataElement, cloudEvent);
             }
         }
@@ -347,8 +360,9 @@ namespace CloudNative.CloudEvents.SystemTextJson
         /// </summary>
         /// <remarks>
         /// <para>
-        /// This implementation converts JSON string tokens to strings when the content type suggests
+        /// This implementation will populate the Data property with the verbatim <see cref="JsonElement"/> if
-        /// that's appropriate, but otherwise returns the token directly.
+        /// the content type is deemed to be JSON according to <see cref="IsJsonMediaType(string)"/>. Otherwise,
+        /// it validates that the token is a string, and the Data property is populated with that string.
         /// </para>
         /// <para>
         /// Override this method to provide more specialized conversions.
@@ -358,12 +372,24 @@ namespace CloudNative.CloudEvents.SystemTextJson
         /// not have a null token type.</param>
         /// <param name="cloudEvent">The event being decoded. This should not be modified except to
         /// populate the <see cref="CloudEvent.Data"/> property, but may be used to provide extra
-        /// information such as the data content type. Will not be null.</param>
+        /// information such as the data content type. Will not be null, and the <see cref="CloudEvent.DataContentType"/>
+        /// property will be non-null.</param>
         /// <returns>The data to populate in the <see cref="CloudEvent.Data"/> property.</returns>
-        protected virtual void DecodeStructuredModeDataProperty(JsonElement dataElement, CloudEvent cloudEvent) =>
+        protected virtual void DecodeStructuredModeDataProperty(JsonElement dataElement, CloudEvent cloudEvent)
-            cloudEvent.Data = dataElement.ValueKind == JsonValueKind.String && cloudEvent.DataContentType?.StartsWith("text/") == true
+        {
-                ? dataElement.GetString()
+            if (IsJsonMediaType(cloudEvent.DataContentType!))
-                : (object) dataElement.Clone(); // Deliberately cast to object to provide the conditional operator expression type.
+            {
+                cloudEvent.Data = dataElement.Clone();
+            }
+            else
+            {
+                if (dataElement.ValueKind != JsonValueKind.String)
+                {
+                    throw new ArgumentException("CloudEvents with a non-JSON datacontenttype can only have string data values.");
+                }
+                cloudEvent.Data = dataElement.GetString();
+            }
+        }
 
         /// <inheritdoc />
         public override ReadOnlyMemory<byte> EncodeBatchModeMessage(IEnumerable<CloudEvent> cloudEvents, out ContentType contentType)
@@ -426,12 +452,16 @@ namespace CloudNative.CloudEvents.SystemTextJson
                     default:
                         writer.WriteStringValue(attribute.Type.Format(value));
                         break;
-
                 }
             }
 
             if (cloudEvent.Data is object)
             {
+                if (cloudEvent.DataContentType is null)
+                {
+                    writer.WritePropertyName(cloudEvent.SpecVersion.DataContentTypeAttribute.Name);
+                    writer.WriteStringValue(JsonMediaType);
+                }
                 EncodeStructuredModeData(cloudEvent, writer);
             }
             writer.WriteEndObject();
@@ -452,26 +482,31 @@ namespace CloudNative.CloudEvents.SystemTextJson
         /// <param name="writer"/>The writer to serialize the data to. Will not be null.</param>
         protected virtual void EncodeStructuredModeData(CloudEvent cloudEvent, Utf8JsonWriter writer)
         {
-            ContentType dataContentType = new ContentType(cloudEvent.DataContentType ?? JsonMediaType);
+            // Binary data is encoded using the data_base64 property, regardless of content type.
-            if (dataContentType.MediaType == JsonMediaType)
+            // TODO: Support other forms of binary data, e.g. ReadOnlyMemory<byte>
-            {
+            if (cloudEvent.Data is byte[] binary)
-                writer.WritePropertyName(DataPropertyName);
-                JsonSerializer.Serialize(writer, cloudEvent.Data, SerializerOptions);
-            }
-            else if (cloudEvent.Data is string text && dataContentType.MediaType.StartsWith("text/"))
-            {
-                writer.WritePropertyName(DataPropertyName);
-                writer.WriteStringValue(text);
-            }
-            else if (cloudEvent.Data is byte[] binary)
             {
                 writer.WritePropertyName(DataBase64PropertyName);
                 writer.WriteStringValue(Convert.ToBase64String(binary));
             }
             else
             {
-                // We assume CloudEvent.Data is not null due to the way this is called.
+                ContentType dataContentType = new ContentType(cloudEvent.DataContentType ?? JsonMediaType);
-                throw new ArgumentException($"{nameof(JsonEventFormatter)} cannot serialize data of type {cloudEvent.Data!.GetType()} with content type '{cloudEvent.DataContentType}'");
+                if (IsJsonMediaType(dataContentType.MediaType))
+                {
+                    writer.WritePropertyName(DataPropertyName);
+                    JsonSerializer.Serialize(writer, cloudEvent.Data, SerializerOptions);
+                }
+                else if (cloudEvent.Data is string text && dataContentType.MediaType.StartsWith("text/"))
+                {
+                    writer.WritePropertyName(DataPropertyName);
+                    writer.WriteStringValue(text);
+                }
+                else
+                {
+                    // We assume CloudEvent.Data is not null due to the way this is called.
+                    throw new ArgumentException($"{nameof(JsonEventFormatter)} cannot serialize data of type {cloudEvent.Data!.GetType()} with content type '{cloudEvent.DataContentType}'");
+                }
             }
         }
 
@@ -484,8 +519,14 @@ namespace CloudNative.CloudEvents.SystemTextJson
             {
                 return Array.Empty<byte>();
             }
+            // Binary data is left alone, regardless of the content type.
+            // TODO: Support other forms of binary data, e.g. ReadOnlyMemory<byte>
+            if (cloudEvent.Data is byte[] bytes)
+            {
+                return bytes;
+            }
             ContentType contentType = new ContentType(cloudEvent.DataContentType ?? JsonMediaType);
-            if (contentType.MediaType == JsonMediaType)
+            if (IsJsonMediaType(contentType.MediaType))
             {
                 var encoding = MimeUtilities.GetEncoding(contentType);
                 if (encoding is UTF8Encoding)
@@ -501,10 +542,6 @@ namespace CloudNative.CloudEvents.SystemTextJson
             {
                 return MimeUtilities.GetEncoding(contentType).GetBytes(text);
             }
-            if (cloudEvent.Data is byte[] bytes)
-            {
-                return bytes;
-            }
             throw new ArgumentException($"{nameof(JsonEventFormatter)} cannot serialize data of type {cloudEvent.Data.GetType()} with content type '{cloudEvent.DataContentType}'");
         }
 
@@ -541,6 +578,15 @@ namespace CloudNative.CloudEvents.SystemTextJson
                 cloudEvent.Data = body.ToArray();
             }
         }
+
+        /// <summary>
+        /// Determines whether the given media type should be handled as JSON.
+        /// The default implementation treats anything ending with "/json" or "+json"
+        /// as JSON.
+        /// </summary>
+        /// <param name="mediaType">The media type to check for JSON. Will not be null.</param>
+        /// <returns>Whether or not <paramref name="mediaType"/> indicates JSON data.</returns>
+        protected virtual bool IsJsonMediaType(string mediaType) => mediaType.EndsWith("/json") || mediaType.EndsWith("+json");
     }
 
     /// <summary>

test/CloudNative.CloudEvents.UnitTests/SystemTextJson/JsonElementAsserter.cs

@@ -39,6 +39,7 @@ namespace CloudNative.CloudEvents.SystemTextJson.UnitTests
                         JsonValueKind.String => property.GetString(),
                         JsonValueKind.Number => property.GetInt32(),
                         JsonValueKind.Null => (object?) null,
+                        JsonValueKind.Object => JsonSerializer.Deserialize(property.GetRawText(), expectation.value.GetType()),
                         _ => throw new Exception($"Unhandled value kind: {property.ValueKind}")
                     };
 

test/CloudNative.CloudEvents.UnitTests/SystemTextJson/JsonEventFormatterTest.cs

@@ -110,6 +110,20 @@ namespace CloudNative.CloudEvents.SystemTextJson.UnitTests
             asserter.AssertProperties(dataProperty, assertCount: true);
         }
 
+        [Fact]
+        public void EncodeStructuredModeMessage_JsonDataType_NumberSerialization()
+        {
+            var cloudEvent = new CloudEvent().PopulateRequiredAttributes();
+            cloudEvent.Data = 10;
+            cloudEvent.DataContentType = "application/json";
+            JsonElement element = EncodeAndParseStructured(cloudEvent);
+            var asserter = new JsonElementAsserter
+            {
+                { "data", JsonValueKind.Number, 10 }
+            };
+            asserter.AssertProperties(element, assertCount: false);
+        }
+
         [Fact]
         public void EncodeStructuredModeMessage_JsonDataType_CustomSerializerOptions()
         {
@@ -146,9 +160,47 @@ namespace CloudNative.CloudEvents.SystemTextJson.UnitTests
             };
             asserter.AssertProperties(dataProperty, assertCount: true);
         }
-        
+
         [Fact]
-        public void EncodeStructuredModeMessage_JsonDataType_JsonElement()
+        public void EncodeStructuredModeMessage_JsonDataType_JsonElementObject()
+        {
+            var cloudEvent = new CloudEvent().PopulateRequiredAttributes();
+            cloudEvent.Data = ParseJson("{ \"value\": { \"Key\": \"value\" } }").GetProperty("value");
+            cloudEvent.DataContentType = "application/json";
+            JsonElement element = EncodeAndParseStructured(cloudEvent);
+            JsonElement data = element.GetProperty("data");
+            var asserter = new JsonElementAsserter
+            {
+                { "Key", JsonValueKind.String, "value" }
+            };
+            asserter.AssertProperties(data, assertCount: true);
+        }
+
+        [Fact]
+        public void EncodeStructuredModeMessage_JsonDataType_JsonElementString()
+        {
+            var cloudEvent = new CloudEvent().PopulateRequiredAttributes();
+            cloudEvent.Data = ParseJson("{ \"value\": \"text\" }").GetProperty("value");
+            cloudEvent.DataContentType = "application/json";
+            JsonElement element = EncodeAndParseStructured(cloudEvent);
+            JsonElement data = element.GetProperty("data");
+            Assert.Equal(JsonValueKind.String, data.ValueKind);
+            Assert.Equal("text", data.GetString());
+        }
+
+        [Fact]
+        public void EncodeStructuredModeMessage_JsonDataType_JsonElementNull()
+        {
+            var cloudEvent = new CloudEvent().PopulateRequiredAttributes();
+            cloudEvent.Data = ParseJson("{ \"value\": null }").GetProperty("value");
+            cloudEvent.DataContentType = "application/json";
+            JsonElement element = EncodeAndParseStructured(cloudEvent);
+            JsonElement data = element.GetProperty("data");
+            Assert.Equal(JsonValueKind.Null, data.ValueKind);
+        }
+
+        [Fact]
+        public void EncodeStructuredModeMessage_JsonDataType_JsonElementNumeric()
         {
             var cloudEvent = new CloudEvent().PopulateRequiredAttributes();
             cloudEvent.Data = ParseJson("{ \"value\": 100 }").GetProperty("value");
@@ -355,6 +407,35 @@ namespace CloudNative.CloudEvents.SystemTextJson.UnitTests
             Assert.Throws<ArgumentException>(() => formatter.EncodeBinaryModeEventData(cloudEvent));
         }
 
+        [Fact]
+        public void EncodeBinaryModeEventData_NoContentType_ConvertsStringToJson()
+        {
+            var cloudEvent = new CloudEvent
+            {
+                Data = "some text"
+            }.PopulateRequiredAttributes();
+
+            // EncodeBinaryModeEventData doesn't actually populate the content type of the CloudEvent,
+            // but treat the data as if we'd explicitly specified application/json.
+            var data = new JsonEventFormatter().EncodeBinaryModeEventData(cloudEvent);
+            string text = BinaryDataUtilities.GetString(data, Encoding.UTF8);
+            Assert.Equal("\"some text\"", text);
+        }
+
+        [Fact]
+        public void EncodeBinaryModeEventData_NoContentType_LeavesBinaryData()
+        {
+            var cloudEvent = new CloudEvent
+            {
+                Data = SampleBinaryData
+            }.PopulateRequiredAttributes();
+
+            // EncodeBinaryModeEventData does *not* implicitly encode binary data as JSON.
+            var data = new JsonEventFormatter().EncodeBinaryModeEventData(cloudEvent);
+            var array = BinaryDataUtilities.AsArray(data);
+            Assert.Equal(array, SampleBinaryData);
+        }
+        
         // Note: batch mode testing is restricted to the batch aspects; we assume that the
         // per-CloudEvent implementation is shared with structured mode, so we rely on
         // structured mode testing for things like custom serialization.
@@ -691,11 +772,13 @@ namespace CloudNative.CloudEvents.SystemTextJson.UnitTests
             Assert.Equal(SampleBinaryData, cloudEvent.Data);
         }
 
-        [Fact]
+        [Theory]
-        public void DecodeStructuredModeMessage_TextContentTypeStringToken()
+        [InlineData("text/plain")]
+        [InlineData("image/png")]
+        public void DecodeStructuredModeMessage_NonJsonContentType_JsonStringToken(string contentType)
         {
             var obj = CreateMinimalValidJObject();
-            obj["datacontenttype"] = "text/plain";
+            obj["datacontenttype"] = contentType;
             obj["data"] = "some text";
             var cloudEvent = DecodeStructuredModeMessage(obj);
             Assert.Equal("some text", cloudEvent.Data);
@@ -704,9 +787,25 @@ namespace CloudNative.CloudEvents.SystemTextJson.UnitTests
         [Theory]
         [InlineData(null)]
         [InlineData("application/json")]
-        [InlineData("text/plain")]
+        public void DecodeStructuredModeMessage_JsonContentType_JsonStringToken(string contentType)
-        [InlineData("application/not-quite-json")]
+        {
-        public void DecodeStructuredModeMessage_JsonToken(string contentType)
+            var obj = CreateMinimalValidJObject();
+            if (contentType is object)
+            {
+                obj["datacontenttype"] = contentType;
+            }
+            obj["data"] = "text";
+            var cloudEvent = DecodeStructuredModeMessage(obj);
+            var element = (JsonElement) cloudEvent.Data!;
+            Assert.Equal(JsonValueKind.String, element.ValueKind);
+            Assert.Equal("text", element.GetString());
+        }
+
+        [Theory]
+        [InlineData(null)]
+        [InlineData("application/json")]
+        [InlineData("application/xyz+json")]
+        public void DecodeStructuredModeMessage_JsonContentType_NonStringValue(string contentType)
         {
             var obj = CreateMinimalValidJObject();
             if (contentType is object)
@@ -720,6 +819,15 @@ namespace CloudNative.CloudEvents.SystemTextJson.UnitTests
             Assert.Equal(10, element.GetInt32());
         }
 
+        [Fact]
+        public void DecodeStructuredModeMessage_NonJsonContentType_NonStringValue()
+        {
+            var obj = CreateMinimalValidJObject();
+            obj["datacontenttype"] = "text/plain";
+            obj["data"] = 10;
+            Assert.Throws<ArgumentException>(() => DecodeStructuredModeMessage(obj));
+        }
+
         [Fact]
         public void DecodeStructuredModeMessage_NullDataBase64Ignored()
         {
@@ -910,6 +1018,75 @@ namespace CloudNative.CloudEvents.SystemTextJson.UnitTests
             Assert.Null(event2.DataContentType);
         }
 
+        // Additional tests for the changes/clarifications in https://github.com/cloudevents/spec/pull/861
+        [Fact]
+        public void EncodeStructured_DefaultContentTypeToApplicationJson()
+        {
+            var cloudEvent = new CloudEvent
+            {
+                Data = new { Key = "value" }
+            }.PopulateRequiredAttributes();
+
+            var encoded = new JsonEventFormatter().EncodeStructuredModeMessage(cloudEvent, out var contentType);
+            Assert.Equal("application/cloudevents+json; charset=utf-8", contentType.ToString());
+            JsonElement obj = ParseJson(encoded);
+            var asserter = new JsonElementAsserter
+            {
+                { "data", JsonValueKind.Object, cloudEvent.Data },
+                { "datacontenttype", JsonValueKind.String, "application/json" },
+                { "id", JsonValueKind.String, "test-id" },
+                { "source", JsonValueKind.String, "//test" },
+                { "specversion", JsonValueKind.String, "1.0" },
+                { "type", JsonValueKind.String, "test-type" },
+            };
+            asserter.AssertProperties(obj, assertCount: true);
+        }
+
+        [Fact]
+        public void EncodeStructured_BinaryData_DefaultContentTypeToApplicationJson()
+        {
+            var cloudEvent = new CloudEvent
+            {
+                Data = SampleBinaryData
+            }.PopulateRequiredAttributes();
+
+            // While it's odd for a CloudEvent to have binary data but no data content type,
+            // the spec says the data should be placed in data_base64, and the content type should
+            // default to application/json. (Checking in https://github.com/cloudevents/spec/issues/933)
+            var encoded = new JsonEventFormatter().EncodeStructuredModeMessage(cloudEvent, out var contentType);
+            Assert.Equal("application/cloudevents+json; charset=utf-8", contentType.ToString());
+            JsonElement obj = ParseJson(encoded);
+            var asserter = new JsonElementAsserter
+            {
+                { "data_base64", JsonValueKind.String, SampleBinaryDataBase64 },
+                { "datacontenttype", JsonValueKind.String, "application/json" },
+                { "id", JsonValueKind.String, "test-id" },
+                { "source", JsonValueKind.String, "//test" },
+                { "specversion", JsonValueKind.String, "1.0" },
+                { "type", JsonValueKind.String, "test-type" },
+            };
+            asserter.AssertProperties(obj, assertCount: true);
+        }
+
+        [Fact]
+        public void DecodeStructured_DefaultContentTypeToApplicationJson()
+        {
+            var obj = new JObject
+            {
+                ["specversion"] = "1.0",
+                ["type"] = "test-type",
+                ["id"] = "test-id",
+                ["source"] = SampleUriText,
+                ["data"] = "some text"
+            };
+            var cloudEvent = DecodeStructuredModeMessage(obj);
+            Assert.Equal("application/json", cloudEvent.DataContentType);
+            var jsonData = Assert.IsType<JsonElement>(cloudEvent.Data);
+            Assert.Equal(JsonValueKind.String, jsonData.ValueKind);
+            Assert.Equal("some text", jsonData.GetString());
+        }
+
+        // Utility methods
         private static object DecodeBinaryModeEventData(byte[] bytes, string contentType)
         {
             var cloudEvent = new CloudEvent().PopulateRequiredAttributes();