Copilot commented on code in PR #812:
URL: https://github.com/apache/maven-wagon/pull/812#discussion_r3214982620
##########
wagon-tcks/wagon-tck-http/pom.xml:
##########
@@ -33,9 +33,12 @@ under the License.
<dependencies>
<dependency>
- <groupId>org.codehaus.plexus</groupId>
- <artifactId>plexus-container-default</artifactId>
- <scope>compile</scope>
+ <groupId>org.eclipse.sisu</groupId>
+ <artifactId>org.eclipse.sisu.plexus</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.eclipse.sisu</groupId>
+ <artifactId>org.eclipse.sisu.inject</artifactId>
</dependency>
Review Comment:
org.eclipse.sisu.plexus and org.eclipse.sisu.inject are declared without
explicit versions, and the parent pom.xml in this PR does not manage them in
<dependencyManagement>. Unless they are managed by an external parent, Maven
will fail to build with 'version is missing' or pull inconsistent transitive
versions. Add version management in the parent (preferred) or specify versions
here.
##########
wagon-provider-test/src/main/java/org/apache/maven/wagon/WagonTestCase.java:
##########
@@ -175,7 +191,8 @@ protected void setupRepositories() throws Exception {
}
protected void customizeContext() throws Exception {
- getContainer().addContextValue("test.repository", localRepositoryPath);
+ // FIME
+ // getContainer().addContextValue("test.repository",
localRepositoryPath);
}
Review Comment:
customizeContext() is now effectively a no-op and contains a 'FIME' typo. If
tests rely on the container context value 'test.repository' (previously set
here), commenting it out may break providers that read this context. Either
restore the context wiring using the new Plexus/JUnit 5 setup, or remove the
method and any dependent behavior entirely.
##########
wagon-providers/wagon-ftp/src/main/java/org/apache/maven/wagon/providers/ftp/FtpWagon.java:
##########
@@ -51,23 +54,22 @@
/**
* FtpWagon
*
- *
- * @plexus.component role="org.apache.maven.wagon.Wagon"
- * role-hint="ftp"
- * instantiation-strategy="per-lookup"
*/
+@Singleton
+@Named("ftp")
public class FtpWagon extends StreamWagon {
private FTPClient ftp;
- /**
- * @plexus.configuration default-value="true"
- */
private boolean passiveMode = true;
- /**
- * @plexus.configuration default-value="ISO-8859-1"
- */
- private String controlEncoding = FTP.DEFAULT_CONTROL_ENCODING;
+ private String controlEncoding = "ISO-8859-1";
+
+ public FtpWagon(
+ @Named("${passiveMode:-true}") boolean passiveMode,
+ @Named("${controlEncoding:-ISO-8859-1}") String controlEncoding) {
+ this.passiveMode = passiveMode;
+ this.controlEncoding = controlEncoding;
+ }
Review Comment:
FtpWagon now only has an injected constructor with @Named("${...}")
qualifiers for primitive/config values. javax.inject.Named is a qualifier, not
a configuration mechanism, and this pattern is not used elsewhere in the
codebase; it may prevent DI containers (Plexus/Sisu) from instantiating the
component and removes the previous no-arg constructor used by reflective
instantiation. Consider restoring a no-arg constructor and using a supported
configuration approach (e.g., Plexus/Sisu configuration injection or setters
with defaults).
##########
wagon-providers/wagon-ftp/src/main/java/org/apache/maven/wagon/providers/ftp/FtpsWagon.java:
##########
@@ -27,28 +31,30 @@
* FtpsWagon
*
*
- * @plexus.component role="org.apache.maven.wagon.Wagon"
- * role-hint="ftps"
- * instantiation-strategy="per-lookup"
*/
+@Singleton
+@Named("ftps")
public class FtpsWagon extends FtpWagon {
private static final Logger LOG = LoggerFactory.getLogger(FtpsWagon.class);
- /**
- * @plexus.configuration default-value="TLS"
- */
private String securityProtocol = "TLS";
- /**
- * @plexus.configuration default-value="false"
- */
private boolean implicit = false;
-
- /**
- * @plexus.configuration default-value="true"
- */
private boolean endpointChecking = true;
+ @Inject
+ public FtpsWagon(
+ @Named("${passiveMode:-true}") boolean passiveMode,
+ @Named("${controlEncoding:-ISO-8859-1}") String controlEncoding,
+ @Named("${securityProtocol:-TLS}") String securityProtocol,
+ @Named("${implicit:-false}") boolean implicit,
+ @Named("${endpointChecking:-true}") boolean endpointChecking) {
+ super(passiveMode, controlEncoding);
+ this.securityProtocol = securityProtocol;
+ this.implicit = implicit;
+ this.endpointChecking = endpointChecking;
+ }
Review Comment:
The FtpsWagon constructor uses @Named("${...}") qualifiers for configuration
values. If these values are not bound by the DI container, component
instantiation will fail at runtime. Consider using a supported configuration
mechanism (or keep a no-arg constructor with default field values and setters)
instead of relying on `@Named` placeholders for primitives.
##########
wagon-provider-test/src/main/java/org/apache/maven/wagon/WagonTestCase.java:
##########
@@ -194,8 +211,13 @@ protected RepositoryPermissions getPermissions() {
return new RepositoryPermissions();
}
+ @Inject
+ private Map<String, Wagon> wagonMap;
+
+ protected abstract Wagon getRawWagon() throws Exception;
+
protected Wagon getWagon() throws Exception {
- Wagon wagon = (Wagon) lookup(Wagon.ROLE, getProtocol());
+ Wagon wagon = getRawWagon(); // wagonMap.get(getProtocol());
Review Comment:
wagonMap is injected but unused, and getWagon() currently ignores
getProtocol() (the intended wagonMap.get(getProtocol()) is commented out). This
makes the base test harness depend entirely on subclasses overriding
getRawWagon(), and it removes the protocol-to-wagon wiring that previously came
from Plexus lookups. Either remove wagonMap injection or implement the intended
selection logic and handle missing entries with a clear error.
##########
wagon-tcks/wagon-tck-http/src/main/java/org/apache/maven/wagon/tck/http/HttpWagonTests.java:
##########
@@ -81,10 +81,10 @@ public static void beforeAll() throws Exception {
container = new DefaultPlexusContainer();
- configurator = (WagonTestCaseConfigurator)
container.lookup(WagonTestCaseConfigurator.class.getName());
+ configurator = new WagonTestCaseConfigurator();
}
Review Comment:
configurator is instantiated with 'new' instead of being looked up from
Plexus, so it will not be contextualized (realm/configurator fields stay null)
and it will not receive any Plexus configuration (wagonHint/useCaseConfigs). If
TCK customization via components.xml is still required, this should be looked
up from the container (or otherwise initialized/configured) rather than
constructed directly.
##########
wagon-tcks/wagon-tck-http/src/main/java/org/apache/maven/wagon/tck/http/HttpWagonTests.java:
##########
@@ -63,14 +63,14 @@ public abstract class HttpWagonTests {
protected static final Logger logger =
LoggerFactory.getLogger(HttpWagonTests.class);
// CHECKSTYLE_ON: ConstantName
- @Before
+ @BeforeEach
public void beforeEach() throws Exception {
serverFixture = new ServerFixture(isSsl());
serverFixture.start();
- wagon = (Wagon) container.lookup(Wagon.ROLE,
configurator.getWagonHint());
+ wagon = container.lookup(Wagon.class);
}
Review Comment:
beforeEach() now does an unqualified container.lookup(Wagon.class). This
drops the previous role-hint selection (configurator.getWagonHint()) and can
fail when multiple Wagon implementations are on the classpath or when none are
registered with a default hint. Consider using container.lookup(Wagon.class,
hint) and ensure the hint is set (e.g., based on isSsl() or provider
configuration).
##########
pom.xml:
##########
@@ -244,6 +247,11 @@ under the License.
<artifactId>javax.servlet-api</artifactId>
<version>4.0.1</version>
</dependency>
+ <dependency>
+ <groupId>org.junit.platform</groupId>
+ <artifactId>junit-platform-suite</artifactId>
+ <version>1.9.3</version>
+ </dependency>
Review Comment:
JUnit 5 tests require a runtime engine (e.g., junit-jupiter-engine or the
junit-jupiter aggregate) on the test classpath; only junit-platform-suite is
managed here. Also, this PR introduces org.junit.jupiter and org.eclipse.sisu
dependencies in modules without managing their versions in this parent POM,
which can lead to version-less dependency build failures or inconsistent
versions across modules. Consider adding dependencyManagement entries for
junit-jupiter-* (including engine) and org.eclipse.sisu.* to ensure tests
actually execute and versions are consistent.
##########
wagon-providers/wagon-ftp/src/main/java/org/apache/maven/wagon/providers/ftp/FtpWagon.java:
##########
@@ -51,23 +54,22 @@
/**
* FtpWagon
*
- *
- * @plexus.component role="org.apache.maven.wagon.Wagon"
- * role-hint="ftp"
- * instantiation-strategy="per-lookup"
*/
+@Singleton
+@Named("ftp")
public class FtpWagon extends StreamWagon {
Review Comment:
Marking FtpWagon as `@Singleton` is a behavioral change from the previous
Plexus 'per-lookup' instantiation strategy noted in the removed javadoc. Wagon
implementations are stateful (repository, connection, FTPClient, listeners) and
typically must not be singletons; this can cause cross-request state bleed and
concurrency issues. Prefer the default (unscoped) lifecycle or an explicit
per-lookup/prototype scope equivalent.
##########
wagon-providers/wagon-http/pom.xml:
##########
@@ -70,8 +70,8 @@ under the License.
<scope>test</scope>
</dependency>
<dependency>
- <groupId>junit</groupId>
- <artifactId>junit</artifactId>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
<scope>test</scope>
</dependency>
Review Comment:
This module declares junit-jupiter-api but not the JUnit Jupiter engine.
Without junit-jupiter-engine (or the junit-jupiter aggregate) in test scope,
Surefire will not execute JUnit 5 tests. Add the engine dependency (typically
test scope) so the migrated tests actually run.
##########
wagon-providers/wagon-ftp/src/main/java/org/apache/maven/wagon/providers/ftp/FtpHttpWagon.java:
##########
@@ -27,15 +30,18 @@
/**
* FtpHttpWagon
*
- *
- * @plexus.component role="org.apache.maven.wagon.Wagon"
- * role-hint="ftph"
- * instantiation-strategy="per-lookup"
*/
+@Singleton
+@Named("ftph")
public class FtpHttpWagon extends FtpWagon {
private static final Logger LOG =
LoggerFactory.getLogger(FtpHttpWagon.class);
+ public FtpHttpWagon(
+ @Named("${passiveMode}") boolean passiveMode,
@Named("${controlEncoding}") String controlEncoding) {
+ super(passiveMode, controlEncoding);
+ }
Review Comment:
FtpHttpWagon is now `@Singleton` and uses @Named("${...}") constructor
qualifiers. Since this wagon inherits stateful connection/config fields,
singleton scope can lead to cross-request state bleed, and the `@Named`
placeholders may not be resolvable by the container. Prefer an unscoped
component with default constructor + supported configuration injection.
##########
wagon-provider-test/src/main/java/org/apache/maven/wagon/WagonTestCase.java:
##########
@@ -109,12 +122,15 @@ public int getSize() {
// Constructors
// ----------------------------------------------------------------------
+ @BeforeEach
protected void setUp() throws Exception {
checksumObserver = new ChecksumObserver();
mockTransferListener = createMock(TransferListener.class);
+ }
- super.setUp();
+ protected String getName() {
+ return getClass().getName();
}
Review Comment:
getName() previously came from JUnit3 TestCase and returned the *test method
name*; returning only the class name here makes
FileTestUtils.createUniqueFile(getName(), getName()) reuse the same path across
multiple test methods in the same class, which can cause test
interference/flakiness. Consider using JUnit 5 TestInfo to incorporate the
current test method name, or switch callers to Files.createTempFile/@TempDir.
##########
wagon-tcks/wagon-tck-http/src/main/java/org/apache/maven/wagon/tck/http/GetWagonTests.java:
##########
@@ -84,86 +83,80 @@ public void proxied()
}
@Test
- public void highLatencyHighTimeout()
- throws ConnectionException, AuthenticationException,
ComponentConfigurationException, IOException,
- TransferFailedException, ResourceDoesNotExistException,
AuthorizationException {
+ public void highLatencyHighTimeout() throws Exception {
getServerFixture().addServlet("/slow/*", new
LatencyServlet(TWO_SECONDS));
testSuccessfulGet("slow/large.txt", "large.txt");
}
@Test
- public void highLatencyLowTimeout()
- throws ConnectionException, AuthenticationException,
ComponentConfigurationException, IOException,
- TransferFailedException, ResourceDoesNotExistException,
AuthorizationException {
+ @Disabled
+ public void highLatencyLowTimeout() throws Exception {
Servlet servlet = new LatencyServlet(TWO_SECONDS);
getServerFixture().addServlet("/slow/*", servlet);
testSuccessfulGet("slow/large.txt", "large.txt");
}
@Test
- public void inifiniteLatencyTimeout()
- throws ConnectionException, AuthenticationException,
ComponentConfigurationException, IOException,
- TransferFailedException, ResourceDoesNotExistException,
AuthorizationException {
+ @Disabled
+ public void inifiniteLatencyTimeout() throws Exception {
if (!isSupported()) {
return;
}
final ValueHolder<TransferFailedException> holder = new
ValueHolder<>(null);
- Runnable r = new Runnable() {
- public void run() {
- Servlet servlet = new LatencyServlet(-1);
- addNotificationTarget(servlet);
-
- getServerFixture().addServlet("/infinite/*", servlet);
- try {
- if (!initTest(null, null)) {
- return;
- }
-
- if (getWagon() instanceof StreamWagon) {
- logger.info("Connection timeout is: " +
getWagon().getTimeout());
- }
-
- File target = newTempFile();
- getWagon().get("infinite/", target);
-
- fail("Should have failed to transfer due to transaction
timeout.");
- } catch (ConnectionException
- | AuthenticationException
- | ResourceDoesNotExistException
- | AuthorizationException
- | ComponentConfigurationException
- | IOException e) {
- throw new IllegalStateException(e);
- } catch (TransferFailedException e) {
- // expected
- holder.setValue(e);
+ Runnable r = () -> {
+ Servlet servlet = new LatencyServlet(-1);
+ addNotificationTarget(servlet);
+
+ getServerFixture().addServlet("/infinite/*", servlet);
+ try {
+ if (!initTest(null, null)) {
+ return;
+ }
+
+ if (getWagon() instanceof StreamWagon) {
+ logger.info("Connection timeout is: " +
getWagon().getTimeout());
}
+
+ File target = newTempFile();
+ getWagon().get("infinite/", target);
+
+ fail("Should have failed to transfer due to transaction
timeout.");
+ } catch (ConnectionException
+ | AuthenticationException
+ | ResourceDoesNotExistException
+ | AuthorizationException
+ | IOException
+ | ComponentConfigurationException e) {
+ throw new IllegalStateException(e);
+ } catch (TransferFailedException e) {
+ // expected
+ holder.setValue(e);
}
};
Thread t = new Thread(r);
t.start();
- try {
- logger.info("Waiting 60 seconds for wagon timeout.");
- t.join(ONE_MINUTE);
- } catch (InterruptedException e) {
- e.printStackTrace();
- }
+ // try {
+ // logger.info("Waiting 60 seconds for wagon timeout.");
+ // t.join(ONE_MINUTE);
+ // } catch (InterruptedException e) {
+ // e.printStackTrace();
+ // }
Review Comment:
The commented-out Thread.join()/interrupt() block should be removed rather
than kept as dead code. It obscures the actual synchronization strategy
(Awaitility) and makes it harder to understand/maintain the test.
##########
wagon-provider-api/src/main/java/org/apache/maven/wagon/Wagon.java:
##########
@@ -34,7 +34,6 @@
*
*/
public interface Wagon {
- String ROLE = Wagon.class.getName();
/**
Review Comment:
This PR removes Wagon.ROLE, but there are still in-repo references to
Wagon.ROLE (e.g., in wagon-ssh and wagon-ssh-common-test). Removing the
constant without updating all call sites will break compilation. Either keep
the constant for compatibility or update the remaining lookups to use the new
container lookup APIs (Class + hint).
##########
wagon-providers/wagon-ftp/src/main/java/org/apache/maven/wagon/providers/ftp/FtpsWagon.java:
##########
@@ -27,28 +31,30 @@
* FtpsWagon
*
*
- * @plexus.component role="org.apache.maven.wagon.Wagon"
- * role-hint="ftps"
- * instantiation-strategy="per-lookup"
*/
+@Singleton
+@Named("ftps")
public class FtpsWagon extends FtpWagon {
Review Comment:
FtpsWagon is now @Singleton, which risks sharing connection/authentication
state across usages (it extends the stateful FtpWagon). This conflicts with the
previous per-lookup instantiation strategy and can introduce concurrency and
test-isolation issues. Prefer leaving it unscoped (prototype) unless the wagon
implementation is proven stateless/thread-safe.
--
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]
