Skip to content

Conversation

@eiriktsarpalis
Copy link
Member

Fix #53539.

@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone May 10, 2022
@eiriktsarpalis eiriktsarpalis requested review from krwq and layomia May 10, 2022 21:07
@eiriktsarpalis eiriktsarpalis self-assigned this May 10, 2022
@ghost
Copy link

ghost commented May 10, 2022

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented May 10, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #53539.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json, new-api-needs-documentation

Milestone: 7.0.0

@piekstra
Copy link

@eiriktsarpalis will this address serialization failures when DateOnly is used as a key in a dictionary on an object? I've added a custom DateOnly JsonConverter that works well when there are DateOnly properties on an object, but fails for some reason when one of the properties is Dictionary<DateOnly, decimal>

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented May 11, 2022

@eiriktsarpalis will this address serialization failures when DateOnly is used as a key in a dictionary on an object? I've added a custom DateOnly JsonConverter that works well when there are DateOnly properties on an object, but fails for some reason when one of the properties is Dictionary<DateOnly, decimal>

Yes, the DateOnly converter comes with support for dictionary keys. In the meantime, if you are running .NET 6 it should be possible to add support for key serialization in your custom converter by overriding the WriteAsPropertyName/ReadAsPropertyName methods.

@piekstra
Copy link

Yes, the DateOnly converter comes with support for dictionary keys. In the meantime, if you are running .NET 6 it should be possible to add support for key serialization in your custom converter by overriding the WriteAsPropertyName/ReadAsPropertyName methods.

Thank you! That was the piece I was missing. Works great now 😄

@eiriktsarpalis eiriktsarpalis requested a review from tarekgh May 11, 2022 19:00
@eiriktsarpalis eiriktsarpalis requested review from krwq and layomia May 12, 2022 16:31
public int Year;
public int Month;
public int Day;
public bool IsCalendarDateOnly;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it would help with #69447 (comment), but defining this field here will increase the size of this struct from 40 bytes to 48 bytes on 64-bit. If you instead move this field to the end, the size should stay at 40.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not seeing a difference in benchmarks when I rearrange the fields, but I could try pushing the change and see if it registers in the performance infrastructure.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support DateOnly and TimeOnly in JsonSerializer

9 participants