-
-
Notifications
You must be signed in to change notification settings - Fork 41
fix: Remove DateTime in favour of TimeProvider and DateTimeOffset #238
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
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.
Pull request overview
This pull request replaces DateTime with DateTimeOffset and introduces TimeProvider for improved testability and better handling of timezone-aware timestamps throughout the application. The changes align with modern .NET best practices for time handling.
Key changes:
- Introduces
TimeProvideras a singleton service registered in DI for controllable time in tests - Converts
RetrySchedulerfrom a static class to an instance class with TimeProvider dependency - Updates all time-dependent entities and services to use
DateTimeOffsetinstead ofDateTime - Adds
FakeTimeProvidertest helper for deterministic time-based testing - Improves validation logic for timezone handling in EventGridEvent and CloudEvent entities
- Updates Topic handling in EventGridEvent to use internal
SetTopic()method for clearer intent
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Program.cs | Registers TimeProvider.System and RetryScheduler as singletons in DI container |
| SubscriptionSettings.cs | Converts validation expiry tracking from DateTime to DateTimeOffset, changes ValidationPeriodExpired to method accepting current time |
| HttpSubscriberSettings.cs | Mirrors SubscriptionSettings changes for HTTP subscriber validation |
| SasKeyValidator.cs | Adds TimeProvider dependency and uses it for token expiry validation with CultureInfo.InvariantCulture |
| RetryScheduler.cs | Converts from static class to instance class with TimeProvider dependency for testable retry scheduling |
| RetryDeliveryBackgroundService.cs | Injects TimeProvider and RetryScheduler, uses timeProvider.GetUtcNow() throughout for consistent time handling |
| InMemoryDeliveryQueue.cs | Adds TimeProvider dependency for determining due deliveries |
| EventGridSchemaFormatter.cs | Adds TimeProvider dependency for setting EventTime when converting CloudEvents |
| DeliveryPropertyResolver.cs | Updates DateTime parsing to DateTimeOffset with InvariantCulture |
| PendingDelivery.cs | Changes EnqueuedTime and NextAttemptTime to DateTimeOffset, converts IsExpired to method accepting current time |
| EventGridEvent.cs | Updates time validation to use DateTimeOffset, improves timezone detection, adds SetTopic() method with tracking |
| DeliveryAttempt.cs | Changes AttemptTime from DateTime to DateTimeOffset |
| DeadLetterEvent.cs | Changes PublishTime and LastDeliveryAttemptTime to DateTimeOffset |
| CloudEvent.cs | Updates time validation to use DateTimeOffset with improved timezone handling |
| ValidateSubscriptionCommandHandler.cs | Injects TimeProvider for validation period checking |
| ValidateAllSubscriptionsCommandHandler.cs | Injects TimeProvider and uses it for generating validation event timestamps |
| SendNotificationEventsToSubscriberCommandHandler.cs | Updates to use SetTopic() method instead of direct property assignment |
| Test files | Updates all test files to use TimeProvider.System or FakeTimeProvider for deterministic testing |
| FakeTimeProvider.cs | New test helper providing controllable time for testing with Advance() and SetUtcNow() methods |
| .editorconfig | Increases severity of primary constructor preference from suggestion to warning for consistency |
| /// <summary> | ||
| /// Determines whether the event has expired based on TTL. | ||
| /// </summary> | ||
| /// <param name="now" > |
Copilot
AI
Dec 23, 2025
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.
Extra space before the closing angle bracket in the parameter documentation.
| /// <param name="now" > | |
| /// <param name="now"> |
| ) | ||
| { | ||
| var decodedExpiration = expiry.ToString("o"); | ||
| var decodedExpiration = expiry.UtcDateTime.ToString("o"); |
Copilot
AI
Dec 23, 2025
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.
Consider using expiry.ToString("o") directly instead of converting to UtcDateTime first. Since expiry is already a DateTimeOffset, the ToString("o") format specifier will produce the ISO 8601 format with timezone information. The intermediate conversion to UtcDateTime is unnecessary.
| var decodedExpiration = expiry.UtcDateTime.ToString("o"); | |
| var decodedExpiration = expiry.ToString("o"); |
| public DateTimeOffset EnqueuedTime { get; init; } = DateTimeOffset.UtcNow; | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the next scheduled attempt time. | ||
| /// </summary> | ||
| public DateTime NextAttemptTime { get; set; } = DateTime.UtcNow; | ||
| public DateTimeOffset NextAttemptTime { get; set; } = DateTimeOffset.UtcNow; |
Copilot
AI
Dec 23, 2025
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.
The default value DateTimeOffset.UtcNow bypasses the TimeProvider abstraction introduced in this PR. While this works as a fallback, it reduces testability since tests cannot control this timestamp. Consider either removing the default value and making this property required, or ensuring callers always explicitly set this value (as tests already do). For consistency with the TimeProvider pattern, the SendNotificationEventsToSubscriberCommandHandler should set these values explicitly using TimeProvider.
| /// Gets or sets the time of the attempt. | ||
| /// </summary> | ||
| public DateTime AttemptTime { get; init; } = DateTime.UtcNow; | ||
| public DateTimeOffset AttemptTime { get; init; } = DateTimeOffset.UtcNow; |
Copilot
AI
Dec 23, 2025
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.
The default value DateTimeOffset.UtcNow bypasses the TimeProvider abstraction. However, in production code (RetryDeliveryBackgroundService line 134), this value is always explicitly set using timeProvider.GetUtcNow(), so the default is never actually used. Consider removing this default value to make the intent clearer and prevent accidental use of the uncontrolled timestamp.
| public DateTimeOffset AttemptTime { get; init; } = DateTimeOffset.UtcNow; | |
| public DateTimeOffset AttemptTime { get; init; } |
| /// <summary> | ||
| /// Determines if the validation period has expired. | ||
| /// </summary> | ||
| /// <param name="now" > |
Copilot
AI
Dec 23, 2025
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.
Extra space before the closing angle bracket in the parameter documentation.
| public class SubscriptionSettings | ||
| { | ||
| private readonly DateTime _expired = DateTime.UtcNow.AddMinutes(5); | ||
| private readonly DateTimeOffset _createdAt = DateTimeOffset.UtcNow; |
Copilot
AI
Dec 23, 2025
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.
The _createdAt field uses DateTimeOffset.UtcNow directly in the field initializer, bypassing the TimeProvider abstraction that was introduced in this PR. This makes the code untestable since tests cannot control this timestamp. Consider initializing this value through a constructor parameter or passing the creation time when ValidationPeriodExpired is called, similar to how it's done in PendingDelivery.IsExpired.
| /// <summary> | ||
| /// Determines if the validation period has expired. | ||
| /// </summary> | ||
| /// <param name="now" > |
Copilot
AI
Dec 23, 2025
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.
Extra space before the closing angle bracket in the parameter documentation.
| public class HttpSubscriberSettings : ISubscriberSettings | ||
| { | ||
| private readonly DateTime _expired = DateTime.UtcNow.AddMinutes(5); | ||
| private readonly DateTimeOffset _createdAt = DateTimeOffset.UtcNow; |
Copilot
AI
Dec 23, 2025
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.
The _createdAt field uses DateTimeOffset.UtcNow directly in the field initializer, bypassing the TimeProvider abstraction that was introduced in this PR. This makes the code untestable since tests cannot control this timestamp. Consider initializing this value through a constructor parameter or passing the creation time when ValidationPeriodExpired is called, similar to how it's done in PendingDelivery.IsExpired.
No description provided.