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

toJson serialization behaves incorrectly for dates on Windows, Linux, and Android #5451

Closed
sync-by-unito bot opened this issue Apr 29, 2022 · 9 comments · Fixed by #6777
Closed

toJson serialization behaves incorrectly for dates on Windows, Linux, and Android #5451

sync-by-unito bot opened this issue Apr 29, 2022 · 9 comments · Fixed by #6777
Assignees

Comments

@sync-by-unito
Copy link

sync-by-unito bot commented Apr 29, 2022

We're testing round-tripping date properties in the dart SDK and noticed that json serialization behaves inconsistently. For example, the date -0001-01-01 00:00:00 - I.e. year 1 BCE is encoded as -001-01-01 00:00:00 on Apple platforms, but as empty string on Windows (looks like all dates equal to or below 0000-01-01 00:00:00 get encoded as empty on Windows).

The date 0000-01-01 00:00:00 gets encoded as 0000-01-01 00:00:00 on Apple platforms but 0-01-01 00:00:00 (note the lack of leading zeroes) and as 1905-06-06 18:35:44 on Android.

The date 27182104-21 00:00:00 gets encoded as 2016-09-19 11:52:32 on Android, but correctly on Apple platforms.

It is possible that there's also a bug in the dart SDK in the way we represent dates on some platforms, but I verified that the reported that it's a serialization bug on windows (Studio correctly visualizes the expected date) and non-negative/zero dates work just fine, so there are strong indications that the bug is in the to_json implementation in Core.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Apr 29, 2022

➤ Jonathan Reams commented:

At least on windows, the function that converts a time_t to a struct tm (which is what you use to format a date/time) considers timestamps less than zero to be an error. That's why its encoded as empty on windows. I imagine android has similar issues.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Apr 29, 2022

➤ Nikola Irinchev commented:

For prioritization purposes, I'd like to clarify that to_json is not a public API in the dart SDK and we're using it in tests to verify that the data that we pass in from dart matches the data that Core receives. It may be a public API in other SDKs though as I've seen some java tickets relating to it. cc [~[email protected]]

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Apr 29, 2022

➤ Christan Melchior commented:

Kotlin is just using it internally for testing as well, and only for Credentials, so we are not checking anything around timestamps.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented May 3, 2022

➤ James Stone commented:

[~[email protected]] can we change the format of timestamps printed for json, or is that going to break the E2E tests?

@sync-by-unito
Copy link
Author

sync-by-unito bot commented May 3, 2022

➤ Tyler Kaye commented:

It will affect our fuzz tests only, but we can make a change in how that is decoded fairly easily. This is just the realm2json tool?

@jedelbo
Copy link
Contributor

jedelbo commented Aug 29, 2022

@ironage @nirinchev Are we going to fix this any time soon or can we close this as a non-issue?

@nirinchev
Copy link
Member

I'd rather that we fix it as that's the most convenient way to verify that the SDK and Core agree on what data is being stored.

@jedelbo
Copy link
Contributor

jedelbo commented Aug 4, 2023

The fix has been reverted

@sync-by-unito sync-by-unito bot assigned jedelbo and unassigned nicola-cab Aug 7, 2023
@jedelbo
Copy link
Contributor

jedelbo commented Aug 9, 2023

But now fixed by #6862

@jedelbo jedelbo closed this as completed Aug 9, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants