slachiewicz commented on code in PR #1677:
URL:
https://github.com/apache/maven-dependency-plugin/pull/1677#discussion_r3745673694
##########
src/main/java/org/apache/maven/plugins/dependency/GetMojo.java:
##########
@@ -20,80 +20,40 @@
import javax.inject.Inject;
-import java.util.ArrayList;
import java.util.List;
-import java.util.Map;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
-import org.apache.maven.artifact.handler.ArtifactHandler;
-import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager;
-import org.apache.maven.artifact.repository.ArtifactRepository;
-import org.apache.maven.artifact.repository.ArtifactRepositoryPolicy;
-import org.apache.maven.artifact.repository.MavenArtifactRepository;
-import org.apache.maven.artifact.repository.layout.ArtifactRepositoryLayout;
-import org.apache.maven.execution.MavenSession;
import org.apache.maven.plugin.AbstractMojo;
import org.apache.maven.plugin.MojoExecutionException;
import org.apache.maven.plugin.MojoFailureException;
import org.apache.maven.plugins.annotations.Mojo;
import org.apache.maven.plugins.annotations.Parameter;
-import org.apache.maven.project.DefaultProjectBuildingRequest;
-import org.apache.maven.project.ProjectBuildingRequest;
-import org.apache.maven.repository.RepositorySystem;
-import org.apache.maven.settings.Settings;
-import org.apache.maven.shared.transfer.artifact.ArtifactCoordinate;
-import org.apache.maven.shared.transfer.artifact.DefaultArtifactCoordinate;
-import org.apache.maven.shared.transfer.artifact.resolve.ArtifactResolver;
-import
org.apache.maven.shared.transfer.artifact.resolve.ArtifactResolverException;
-import
org.apache.maven.shared.transfer.dependencies.DefaultDependableCoordinate;
-import org.apache.maven.shared.transfer.dependencies.DependableCoordinate;
-import
org.apache.maven.shared.transfer.dependencies.resolve.DependencyResolver;
-import
org.apache.maven.shared.transfer.dependencies.resolve.DependencyResolverException;
+import org.apache.maven.plugins.dependency.utils.ParamArtifact;
+import org.apache.maven.plugins.dependency.utils.ResolverUtil;
+import org.eclipse.aether.artifact.Artifact;
+import org.eclipse.aether.repository.RemoteRepository;
+import org.eclipse.aether.resolution.ArtifactDescriptorException;
+import org.eclipse.aether.resolution.ArtifactResolutionException;
+import org.eclipse.aether.resolution.DependencyResolutionException;
/**
* Resolves a single artifact, eventually transitively, from the specified
remote repositories. Caveat: will always
* check the central repository defined in the super pom. You could use a
mirror entry in your <code>settings.xml</code>
*/
@Mojo(name = "get", requiresProject = false, threadSafe = true)
public class GetMojo extends AbstractMojo {
- private static final Pattern ALT_REPO_SYNTAX_PATTERN =
Pattern.compile("(.+)::(.*)::(.+)");
- private final MavenSession session;
+ private final ResolverUtil resolverUtil;
- private final ArtifactResolver artifactResolver;
-
- private final DependencyResolver dependencyResolver;
-
- private final ArtifactHandlerManager artifactHandlerManager;
+ private final ParamArtifact paramArtifact = new ParamArtifact();
/**
- * Map that contains the layouts.
- */
- private final Map<String, ArtifactRepositoryLayout> repositoryLayouts;
-
- /**
- * The repository system.
- */
- private final RepositorySystem repositorySystem;
-
- private final DefaultDependableCoordinate coordinate = new
DefaultDependableCoordinate();
-
- /**
- * Repositories in the format id::[layout]::url or just url, separated by
comma. i.e.
- *
central::default::https://repo.maven.apache.org/maven2,myrepo::::https://repo.acme.com,https://repo.acme2.com.
+ * Repositories in the format {@code id::[layout::]url} or just URLs,
separated by comma. That is,
Review Comment:
I shared this concern, so I tested both binding paths against real Maven
rather than reasoning about them. Both work, and there are now integration
tests pinning them:
* **`-DremoteRepositories=a,b`** —
`src/it/projects/get-remote-repositories`, driven through the invoker's
user-properties file.
* **POM element text**, `<remoteRepositories>a,b</remoteRepositories>` —
`src/it/projects/get-remote-repositories-pom-config`. This is the form that
worked while the parameter was a `String`, so it is the actual compatibility
risk, and it still resolves.
Both ITs name two repositories in one comma-separated value and use both
accepted forms (`id::layout::url` and `id::url`).
They are not vacuous: collapsing the value into a single element
(`Collections.singletonList(String.join(",", remoteRepositories))`) makes the
goal fail with exactly the failure mode you describe —
```
Failed to execute goal ...:get (default-cli) on project
get-remote-repositories:
Invalid repository:
repo1::default::https://repo1.invalid/maven2,repo2::https://repo2.invalid/maven2
```
The mechanism is that sisu-plexus' `AbstractCollectionConverter.csvToXml`
turns a plain string value into synthetic list children by splitting on `,`,
and that is the same code path for a user property and for element text.
So I have kept `List<String>`: it matches `list-classes`, which has had this
exact parameter type since MDEP-924 in 3.7.0. Your documentation point was fair
though — the javadoc now also shows the one-per-element POM form.
##########
src/main/java/org/apache/maven/plugins/dependency/GetMojo.java:
##########
@@ -20,80 +20,40 @@
import javax.inject.Inject;
-import java.util.ArrayList;
import java.util.List;
-import java.util.Map;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
-import org.apache.maven.artifact.handler.ArtifactHandler;
-import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager;
-import org.apache.maven.artifact.repository.ArtifactRepository;
-import org.apache.maven.artifact.repository.ArtifactRepositoryPolicy;
-import org.apache.maven.artifact.repository.MavenArtifactRepository;
-import org.apache.maven.artifact.repository.layout.ArtifactRepositoryLayout;
-import org.apache.maven.execution.MavenSession;
import org.apache.maven.plugin.AbstractMojo;
import org.apache.maven.plugin.MojoExecutionException;
import org.apache.maven.plugin.MojoFailureException;
import org.apache.maven.plugins.annotations.Mojo;
import org.apache.maven.plugins.annotations.Parameter;
-import org.apache.maven.project.DefaultProjectBuildingRequest;
-import org.apache.maven.project.ProjectBuildingRequest;
-import org.apache.maven.repository.RepositorySystem;
-import org.apache.maven.settings.Settings;
-import org.apache.maven.shared.transfer.artifact.ArtifactCoordinate;
-import org.apache.maven.shared.transfer.artifact.DefaultArtifactCoordinate;
-import org.apache.maven.shared.transfer.artifact.resolve.ArtifactResolver;
-import
org.apache.maven.shared.transfer.artifact.resolve.ArtifactResolverException;
-import
org.apache.maven.shared.transfer.dependencies.DefaultDependableCoordinate;
-import org.apache.maven.shared.transfer.dependencies.DependableCoordinate;
-import
org.apache.maven.shared.transfer.dependencies.resolve.DependencyResolver;
-import
org.apache.maven.shared.transfer.dependencies.resolve.DependencyResolverException;
+import org.apache.maven.plugins.dependency.utils.ParamArtifact;
+import org.apache.maven.plugins.dependency.utils.ResolverUtil;
+import org.eclipse.aether.artifact.Artifact;
+import org.eclipse.aether.repository.RemoteRepository;
+import org.eclipse.aether.resolution.ArtifactDescriptorException;
+import org.eclipse.aether.resolution.ArtifactResolutionException;
+import org.eclipse.aether.resolution.DependencyResolutionException;
/**
* Resolves a single artifact, eventually transitively, from the specified
remote repositories. Caveat: will always
* check the central repository defined in the super pom. You could use a
mirror entry in your <code>settings.xml</code>
*/
@Mojo(name = "get", requiresProject = false, threadSafe = true)
public class GetMojo extends AbstractMojo {
- private static final Pattern ALT_REPO_SYNTAX_PATTERN =
Pattern.compile("(.+)::(.*)::(.+)");
- private final MavenSession session;
+ private final ResolverUtil resolverUtil;
- private final ArtifactResolver artifactResolver;
-
- private final DependencyResolver dependencyResolver;
-
- private final ArtifactHandlerManager artifactHandlerManager;
+ private final ParamArtifact paramArtifact = new ParamArtifact();
/**
- * Map that contains the layouts.
- */
- private final Map<String, ArtifactRepositoryLayout> repositoryLayouts;
-
- /**
- * The repository system.
- */
- private final RepositorySystem repositorySystem;
-
- private final DefaultDependableCoordinate coordinate = new
DefaultDependableCoordinate();
-
- /**
- * Repositories in the format id::[layout]::url or just url, separated by
comma. i.e.
- *
central::default::https://repo.maven.apache.org/maven2,myrepo::::https://repo.acme.com,https://repo.acme2.com.
+ * Repositories in the format {@code id::[layout::]url} or just URLs,
separated by comma. That is,
+ * <code>
+ *
central::default::https://repo.maven.apache.org/maven2,myrepo::https://repo.acme.com,https://repo.acme2.com
+ * </code>
*/
@Parameter(property = "remoteRepositories")
- private String remoteRepositories;
-
- /**
- * A string of the form
groupId:artifactId:version[:packaging[:classifier]].
- */
- @Parameter(property = "artifact")
- private String artifact;
-
- @Parameter(defaultValue = "${project.remoteArtifactRepositories}",
readonly = true, required = true)
- private List<ArtifactRepository> pomRemoteRepositories;
+ private List<String> remoteRepositories;
Review Comment:
I shared this concern, so I tested both binding paths against real Maven
rather than reasoning about them. Both work, and there are now integration
tests pinning them:
* **`-DremoteRepositories=a,b`** —
`src/it/projects/get-remote-repositories`, driven through the invoker's
user-properties file.
* **POM element text**, `<remoteRepositories>a,b</remoteRepositories>` —
`src/it/projects/get-remote-repositories-pom-config`. This is the form that
worked while the parameter was a `String`, so it is the actual compatibility
risk, and it still resolves.
Both ITs name two repositories in one comma-separated value and use both
accepted forms (`id::layout::url` and `id::url`).
They are not vacuous: collapsing the value into a single element
(`Collections.singletonList(String.join(",", remoteRepositories))`) makes the
goal fail with exactly the failure mode you describe —
```
Failed to execute goal ...:get (default-cli) on project
get-remote-repositories:
Invalid repository:
repo1::default::https://repo1.invalid/maven2,repo2::https://repo2.invalid/maven2
```
The mechanism is that sisu-plexus' `AbstractCollectionConverter.csvToXml`
turns a plain string value into synthetic list children by splitting on `,`,
and that is the same code path for a user property and for element text.
So I have kept `List<String>`: it matches `list-classes`, which has had this
exact parameter type since MDEP-924 in 3.7.0. Your documentation point was fair
though — the javadoc now also shows the one-per-element POM form.
--
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]
