v14: Refactor and enhance `System.Text.Json` converters #15960

ronaldbarendse · 2024-03-30T16:28:51Z

This refactors and enhances the System.Text.Json converters provided by the CMS, most of which were migrated from Newtonsoft.Json in PRs #15728 and #15685. The next version of Umbraco Deploy will also use STJ and the changes in this PR enhances these built-in converters, reducing the amount of duplicate code.

To summarize, I've done the following:

Update JsonUdiConverter to support both Udi and GuidUdi (removing JsonGuidUdiConverter), plus now also adding support for StringUdi (used in Deploy for languages, templates and other file based entities);
Rename JsonBoolConverter to JsonFuzzyBooleanConverter and update the implementation to always require a boolean (like) value (removing the fallback to false for invalid values);
Add ReadOnlyJsonConverter<T> and WriteOnlyJsonConverter<T> base converters;
Rename and improve the following converters:
- AutoInterningStringConverter to JsonStringInternConverter;
- AutoInterningStringKeyCaseInsensitiveDictionaryConverter<TValue> to JsonDictionaryStringInternIgnoreCaseConverter<TValue>;
- CaseInsensitiveDictionaryConverter<TValue> to JsonDictionaryStringIgnoreCaseConverter<TValue>;
- ForceUtcDateTimeConverter to JsonUniversalDateTimeConverter;
- Aligned the MessagePack formatters by renaming MessagePackAutoInterningStringKeyCaseInsensitiveDictionaryFormatter<TValue> to MessagePackDictionaryStringInternIgnoreCaseFormatter<TValue>;
Rename SystemTextJsonSerializer to DefaultJsonSerializer and SystemTextConfigurationEditorJsonSerializer to DefaultConfigurationEditorJsonSerializer;
Add JsonUdiRangeConverter to support UdiRange;
And finally also added the sealed keyword to all converters.

…ationEditorJsonSerializer

Nuklon · 2024-04-01T20:10:04Z

src/Umbraco.Infrastructure/Serialization/JsonStringInternConverter.cs

+using System.Text.Json;
+
+namespace Umbraco.Cms.Infrastructure.Serialization;
+
+/// <summary>
+/// Converts a string to or from JSON, interning the string when reading.
+/// </summary>
+public sealed class JsonStringInternConverter : ReadOnlyJsonConverter<string>
+{
+    /// <inheritdoc />
+    public override string? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
+        => reader.GetString() is string value
+        ? string.Intern(value)
+        : null;
+}


Maybe this entire converter should be removed? string.Intern has many downsides: dotnet/runtime#28368 (comment)

Looks like the string interning was discussed in PR #8365 and implemented in PR #8376. This specific converter interns the culture and segment property values of the PropertyData model, which is repeated/duplicated for each variant property value and might actually be worth this downside because of the memory savings and performance enhancements (when comparing the culture/segment values).

Maybe @nzdev can give his take on this, especially since this was added in v8 running .NET Framework and modern .NET might already have optimizations for this and/or better ways to achieve similar performance improvements?

Looking at the code of string.Intern, it still has the same downsides in .NET 8.0. I think the recommendation there to use a custom collection would be better.

Nuklon · 2024-04-01T20:14:36Z

src/Umbraco.Infrastructure/Serialization/JsonFuzzyBooleanConverter.cs

+/// <summary>
+/// Converts a boolean (like) value to or from JSON.
+/// </summary>
+public sealed class JsonFuzzyBooleanConverter : JsonConverter<bool>


I don't think this name makes any sense. Fuzzy matching means matching something approximately, there's nothing approximately about this.

This does however parse values to their Boolean equivalent in a non-strict way, so what name would you suggest?

It's also worthwhile to note that once parsed, it writes actual true/false values back again... Maybe JsonConvertToBooleanConverter would be a better name?

ConvertToXConverter feels a bit repetitive. Why not just JsonBooleanConverter?

Nuklon · 2024-04-01T20:18:49Z

src/Umbraco.Infrastructure/Serialization/JsonObjectConverter.cs

        {
            return;
        }

        // If an object is equals "new object()", Json.Serialize would recurse forever and cause a stack overflow
        // We have no good way of checking if its an empty object
        // which is why we try to check if the object has any properties, and thus will be empty.
-        if (objectToWrite.GetType().Name is "Object" && !objectToWrite.GetType().GetProperties().Any())
+        if (value.GetType().Name is "Object" && !value.GetType().GetProperties().Any())


Suggested change

if (value.GetType().Name is "Object" && !value.GetType().GetProperties().Any())

if (value.GetType() == typeof(object) && !value.GetType().GetProperties().Any())

Looking at the reason for having the JsonObjectConverter in PR #13537, it seems to be because by default STJ deserializes objects to JsonElement, which is 'hard to work with' (I assume because it's completely immutable). Is that correct @Zeegaan?

If that is the only reason, maybe we should set UnknownTypeHandling to JsonUnknownTypeHandling.JsonNode instead?

src/Umbraco.Infrastructure/Serialization/JsonDictionaryStringInternIgnoreCaseConverter.cs

src/Umbraco.Infrastructure/Serialization/JsonDictionaryStringIgnoreCaseConverter.cs

...co.PublishedCache.NuCache/DataSource/MessagePackDictionaryStringInternIgnoreCaseFormatter.cs

Nuklon · 2024-04-03T13:36:25Z

src/Umbraco.PublishedCache.NuCache/DataSource/ContentCacheDataModel.cs

@@ -15,14 +15,14 @@ public class ContentCacheDataModel
    // dont serialize empty properties
    [DataMember(Order = 0)]
    [JsonPropertyName("pd")]
-    [JsonConverter(typeof(AutoInterningStringKeyCaseInsensitiveDictionaryConverter<PropertyData[]>))]
-    [MessagePackFormatter(typeof(MessagePackAutoInterningStringKeyCaseInsensitiveDictionaryFormatter<PropertyData[]>))]
+    [JsonConverter(typeof(JsonDictionaryStringInternIgnoreCaseConverter<PropertyData[]>))]


Just commenting on this one, an alternative is to use the built-in converter and set PreferredObjectCreationHandling to JsonObjectCreationHandling.Populate so you can define the collection type (with comparer) in this file, then you don't need this custom converter.

bergmania · 2024-04-10T08:54:55Z

Most parts make sense to me, but personally I do not understand why
DefaultJsonSerializer should be better than SystemTextJsonSerializer(Same with DefaultConfigurationEditorJsonSerializer and SystemTextConfigurationEditorJsonSerializer).

I would prefer those to keep the names.

Regarding interning or not, I think that part should be kept in a separate PR. I'm sure more people have opinions about that.

ronaldbarendse · 2024-04-10T13:04:36Z

Most parts make sense to me, but personally I do not understand why DefaultJsonSerializer should be better than SystemTextJsonSerializer(Same with DefaultConfigurationEditorJsonSerializer and SystemTextConfigurationEditorJsonSerializer).

We don't ship different implementations, so prepending them with Default clearly communicates that. These also aren't SystemText implementations of the IJsonSerializer and IConfigurationEditorJsonSerializer, but naming them SystemTextJsonJsonSerializer and SystemTextJsonConfigurationEditorJsonSerializer seemed a bit too verbose.

Regarding interning or not, I think that part should be kept in a separate PR. I'm sure more people have opinions about that.

I agree! String interning can still improve performance and lower memory usage, but maybe using a custom collection as @Nuklon suggested would be a better alternative...

…eature/jsonconverters

…eature/jsonconverters # Conflicts: # tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/RichTextPropertyEditorHelperTests.cs

bergmania · 2024-04-10T17:44:11Z

@ronaldbarendse I have reverted the renaming of these classes. SystemText* is a good name indicating what technology is named. The verbose class name is not really exposed, because people use the technology agnostic interface name, when they inject.

For future PRs, I will recommend to keep to the changes you need and that blocks you, so silly things like naming are not blocking the PR :)

ronaldbarendse added 6 commits March 30, 2024 14:47

Update JsonUdiConverter to support Udi, GuidUdi and StringUdi types

e30b221

Require boolean (like) value and rename to JsonFuzzyBooleanConverter

833a216

Add read/write only JsonConverters and align naming

9cd9c9e

Rename SystemTextJsonSerializer to DefaultJsonSerializer

7b6a833

Rename SystemTextConfigurationEditorJsonSerializer to DefaultConfigur…

0c42e7c

…ationEditorJsonSerializer

Add JsonUdiRangeConverter

eee5574

Nuklon reviewed Apr 1, 2024

View reviewed changes

Nuklon reviewed Apr 3, 2024

View reviewed changes

src/Umbraco.Infrastructure/Serialization/JsonDictionaryStringInternIgnoreCaseConverter.cs Show resolved Hide resolved

Nuklon reviewed Apr 3, 2024

View reviewed changes

src/Umbraco.Infrastructure/Serialization/JsonDictionaryStringIgnoreCaseConverter.cs Show resolved Hide resolved

Nuklon reviewed Apr 3, 2024

View reviewed changes

...co.PublishedCache.NuCache/DataSource/MessagePackDictionaryStringInternIgnoreCaseFormatter.cs Show resolved Hide resolved

Nuklon reviewed Apr 3, 2024

View reviewed changes

ronaldbarendse added 3 commits April 5, 2024 16:45

Rename JsonFuzzyBooleanConverter back to JsonBooleanConverter

fe6fb2c

Fix value type check in JsonObjectConverter

72ca385

Merge branch 'v14/dev' into v14/feature/jsonconverters

9167a66

bergmania added 4 commits April 10, 2024 19:39

Revert class names

5c41e0d

Merge remote-tracking branch 'refs/remotes/origin/v14/dev' into v14/f…

f1ede3d

…eature/jsonconverters

Updated tests

b86b36e

Merge remote-tracking branch 'refs/remotes/origin/v14/dev' into v14/f…

ef92770

…eature/jsonconverters # Conflicts: # tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/RichTextPropertyEditorHelperTests.cs

bergmania approved these changes Apr 10, 2024

View reviewed changes

Post fix after merge.

4df4755

bergmania merged commit 16dd532 into v14/dev Apr 10, 2024
14 of 16 checks passed

bergmania deleted the v14/feature/jsonconverters branch April 10, 2024 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v14: Refactor and enhance `System.Text.Json` converters #15960

v14: Refactor and enhance `System.Text.Json` converters #15960

ronaldbarendse commented Mar 30, 2024

Nuklon Apr 1, 2024

ronaldbarendse Apr 3, 2024

Nuklon Apr 3, 2024

Nuklon Apr 1, 2024

ronaldbarendse Apr 3, 2024

Nuklon Apr 3, 2024

Nuklon Apr 1, 2024

ronaldbarendse Apr 3, 2024

Nuklon Apr 3, 2024

bergmania commented Apr 10, 2024

ronaldbarendse commented Apr 10, 2024

bergmania commented Apr 10, 2024

	if (value.GetType().Name is "Object" && !value.GetType().GetProperties().Any())
	if (value.GetType() == typeof(object) && !value.GetType().GetProperties().Any())

v14: Refactor and enhance System.Text.Json converters #15960

v14: Refactor and enhance System.Text.Json converters #15960

Conversation

ronaldbarendse commented Mar 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmania commented Apr 10, 2024

ronaldbarendse commented Apr 10, 2024

bergmania commented Apr 10, 2024

v14: Refactor and enhance `System.Text.Json` converters #15960

v14: Refactor and enhance `System.Text.Json` converters #15960