Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System.Text.Json: JsonStringEnumConverter ignores its JsonNamingPolicy during deserialization. #31619

Closed
Tracked by #63918
dbc2 opened this issue Feb 1, 2020 · 21 comments · Fixed by #73348
Closed
Tracked by #63918
Assignees
Milestone

Comments

@dbc2
Copy link

dbc2 commented Feb 1, 2020

In System.Text.Json in .Net Core 3.1, I can serialize an enum using a naming policy, however when I attempt to deserialize the same enum with a naming policy, the policy is ignored and an exception is thrown when the naming policy does anything beyond modifying the case of the enum value names.

The issue can be reproduced as follows. Define the following simple snake case naming policy:

public class SnakeCaseNamingPolicy : JsonNamingPolicy
{
    public override string ConvertName(string name)
    {
        return string.Concat(name.Select((x, i) => i > 0 && char.IsUpper(x) ? "_" + Char.ToLowerInvariant(x).ToString() : Char.ToLowerInvariant(x).ToString()));
    }
}

And the following enum:

public enum TestEnum
{
    ValueZero,
    ValueOne,
    ValueTwo,
    ValueThree
}

Then attempting to round-trip values of TestEnum will fail during deserialization:

var values = (TestEnum [])Enum.GetValues(typeof(TestEnum));

var options = new JsonSerializerOptions
{
    Converters = { new JsonStringEnumConverter(new SnakeCaseNamingPolicy()) },
    WriteIndented = true,
};

var json = JsonSerializer.Serialize(values, options);

Console.WriteLine("Serialized {0}:", values);
Console.WriteLine(json);

var valuesIn = JsonSerializer.Deserialize<TestEnum []>(json, options); // Exception thrown here!

Assert.IsTrue(valuesIn.SequenceEqual(values));

The enum values are serialized with the naming policy successfully:

[
  "value_zero",
  "value_one",
  "value_two",
  "value_three"
]

But deserialization throws this exception:

Failed with unhandled exception: 
System.Text.Json.JsonException: The JSON value could not be converted to TestEnum[]. Path: $[0] | LineNumber: 1 | BytePositionInLine: 14.
   at System.Text.Json.ThrowHelper.ThrowJsonException()
   at System.Text.Json.Serialization.Converters.JsonConverterEnum`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options)
   at System.Text.Json.JsonPropertyInfoNotNullable`4.OnReadEnumerable(ReadStack& state, Utf8JsonReader& reader)
   at System.Text.Json.JsonPropertyInfo.ReadEnumerable(JsonTokenType tokenType, ReadStack& state, Utf8JsonReader& reader)
   at System.Text.Json.JsonPropertyInfo.Read(JsonTokenType tokenType, ReadStack& state, Utf8JsonReader& reader)
   at System.Text.Json.JsonSerializer.ReadCore(JsonSerializerOptions options, Utf8JsonReader& reader, ReadStack& readStack)
   at System.Text.Json.JsonSerializer.ReadCore(Type returnType, JsonSerializerOptions options, Utf8JsonReader& reader)
   at System.Text.Json.JsonSerializer.Deserialize(String json, Type returnType, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)

The problem seems to be that JsonConverterEnum<T>.Read() completely ignores the specified _namingPolicy and simply calls Enum.TryParse():

            // Try parsing case sensitive first
            string? enumString = reader.GetString();
            if (!Enum.TryParse(enumString, out T value)
                && !Enum.TryParse(enumString, ignoreCase: true, out value))
            {
                ThrowHelper.ThrowJsonException();
                return default;
            }
            return value;

A close reading of the documentation here seems to imply that this is an intended limitation:

Reading is case insensitive. Writing can be customized by using a JsonNamingPolicy.

However the inconsistency and inability to round-trip with the same settings is an odd and unexpected restriction. Newtonsoft does not have this limitation -- it can round-trip enum values even when using SnakeCaseNamingStrategy.

Demo fiddle here showing that Json.NET can round-trip enums using SnakeCaseNamingStrategy while System.Text.Json cannot do so with SnakeCaseNamingPolicy.

@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 2, 2020
@NoneGiven
Copy link

#1197 is related in that it concerns questionable assumptions about naming policy made by these predefined converters. Of note, if #782 (which is api-approved) is implemented, then this issue will become a bug when using a built-in naming policy rather than just a custom one.

@layomia layomia added this to the 5.0 milestone Feb 2, 2020
@layomia layomia added bug and removed untriaged New issue has not been triaged by the area owner labels Feb 2, 2020
@CodeBlanch
Copy link
Contributor

Anyone looking for a workaround I have a JsonConverter named JsonStringEnumMemberConverter in Macross.Json.Extensions package which fixes this issue and then adds in support for EnumMemberAttribute & Nullable enums. A modified version of dotnet/corefx#41648, basically.

@Bukran
Copy link

Bukran commented Mar 24, 2020

JsonStringEnumMemberConverter

Thanks a lot! I was stumbling against deserialization.

