Chore/improve datetime conversion - #3
Conversation
Refactor the `toJodaDateTime` method to explicitly construct `org.joda.time.DateTime` from `LocalDateTime` components. This avoids reliance on `ZoneId.systemDefault()` and `Instant`, making the conversion more direct and preventing potential timezone-related ambiguities for local date and time values.
…ers (including zero)
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable SERVER_PORT environment variable in the Dockerfile, updates the health check and documentation, and adds @PositiveOrZero validation constraints to the kwh and powerLimit fields in TokenRequest with corresponding unit tests. It also refactors the conversion to Joda DateTime in StrategySupport. Feedback points out that constructing the Joda DateTime without an explicit timezone can cause runtime crashes (IllegalInstantException) during Daylight Saving Time (DST) transitions, and recommends specifying UTC instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return new DateTime( | ||
| issueDate.getYear(), | ||
| issueDate.getMonthValue(), | ||
| issueDate.getDayOfMonth(), | ||
| issueDate.getHour(), | ||
| issueDate.getMinute(), | ||
| issueDate.getSecond(), | ||
| issueDate.get(ChronoField.MILLI_OF_SECOND) | ||
| ); |
There was a problem hiding this comment.
Using the Joda-Time DateTime constructor without specifying a timezone implicitly uses the system's default timezone (DateTimeZone.getDefault()). If the server is running in a timezone that observes Daylight Saving Time (DST), this constructor will throw an org.joda.time.IllegalInstantException if the requested issueDate falls within a DST gap (when clocks jump forward and that local time does not exist).
This is a regression compared to the original code, which used LocalDateTime.atZone(ZoneId.systemDefault()) and automatically adjusted the local time forward to a valid instant instead of throwing an exception.
To prevent runtime crashes during DST transitions and to ensure consistent, timezone-independent token generation (since STS standard TID calculations rely on exact wall-clock minutes since a base date), it is highly recommended to construct the Joda DateTime in UTC (DateTimeZone.UTC).
return new DateTime(
issueDate.getYear(),
issueDate.getMonthValue(),
issueDate.getDayOfMonth(),
issueDate.getHour(),
issueDate.getMinute(),
issueDate.getSecond(),
issueDate.get(ChronoField.MILLI_OF_SECOND),
org.joda.time.DateTimeZone.UTC
);Explicitly sets `issueDate` and internal base dates to UTC when converting to `JodaTime.DateTime`. This ensures consistent token generation independent of the server's default timezone and aligns documentation across the README, Javadoc, and Swagger with this behavior.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the application to interpret token issue dates consistently as UTC instead of using the system default timezone, and adds non-negativity validation constraints (@PositiveOrZero) to the kwh and powerLimit fields in TokenRequest. Additionally, the Dockerfile is updated to support custom ports via a SERVER_PORT environment variable. The reviewer suggested simplifying the LocalDateTime to Joda DateTime conversion in StrategySupport by directly converting to UTC epoch milliseconds and adding a defensive null check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
No description provided.