[
https://issues.apache.org/jira/browse/MNG-7924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17784304#comment-17784304
]
ASF GitHub Bot commented on MNG-7924:
-------------------------------------
gnodet commented on code in PR #1299:
URL: https://github.com/apache/maven/pull/1299#discussion_r1387543172
##########
api/maven-api-core/src/main/java/org/apache/maven/api/DependencyProperties.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.api;
+
+import java.util.Map;
+
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Immutable;
+
+/**
+ * Dependency properties supported by Maven Core.
+ *
+ * @since 4.0.0
+ */
+@Experimental
+@Immutable
+public interface DependencyProperties {
+ /**
+ * Boolean flag telling that dependency contains all of its dependencies.
+ * Note: this flag must be kept in sync with resolver!
+ */
+ String FLAG_INCLUDES_DEPENDENCIES = "includesDependencies";
+
+ /**
+ * Boolean flag telling that dependency is meant to be placed on class
path.
+ */
+ String FLAG_CLASS_PATH_CONSTITUENT = "classPathConstituent";
+
+ /**
+ * Returns immutable "map view" of all the properties.
+ */
+ Map<String, String> asMap();
Review Comment:
Add `@Nonnull` to the return type.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Type.java:
##########
@@ -51,14 +51,21 @@ public interface Type {
* Returns the dependency type id.
* The id uniquely identifies this <i>dependency type</i>.
*
- * @return the id of this type
+ * @return the id of this type, never {@code null}.
*/
String getId();
+ /**
+ * Returns the dependency type language.
Review Comment:
Is that natural language or code language ?
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Type.java:
##########
@@ -51,14 +51,21 @@ public interface Type {
* Returns the dependency type id.
* The id uniquely identifies this <i>dependency type</i>.
*
- * @return the id of this type
+ * @return the id of this type, never {@code null}.
*/
String getId();
+ /**
+ * Returns the dependency type language.
+ *
+ * @return the language of this type, never {@code null}.
+ */
+ String getLanguage();
+
/**
* Get the file extension of artifacts of this type.
*
- * @return the file extension
+ * @return the file extension, never {@code null}.
Review Comment:
Add @NonNull to the return type.
Is this actually never null ? The ones coming from maven can be null afaik.
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java:
##########
@@ -59,8 +62,37 @@ private MavenRepositorySystemUtils() {
* the session with authentication, mirror, proxy and other information
required for your environment.
*
* @return The new repository system session, never {@code null}.
+ * @deprecated This method is deprecated.
*/
+ @Deprecated
public static DefaultRepositorySystemSession newSession() {
+ DefaultArtifactTypeRegistry stereotypes = new
DefaultArtifactTypeRegistry();
+ stereotypes.add(new DefaultArtifactType("pom"));
Review Comment:
Should we use `DefaultTypeRegistry` and the various providers here instead ?
##########
api/maven-api-core/src/main/java/org/apache/maven/api/DependencyProperties.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.api;
+
+import java.util.Map;
+
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Immutable;
+
+/**
+ * Dependency properties supported by Maven Core.
+ *
+ * @since 4.0.0
+ */
+@Experimental
+@Immutable
+public interface DependencyProperties {
+ /**
+ * Boolean flag telling that dependency contains all of its dependencies.
+ * Note: this flag must be kept in sync with resolver!
+ */
+ String FLAG_INCLUDES_DEPENDENCIES = "includesDependencies";
+
+ /**
+ * Boolean flag telling that dependency is meant to be placed on class
path.
+ */
+ String FLAG_CLASS_PATH_CONSTITUENT = "classPathConstituent";
+
+ /**
+ * Returns immutable "map view" of all the properties.
+ */
+ Map<String, String> asMap();
+
+ /**
+ * Returns {@code true} if given flag is {@code true}.
+ */
+ boolean checkFlag(String flag);
Review Comment:
Add either `@Nonnull` or `@Nullable` to `flag`.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Type.java:
##########
@@ -67,17 +74,19 @@ public interface Type {
* The default classifier can be overridden when specifying
* the {@link Dependency#getClassifier()}.
*
- * @return the default classifier
+ * @return the default classifier, or {@code null}.
*/
Review Comment:
Add `@Nullable` to the return type.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Type.java:
##########
@@ -51,14 +51,21 @@ public interface Type {
* Returns the dependency type id.
* The id uniquely identifies this <i>dependency type</i>.
*
- * @return the id of this type
+ * @return the id of this type, never {@code null}.
*/
String getId();
Review Comment:
Add `@Nonnull` to the return type.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Type.java:
##########
@@ -51,14 +51,21 @@ public interface Type {
* Returns the dependency type id.
* The id uniquely identifies this <i>dependency type</i>.
*
- * @return the id of this type
+ * @return the id of this type, never {@code null}.
*/
String getId();
+ /**
+ * Returns the dependency type language.
+ *
+ * @return the language of this type, never {@code null}.
+ */
+ String getLanguage();
Review Comment:
Add `@Nonnull` to the return type.
Has this ever been in use anywhere ?
##########
maven-core/src/main/java/org/apache/maven/artifact/handler/manager/DefaultArtifactHandlerManager.java:
##########
@@ -22,51 +22,55 @@
import javax.inject.Named;
import javax.inject.Singleton;
+import java.util.Collections;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
+import org.apache.maven.api.Type;
+import org.apache.maven.api.services.TypeRegistry;
import org.apache.maven.artifact.handler.ArtifactHandler;
import org.apache.maven.artifact.handler.DefaultArtifactHandler;
+import static java.util.Objects.requireNonNull;
+
/**
*/
@Named
@Singleton
public class DefaultArtifactHandlerManager implements ArtifactHandlerManager {
+ private final TypeRegistry typeRegistry;
- private final Map<String, ArtifactHandler> artifactHandlers;
-
- private final Map<String, ArtifactHandler> allHandlers = new
ConcurrentHashMap<>();
+ private final ConcurrentHashMap<String, ArtifactHandler> handlers;
@Inject
- public DefaultArtifactHandlerManager(Map<String, ArtifactHandler>
artifactHandlers) {
- this.artifactHandlers = artifactHandlers;
+ public DefaultArtifactHandlerManager(TypeRegistry typeRegistry) {
+ this.typeRegistry = requireNonNull(typeRegistry, "null typeRegistry");
+ this.handlers = new ConcurrentHashMap<>();
}
- public ArtifactHandler getArtifactHandler(String type) {
- ArtifactHandler handler = allHandlers.get(type);
-
- if (handler == null) {
- handler = artifactHandlers.get(type);
-
- if (handler == null) {
- handler = new DefaultArtifactHandler(type);
- } else {
- allHandlers.put(type, handler);
- }
- }
-
- return handler;
+ public ArtifactHandler getArtifactHandler(String id) {
+ return handlers.computeIfAbsent(id, k -> {
+ Type type = typeRegistry.getType(id);
+ return new DefaultArtifactHandler(
+ id,
+ type.getExtension(),
+ type.getClassifier(),
+ null,
+ null,
+ type.isIncludesDependencies(),
+ type.getLanguage(),
+ type.isAddedToClassPath()); // TODO: watch out for module
path
+ });
}
public void addHandlers(Map<String, ArtifactHandler> handlers) {
// legacy support for maven-gpg-plugin:1.0
- allHandlers.putAll(handlers);
+ throw new IllegalArgumentException("you cannot do this anymore");
Review Comment:
`throw new UnsupportedOperationException("Adding handlers is not supported
anymore")` ?
##########
maven-core/src/main/java/org/apache/maven/artifact/handler/manager/DefaultArtifactHandlerManager.java:
##########
@@ -22,51 +22,55 @@
import javax.inject.Named;
import javax.inject.Singleton;
+import java.util.Collections;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
+import org.apache.maven.api.Type;
+import org.apache.maven.api.services.TypeRegistry;
import org.apache.maven.artifact.handler.ArtifactHandler;
import org.apache.maven.artifact.handler.DefaultArtifactHandler;
+import static java.util.Objects.requireNonNull;
+
/**
*/
@Named
@Singleton
public class DefaultArtifactHandlerManager implements ArtifactHandlerManager {
+ private final TypeRegistry typeRegistry;
- private final Map<String, ArtifactHandler> artifactHandlers;
-
- private final Map<String, ArtifactHandler> allHandlers = new
ConcurrentHashMap<>();
+ private final ConcurrentHashMap<String, ArtifactHandler> handlers;
@Inject
- public DefaultArtifactHandlerManager(Map<String, ArtifactHandler>
artifactHandlers) {
- this.artifactHandlers = artifactHandlers;
+ public DefaultArtifactHandlerManager(TypeRegistry typeRegistry) {
+ this.typeRegistry = requireNonNull(typeRegistry, "null typeRegistry");
+ this.handlers = new ConcurrentHashMap<>();
}
- public ArtifactHandler getArtifactHandler(String type) {
- ArtifactHandler handler = allHandlers.get(type);
-
- if (handler == null) {
- handler = artifactHandlers.get(type);
-
- if (handler == null) {
- handler = new DefaultArtifactHandler(type);
- } else {
- allHandlers.put(type, handler);
- }
- }
-
- return handler;
+ public ArtifactHandler getArtifactHandler(String id) {
+ return handlers.computeIfAbsent(id, k -> {
+ Type type = typeRegistry.getType(id);
+ return new DefaultArtifactHandler(
+ id,
+ type.getExtension(),
+ type.getClassifier(),
+ null,
+ null,
+ type.isIncludesDependencies(),
+ type.getLanguage(),
+ type.isAddedToClassPath()); // TODO: watch out for module
path
+ });
}
public void addHandlers(Map<String, ArtifactHandler> handlers) {
// legacy support for maven-gpg-plugin:1.0
- allHandlers.putAll(handlers);
+ throw new IllegalArgumentException("you cannot do this anymore");
}
@Deprecated
public Set<String> getHandlerTypes() {
- return artifactHandlers.keySet();
+ return Collections.emptySet();
Review Comment:
That looks wrong. Should we either delegate to `TypeRegistry` or throw an
exception ?
##########
maven-core/src/main/java/org/apache/maven/artifact/handler/manager/LegacyArtifactHandlerManager.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.artifact.handler.manager;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.maven.artifact.handler.ArtifactHandler;
+import org.apache.maven.artifact.handler.DefaultArtifactHandler;
+
+/**
+ */
+@Named
+@Singleton
+public class LegacyArtifactHandlerManager implements ArtifactHandlerManager {
Review Comment:
Why do we need the `LegacyArtifactHandlerManager` _and_ the
`DefaultArtifactHandlerManager` at the same time ?
> Better control over and better integration with Resolver
> --------------------------------------------------------
>
> Key: MNG-7924
> URL: https://issues.apache.org/jira/browse/MNG-7924
> Project: Maven
> Issue Type: Task
> Components: Artifacts and Repositories
> Reporter: Tamas Cservenak
> Priority: Major
> Fix For: 4.0.0-alpha-9
>
>
> Integrate better and obtain better control over Resolver. These changes did
> stem from "[JPMS module
> experiment|https://cwiki.apache.org/confluence/display/MAVEN/Experiment+-+Explicit+JPMS+support]";
> and are considered improvement but *does not implement any functionality*
> related to JPMS module support.
> Changes:
> * Maven4 should stop "disconnected coexistence" of two type systems
> (ArtifactHandlers and Resolver ArtifactTypeRegistry), it should unify them.
> * Maven4 Core should provide generic and extensible means to introduce new
> artifact types (fully in extension, and extension should get extended data
> via "roundtrip" in core/resolver)
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
