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

Apply DictionaryKeyPolicy to enum keys (and other primitive key types?) #47765

Closed
trueromanus opened this issue Feb 2, 2021 · 11 comments
Closed
Labels
area-System.Text.Json bug help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@trueromanus
Copy link

Description

Example for reproducing issue:

    public enum ETestEnum {
        TestValue1 = 1,
        TestValue2 = 2
    };

    public class Info
    {
        public Dictionary<ETestEnum, int> Queue { get; set; }
    }

    public void Main(string[] args) {
            string jsonString = JsonSerializer.Serialize(new Info
            {
                Queue = new Dictionary<ETestEnum, int> { [ETestEnum.TestValue1] = 1 },
            }, new JsonSerializerOptions {
                PropertyNameCaseInsensitive = true,
                PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
                DictionaryKeyPolicy = JsonNamingPolicy.CamelCase,
                Converters = {
                    new JsonStringEnumConverter(JsonNamingPolicy.CamelCase)
                }
            } );
    }

jsonString in result: {"queue":{"TestValue1":1}}
jsonString in expected result: {"queue":{"testValue1":1}}

Key name in dictionary not in camel case.

I investigated little bit and find out that DictionaryKeyPolicy and Converters (in my case JsonStringEnumConverter(JsonNamingPolicy.CamelCase)) don't executing while serialization process.

Configuration

  • Which version of .NET is the code running on? net 5.0.102
  • What OS and version, and what distro if applicable? Windows 10 2004
  • What is the architecture (x64, x86, ARM, ARM64)? x64
  • Do you know whether it is specific to that configuration? I don't think so.

Regression?

I don't know because used Newtonsoft.Json.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Feb 2, 2021
@ghost
Copy link

ghost commented Feb 2, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Example for reproducing issue:

    public enum ETestEnum {
        TestValue1 = 1,
        TestValue2 = 2
    };

    public class Info
    {
        public Dictionary<ETestEnum, int> Queue { get; set; }
    }

    public void Main(string[] args) {
            string jsonString = JsonSerializer.Serialize(new Info
            {
                Queue = new Dictionary<ETestEnum, int> { [ETestEnum.TestValue1] = 1 },
            }, new JsonSerializerOptions {
                PropertyNameCaseInsensitive = true,
                PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
                DictionaryKeyPolicy = JsonNamingPolicy.CamelCase,
                Converters = {
                    new JsonStringEnumConverter(JsonNamingPolicy.CamelCase)
                }
            } );
    }

jsonString in result: {"queue":{"TestValue1":1}}
jsonString in expected result: {"queue":{"testValue1":1}}

Key name in dictionary not in camel case.

I investigated little bit and find out that DictionaryKeyPolicy and Converters (in my case JsonStringEnumConverter(JsonNamingPolicy.CamelCase)) don't executing while serialization process.

Configuration

  • Which version of .NET is the code running on? net 5.0.102
  • What OS and version, and what distro if applicable? Windows 10 2004
  • What is the architecture (x64, x86, ARM, ARM64)? x64
  • Do you know whether it is specific to that configuration? I don't think so.

Regression?

I don't know because used Newtonsoft.Json.

Author: trueromanus
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis eiriktsarpalis added bug and removed untriaged New issue has not been triaged by the area owner labels Feb 2, 2021
@eiriktsarpalis eiriktsarpalis added this to the 6.0.0 milestone Feb 2, 2021
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Feb 2, 2021

That seems like a bug to me. For example using the same serializer options I get the following behaviour:

var dict = new Dictionary<ETestEnum, ETestEnum> { [ETestEnum.TestValue1] = ETestEnum.TestValue1 };
JsonSerializer.Serialize(dict, options); // "{\"TestValue1\":\"testValue1\"}"

@layomia thoughts?

@layomia
Copy link
Contributor

layomia commented Feb 2, 2021

cc @jozkee

When we designed the feature to support non-string primitives as dictionary keys #32676, we elected to keep applying DictionaryKeyPolicy to only string keys, as it wasn't clear whether doing so for other types with JSON string representations (enums, Guids, DateTime etc.) is a first-class scenario (#32909 (comment)). It's good to see some feedback around this. Enums seem like a reasonable type to apply the policy to. I'm curious what thoughts are around which other types to include.

@SkiFoD
Copy link
Contributor

SkiFoD commented Feb 9, 2021

@layomia I made a quick research. In my opinion enum is the only type to include.

@MaceWindu
Copy link

@layomia

I'm curious what thoughts are around which other types to include.

E.g. we use Guid for dictionary key and with Json.net it was serialized using Guid converter (we use .ToString("N") in converter), but System.Text.Json just call ToString on guid value.

@yuretz
Copy link

yuretz commented Mar 25, 2021

Even if you decide that DictionaryKeyPolicy doesn't apply to enum dictionary keys, JsonStringEnumConverter with naming policy set to JsonNamingPolicy.CamelCase and added to JsonSerializerOptions.Converters shall be used for serialization anyway, and it is currently not the case, which is wrong.

@layomia layomia changed the title JsonSerializer.Serialize (System.Text.Json) don't work with Enum as key in Dictionary Apply DictionaryKeyPolicy to enum keys (and other primitive key types?) Jun 1, 2021
@layomia
Copy link
Contributor

layomia commented Jun 1, 2021

but System.Text.Json just call ToString on guid value.

