Copilot commented on code in PR #2211:
URL: https://github.com/apache/maven-resolver/pull/2211#discussion_r4323275075
##########
maven-resolver-transport-url/src/main/java/org/eclipse/aether/transport/url/UrlTransporter.java:
##########
@@ -165,11 +167,8 @@ public UrlTransporter(
UrlTransporterConfigurationKeys.DEFAULT_REDIRECT_MODE,
UrlTransporterConfigurationKeys.CONFIG_PROP_REDIRECT_MODE +
"." + repository.getId(),
UrlTransporterConfigurationKeys.CONFIG_PROP_REDIRECT_MODE));
Review Comment:
`UrlTransporterConfigurationKeys.CONFIG_PROP_REDIRECT_MODE` and
`CONFIG_PROP_REDIRECT_ALLOW_DOWNGRADE` are now marked `@Deprecated`, but this
class doesn’t suppress deprecation warnings like the Jetty/Apache/JDK
transporters do. If the build treats warnings as errors, this can break
compilation. Consider adding a targeted `@SuppressWarnings(\"deprecation\")`
near the constructor/initialization that references these deprecated keys (or
refactor to avoid referencing deprecated constants directly, while still
supporting them as fallback strings).
##########
maven-resolver-util/src/test/java/org/eclipse/aether/util/connector/transport/http/HttpTransporterUtilsTest.java:
##########
@@ -21,15 +21,78 @@
import java.net.URI;
import java.net.URISyntaxException;
+import org.eclipse.aether.ConfigurationProperties;
+import org.eclipse.aether.DefaultRepositorySystemSession;
+import org.eclipse.aether.internal.test.util.TestUtils;
import org.eclipse.aether.repository.RemoteRepository;
import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
public class HttpTransporterUtilsTest {
+ @Test
+ void getHttpOriginScopedHeaders() {
Review Comment:
These test method names use a `get...` prefix but are exercising boolean
`is...` methods (`isHttpOriginScopedHeaders`, `isHttpFollowInsecureRedirect`).
Renaming to `isHttpOriginScopedHeaders` / `isHttpFollowInsecureRedirect` (or
similar) would better match what’s being tested and improve readability.
##########
maven-resolver-test-http/src/main/java/org/eclipse/aether/internal/test/util/http/HttpServer.java:
##########
@@ -840,10 +840,12 @@ public boolean handle(Request req, Response response,
Callback callback) throws
return false;
}
StringBuilder location = new StringBuilder(128);
- String scheme = Request.getParameters(req).getValue("scheme");
+ var parameters = Request.getParameters(req);
+ String scheme = parameters.getValue("scheme");
+ String host = parameters.getValue("host");
Review Comment:
Use of `var` requires Java 10+ source level. If this module is compiled with
an older `--release`/source level (common for libraries that support Java 8),
this will fail to compile. Prefer an explicit type here (e.g., the concrete
type returned by `Request.getParameters(req)`), which preserves compatibility
across source levels.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/connector/transport/http/HttpTransporterUtils.java:
##########
@@ -115,6 +115,105 @@ public static Map<String, String>
getHttpHeaders(RepositorySystemSession session
ConfigurationProperties.HTTP_HEADERS);
}
+ /**
+ * Checks whether two URIs have the same HTTP origin (scheme, host and
effective port).
+ *
+ * @param origin the repository origin
+ * @param target the URI to compare with the origin
+ * @return {@code true} when both URIs have the same HTTP origin
+ * @since 2.0.25
+ */
+ public static boolean isSameOrigin(URI origin, URI target) {
+ if (origin.getScheme() == null
+ || origin.getHost() == null
+ || target.getScheme() == null
+ || target.getHost() == null) {
+ return false;
+ }
+ return origin.getScheme().equalsIgnoreCase(target.getScheme())
+ && origin.getHost().equalsIgnoreCase(target.getHost())
+ && effectivePort(origin) == effectivePort(target);
+ }
+
+ private static int effectivePort(URI uri) {
+ if (uri.getPort() >= 0) {
+ return uri.getPort();
+ }
+ if ("https".equalsIgnoreCase(uri.getScheme())) {
+ return 443;
+ }
+ return "http".equalsIgnoreCase(uri.getScheme()) ? 80 : -1;
+ }
Review Comment:
`isSameOrigin` is documented as comparing *HTTP* origins, but it can return
`true` for non-HTTP schemes when both URIs have the same scheme/host and no
explicit port (because `effectivePort()` returns `-1` for unknown schemes on
both sides). This can produce surprising results and weakens the “HTTP origin”
contract. Consider returning `false` unless both schemes are `http`/`https` (or
alternatively make `effectivePort()` return a sentinel that never matches
and/or gate the comparison on supported schemes).
##########
maven-resolver-util/src/test/java/org/eclipse/aether/util/connector/transport/http/HttpTransporterUtilsTest.java:
##########
@@ -21,15 +21,78 @@
import java.net.URI;
import java.net.URISyntaxException;
+import org.eclipse.aether.ConfigurationProperties;
+import org.eclipse.aether.DefaultRepositorySystemSession;
+import org.eclipse.aether.internal.test.util.TestUtils;
import org.eclipse.aether.repository.RemoteRepository;
import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
public class HttpTransporterUtilsTest {
+ @Test
+ void getHttpOriginScopedHeaders() {
+ DefaultRepositorySystemSession session = TestUtils.newSession();
+ RemoteRepository repository = new RemoteRepository.Builder("repo", "",
"https://host.com";).build();
+
+ assertTrue(HttpTransporterUtils.isHttpOriginScopedHeaders(session,
repository));
+
+
session.setConfigProperty(ConfigurationProperties.HTTP_ORIGIN_SCOPED_HEADERS,
false);
+ assertFalse(HttpTransporterUtils.isHttpOriginScopedHeaders(session,
repository));
+
+
session.setConfigProperty(ConfigurationProperties.HTTP_ORIGIN_SCOPED_HEADERS +
".repo", true);
+ assertTrue(HttpTransporterUtils.isHttpOriginScopedHeaders(session,
repository));
+
+ session = TestUtils.newSession();
+
session.setConfigProperty("aether.transport.apache.originScopedHeaders.repo",
false);
+ assertFalse(HttpTransporterUtils.isHttpOriginScopedHeaders(
+ session, repository,
"aether.transport.apache.originScopedHeaders"));
+
+
session.setConfigProperty(ConfigurationProperties.HTTP_ORIGIN_SCOPED_HEADERS,
true);
+ assertTrue(HttpTransporterUtils.isHttpOriginScopedHeaders(
+ session, repository,
"aether.transport.apache.originScopedHeaders"));
+ }
+
+ @Test
+ void getHttpFollowInsecureRedirect() {
Review Comment:
These test method names use a `get...` prefix but are exercising boolean
`is...` methods (`isHttpOriginScopedHeaders`, `isHttpFollowInsecureRedirect`).
Renaming to `isHttpOriginScopedHeaders` / `isHttpFollowInsecureRedirect` (or
similar) would better match what’s being tested and improve readability.
--
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]
