gnodet-bot commented on code in PR #2180:
URL: https://github.com/apache/maven-resolver/pull/2180#discussion_r4227160215
##########
maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk11/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporter.java:
##########
@@ -217,13 +229,13 @@ final class JdkTransporter extends AbstractTransporter
implements HttpTransporte
this.connectTimeout =
HttpTransporterUtils.getHttpConnectTimeout(session, repository);
this.requestTimeout =
HttpTransporterUtils.getHttpRequestTimeout(session, repository);
Optional<Boolean> expectContinue =
HttpTransporterUtils.getHttpExpectContinue(session, repository);
- if (javaVersion > 19) {
+ if (isExpectContinueSupported(javaVersion)) {
this.expectContinue = expectContinue.orElse(null);
} else {
this.expectContinue = null;
if (expectContinue.isPresent()) {
LOGGER.warn(
- "Configuration for Expect-Continue set but is ignored
on Java versions below 20 (current java version is {}) due
https://bugs.openjdk.org/browse/JDK-8286171";,
+ "Configuration for Expect-Continue set but is ignored
on Java version {} due https://bugs.openjdk.org/browse/JDK-8286171";,
Review Comment:
⚠️ **Medium — warning message lost threshold context**
The previous message:
> `"Configuration for Expect-Continue set but is ignored on Java versions
below 20 (current java version is {}) due
https://bugs.openjdk.org/browse/JDK-8286171"`
The new message:
> `"Configuration for Expect-Continue set but is ignored on Java version {}
due https://bugs.openjdk.org/browse/JDK-8286171"`
The new message is ambiguous — it no longer tells users *why* their version
is unsupported. Consider something like:
```suggestion
"Configuration for Expect-Continue set but is
ignored on Java version {} (supported on 17.0.17+ and 20+) due
https://bugs.openjdk.org/browse/JDK-8286171";,
```
##########
maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk11/src/test/java/org/eclipse/aether/transport/jdk/JdkTransporterTest.java:
##########
@@ -172,4 +178,41 @@ void testMaximumHttpVersionAtRuntime() throws Exception {
assertEquals(Version.HTTP_2,
jdkTransporter.getHttpVersion(session, remoteRepository));
}
}
+
+ /*
+ @ParameterizedTest
+ @CsvSource({
+ "11.0.20, false",
+ "17.0.16, false",
+ "17.0.17, true",
+ "17.0.18, true",
+ "18.0.2, false",
+ "19.0.2, false",
+ "20, true",
+ "21.0.2, true"
+ })
+ void testIsExpectContinueSupported(String versionStr, boolean
expected) {
+ assertEquals(expected,
JdkTransporter.isExpectContinueSupported(Runtime.Version.parse(versionStr)));
+ }
+ */
+
+ @Test
+ void testPut_ExpectContinueExplicitlyEnabledOnJava17() throws Exception {
+ Runtime.Version version = Runtime.version();
+ Assumptions.assumeTrue(version.feature() == 17 &&
version.compareTo(Runtime.Version.parse("17.0.17")) >= 0);
Review Comment:
⚠️ **High — commented-out parametrized test is the only boundary-case
coverage**
The `/* ... */` block is the only test that exercises all version boundaries
for `isExpectContinueSupported` (11, 17.0.16, 17.0.17, 18, 19, 20, 21). Please
uncomment it and convert it to a proper `@ParameterizedTest`.
The active integration test below
(`testPut_ExpectContinueExplicitlyEnabledOnJava17`) is valid as a regression
test but has narrow coverage: it skips entirely on JDK 18, 19, 20, and 21 via
the `Assumptions.assumeTrue(version.feature() == 17 && ...)` guard. It doesn't
exercise the JDK 20+ path at all.
##########
maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk11/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporter.java:
##########
@@ -134,6 +134,18 @@ final class JdkTransporter extends AbstractTransporter
implements HttpTransporte
"EEE, dd MMM yyyy HH:mm:ss z", Locale.ENGLISH)
.withZone(ZoneId.of("GMT"));
+ private static final Runtime.Version JAVA_17_0_17 =
Runtime.Version.parse("17.0.17");
+
+ static boolean isExpectContinueSupported(Runtime.Version version) {
Review Comment:
💡 **Low — missing Javadoc on `isExpectContinueSupported`**
This method is package-private and has non-trivial logic (JDK 17.0.17
boundary). A short Javadoc explaining the version matrix would help future
readers:
```java
/**
* Returns {@code true} if the given JVM version supports the {@code Expect:
100-continue}
* header. Disabled on JDK 18.x and 19.x (unfixed JDK-8286171), and on JDK
17 before
* 17.0.17 (backport JDK-8364017). Enabled on 17.0.17+, and all versions 20+.
*/
```
--
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]
