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

Fix date time serialization to use ISO8601 #664

Merged
merged 2 commits into from
Nov 7, 2024
Merged

Conversation

mtmk
Copy link
Collaborator

@mtmk mtmk commented Oct 22, 2024

Doesn't make sense to serialize datetime types with the default culture dependent formatting since it might differ between different platforms. with this PR we use ISO8601 formatting as indicated by O to maximize the compatibility among different platforms and languages.

@mtmk mtmk added the breaking label Oct 22, 2024
Copy link
Collaborator

@to11mtm to11mtm left a comment

Choose a reason for hiding this comment

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

Nice and clean. Should we be having any sort of 'fallback' for backward compat or is it not needed?

if (input.Offset == TimeSpan.Zero)
{
// This will make it place `Z` instead of `+00:00` at the end
result = Utf8Formatter.TryFormat(input.UtcDateTime, span, out written, new StandardFormat('O'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Serialization is a hot path, so it's worth noting one thing here...

SharpLab seems to show better ASM for a readonly prop being referenced vs new even for simple case

Looks like Ctor can throw which may prevent some inlining However for all of this pedanticism, it may or may not get handled by PGO when it's actually run. Might be worth trying to run a microbench between the two, while also noting 'real' implementation of a readonly based on gist could be static (alas, Sharplab can't do statics easily) and likely could provide additional optimizations/benefits.

@mtmk
Copy link
Collaborator Author

mtmk commented Oct 25, 2024

Should we be having any sort of 'fallback' for backward compat or is it not needed?

I'm guessing not many if anyone is using this feature i.e. passing DateTime/Offset as data. I'd consider the old implementation as a bug since it would only work both sender and receiver of the data are using the same culture. e.g. if one using US and other using UK many dates will be muddled up e.g. 10/12/2024 might mean October or December.

so I'm seeing this as a bug fix and although it might break some installations I'm assuming nobody is really using this hence it's not worth the effort and confusion trying to create compatibility. But I might be wrong - I am at least 4-5 times a day 😅

@caleblloyd
Copy link
Collaborator

Looks good, it appears that the ISO8601 format used also overlaps rfc3339 which is beneficial interop with other languages too

@mtmk mtmk merged commit 7c00b41 into main Nov 7, 2024
13 checks passed
@mtmk mtmk deleted the fix-date-time-serializer branch November 7, 2024 17:47
mtmk added a commit that referenced this pull request Nov 13, 2024
* Add OnSocketAvailableAsync Hook (#647)
* Refactor exception handling in NatsException (#677)
* Fixed an exception which happens when PutAsync is used more than once and activity logging is enabled in main project (#675)
* open Header.Writer class and create GetBytesLength method (#674)
* Fix date time serialization to use ISO8601 (#664)
@mtmk mtmk mentioned this pull request Nov 13, 2024
mtmk added a commit that referenced this pull request Nov 13, 2024
* Add OnSocketAvailableAsync Hook (#647)
* Refactor exception handling in NatsException (#677)
* Fixed an exception which happens when PutAsync is used more than once and activity logging is enabled in main project (#675)
* open Header.Writer class and create GetBytesLength method (#674)
* Fix date time serialization to use ISO8601 (#664)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants