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 ? -- 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]
