Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a Daylight Saving Time (DST) issue in the Spring Cloud Azure Feature Management “recurring time window” logic, aiming to make recurrence validation/evaluation behave consistently across DST transitions.
Changes:
- Update recurrence validation to use UTC when computing week-based gaps (avoids DST-dependent day lengths).
- Add DST-focused unit tests for
RecurrenceValidatorandRecurrenceEvaluator. - Add DST-handling logic in weekly recurrence evaluation when comparing fixed-offset start times with region-based “now” times.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
sdk/spring/spring-cloud-azure-feature-management/src/main/java/com/azure/spring/cloud/feature/management/implementation/timewindow/recurrence/RecurrenceValidator.java |
Switches “today” computation to UTC to avoid DST affecting duration/gap calculations. |
sdk/spring/spring-cloud-azure-feature-management/src/main/java/com/azure/spring/cloud/feature/management/implementation/timewindow/recurrence/RecurrenceEvaluator.java |
Adds DST-aware zone conversion logic for weekly recurrence computations. |
sdk/spring/spring-cloud-azure-feature-management/src/test/java/com/azure/spring/cloud/feature/management/filters/recurrence/RecurrenceValidatorDSTTest.java |
Introduces DST-related validation tests (currently mostly “no exception” assertions). |
sdk/spring/spring-cloud-azure-feature-management/src/test/java/com/azure/spring/cloud/feature/management/filters/recurrence/RecurrenceEvaluatorDSTTest.java |
Introduces DST-related evaluation tests for fixed-offset vs region-zone comparisons. |
...com/azure/spring/cloud/feature/management/filters/recurrence/RecurrenceEvaluatorDSTTest.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Test that validator uses UTC for today's date calculation. | ||
| * This ensures consistency regardless of the system's default timezone | ||
| * or DST transitions. | ||
| */ | ||
| @Test | ||
| public void validateSettingsUsesUTCForDateCalculation() { | ||
| final TimeWindowFilterSettings settings = new TimeWindowFilterSettings(); | ||
| final RecurrencePattern pattern = new RecurrencePattern(); | ||
| final RecurrenceRange range = new RecurrenceRange(); | ||
| final Recurrence recurrence = new Recurrence(); | ||
|
|
||
| pattern.setType("Weekly"); | ||
| pattern.setDaysOfWeek(List.of("Monday", "Wednesday", "Friday")); | ||
| pattern.setFirstDayOfWeek("Monday"); | ||
| pattern.setInterval(1); | ||
|
|
||
| recurrence.setRange(range); | ||
| recurrence.setPattern(pattern); | ||
|
|
||
| // Start on a Monday | ||
| settings.setStart("2024-03-04T10:00:00+01:00"); // Monday | ||
| settings.setEnd("2024-03-04T12:00:00+01:00"); | ||
| settings.setRecurrence(recurrence); | ||
|
|
||
| // Should not throw exception, uses UTC internally for validation | ||
| assertDoesNotThrow(() -> RecurrenceValidator.validateSettings(settings), | ||
| "Validator should use UTC and not throw exception"); | ||
| } |
There was a problem hiding this comment.
These validator DST tests currently only assert that validateSettings doesn’t throw for very short windows (1–2 hours). That will pass regardless of whether the DST gap is treated as 23/24/25 hours, so it doesn’t actually validate the UTC/DST fix.
To make this test meaningful, use a window duration close to the minimum day gap (e.g., between 23h and 24h) and drive the reference “today/week” used by isDurationCompliantWithDaysOfWeek deterministically (e.g., by refactoring the validator to accept a Clock/reference ZonedDateTime for testing).
| final long numberOfInterval = Duration.between(firstDayOfFirstWeek, now).toSeconds() | ||
| / Duration.ofDays((long) interval * RecurrenceConstants.DAYS_PER_WEEK).toSeconds(); | ||
| final ZonedDateTime firstDayOfMostRecentOccurringWeek = firstDayOfFirstWeek.plusDays( | ||
| ZonedDateTime firstDayOfMostRecentOccurringWeek = firstDayOfFirstWeek.plusDays( | ||
| numberOfInterval * (interval * RecurrenceConstants.DAYS_PER_WEEK)); | ||
|
|
There was a problem hiding this comment.
numberOfInterval is computed using Duration.between(...).toSeconds() / Duration.ofDays(...).toSeconds(). Around DST transitions this can undercount by 1 when the local wall-clock time matches the weekly boundary but the underlying instants differ by +/-1 hour (e.g., 3 weeks minus 1 hour after spring-forward). This means the “most recent occurring week” can be shifted back a week, which defeats the DST fix.
Consider computing the interval count using calendar-based units (e.g., ChronoUnit.DAYS/WEEKS between LocalDates after aligning now/firstDayOfFirstWeek to the same intended zone), instead of seconds-based duration division.
...com/azure/spring/cloud/feature/management/filters/recurrence/RecurrenceEvaluatorDSTTest.java
Outdated
Show resolved
Hide resolved
| final long numberOfInterval = Duration.between(firstDayOfFirstWeek, now).toSeconds() | ||
| / Duration.ofDays((long) interval * RecurrenceConstants.DAYS_PER_WEEK).toSeconds(); | ||
| final ZonedDateTime firstDayOfMostRecentOccurringWeek = firstDayOfFirstWeek.plusDays( | ||
| ZonedDateTime firstDayOfMostRecentOccurringWeek = firstDayOfFirstWeek.plusDays( | ||
| numberOfInterval * (interval * RecurrenceConstants.DAYS_PER_WEEK)); | ||
|
|
There was a problem hiding this comment.
numberOfInterval is computed using Duration.between(...).toSeconds() / Duration.ofDays(...).toSeconds(), which is DST-sensitive when start is a fixed ZoneOffset but now is a region-based ZoneId. Across a spring-forward transition, the elapsed seconds for an exact N-week boundary can be 1 hour short, causing an off-by-one interval (and a wrong previous occurrence) even after the later zone conversion. Consider basing the interval calculation on calendar weeks/dates in the chosen evaluation zone (e.g., normalize to the target zone first, then use ChronoUnit.WEEKS/ChronoUnit.DAYS on LocalDate) and apply the same approach to the daily path too (it has the same seconds-per-day division issue).
...com/azure/spring/cloud/feature/management/filters/recurrence/RecurrenceEvaluatorDSTTest.java
Outdated
Show resolved
Hide resolved
| // Handle DST transitions: If the calculated week start has a fixed offset (ZoneOffset) | ||
| // and 'now' has a region zone, check if they represent the same geographic location. | ||
| // We do this by checking if the fixed offset matches what the region zone's offset | ||
| // was at the *original start time*. If it matches, they're in the same timezone, | ||
| // and we should convert to the region zone for DST-aware comparisons. | ||
| if (firstDayOfMostRecentOccurringWeek.getZone() instanceof ZoneOffset | ||
| && !(now.getZone() instanceof ZoneOffset)) { | ||
| // Check if the fixed offset matches the region zone's offset at the *start* instant | ||
| // (not at firstDayOfMostRecentOccurringWeek's instant, which might have crossed DST) | ||
| ZoneOffset offsetAtStart = now.getZone().getRules().getOffset(start.toInstant()); | ||
| if (start.getOffset().equals(offsetAtStart)) { | ||
| // Same geographic location, convert to region zone for DST-aware comparisons | ||
| firstDayOfMostRecentOccurringWeek = firstDayOfMostRecentOccurringWeek | ||
| .withZoneSameLocal(now.getZone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The PR description/checklist indicates the module CHANGELOG hasn’t been updated. Since this PR fixes a customer-visible DST bug in recurrence evaluation/validation, please add an entry under “## 7.1.0-beta.1 (Unreleased) -> Bugs Fixed” in sdk/spring/spring-cloud-azure-feature-management/CHANGELOG.md.
Description
Fixes an issue where timezone info doesn't work around Daylight Savings time for the recurring time filter.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines