-
-
Notifications
You must be signed in to change notification settings - Fork 187
Fix ArgumentOutOfRangeException when using DateTime.MinValue/MaxValue with timezone offsets #843
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
Conversation
… edge cases Co-authored-by: sebastienros <[email protected]>
Co-authored-by: sebastienros <[email protected]>
|
|
||
| var context = new TemplateContext { TimeZone = plusTwoTimezone }; | ||
| context.SetValue("foo", "bar"); | ||
| context.SetValue("date", DateTime.MinValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot what it the value is DateTime.MinValue + 1hour, there would still be an exception. The constructor need to check if there is an overflow, not just special values like MinValue and MaxValue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the implementation to use a try-catch approach instead of checking for exact MinValue/MaxValue. Now handles any DateTime value that would cause overflow when converted to DateTimeOffset, including cases like DateTime.MinValue + 1 hour.
Added tests for these scenarios:
- DateTime.MinValue + 1 hour with +2 offset
- DateTime.MaxValue - 1 hour with -2 offset
Fixed in commit 710c03e
…catch Changed from checking exact MinValue/MaxValue to using try-catch approach that handles any DateTime value that would cause overflow when converted to DateTimeOffset with timezone offset. Added tests for DateTime.MinValue + 1 hour and DateTime.MaxValue - 1 hour scenarios. Co-authored-by: sebastienros <[email protected]>
| _value = value; | ||
| } | ||
|
|
||
| public DateTimeValue(DateTime value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Check for values less than one day of the boundaries before doing the try/catch. Otherwise just do the conversion directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Check for values less than one day of the boundaries before doing the try/catch. Otherwise just do the conversion directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimized the implementation to check if the value is within one day of the boundaries before using try-catch. Normal DateTime values now use direct conversion without exception overhead, while values near MinValue/MaxValue still use try-catch to handle potential overflow.
Fixed in commit 4b66dc8
Check if value is within one day of MinValue or MaxValue before using try-catch. For normal DateTime values (the common case), use direct conversion to avoid exception overhead. This improves performance while still handling edge cases correctly. Co-authored-by: sebastienros <[email protected]>
Fix exception when using DateTime.MinValue/MaxValue with timezone offsets
Summary
Successfully fixed the
ArgumentOutOfRangeExceptionthat occurred when usingDateTimevalues nearMinValueorMaxValuein environments with non-UTC timezone offsets.Changes Made
DateTimeconstructor inDateTimeValueclass with optimized approachArgumentOutOfRangeExceptionand maps to appropriate boundary valueShouldHandleDateTimeMinValueWithPositiveTimezoneOffset: Tests DateTime.MinValue with +2 offsetShouldHandleDateTimeNearMinValueWithPositiveTimezoneOffset: Tests DateTime.MinValue + 1 hour with +2 offsetShouldHandleDateTimeMaxValueWithNegativeTimezoneOffset: Tests DateTime.MaxValue with -2 offsetShouldHandleDateTimeNearMaxValueWithNegativeTimezoneOffset: Tests DateTime.MaxValue - 1 hour with -2 offsetTesting
The fix was validated by:
Root Cause
When a
DateTimewithKind.Unspecifiedis implicitly converted toDateTimeOffset, it uses the local system timezone. For DateTime values near the boundaries with incompatible timezone offsets, the UTC representation falls outside the valid range, causingArgumentOutOfRangeException.Performance
The implementation is optimized to avoid exception overhead for the common case:
Normal dates (not near boundaries): Direct conversion without try-catch
Dates within 1 day of MinValue: Try-catch to handle potential underflow
Dates within 1 day of MaxValue: Try-catch to handle potential overflow
Fixes Exception on DateTime.MinValue #773
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.