@layomia layomia added this to Backlog in System.Text.Json - 6.0 via automation Apr 7, 2020
@layomia layomia added the help wanted [up-for-grabs] Good issue for external contributors label Apr 14, 2020
@Macromullet
Copy link
Contributor

I don't see how this can be addressed with the current API shape, as JsonNamingPolicy.ConvertName is potentially a one-way function. For instance, if you chose HashFunction(name) as the implementation, it's not possible to convert it back. The only way to support this would be to define something like WPF does with converters, where you have Convert and ConvertBack, but that seems to be functionality outside the scope of what System.Text.Json should do.

@layomia
Copy link
Contributor

layomia commented Aug 26, 2020

from @liul in #41138:

Use the obtained string value directly here. But why not use _namingPolicy'?

like:

        internal override T ReadWithQuotes(ref Utf8JsonReader reader)
        {
            string? enumString = reader.GetString();
            
            var v =  _namingPolicy == null ? enumString : FormatEnumValueToString(enumString, encoder);
             
            // Try parsing case sensitive first
            if (!Enum.TryParse(v, out T value)
                && !Enum.TryParse(v, ignoreCase: true, out value))
            {
                ThrowHelper.ThrowJsonException();
            }

            return value;
        }

@2m0nd
Copy link

2m0nd commented Sep 15, 2020

Maybe create cache with source enum value names? And restore source name if exist namingPolicy?
#42302

0xced added a commit to 0xced/StacMan that referenced this issue Apr 21, 2021
* Upgrade projects to the new "SDK style" csproj format
  * Remove the packages directory and packages.config file
  * Remove `AssemblyInfo.cs` files (replaced with equivalent csproj properties)
  * Remove `*.vsmdi` and `*.testsettings` files
  * Run tests on .NET Framework 4.7.2 + .NET Core 3.1 + .NET 5.0
  * Update test dependencies to their latest version 
    * Microsoft.NET.Test.Sdk → 16.9.4
    * Moq → 4.16.1
    * MSTest.TestAdapter → 2.2.3
    * MSTest.TestFramework → 2.2.3

* Use `System.Text.Json` instead of `System.Web.Script.Serialization`
  * Use `JsonPropertyNameAttribute` instead of `FieldAttribute`
  * Make properties `{ get; init; }` instead of `{ get; internal set; }` with the help of the `IsExternalInit` NuGet package to support .NET Standard 2.0
  * Define `EnumMember` attributes for all enums and use `JsonStringEnumMemberConverter` from the `Macross.Json.Extensions` NuGet package to workaround [issues in JsonStringEnumConverter](dotnet/runtime#31619)

Fixes emmettnicholas#11
0xced added a commit to 0xced/StacMan that referenced this issue Apr 22, 2021
* Upgrade projects to the new "SDK style" csproj format
  * Remove the packages directory and packages.config file
  * Remove `AssemblyInfo.cs` files (replaced with equivalent csproj properties)
  * Remove `*.vsmdi` and `*.testsettings` files
  * Run tests on .NET Framework 4.7.2 + .NET Core 3.1 + .NET 5.0
  * Update test dependencies to their latest version 
    * Microsoft.NET.Test.Sdk → 16.9.4
    * Moq → 4.16.1
    * MSTest.TestAdapter → 2.2.3
    * MSTest.TestFramework → 2.2.3

* Use `System.Text.Json` instead of `System.Web.Script.Serialization`
  * Use `JsonPropertyNameAttribute` instead of `FieldAttribute`
  * Make properties `{ get; init; }` instead of `{ get; internal set; }` with the help of the `IsExternalInit` NuGet package to support .NET Standard 2.0
  * Define `JsonPropertyName` attributes for all enums and use `JsonStringEnumMemberConverter` from the `Macross.Json.Extensions` NuGet package to workaround [issues in JsonStringEnumConverter](dotnet/runtime#31619)

Fixes emmettnicholas#11
@avonheimburg
Copy link

I don't see how this can be addressed with the current API shape, as JsonNamingPolicy.ConvertName is potentially a one-way function. For instance, if you chose HashFunction(name) as the implementation, it's not possible to convert it back.

For enums, you could actually convert back even with a one-way function. You would need to convert all possible enum values to their serialized form using JsonNamingPolicy.ConvertName and could then simply look up the value you want to deserialize. The only limitation is that the naming policy actually assigns distinct names to all enum values.

@layomia
Copy link
Contributor

layomia commented Jun 7, 2021

Triage: per my comments in #42302 (review) I do not believe this feature is feasible. We'll close this issue.

As a workaround, users can write custom converters for enums that can support a close set of naming-policy-transformed enum names and honor them on deserialization. @tdykstra let's consider providing an example in the JSON docs.

@layomia layomia closed this as completed Jun 7, 2021
@AlgorithmsAreCool
Copy link
Contributor

Yes we would need to include flags for consistency and to avoid confusion. I do not believe the perf penalty will be acceptable.

Although I realize there are no good answers, I personally think the current solution of ignoring the naming convention for deserialization is more harmful and confusing than poor/missing Flags support.

Again without numbers to back me up, I would expect it is relatively rare for a web APIs to use Flags encoding. That being said, I realize people are using STJ to store and retrieve locally generated data that might need Flags more commonly.

Perhaps a compromise, provide Flags support with the performance fallback, but hidden behind a experimental/preview environment variable until a better solution can be found?

Also, perhaps the limited space cache can be improved by switching to a simple LRU style cache that will avoid a performance "cliff" when the cache overflows. This might amortize the pain somewhat

@tdykstra
Copy link

tdykstra commented Aug 5, 2021

If I run the Main method of the sample and instead of using the custom converter I use the built-in JsonStringEnumConverter with JsonNamingPolicy.CamelCase, everything works correctly. What am I missing that the custom converter is required for?

using System;
using System.Reflection;
using System.Text.Json;
using System.Text.Json.Serialization;

#nullable enable

namespace EnumConverter
{
    public class Program
    {
        public static void Main()
        {
            JsonSerializerOptions options = new() { Converters = { new JsonStringEnumConverter(JsonNamingPolicy.CamelCase) } };

            // If serialization happens first then we likely can support arbitrary values and flag combinations.
            SerializeThenDeserialize(DayOfWeek.Monday);
            SerializeThenDeserialize(BindingFlags.Instance | BindingFlags.Public);

            // Deserialize a "known" enum value without serializing first.
            BindingFlags value = JsonSerializer.Deserialize<BindingFlags>(@"""declaredOnly, createInstance""", options);
            Console.WriteLine(value);

            // Deserialize an "unknown" enum value without serializing first. Recall Enum.ToString() has specific and deterministic ordering for flags.
            try
            {
                JsonSerializer.Deserialize<BindingFlags>(@"""createInstance, declaredOnly""", options);
            }
            catch (JsonException) { }

            void SerializeThenDeserialize(object enumVal)
            {
                string json = JsonSerializer.Serialize(enumVal, options);
                Console.WriteLine(json);

                enumVal = JsonSerializer.Deserialize(json, enumVal.GetType(), options)!;
                Console.WriteLine(enumVal);
            }
        }
    }
}

@layomia
Copy link
Contributor

layomia commented Aug 5, 2021

@tdykstra I think it's working because we have logic trying case-insensitive matching for enum string values, which works well with PascalCase -> camelCase transformations. Can you try your repro using a policy such as snake_case -

public class SimpleSnakeCasePolicy : JsonNamingPolicy
{
public override string ConvertName(string name)
{
return string.Concat(name.Select((x, i) => i > 0 && char.IsUpper(x) ? "_" + x.ToString() : x.ToString())).ToLower();
}
}
?

@tdykstra
Copy link

tdykstra commented Aug 6, 2021

That explains it, thanks! Snake case test cases work only with the custom converter. I'll update the docs.

@AlgorithmsAreCool
Copy link
Contributor

@layomia I am assuming there is a 0% chance of a fix for this making it into .NET 6. Is that assumption correct?

@sabitkondakci
Copy link

@layomia I am assuming there is a 0% chance of a fix for this making it into .NET 6. Is that assumption correct?

Will this feature be in .Net6 sooner or later?

@eiriktsarpalis
Copy link
Member

Will this feature be in .Net6 sooner or later?

We didn't get around to fixing this in time for .NET 6. It will be considered for .NET 7.

0xced added a commit to 0xced/StacMan that referenced this issue Feb 18, 2022
* Upgrade projects to the new "SDK style" csproj format
  * Remove the packages directory and packages.config file
  * Remove `AssemblyInfo.cs` files (replaced with equivalent csproj properties)
  * Remove `*.vsmdi` and `*.testsettings` files
  * Run tests on .NET Framework 4.7.2 + .NET Core 3.1 + .NET 6.0
  * Update test dependencies to their latest version 
    * Microsoft.NET.Test.Sdk → 17.1.0
    * Moq → 4.16.1
    * MSTest.TestAdapter → 2.2.8
    * MSTest.TestFramework → 2.2.8

* Use `System.Text.Json` instead of `System.Web.Script.Serialization`
  * Use `JsonPropertyNameAttribute` instead of `FieldAttribute`
  * Make properties `{ get; init; }` instead of `{ get; internal set; }` with the help of the `IsExternalInit` NuGet package to support .NET Standard 2.0
  * Define `JsonPropertyName` attributes for all enums and use `JsonStringEnumMemberConverter` from the `Macross.Json.Extensions` NuGet package to workaround [issues in JsonStringEnumConverter](dotnet/runtime#31619)

Fixes emmettnicholas#11
@brianlagunas
Copy link

I also need this functionality. Seems silly to me that a naming policy would only apply on serialization and not deserialization. Seems like a massive oversight.

Please fix this.

@jeffhandley jeffhandley modified the milestones: 7.0.0, Future Jul 10, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 4, 2022
@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 7.0.0 Aug 5, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.