gnodet-bot commented on code in PR #599:
URL: https://github.com/apache/maven-jar-plugin/pull/599#discussion_r4084138194
##########
src/main/java/org/apache/maven/plugins/jar/AbstractJarMojo.java:
##########
@@ -270,11 +288,28 @@ protected String getOutputTimestamp() {
for (int i = time.length(); --i >= 0; ) {
char c = time.charAt(i);
if ((c < '0' || c > '9') && (i != 0 || c != '-')) {
+ // Not a plain integer — treat as ISO 8601 and pass through
as-is.
return time;
}
}
+ // Plain integer: convert from seconds to ISO 8601, clamping to
EPOCH_MIN if needed.
try {
- return Instant.ofEpochSecond(Long.parseLong(time)).toString();
+ long seconds = Long.parseLong(time);
+ if (seconds < EPOCH_MIN) {
+ Instant instant = Instant.ofEpochSecond(seconds);
Review Comment:
⚠️ **Correctness: `DateTimeException` here produces a misleading error
message**
`Instant.ofEpochSecond(seconds)` throws `DateTimeException` when `seconds`
falls outside the range Instant can represent (roughly ±1 billion years, i.e.
`seconds < -31_557_014_167_219_200L`). That exception is then caught on line
313 and wrapped as `"Timestamp \"X\" is not a number of seconds."` — which is
false: `X` is a valid long integer, it's just astronomically old.
In practice a user passing `-9223372036854775808` (Long.MIN_VALUE) would hit
this path. The fix is to guard the `Instant.ofEpochSecond(seconds)` call, or to
clamp before instantiation:
```suggestion
Instant instant = Instant.ofEpochSecond(Math.max(seconds,
Instant.MIN.getEpochSecond()));
```
Alternatively, skip the instantiation in the warning path entirely (since
all we need for display is the original `time` string and `EPOCH_MIN`) and
avoid the possibility altogether:
```java
log.warn(() -> "Output timestamp \"" + time + "\" is before the minimum
value accepted by the jar tool ("
+ Instant.ofEpochSecond(EPOCH_MIN) + "). Clamping to minimum. "
+ "If you use SOURCE_DATE_EPOCH=0, set it to at least " + EPOCH_MIN
+ " (1980-01-01T00:00:02Z).");
return Instant.ofEpochSecond(EPOCH_MIN).toString();
```
(This also removes the redundant second `Instant.ofEpochSecond(EPOCH_MIN)`
call — see next comment.)
##########
src/main/java/org/apache/maven/plugins/jar/AbstractJarMojo.java:
##########
@@ -270,11 +288,28 @@ protected String getOutputTimestamp() {
for (int i = time.length(); --i >= 0; ) {
char c = time.charAt(i);
if ((c < '0' || c > '9') && (i != 0 || c != '-')) {
+ // Not a plain integer — treat as ISO 8601 and pass through
as-is.
return time;
}
}
+ // Plain integer: convert from seconds to ISO 8601, clamping to
EPOCH_MIN if needed.
try {
- return Instant.ofEpochSecond(Long.parseLong(time)).toString();
+ long seconds = Long.parseLong(time);
+ if (seconds < EPOCH_MIN) {
+ Instant instant = Instant.ofEpochSecond(seconds);
+ log.warn("Output timestamp \""
+ + time
+ + "\" (resolved to "
+ + instant
+ + ") is before the minimum value accepted by the jar
tool ("
+ + Instant.ofEpochSecond(EPOCH_MIN)
+ + "). Clamping to minimum. "
+ + "If you use SOURCE_DATE_EPOCH=0, set it to at least "
+ + EPOCH_MIN
+ + " (1980-01-01T00:00:02Z).");
+ return Instant.ofEpochSecond(EPOCH_MIN).toString();
Review Comment:
🔧 **Minor: `Instant.ofEpochSecond(EPOCH_MIN)` is called twice + `log.warn`
is eager**
Lines 305 and 310 both call `Instant.ofEpochSecond(EPOCH_MIN)` — two
identical allocations for the same constant. Additionally,
`log.warn(CharSequence)` eagerly builds the concatenated string even when WARN
is disabled; `Log.warn(Supplier<String>)` is available (the test mock already
implements it) and should be used here.
Suggested refactor of lines 299–310:
```suggestion
String minIso = Instant.ofEpochSecond(EPOCH_MIN).toString();
log.warn(() -> "Output timestamp \"" + time
+ "\" is before the minimum value accepted by the
jar tool ("
+ minIso + "). Clamping to minimum. "
+ "If you use SOURCE_DATE_EPOCH=0, set it to at
least "
+ EPOCH_MIN + " (1980-01-01T00:00:02Z).");
return minIso;
```
(This also eliminates the `Instant.ofEpochSecond(seconds)` call on line 299
that can throw `DateTimeException` for astronomically negative values — see
prior comment.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
