Skip to content

Conversation

@martincostello
Copy link
Contributor

@martincostello martincostello commented Oct 9, 2024

Use long instead of int to avoid an implicit conversion and the Y2038 bug.

Resolves #590.


Before the change?

  • An implicit conversion from int to long occurs when creating a DateTimeOffset from a JSON number.
  • Numeric timestamps beyond 2038-01-19T03:14:07Z cannot be deserialized:
System.Text.Json.JsonException : The JSON value could not be converted to System.DateTimeOffset. Path: $ | LineNumber: 0 | BytePositionInLine: 10.
---- System.FormatException : Either the JSON value is not in a supported format, or is out of bounds for an Int32.

ThrowHelper.ReThrowWithPath(ReadStack& state, Utf8JsonReader& reader, Exception ex)
JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 utf8Json, JsonTypeInfo jsonTypeInfo, Nullable`1 actualByteCount)
JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 json, JsonTypeInfo jsonTypeInfo)
JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
DateTimeOffsetConverterTests.CanDeserializeNumberY2038() line 50
----- Inner Stack Trace -----
Utf8JsonReader.GetInt32()
DateTimeOffsetConverter.HandleNumber(Utf8JsonReader reader) line 28
DateTimeOffsetConverter.ReadInternal(Utf8JsonReader& reader) line 14
DateTimeOffsetConverter.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options) line 6
JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)

After the change?

  • No implicit conversion occurs.
  • Values after 2038-01-19T03:14:07Z can be deserialized

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

  • Yes
  • No

Use `long` instead of `int` to avoid an implicit conversion and the Y2038 bug.
@github-actions
Copy link

github-actions bot commented Oct 9, 2024

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

Add unit tests for handling 64-bit integer dates.
@martincostello martincostello marked this pull request as ready for review October 9, 2024 12:53
@JamieMagee
Copy link
Member

Thanks for catching this! I wonder if anyone will be using this code in 2038 😅

I wonder if this would be a good Roslyn analyzer?

@JamieMagee JamieMagee merged commit 62c861d into octokit:main Oct 9, 2024
7 checks passed
@martincostello martincostello deleted the patch-1 branch October 9, 2024 14:01
@martincostello
Copy link
Contributor Author

I wonder if anyone will be using this code in 2038 😅

I had the same thought, but you know what they say:

image

I wonder if this would be a good Roslyn analyzer?

🤔 maybe - I'll raise an issue with the suggestion and see what folks think.

@martincostello
Copy link
Contributor Author

I'll raise an issue with the suggestion and see what folks think.

dotnet/runtime#108712

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

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[BUG]: DateTimeOffsetConverter uses 32-bit integer instead of 64-bit

2 participants