I think this issue is not about what format we write data types with, but about whether to apply the dictionary naming policy to instances of those types when they are dictionary keys. It doesn't seem meaningful to apply something like camelCase policy (as a concrete example) to a guid.

JsonStringEnumConverter with naming policy set to JsonNamingPolicy.CamelCase and added to JsonSerializerOptions.Converters shall be used for serialization anyway, and it is currently not the case, which is wrong.

From the example from @eiriktsarpalis above, the policy is being applied to the dictionary value as expected.


I do think it makes sense to apply the policy to enum keys. It would be a breaking change to do this however, but it might be worth it since the workaround of having a custom converter for the parent dictionary could be cumbersome.

[I'd normally push something like this to future to wait for more customer feedback, but since it would be a breaking change we should address 6.0 before more folks depend on behavior that we want to change.]

@layomia
Copy link
Contributor

layomia commented Jun 9, 2021

Marking this as up-for-grabs. The task is to apply DictionaryKeyPolicy to enum keys on serialization. I can help with any questions.

@layomia layomia added the help wanted [up-for-grabs] Good issue for external contributors label Jun 9, 2021
@SkiFoD
Copy link
Contributor

SkiFoD commented Jun 9, 2021

@layomia Hey, I would like to work on this one.

@layomia
Copy link
Contributor

layomia commented Jul 23, 2021

We have an open PR to fix this #54429, but it won't block the release if we don't get this functionality in. I'll mark as .NET 7.

@layomia layomia modified the milestones: 6.0.0, 7.0.0 Jul 23, 2021
SkiFoD added a commit to SkiFoD/runtime that referenced this issue Jul 27, 2021
layomia pushed a commit that referenced this issue Aug 3, 2021
* Add DictionaryKeyPolicy support for EnumConverter [#47765]

* Moved DictionaryKeyPolicy parsing code to WriteWithQuotes [#47765]

* Refactored the bugfix in accordance to the comments (#47765)

* Made few minor corrections (#47765)
@layomia
Copy link
Contributor

layomia commented Aug 3, 2021

Fixed by #54429.

@layomia layomia closed this as completed Aug 3, 2021
thaystg added a commit to thaystg/runtime that referenced this issue Aug 3, 2021
* origin/main: (64 commits)
  [wasm][debugger] Create test Inherited Properties (dotnet#56754)
  Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
  Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
  [mono] Remove gdb xdebug and binary writer support, it hasn't worked in a while. (dotnet#56759)
  Update windows-requirements.md (dotnet#56476)
  Update doc and generic parameter name for JsonValue.GetValue (dotnet#56639)
  [wasm][debugger] Inspect static class (dotnet#56740)
  Fix stack overflow handling issue in GC stress (dotnet#56733)
  Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported (dotnet#56604)
  Move Windows Compat pack to NuGet pack task (dotnet#56686)
  Fix build error when building some packages (dotnet#56767)
  Simplify JIT shutdown logic in crossgen2 (dotnet#56687)
  Fix race in crossdac publishing with PGO (dotnet#56762)
  Add DictionaryKeyPolicy support for EnumConverter [dotnet#47765] (dotnet#54429)
  Use ComWrappers in some Marshal unit-tests and update platform metadata  (dotnet#56595)
  Set `DisableImplicitNamespaceImports_Dotnet=true` to workaround sdk issue (dotnet#56744)
  Make sure ServerGCHeapDetails is up to date (dotnet#56056)
  [libraries] Reenable System.Diagnostics.DiagnosticSorce.Switches.Tests on mobile (dotnet#56737)
  Disable failing arm64 win10 Graphics.FromHdc tests  (dotnet#56732)
  Match xplat event source conditions (dotnet#56435)
  ...
thaystg added a commit to thaystg/runtime that referenced this issue Aug 4, 2021
…ger_proxy_attribute

* origin/main: (340 commits)
  add RID for Debian 11 (dotnet#56789)
  [wasm] [debugger] Skip thread static field (dotnet#56749)
  Fix timeouts in coreroot_determinism test in GC stress mode (dotnet#56770)
  Use File.OpenHandle in Socket.SendFile directly (dotnet#56777)
  accept empty realm for digest auth (dotnet#56369) (dotnet#56455)
  [wasm][debugger] Create test Inherited Properties (dotnet#56754)
  Mark new test as incompatible with GC Mark4781_1GcStressIncompatible (dotnet#56739)
  Ensure MetadataEnumResult is sufficiently updated by MetaDataImport::Enum (dotnet#56756)
  [mono] Remove gdb xdebug and binary writer support, it hasn't worked in a while. (dotnet#56759)
  Update windows-requirements.md (dotnet#56476)
  Update doc and generic parameter name for JsonValue.GetValue (dotnet#56639)
  [wasm][debugger] Inspect static class (dotnet#56740)
  Fix stack overflow handling issue in GC stress (dotnet#56733)
  Use ReflectionOnly as serialization mode in case dynamic code runtime feature is not supported (dotnet#56604)
  Move Windows Compat pack to NuGet pack task (dotnet#56686)
  Fix build error when building some packages (dotnet#56767)
  Simplify JIT shutdown logic in crossgen2 (dotnet#56687)
  Fix race in crossdac publishing with PGO (dotnet#56762)
  Add DictionaryKeyPolicy support for EnumConverter [dotnet#47765] (dotnet#54429)
  Use ComWrappers in some Marshal unit-tests and update platform metadata  (dotnet#56595)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json bug help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

6 participants