You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maven.apache.org by cs...@apache.org on 2024/06/11 20:51:02 UTC
(maven) branch master updated: [MNG-8150] Handle absent source/target files in transfer listener (#1575)
This is an automated email from the ASF dual-hosted git repository.
cstamas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/maven.git
The following commit(s) were added to refs/heads/master by this push:
new e995ba62eb [MNG-8150] Handle absent source/target files in transfer listener (#1575)
e995ba62eb is described below
commit e995ba62eb5198e68171f104799706a391fbf238
Author: Pavlo Shevchenko <pa...@gmail.com>
AuthorDate: Tue Jun 11 20:50:56 2024 +0000
[MNG-8150] Handle absent source/target files in transfer listener (#1575)
The PR address two issues observed in the `SimplexTransferListener` and `ConsoleMavenTransferListener`:
1. [TransferResource#getFile()](https://github.com/apache/maven-resolver/blob/master/maven-resolver-api/src/main/java/org/eclipse/aether/transfer/TransferResource.java#L170) can be null. The current `SimplexTransferListener` will break with an NPE if the `file` is not set on the resource.
2. `TransferResource` is not immutable and does not implement `equals` or `hashCode,` making its usage in collections brittle. Listener consumers are not guaranteed to reuse the same instance across listener invocations. I suggest wrapping it in an immutable identifier.
Resolves https://issues.apache.org/jira/browse/MNG-8150
- [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
---
https://issues.apache.org/jira/browse/MNG-8150
---
.../cli/transfer/ConsoleMavenTransferListener.java | 32 +++++++++--------
.../cli/transfer/SimplexTransferListener.java | 17 +++++----
.../cli/transfer/TransferResourceIdentifier.java | 35 ++++++++++++++++++
.../cli/transfer/SimplexTransferListenerTest.java | 41 ++++++++++++++++++----
4 files changed, 96 insertions(+), 29 deletions(-)
diff --git a/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java
index 5b08c31e33..b423193d87 100644
--- a/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java
+++ b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java
@@ -31,15 +31,16 @@ import org.eclipse.aether.transfer.TransferResource;
/**
* Console download progress meter.
- *
+ * <p>
+ * This listener is not thread-safe and should be wrapped in the {@link SimplexTransferListener} in a multi-threaded scenario.
*/
public class ConsoleMavenTransferListener extends AbstractMavenTransferListener {
- private Map<TransferResource, Long> transfers = new LinkedHashMap<>();
- private FileSizeFormat format = new FileSizeFormat(Locale.ENGLISH); // use in a synchronized fashion
- private StringBuilder buffer = new StringBuilder(128); // use in a synchronized fashion
+ private final Map<TransferResourceIdentifier, TransferResourceAndSize> transfers = new LinkedHashMap<>();
+ private final FileSizeFormat format = new FileSizeFormat(Locale.ENGLISH); // use in a synchronized fashion
+ private final StringBuilder buffer = new StringBuilder(128); // use in a synchronized fashion
- private boolean printResourceNames;
+ private final boolean printResourceNames;
private int lastLength;
public ConsoleMavenTransferListener(
@@ -65,18 +66,19 @@ public class ConsoleMavenTransferListener extends AbstractMavenTransferListener
@Override
public void transferProgressed(TransferEvent event) throws TransferCancelledException {
TransferResource resource = event.getResource();
- transfers.put(resource, event.getTransferredBytes());
+ transfers.put(
+ new TransferResourceIdentifier(resource),
+ new TransferResourceAndSize(resource, event.getTransferredBytes()));
buffer.append("Progress (").append(transfers.size()).append("): ");
- Iterator<Map.Entry<TransferResource, Long>> entries =
- transfers.entrySet().iterator();
+ Iterator<TransferResourceAndSize> entries = transfers.values().iterator();
while (entries.hasNext()) {
- Map.Entry<TransferResource, Long> entry = entries.next();
- long total = entry.getKey().getContentLength();
- Long complete = entry.getValue();
+ TransferResourceAndSize entry = entries.next();
+ long total = entry.resource.getContentLength();
+ Long complete = entry.transferredBytes;
- String resourceName = entry.getKey().getResourceName();
+ String resourceName = entry.resource.getResourceName();
if (printResourceNames) {
int idx = resourceName.lastIndexOf('/');
@@ -120,7 +122,7 @@ public class ConsoleMavenTransferListener extends AbstractMavenTransferListener
@Override
public void transferSucceeded(TransferEvent event) {
- transfers.remove(event.getResource());
+ transfers.remove(new TransferResourceIdentifier(event.getResource()));
overridePreviousTransfer(event);
super.transferSucceeded(event);
@@ -128,7 +130,7 @@ public class ConsoleMavenTransferListener extends AbstractMavenTransferListener
@Override
public void transferFailed(TransferEvent event) {
- transfers.remove(event.getResource());
+ transfers.remove(new TransferResourceIdentifier(event.getResource()));
overridePreviousTransfer(event);
super.transferFailed(event);
@@ -144,4 +146,6 @@ public class ConsoleMavenTransferListener extends AbstractMavenTransferListener
buffer.setLength(0);
}
}
+
+ private record TransferResourceAndSize(TransferResource resource, long transferredBytes) {}
}
diff --git a/maven-embedder/src/main/java/org/apache/maven/cli/transfer/SimplexTransferListener.java b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/SimplexTransferListener.java
index 3ed85991e3..8353fa020b 100644
--- a/maven-embedder/src/main/java/org/apache/maven/cli/transfer/SimplexTransferListener.java
+++ b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/SimplexTransferListener.java
@@ -18,7 +18,6 @@
*/
package org.apache.maven.cli.transfer;
-import java.io.File;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.ArrayBlockingQueue;
@@ -129,7 +128,7 @@ public final class SimplexTransferListener extends AbstractTransferListener {
LOGGER.warn("Invalid TransferEvent.EventType={}; ignoring it", type);
}
} catch (TransferCancelledException e) {
- ongoing.put(transferEvent.getResource().getFile(), Boolean.FALSE);
+ ongoing.put(new TransferResourceIdentifier(transferEvent.getResource()), Boolean.FALSE);
}
});
}
@@ -150,17 +149,17 @@ public final class SimplexTransferListener extends AbstractTransferListener {
}
}
- private final ConcurrentHashMap<File, Boolean> ongoing = new ConcurrentHashMap<>();
+ private final ConcurrentHashMap<TransferResourceIdentifier, Boolean> ongoing = new ConcurrentHashMap<>();
@Override
public void transferInitiated(TransferEvent event) {
- ongoing.putIfAbsent(event.getResource().getFile(), Boolean.TRUE);
+ ongoing.putIfAbsent(new TransferResourceIdentifier(event.getResource()), Boolean.TRUE);
put(event, false);
}
@Override
public void transferStarted(TransferEvent event) throws TransferCancelledException {
- if (ongoing.get(event.getResource().getFile()) == Boolean.FALSE) {
+ if (ongoing.get(new TransferResourceIdentifier(event.getResource())) == Boolean.FALSE) {
throw new TransferCancelledException();
}
put(event, false);
@@ -168,7 +167,7 @@ public final class SimplexTransferListener extends AbstractTransferListener {
@Override
public void transferProgressed(TransferEvent event) throws TransferCancelledException {
- if (ongoing.get(event.getResource().getFile()) == Boolean.FALSE) {
+ if (ongoing.get(new TransferResourceIdentifier(event.getResource())) == Boolean.FALSE) {
throw new TransferCancelledException();
}
put(event, false);
@@ -176,7 +175,7 @@ public final class SimplexTransferListener extends AbstractTransferListener {
@Override
public void transferCorrupted(TransferEvent event) throws TransferCancelledException {
- if (ongoing.get(event.getResource().getFile()) == Boolean.FALSE) {
+ if (ongoing.get(new TransferResourceIdentifier(event.getResource())) == Boolean.FALSE) {
throw new TransferCancelledException();
}
put(event, false);
@@ -184,13 +183,13 @@ public final class SimplexTransferListener extends AbstractTransferListener {
@Override
public void transferSucceeded(TransferEvent event) {
- ongoing.remove(event.getResource().getFile());
+ ongoing.remove(new TransferResourceIdentifier(event.getResource()));
put(event, ongoing.isEmpty());
}
@Override
public void transferFailed(TransferEvent event) {
- ongoing.remove(event.getResource().getFile());
+ ongoing.remove(new TransferResourceIdentifier(event.getResource()));
put(event, ongoing.isEmpty());
}
diff --git a/maven-embedder/src/main/java/org/apache/maven/cli/transfer/TransferResourceIdentifier.java b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/TransferResourceIdentifier.java
new file mode 100644
index 0000000000..f45139886b
--- /dev/null
+++ b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/TransferResourceIdentifier.java
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.cli.transfer;
+
+import java.io.File;
+
+import org.apache.maven.api.annotations.Nullable;
+import org.eclipse.aether.transfer.TransferResource;
+
+/**
+ * Immutable identifier of a {@link TransferResource}.
+ * The {@link TransferResource} is not immutable and does not implement {@code Objects#equals} and {@code Objects#hashCode} methods,
+ * making it not very suitable for usage in collections.
+ */
+record TransferResourceIdentifier(String repositoryId, String repositoryUrl, String resourceName, @Nullable File file) {
+ TransferResourceIdentifier(TransferResource resource) {
+ this(resource.getRepositoryId(), resource.getRepositoryUrl(), resource.getResourceName(), resource.getFile());
+ }
+}
diff --git a/maven-embedder/src/test/java/org/apache/maven/cli/transfer/SimplexTransferListenerTest.java b/maven-embedder/src/test/java/org/apache/maven/cli/transfer/SimplexTransferListenerTest.java
index 93f8bf6399..4e285b9930 100644
--- a/maven-embedder/src/test/java/org/apache/maven/cli/transfer/SimplexTransferListenerTest.java
+++ b/maven-embedder/src/test/java/org/apache/maven/cli/transfer/SimplexTransferListenerTest.java
@@ -21,11 +21,13 @@ package org.apache.maven.cli.transfer;
import java.io.File;
import org.eclipse.aether.DefaultRepositorySystemSession;
+import org.eclipse.aether.RepositorySystemSession;
import org.eclipse.aether.transfer.TransferCancelledException;
import org.eclipse.aether.transfer.TransferEvent;
import org.eclipse.aether.transfer.TransferListener;
import org.eclipse.aether.transfer.TransferResource;
import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -67,17 +69,44 @@ class SimplexTransferListenerTest {
DefaultRepositorySystemSession session = new DefaultRepositorySystemSession(h -> false); // no close handle
// for technical reasons we cannot throw here, even if delegate does cancel transfer
- listener.transferInitiated(new TransferEvent.Builder(session, resource)
- .setType(TransferEvent.EventType.INITIATED)
- .build());
+ listener.transferInitiated(event(session, resource, TransferEvent.EventType.INITIATED));
Thread.sleep(500); // to make sure queue is processed, cancellation applied
// subsequent call will cancel
assertThrows(
TransferCancelledException.class,
- () -> listener.transferStarted(new TransferEvent.Builder(session, resource)
- .resetType(TransferEvent.EventType.STARTED)
- .build()));
+ () -> listener.transferStarted(event(session, resource, TransferEvent.EventType.STARTED)));
+ }
+
+ @Test
+ void handlesAbsentTransferSource() throws InterruptedException, TransferCancelledException {
+ TransferResource resource = new TransferResource(null, null, "http://maven.org/test/test-resource", null, null);
+
+ RepositorySystemSession session = Mockito.mock(RepositorySystemSession.class);
+ TransferListener delegate = Mockito.mock(TransferListener.class);
+ SimplexTransferListener listener = new SimplexTransferListener(delegate);
+
+ TransferEvent transferInitiatedEvent = event(session, resource, TransferEvent.EventType.INITIATED);
+ TransferEvent transferStartedEvent = event(session, resource, TransferEvent.EventType.STARTED);
+ TransferEvent transferProgressedEvent = event(session, resource, TransferEvent.EventType.PROGRESSED);
+ TransferEvent transferSucceededEvent = event(session, resource, TransferEvent.EventType.SUCCEEDED);
+
+ listener.transferInitiated(transferInitiatedEvent);
+ listener.transferStarted(transferStartedEvent);
+ listener.transferProgressed(transferProgressedEvent);
+ listener.transferSucceeded(transferSucceededEvent);
+
+ Thread.sleep(500); // to make sure queue is processed, cancellation applied
+
+ Mockito.verify(delegate).transferInitiated(transferInitiatedEvent);
+ Mockito.verify(delegate).transferStarted(transferStartedEvent);
+ Mockito.verify(delegate).transferProgressed(transferProgressedEvent);
+ Mockito.verify(delegate).transferSucceeded(transferSucceededEvent);
+ }
+
+ private static TransferEvent event(
+ RepositorySystemSession session, TransferResource resource, TransferEvent.EventType type) {
+ return new TransferEvent.Builder(session, resource).setType(type).build();
}
}