-
-
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
Changes from all commits
eb3cf49
c242c53
5c159a4
710c03e
4b66dc8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,45 @@ public DateTimeValue(DateTimeOffset value) | |
| _value = value; | ||
| } | ||
|
|
||
| public DateTimeValue(DateTime value) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| { | ||
| // Handle edge cases where DateTime cannot be safely converted to DateTimeOffset | ||
| // with local timezone offset due to overflow (e.g., DateTime.MinValue with positive offset) | ||
|
|
||
| // Check if the value is within one day of the boundaries where overflow might occur | ||
| if (value <= DateTime.MinValue.AddDays(1)) | ||
| { | ||
| // Value is close to MinValue - attempt conversion with try-catch | ||
| try | ||
| { | ||
| _value = value; | ||
| } | ||
| catch (ArgumentOutOfRangeException) | ||
| { | ||
| // Offset caused underflow - use minimum boundary | ||
| _value = DateTimeOffset.MinValue; | ||
| } | ||
| } | ||
| else if (value >= DateTime.MaxValue.AddDays(-1)) | ||
| { | ||
| // Value is close to MaxValue - attempt conversion with try-catch | ||
| try | ||
| { | ||
| _value = value; | ||
| } | ||
| catch (ArgumentOutOfRangeException) | ||
| { | ||
| // Offset caused overflow - use maximum boundary | ||
| _value = DateTimeOffset.MaxValue; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // Normal case - direct conversion without try-catch overhead | ||
| _value = value; | ||
| } | ||
| } | ||
|
|
||
| public override FluidValues Type => FluidValues.DateTime; | ||
|
|
||
| public override bool Equals(FluidValue other) | ||
|
|
||
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:
Fixed in commit 710c03e