From e0b2bfaff5de5a34e09f0c65fd736d4dbd0315f2 Mon Sep 17 00:00:00 2001 From: Martin Desruisseaux Date: Sat, 26 Oct 2024 12:31:11 +0200 Subject: [PATCH 1/3] Documentation and formatting in `MavenProject` (no significant code change): * Added some Javadoc and `@Nullable`/`@Nonnull` annotations. * Removed some empty lines, not because they are bad but because Checkstyle imposes a limit of 2000 lines. * Renamed some parameters or local variables for avoiding to hide a field, except when it should be the same thing. --- .../apache/maven/project/MavenProject.java | 511 ++++++++++++------ 1 file changed, 358 insertions(+), 153 deletions(-) diff --git a/impl/maven-core/src/main/java/org/apache/maven/project/MavenProject.java b/impl/maven-core/src/main/java/org/apache/maven/project/MavenProject.java index e155609de55e..ed5bd456d082 100644 --- a/impl/maven-core/src/main/java/org/apache/maven/project/MavenProject.java +++ b/impl/maven-core/src/main/java/org/apache/maven/project/MavenProject.java @@ -35,6 +35,8 @@ import java.util.function.Predicate; import org.apache.maven.RepositoryUtils; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.annotations.Nullable; import org.apache.maven.artifact.Artifact; import org.apache.maven.artifact.ArtifactUtils; import org.apache.maven.artifact.DependencyResolutionRequiredException; @@ -92,7 +94,6 @@ *

*/ public class MavenProject implements Cloneable { - private static final Logger LOGGER = LoggerFactory.getLogger(MavenProject.class); public static final String EMPTY_PROJECT_GROUP_ID = "unknown"; @@ -101,96 +102,215 @@ public class MavenProject implements Cloneable { public static final String EMPTY_PROJECT_VERSION = "0"; + @Nonnull private Model model; + @Nullable private MavenProject parent; + /** + * The path to the {@code pom.xml} file. By default, this file is located in {@link #basedir}. + */ + @Nullable private File file; + /** + * The project base directory. + * By default, this is the parent of the {@linkplain #getFile() POM file}. + */ + @Nullable private File basedir; + // TODO: what is the difference with {@code basedir}? + @Nullable private Path rootDirectory; + /** + * The artifacts specified by {@code setResolvedArtifacts(Set)}. This is for Maven internal usage only. + * This collection should be handled as if it was immutable, + * because it may be shared by cloned {@code MavenProject} instances. + */ + @Nullable private Set resolvedArtifacts; + /** + * The filter to apply on {@link #resolvedArtifacts} when {@link #artifacts} is lazily populated. + * May be {@code null} to exclude all resolved artifacts. + */ + @Nullable private ArtifactFilter artifactFilter; + /** + * All dependencies that this project has, including transitive ones. + * May be lazily computed from {@link #resolvedArtifacts}. + * This is an immutable collection potentially shared by cloned {@code MavenProject} instances. + */ + @Nullable // Computed when first requested. private Set artifacts; + @Nullable private Artifact parentArtifact; + /** + * The plugin dependencies that this project has. + * This is an immutable collection potentially shared by cloned {@code MavenProject} instances. + */ + @Nonnull private Set pluginArtifacts; + /** + * The artifact repositories of this project. + * This is an immutable collection potentially shared by cloned {@code MavenProject} instances. + */ + @Nonnull private List remoteArtifactRepositories; + /** + * The plugin repositories of this project. + * This is an immutable collection potentially shared by cloned {@code MavenProject} instances. + */ + @Nonnull private List pluginArtifactRepositories; + /** + * Cached value computed from {@link #remoteArtifactRepositories} when first requested. + * This is an immutable collection potentially shared by cloned {@code MavenProject} instances. + */ + @Nullable // Computed when first requested. private List remoteProjectRepositories; + /** + * Cached value computed from {@link #pluginArtifactRepositories} when first requested. + * This is an immutable collection potentially shared by cloned {@code MavenProject} instances. + */ + @Nullable // Computed when first requested. private List remotePluginRepositories; + /** + * List of the attached artifacts to this project. + */ + @Nonnull private List attachedArtifacts = new ArrayList<>(); + @Nullable private MavenProject executionProject; + /** + * The collected projects. + * This is an immutable collection potentially shared by cloned {@code MavenProject} instances. + */ + @Nonnull private List collectedProjects; + /** + * Root directories of source codes to compile. + */ + @Nonnull private List compileSourceRoots = new ArrayList<>(); + /** + * Root directories of test codes to compile. + */ + @Nonnull private List testCompileSourceRoots = new ArrayList<>(); + /** + * Root directories of scriot codes. + */ + @Nonnull private List scriptSourceRoots = new ArrayList<>(); + @Nullable private ArtifactRepository releaseArtifactRepository; + @Nullable private ArtifactRepository snapshotArtifactRepository; + /** + * The active profiles. + * This is an immutable collection potentially shared by cloned {@code MavenProject} instances. + */ + @Nonnull private List activeProfiles = new ArrayList<>(); + /** + * The identifiers of all profiles that contributed to this project's effective model. + */ + @Nonnull private Map> injectedProfileIds = new LinkedHashMap<>(); + /** + * Direct dependencies that this project has. + * This is an immutable collection potentially shared by cloned {@code MavenProject} instances. + */ + @Nullable // For compatibility with previous behavior. + @Deprecated private Set dependencyArtifacts; + @Nullable private Artifact artifact; - // calculated. + @Nullable // calculated when first requested. private Map artifactMap; + @Nullable private Model originalModel; + @Nullable // Computed when first requested. private Map pluginArtifactMap; + @Nonnull private Set reportArtifacts; + @Nullable private Map reportArtifactMap; + @Nonnull private Set extensionArtifacts; + @Nullable // Computed when first requested. private Map extensionArtifactMap; + @Nonnull private Map managedVersionMap; + /** + * Projects by identifiers. + */ + @Nonnull private Map projectReferences = new HashMap<>(); private boolean executionRoot; + @Nullable private File parentFile; + /** + * Key-value pairs providing context. + */ + @Nonnull private Map context; + @Nullable private ClassRealm classRealm; + @Nullable private DependencyFilter extensionDependencyFilter; + /** + * The life cycle phases. + */ + @Nonnull private final Set lifecyclePhases = Collections.synchronizedSet(new LinkedHashSet<>()); + /** + * Creates an initially empty project. + */ public MavenProject() { Model model = new Model(); - model.setGroupId(EMPTY_PROJECT_GROUP_ID); model.setArtifactId(EMPTY_PROJECT_ARTIFACT_ID); model.setVersion(EMPTY_PROJECT_VERSION); - setModel(model); } @@ -198,19 +318,32 @@ public MavenProject(org.apache.maven.api.model.Model model) { this(new Model(model)); } - public MavenProject(Model model) { + /** + * Creates a project derived from the given model. + * + * @param model the model to wrap in a maven project + */ + public MavenProject(@Nonnull Model model) { setModel(model); } - public MavenProject(MavenProject project) { + /** + * Creates a copy of the given project. + * + * @param project the project to copy + * @see #clone() + */ + public MavenProject(@Nonnull MavenProject project) { deepCopy(project); } + // TODO: where is the difference with {@code basedir} and {@code rootDirectory}? + @Nullable public File getParentFile() { return parentFile; } - public void setParentFile(File parentFile) { + public void setParentFile(@Nullable File parentFile) { this.parentFile = parentFile; } @@ -218,15 +351,17 @@ public void setParentFile(File parentFile) { // Accessors // ---------------------------------------------------------------------- + @Nullable public Artifact getArtifact() { return artifact; } - public void setArtifact(Artifact artifact) { - this.artifact = artifact; + public void setArtifact(@Nullable Artifact target) { + artifact = target; } // TODO I would like to get rid of this. jvz. + @Nonnull public Model getModel() { return model; } @@ -236,11 +371,12 @@ public Model getModel() { * * @return the parent, or null if no parent is declared or there was an error building it */ + @Nullable public MavenProject getParent() { return parent; } - public void setParent(MavenProject parent) { + public void setParent(@Nullable MavenProject parent) { this.parent = parent; } @@ -248,11 +384,21 @@ public boolean hasParent() { return getParent() != null; } + /** + * {@return the path to the {@code pom.xml} file}. + * By default, this file is located in the {@linkplain #getBasedir() base directory}. + */ + @Nullable public File getFile() { return file; } - public void setFile(File file) { + /** + * Sets the {@code pom.xml} file to the given file, and the base directory to the parent of that file. + * + * @param file the new {@code pom.xml} file, as a file located in the new base directory + */ + public void setFile(@Nullable File file) { this.file = file; this.basedir = file != null ? file.getParentFile() : null; } @@ -260,12 +406,18 @@ public void setFile(File file) { /** * Sets project {@code file} without changing project {@code basedir}. * + * @param file the new {@code pom.xml} file * @since 3.2.4 */ - public void setPomFile(File file) { + public void setPomFile(@Nullable File file) { this.file = file; } + /** + * {@return the project base directory}. + * By default, this is the parent of the {@linkplain #getFile() POM file}. + */ + @Nullable public File getBasedir() { return basedir; } @@ -290,15 +442,14 @@ private void addPath(List paths, String path) { if (path != null) { path = path.trim(); if (!path.isEmpty()) { - File file = new File(path); - if (file.isAbsolute()) { - path = file.getAbsolutePath(); + File f = new File(path); + if (f.isAbsolute()) { + path = f.getAbsolutePath(); } else if (".".equals(path)) { path = getBasedir().getAbsolutePath(); } else { path = new File(getBasedir(), path).getAbsolutePath(); } - if (!paths.contains(path)) { paths.add(path); } @@ -306,18 +457,36 @@ private void addPath(List paths, String path) { } } - public void addCompileSourceRoot(String path) { + /** + * If the given path is non-null, adds it to the source root directories. Otherwise, does nothing. + * + * @param path the path to add if non-null + */ + public void addCompileSourceRoot(@Nullable String path) { addPath(getCompileSourceRoots(), path); } - public void addTestCompileSourceRoot(String path) { + /** + * If the given path is non-null, adds it to the test root directories. Otherwise, does nothing. + * + * @param path the path to add if non-null + */ + public void addTestCompileSourceRoot(@Nullable String path) { addPath(getTestCompileSourceRoots(), path); } + /** + * {@return the source root directories as an unmodifiable list}. + */ + @Nonnull public List getCompileSourceRoots() { return compileSourceRoots; } + /** + * {@return the test root directories as an unmodifiable list}. + */ + @Nonnull public List getTestCompileSourceRoots() { return testCompileSourceRoots; } @@ -374,7 +543,7 @@ private List getClasspathElements(final Predicate scopeFilter, f } /** - * Returns the elements placed on the classpath for compilation. + * {@return the elements placed on the classpath for compilation}. * This method can be invoked when the caller does not support module-path. * * @throws DependencyResolutionRequiredException if an artifact file is used, but has not been resolved @@ -388,7 +557,7 @@ public List getCompileClasspathElements() throws DependencyResolutionReq } /** - * Returns the elements placed on the classpath for tests. + * {@return the elements placed on the classpath for tests}. * This method can be invoked when the caller does not support module-path. * * @throws DependencyResolutionRequiredException if an artifact file is used, but has not been resolved @@ -402,7 +571,7 @@ public List getTestClasspathElements() throws DependencyResolutionRequir } /** - * Returns the elements placed on the classpath for runtime. + * {@return the elements placed on the classpath for runtime}. * This method can be invoked when the caller does not support module-path. * * @throws DependencyResolutionRequiredException if an artifact file is used, but has not been resolved @@ -437,11 +606,9 @@ public void setGroupId(String groupId) { public String getGroupId() { String groupId = getModel().getGroupId(); - if ((groupId == null) && (getModel().getParent() != null)) { groupId = getModel().getParent().getGroupId(); } - return groupId; } @@ -472,11 +639,9 @@ public void setVersion(String version) { public String getVersion() { String version = getModel().getVersion(); - if ((version == null) && (getModel().getParent() != null)) { version = getModel().getParent().getVersion(); } - return version; } @@ -628,11 +793,17 @@ public void addLicense(License license) { getModel().addLicense(license); } - public void setArtifacts(Set artifacts) { + /** + * Sets all dependencies that this project has, including transitive ones. + * + *

Side effects

+ * Invoking this method will also modify the values returned by {@link #getArtifactMap()}. + * + * @param artifacts the new project dependencies + */ + public void setArtifacts(@Nonnull Set artifacts) { this.artifacts = artifacts; - - // flush the calculated artifactMap - artifactMap = null; + artifactMap = null; // flush the calculated artifactMap } /** @@ -640,9 +811,10 @@ public void setArtifacts(Set artifacts) { * what phases have run dependencies in some scopes won't be included. e.g. if only compile phase has run, * dependencies with scope test won't be included. * - * @return {@link Set} < {@link Artifact} > - * @see #getDependencyArtifacts() to get only direct dependencies + * @return set of dependencies */ + @Nonnull + @SuppressWarnings("ReturnOfCollectionOrArrayField") // The returned set is unmodifiable. public Set getArtifacts() { if (artifacts == null) { if (artifactFilter == null || resolvedArtifacts == null) { @@ -659,6 +831,7 @@ public Set getArtifacts() { return artifacts; } + @Nonnull public Map getArtifactMap() { if (artifactMap == null) { artifactMap = ArtifactUtils.artifactMapByVersionlessId(getArtifacts()); @@ -666,28 +839,43 @@ public Map getArtifactMap() { return artifactMap; } - public void setPluginArtifacts(Set pluginArtifacts) { + /** + * Sets all plugins that this project has, including transitive ones. + * + *

Side effects

+ * Invoking this method will also modify the values returned by {@link #getPluginArtifactMap()}. + * + * @param pluginArtifacts the new project plugins + */ + public void setPluginArtifacts(@Nonnull Set pluginArtifacts) { this.pluginArtifacts = pluginArtifacts; - this.pluginArtifactMap = null; } + /** + * All plugins that this project has. + * + * @return unmodifiable set of plugins + */ + @Nonnull + @SuppressWarnings("ReturnOfCollectionOrArrayField") // The returned set is unmodifiable. public Set getPluginArtifacts() { return pluginArtifacts; } + @Nonnull public Map getPluginArtifactMap() { if (pluginArtifactMap == null) { pluginArtifactMap = ArtifactUtils.artifactMapByVersionlessId(getPluginArtifacts()); } - return pluginArtifactMap; } - public void setParentArtifact(Artifact parentArtifact) { + public void setParentArtifact(@Nullable Artifact parentArtifact) { this.parentArtifact = parentArtifact; } + @Nullable public Artifact getParentArtifact() { return parentArtifact; } @@ -716,54 +904,65 @@ public List getModules() { public PluginManagement getPluginManagement() { PluginManagement pluginMgmt = null; - Build build = getModel().getBuild(); if (build != null) { pluginMgmt = build.getPluginManagement(); } - return pluginMgmt; } private Build getModelBuild() { Build build = getModel().getBuild(); - if (build == null) { build = new Build(); getModel().setBuild(build); } - return build; } - public void setRemoteArtifactRepositories(List remoteArtifactRepositories) { + /** + * Sets the artifact repositories of this project. + * + * @param remoteArtifactRepositories the new artifact repositories + */ + public void setRemoteArtifactRepositories(@Nonnull List remoteArtifactRepositories) { this.remoteArtifactRepositories = remoteArtifactRepositories; this.remoteProjectRepositories = RepositoryUtils.toRepos(getRemoteArtifactRepositories()); } + /** + * {@return the artifact repositories of this project}. The returned list is unmodifiable for ensuring + * that {@link #getRemoteProjectRepositories()} stay consistent with the values returned by this method. + */ + @Nonnull + @SuppressWarnings("ReturnOfCollectionOrArrayField") // The returned list is unmodifiable. public List getRemoteArtifactRepositories() { if (remoteArtifactRepositories == null) { remoteArtifactRepositories = new ArrayList<>(); } - return remoteArtifactRepositories; } - public void setPluginArtifactRepositories(List pluginArtifactRepositories) { + /** + * Sets the plugin repositories of this project. + * + * @param pluginArtifactRepositories the new artifact repositories + */ + public void setPluginArtifactRepositories(@Nonnull List pluginArtifactRepositories) { this.pluginArtifactRepositories = pluginArtifactRepositories; this.remotePluginRepositories = RepositoryUtils.toRepos(getPluginArtifactRepositories()); } /** - * @return a list of ArtifactRepository objects constructed from the Repository objects returned by - * getPluginRepositories. + * {@return the plugin repositories of this project}. */ + @Nonnull + @SuppressWarnings("ReturnOfCollectionOrArrayField") // The returned list is unmodifiable. public List getPluginArtifactRepositories() { if (pluginArtifactRepositories == null) { pluginArtifactRepositories = new ArrayList<>(); } - return pluginArtifactRepositories; } @@ -777,23 +976,31 @@ public List getPluginRepositories() { return getModel().getPluginRepositories(); } + @Nonnull public List getRemoteProjectRepositories() { return remoteProjectRepositories; } + @Nonnull public List getRemotePluginRepositories() { return remotePluginRepositories; } - public void setActiveProfiles(List activeProfiles) { + /** + * Sets the active profiles of this project. + * + * @param activeProfiles the new active profiles of this project + */ + public void setActiveProfiles(@Nonnull List activeProfiles) { this.activeProfiles = activeProfiles; } + @Nonnull public List getActiveProfiles() { return activeProfiles; } - public void setInjectedProfileIds(String source, List injectedProfileIds) { + public void setInjectedProfileIds(@Nonnull String source, @Nullable List injectedProfileIds) { if (injectedProfileIds != null) { this.injectedProfileIds.put(source, new ArrayList<>(injectedProfileIds)); } else { @@ -808,9 +1015,9 @@ public void setInjectedProfileIds(String source, List injectedProfileIds * {@code ::} for a POM profile or {@code external} for profiles from the * {@code settings.xml}. * - * @return The identifiers of all injected profiles, indexed by the source from which the profiles originated, never - * {@code null}. + * @return The identifiers of all injected profiles, indexed by the source from which the profiles originated */ + @Nonnull public Map> getInjectedProfileIds() { return this.injectedProfileIds; } @@ -826,7 +1033,8 @@ public Map> getInjectedProfileIds() { * @deprecated Please use {@link MavenProjectHelper} * @throws DuplicateArtifactAttachmentException will never happen but leave it for backward compatibility */ - public void addAttachedArtifact(Artifact artifact) throws DuplicateArtifactAttachmentException { + @Deprecated + public void addAttachedArtifact(@Nonnull Artifact artifact) throws DuplicateArtifactAttachmentException { // if already there we remove it and add again int index = attachedArtifacts.indexOf(artifact); if (index >= 0) { @@ -842,6 +1050,7 @@ public void addAttachedArtifact(Artifact artifact) throws DuplicateArtifactAttac * * @return the attached artifacts of this project */ + @Nonnull public List getAttachedArtifacts() { if (attachedArtifacts == null) { attachedArtifacts = new ArrayList<>(); @@ -852,12 +1061,10 @@ public List getAttachedArtifacts() { public Xpp3Dom getGoalConfiguration( String pluginGroupId, String pluginArtifactId, String executionId, String goalId) { Xpp3Dom dom = null; - if (getBuildPlugins() != null) { for (Plugin plugin : getBuildPlugins()) { if (pluginGroupId.equals(plugin.getGroupId()) && pluginArtifactId.equals(plugin.getArtifactId())) { dom = (Xpp3Dom) plugin.getConfiguration(); - if (executionId != null) { for (PluginExecution execution : plugin.getExecutions()) { if (executionId.equals(execution.getId())) { @@ -871,67 +1078,77 @@ public Xpp3Dom getGoalConfiguration( } } } - if (dom != null) { // make a copy so the original in the POM doesn't get messed with dom = new Xpp3Dom(dom); } - return dom; } + @Nonnull public MavenProject getExecutionProject() { return (executionProject == null ? this : executionProject); } - public void setExecutionProject(MavenProject executionProject) { + public void setExecutionProject(@Nullable MavenProject executionProject) { this.executionProject = executionProject; } + @Nonnull public List getCollectedProjects() { return collectedProjects; } - public void setCollectedProjects(List collectedProjects) { + /** + * Sets the collected project. + * + * @param collectedProjects the collected projects + */ + public void setCollectedProjects(@Nonnull List collectedProjects) { this.collectedProjects = collectedProjects; } /** - * Direct dependencies that this project has. + * {@return the direct dependencies that this project has, or null if none}. + * Note that this method returns {@code null} instead of an empty set when there is no dependencies. + * This unusual behavior is kept for compatibility reasons with existing codes, which test for nullity instead + * of emptiness. * - * @return {@link Set} < {@link Artifact} > * @see #getArtifacts() to get all transitive dependencies */ @Deprecated + @Nullable // For compatibility with previous behavior. public Set getDependencyArtifacts() { return dependencyArtifacts; } @Deprecated - public void setDependencyArtifacts(Set dependencyArtifacts) { + public void setDependencyArtifacts(@Nonnull Set dependencyArtifacts) { this.dependencyArtifacts = dependencyArtifacts; } - public void setReleaseArtifactRepository(ArtifactRepository releaseArtifactRepository) { + public void setReleaseArtifactRepository(@Nullable ArtifactRepository releaseArtifactRepository) { this.releaseArtifactRepository = releaseArtifactRepository; } - public void setSnapshotArtifactRepository(ArtifactRepository snapshotArtifactRepository) { + public void setSnapshotArtifactRepository(@Nullable ArtifactRepository snapshotArtifactRepository) { this.snapshotArtifactRepository = snapshotArtifactRepository; } - public void setOriginalModel(Model originalModel) { + public void setOriginalModel(@Nullable Model originalModel) { this.originalModel = originalModel; } + @Nullable public Model getOriginalModel() { return originalModel; } - public void setManagedVersionMap(Map map) { + public void setManagedVersionMap(@Nonnull Map map) { managedVersionMap = map; } + @Nonnull public Map getManagedVersionMap() { return managedVersionMap; } @@ -943,9 +1160,7 @@ public boolean equals(Object other) { } else if (!(other instanceof MavenProject)) { return false; } - MavenProject that = (MavenProject) other; - return Objects.equals(getArtifactId(), that.getArtifactId()) && Objects.equals(getGroupId(), that.getGroupId()) && Objects.equals(getVersion(), that.getVersion()); @@ -965,7 +1180,7 @@ public List getBuildExtensions() { } } - public void addProjectReference(MavenProject project) { + public void addProjectReference(@Nonnull MavenProject project) { projectReferences.put( getProjectReferenceId(project.getGroupId(), project.getArtifactId(), project.getVersion()), project); } @@ -978,6 +1193,7 @@ public List getFilters() { return getBuild().getFilters(); } + @Nonnull public Map getProjectReferences() { return projectReferences; } @@ -999,7 +1215,7 @@ public Plugin getPlugin(String pluginKey) { } /** - * Default toString + * {@return a string representation for debugging purposes}. */ @Override public String toString() { @@ -1014,12 +1230,15 @@ public String toString() { sb.append(" @ "); sb.append(getFile().getPath()); } - return sb.toString(); } /** + * {@return a deep copy of this object}. + * * @since 2.0.9 + * + * @see #MavenProject(MavenProject) */ @Override public MavenProject clone() { @@ -1029,119 +1248,113 @@ public MavenProject clone() { } catch (CloneNotSupportedException e) { throw new UnsupportedOperationException(e); } - clone.deepCopy(this); - return clone; } - public void setModel(Model model) { + public void setModel(@Nonnull Model model) { this.model = model; } - protected void setAttachedArtifacts(List attachedArtifacts) { + /** + * Sets the artifacts attached to this project. + * + * @param attachedArtifacts the new artifacts attached to this project + */ + protected void setAttachedArtifacts(@Nonnull List attachedArtifacts) { this.attachedArtifacts = attachedArtifacts; } - protected void setCompileSourceRoots(List compileSourceRoots) { + /** + * Sets the source root directories of this project. + * + * @param compileSourceRoots the new source root directories + */ + protected void setCompileSourceRoots(@Nonnull List compileSourceRoots) { this.compileSourceRoots = compileSourceRoots; } - protected void setTestCompileSourceRoots(List testCompileSourceRoots) { + /** + * Sets the test source directories of this project. + * + * @param testCompileSourceRoots the new test source directories + */ + protected void setTestCompileSourceRoots(@Nonnull List testCompileSourceRoots) { this.testCompileSourceRoots = testCompileSourceRoots; } + @Nullable protected ArtifactRepository getReleaseArtifactRepository() { return releaseArtifactRepository; } + @Nullable protected ArtifactRepository getSnapshotArtifactRepository() { return snapshotArtifactRepository; } private void deepCopy(MavenProject project) { // disown the parent - // copy fields file = project.file; basedir = project.basedir; - // don't need a deep copy, they don't get modified or added/removed to/from - but make them unmodifiable to be // sure! if (project.getDependencyArtifacts() != null) { setDependencyArtifacts(Collections.unmodifiableSet(project.getDependencyArtifacts())); } - if (project.getArtifacts() != null) { setArtifacts(Collections.unmodifiableSet(project.getArtifacts())); } - if (project.getParentFile() != null) { parentFile = new File(project.getParentFile().getAbsolutePath()); } - if (project.getPluginArtifacts() != null) { setPluginArtifacts(Collections.unmodifiableSet(project.getPluginArtifacts())); } - if (project.getReportArtifacts() != null) { setReportArtifacts(Collections.unmodifiableSet(project.getReportArtifacts())); } - if (project.getExtensionArtifacts() != null) { setExtensionArtifacts(Collections.unmodifiableSet(project.getExtensionArtifacts())); } - setParentArtifact((project.getParentArtifact())); - if (project.getRemoteArtifactRepositories() != null) { setRemoteArtifactRepositories(Collections.unmodifiableList(project.getRemoteArtifactRepositories())); } - if (project.getPluginArtifactRepositories() != null) { setPluginArtifactRepositories(Collections.unmodifiableList(project.getPluginArtifactRepositories())); } - if (project.getActiveProfiles() != null) { setActiveProfiles((Collections.unmodifiableList(project.getActiveProfiles()))); } - if (project.getAttachedArtifacts() != null) { // clone properties modifiable by plugins in a forked lifecycle setAttachedArtifacts(new ArrayList<>(project.getAttachedArtifacts())); } - if (project.getCompileSourceRoots() != null) { // clone source roots setCompileSourceRoots((new ArrayList<>(project.getCompileSourceRoots()))); } - if (project.getTestCompileSourceRoots() != null) { setTestCompileSourceRoots((new ArrayList<>(project.getTestCompileSourceRoots()))); } - if (project.getScriptSourceRoots() != null) { setScriptSourceRoots((new ArrayList<>(project.getScriptSourceRoots()))); } - if (project.getModel() != null) { setModel(project.getModel().clone()); } - if (project.getOriginalModel() != null) { setOriginalModel(project.getOriginalModel()); } - setExecutionRoot(project.isExecutionRoot()); - if (project.getArtifact() != null) { setArtifact(ArtifactUtils.copyArtifact(project.getArtifact())); } - if (project.getManagedVersionMap() != null) { setManagedVersionMap(project.getManagedVersionMap()); } - lifecyclePhases.addAll(project.lifecyclePhases); } @@ -1153,10 +1366,13 @@ private static String getProjectReferenceId(String groupId, String artifactId, S /** * Sets the value of the context value of this project identified by the given key. If the supplied value is - * null, the context value is removed from this project. Context values are intended to allow core + * {@code null}, the context value is removed from this project. Context values are intended to allow core * extensions to associate derived state with project instances. + * + * @param key the key to associate to a value + * @param value the value to associate to the given key, or {@code null} for removing the association */ - public void setContextValue(String key, Object value) { + public void setContextValue(@Nonnull String key, @Nullable Object value) { if (context == null) { context = new HashMap<>(); } @@ -1169,6 +1385,9 @@ public void setContextValue(String key, Object value) { /** * Returns context value of this project associated with the given key or null if this project has no such value. + * + * @param key the key of the value to get + * @return the associated value, or {@code null} if none */ public Object getContextValue(String key) { if (context == null) { @@ -1183,6 +1402,7 @@ public Object getContextValue(String key) { * without prior notice and must not be used by plugins. * * @param classRealm The class realm hosting the build extensions of this project, may be {@code null}. + * @hidden */ public void setClassRealm(ClassRealm classRealm) { this.classRealm = classRealm; @@ -1195,6 +1415,7 @@ public void setClassRealm(ClassRealm classRealm) { * used by plugins. * * @return The project's class realm or {@code null}. + * @hidden */ public ClassRealm getClassRealm() { return classRealm; @@ -1206,6 +1427,7 @@ public ClassRealm getClassRealm() { * In particular, this method can be changed or deleted without prior notice and must not be used by plugins. * * @param extensionDependencyFilter The dependency filter to apply to plugins, may be {@code null}. + * @hidden */ public void setExtensionDependencyFilter(DependencyFilter extensionDependencyFilter) { this.extensionDependencyFilter = extensionDependencyFilter; @@ -1218,6 +1440,7 @@ public void setExtensionDependencyFilter(DependencyFilter extensionDependencyFil * used by plugins. * * @return The dependency filter or {@code null}. + * @hidden */ public DependencyFilter getExtensionDependencyFilter() { return extensionDependencyFilter; @@ -1229,9 +1452,10 @@ public DependencyFilter getExtensionDependencyFilter() { * part of the public API. In particular, this method can be changed or deleted without prior notice and must not be * used by plugins. * - * @param artifacts The set of artifacts, may be {@code null}. + * @param artifacts the set of artifacts, may be {@code null} + * @hidden */ - public void setResolvedArtifacts(Set artifacts) { + public void setResolvedArtifacts(@Nullable Set artifacts) { this.resolvedArtifacts = (artifacts != null) ? artifacts : Collections.emptySet(); this.artifacts = null; this.artifactMap = null; @@ -1243,9 +1467,10 @@ public void setResolvedArtifacts(Set artifacts) { * part of the public API. In particular, this method can be changed or deleted without prior notice and must not be * used by plugins. * - * @param artifactFilter The artifact filter, may be {@code null} to exclude all artifacts. + * @param artifactFilter the artifact filter, may be {@code null} to exclude all artifacts + * @hidden */ - public void setArtifactFilter(ArtifactFilter artifactFilter) { + public void setArtifactFilter(@Nullable ArtifactFilter artifactFilter) { this.artifactFilter = artifactFilter; this.artifacts = null; this.artifactMap = null; @@ -1258,6 +1483,7 @@ public void setArtifactFilter(ArtifactFilter artifactFilter) { * * @param phase The phase to check for, must not be {@code null}. * @return {@code true} if the phase has been seen. + * @hidden */ public boolean hasLifecyclePhase(String phase) { return lifecyclePhases.contains(phase); @@ -1269,6 +1495,7 @@ public boolean hasLifecyclePhase(String phase) { * used by plugins. * * @param lifecyclePhase The lifecycle phase to add, must not be {@code null}. + * @hidden */ public void addLifecyclePhase(String lifecyclePhase) { lifecyclePhases.add(lifecyclePhase); @@ -1295,45 +1522,33 @@ public String getModulePathAdjustment(MavenProject moduleProject) throws IOExcep // FIXME: This is hacky. What if module directory doesn't match artifactid, and parent // is coming from the repository?? String module = moduleProject.getArtifactId(); - File moduleFile = moduleProject.getFile(); - if (moduleFile != null) { File moduleDir = moduleFile.getCanonicalFile().getParentFile(); - module = moduleDir.getName(); } - if (moduleAdjustments == null) { moduleAdjustments = new HashMap<>(); - List modules = getModules(); if (modules != null) { for (String modulePath : modules) { String moduleName = modulePath; - if (moduleName.endsWith("/") || moduleName.endsWith("\\")) { moduleName = moduleName.substring(0, moduleName.length() - 1); } - int lastSlash = moduleName.lastIndexOf('/'); - if (lastSlash < 0) { lastSlash = moduleName.lastIndexOf('\\'); } - String adjustment = null; - if (lastSlash > -1) { moduleName = moduleName.substring(lastSlash + 1); adjustment = modulePath.substring(0, lastSlash); } - moduleAdjustments.put(moduleName, adjustment); } } } - return moduleAdjustments.get(module); } @@ -1344,13 +1559,18 @@ public Set createArtifacts(ArtifactFactory artifactFactory, String inh artifactFactory, getModel().getDependencies(), inheritedScope, filter, this); } + /** + * Sets the test script directories of this project. + * + * @param scriptSourceRoots the new test script directories + */ @Deprecated - protected void setScriptSourceRoots(List scriptSourceRoots) { + protected void setScriptSourceRoots(@Nonnull List scriptSourceRoots) { this.scriptSourceRoots = scriptSourceRoots; } @Deprecated - public void addScriptSourceRoot(String path) { + public void addScriptSourceRoot(@Nullable String path) { if (path != null) { path = path.trim(); if (path.length() != 0) { @@ -1361,15 +1581,16 @@ public void addScriptSourceRoot(String path) { } } + @Nonnull @Deprecated public List getScriptSourceRoots() { return scriptSourceRoots; } + @Nonnull @Deprecated public List getCompileArtifacts() { List list = new ArrayList<>(getArtifacts().size()); - for (Artifact a : getArtifacts()) { // TODO classpath check doesn't belong here - that's the other method if (a.getArtifactHandler().isAddedToClasspath()) { @@ -1382,16 +1603,14 @@ public List getCompileArtifacts() { return list; } + @Nonnull @Deprecated public List getCompileDependencies() { Set artifacts = getArtifacts(); - if ((artifacts == null) || artifacts.isEmpty()) { return Collections.emptyList(); } - List list = new ArrayList<>(artifacts.size()); - for (Artifact a : getArtifacts()) { // TODO let the scope handler deal with this if (isCompilePathElement(a.getScope())) { @@ -1410,10 +1629,10 @@ public List getCompileDependencies() { return Collections.unmodifiableList(list); } + @Nonnull @Deprecated public List getTestArtifacts() { List list = new ArrayList<>(getArtifacts().size()); - for (Artifact a : getArtifacts()) { // TODO classpath check doesn't belong here - that's the other method if (a.getArtifactHandler().isAddedToClasspath()) { @@ -1423,19 +1642,16 @@ public List getTestArtifacts() { return list; } + @Nonnull @Deprecated public List getTestDependencies() { Set artifacts = getArtifacts(); - if ((artifacts == null) || artifacts.isEmpty()) { return Collections.emptyList(); } - List list = new ArrayList<>(artifacts.size()); - for (Artifact a : getArtifacts()) { Dependency dependency = new Dependency(); - dependency.setArtifactId(a.getArtifactId()); dependency.setGroupId(a.getGroupId()); dependency.setVersion(a.getVersion()); @@ -1448,38 +1664,34 @@ public List getTestDependencies() { return Collections.unmodifiableList(list); } + @Nonnull @Deprecated // used by the Maven ITs public List getRuntimeDependencies() { Set artifacts = getArtifacts(); - if ((artifacts == null) || artifacts.isEmpty()) { return Collections.emptyList(); } - List list = new ArrayList<>(artifacts.size()); - for (Artifact a : getArtifacts()) { // TODO let the scope handler deal with this if (isRuntimePathElement(a.getScope())) { Dependency dependency = new Dependency(); - dependency.setArtifactId(a.getArtifactId()); dependency.setGroupId(a.getGroupId()); dependency.setVersion(a.getVersion()); dependency.setScope(a.getScope()); dependency.setType(a.getType()); dependency.setClassifier(a.getClassifier()); - list.add(dependency); } } return Collections.unmodifiableList(list); } + @Nonnull @Deprecated public List getRuntimeArtifacts() { List list = new ArrayList<>(getArtifacts().size()); - for (Artifact a : getArtifacts()) { // TODO classpath check doesn't belong here - that's the other method if (a.getArtifactHandler().isAddedToClasspath() && isRuntimePathElement(a.getScope())) { @@ -1489,15 +1701,14 @@ public List getRuntimeArtifacts() { return list; } + @Nonnull @Deprecated public List getSystemClasspathElements() throws DependencyResolutionRequiredException { List list = new ArrayList<>(getArtifacts().size()); - String d = getBuild().getOutputDirectory(); if (d != null) { list.add(d); } - for (Artifact a : getArtifacts()) { if (a.getArtifactHandler().isAddedToClasspath()) { // TODO let the scope handler deal with this @@ -1512,10 +1723,10 @@ public List getSystemClasspathElements() throws DependencyResolutionRequ return list; } + @Nonnull @Deprecated public List getSystemArtifacts() { List list = new ArrayList<>(getArtifacts().size()); - for (Artifact a : getArtifacts()) { // TODO classpath check doesn't belong here - that's the other method if (a.getArtifactHandler().isAddedToClasspath()) { @@ -1528,28 +1739,24 @@ public List getSystemArtifacts() { return list; } + @Nonnull @Deprecated public List getSystemDependencies() { Set artifacts = getArtifacts(); - if ((artifacts == null) || artifacts.isEmpty()) { return Collections.emptyList(); } - List list = new ArrayList<>(artifacts.size()); - for (Artifact a : getArtifacts()) { // TODO let the scope handler deal with this if (Artifact.SCOPE_SYSTEM.equals(a.getScope())) { Dependency dependency = new Dependency(); - dependency.setArtifactId(a.getArtifactId()); dependency.setGroupId(a.getGroupId()); dependency.setVersion(a.getVersion()); dependency.setScope(a.getScope()); dependency.setType(a.getType()); dependency.setClassifier(a.getClassifier()); - list.add(dependency); } } @@ -1567,9 +1774,8 @@ public Reporting getReporting() { } @Deprecated - public void setReportArtifacts(Set reportArtifacts) { + public void setReportArtifacts(@Nonnull Set reportArtifacts) { this.reportArtifacts = reportArtifacts; - reportArtifactMap = null; } @@ -1578,33 +1784,34 @@ public Set getReportArtifacts() { return reportArtifacts; } + @Nonnull @Deprecated public Map getReportArtifactMap() { if (reportArtifactMap == null) { reportArtifactMap = ArtifactUtils.artifactMapByVersionlessId(getReportArtifacts()); } - return reportArtifactMap; } @Deprecated - public void setExtensionArtifacts(Set extensionArtifacts) { + public void setExtensionArtifacts(@Nonnull Set extensionArtifacts) { this.extensionArtifacts = extensionArtifacts; - extensionArtifactMap = null; } + @Nonnull @Deprecated + @SuppressWarnings("ReturnOfCollectionOrArrayField") // The set is already unmodifiable. public Set getExtensionArtifacts() { return extensionArtifacts; } + @Nonnull @Deprecated public Map getExtensionArtifactMap() { if (extensionArtifactMap == null) { extensionArtifactMap = ArtifactUtils.artifactMapByVersionlessId(getExtensionArtifacts()); } - return extensionArtifactMap; } @@ -1619,18 +1826,15 @@ public List getReportPlugins() { @Deprecated public Xpp3Dom getReportConfiguration(String pluginGroupId, String pluginArtifactId, String reportSetId) { Xpp3Dom dom = null; - // ---------------------------------------------------------------------- // I would like to be able to look up the Mojo object using a key but // we have a limitation in modello that will be remedied shortly. So // for now I have to iterate through and see what we have. // ---------------------------------------------------------------------- - if (getReportPlugins() != null) { for (ReportPlugin plugin : getReportPlugins()) { if (pluginGroupId.equals(plugin.getGroupId()) && pluginArtifactId.equals(plugin.getArtifactId())) { dom = (Xpp3Dom) plugin.getConfiguration(); - if (reportSetId != null) { ReportSet reportSet = plugin.getReportSetsAsMap().get(reportSetId); if (reportSet != null) { @@ -1645,12 +1849,10 @@ public Xpp3Dom getReportConfiguration(String pluginGroupId, String pluginArtifac } } } - if (dom != null) { // make a copy so the original in the POM doesn't get messed with dom = new Xpp3Dom(dom); } - return dom; } @@ -1702,17 +1904,19 @@ public ProjectBuildingRequest getProjectBuildingRequest() { * @param projectBuildingRequest The project building request, may be {@code null}. * @since 2.1 */ - // used by maven-dependency-tree - @Deprecated + @Deprecated // used by maven-dependency-tree public void setProjectBuildingRequest(ProjectBuildingRequest projectBuildingRequest) { this.projectBuilderConfiguration = projectBuildingRequest; } /** + * TODO: what is the difference with {@code basedir}? + * + * @return the root directory for this project + * @throws IllegalStateException if the root directory cannot be found * @since 4.0.0 - * @return the rootDirectory for this project - * @throws IllegalStateException if the rootDirectory cannot be found */ + @Nonnull public Path getRootDirectory() { if (rootDirectory == null) { throw new IllegalStateException(RootLocator.UNABLE_TO_FIND_ROOT_PROJECT_MESSAGE); @@ -1720,6 +1924,7 @@ public Path getRootDirectory() { return rootDirectory; } + @Nullable public void setRootDirectory(Path rootDirectory) { this.rootDirectory = rootDirectory; } From a54927f8afb96ed5d02321e256e42743eef66e61 Mon Sep 17 00:00:00 2001 From: Martin Desruisseaux Date: Sat, 26 Oct 2024 12:56:00 +0200 Subject: [PATCH 2/3] Code factorisation in `MavenProject` (the behaviour should stay equivalent to previous code): * Reduced some code duplication, e.g. with the addition of a private `toDependency(Artifact)` method. * Rewrite some methods using Stream because it saves some of those precious lines limited by Checkstyle. * Avoid invoking the same methods many times when the returned value should not change. --- .../apache/maven/project/MavenProject.java | 310 ++++++++---------- 1 file changed, 131 insertions(+), 179 deletions(-) diff --git a/impl/maven-core/src/main/java/org/apache/maven/project/MavenProject.java b/impl/maven-core/src/main/java/org/apache/maven/project/MavenProject.java index ed5bd456d082..e13de5e5de61 100644 --- a/impl/maven-core/src/main/java/org/apache/maven/project/MavenProject.java +++ b/impl/maven-core/src/main/java/org/apache/maven/project/MavenProject.java @@ -30,9 +30,11 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Properties; import java.util.Set; import java.util.function.Predicate; +import java.util.stream.Stream; import org.apache.maven.RepositoryUtils; import org.apache.maven.api.annotations.Nonnull; @@ -58,6 +60,7 @@ import org.apache.maven.model.MailingList; import org.apache.maven.model.Model; import org.apache.maven.model.Organization; +import org.apache.maven.model.Parent; import org.apache.maven.model.Plugin; import org.apache.maven.model.PluginExecution; import org.apache.maven.model.PluginManagement; @@ -519,18 +522,21 @@ private static boolean isTestPathElement(final String scope) { */ private List getClasspathElements(final Predicate scopeFilter, final boolean includeTestDir) throws DependencyResolutionRequiredException { - final List list = new ArrayList<>(getArtifacts().size() + 2); + @SuppressWarnings("LocalVariableHidesMemberVariable") // Usually the same content as the field. + Set artifacts = getArtifacts(); + final var list = new ArrayList(artifacts.size() + 2); + final var build = getBuild(); if (includeTestDir) { - String d = getBuild().getTestOutputDirectory(); + String d = build.getTestOutputDirectory(); if (d != null) { list.add(d); } } - String d = getBuild().getOutputDirectory(); + String d = build.getOutputDirectory(); if (d != null) { list.add(d); } - for (Artifact a : getArtifacts()) { + for (Artifact a : artifacts) { final File f = a.getFile(); if (f != null && scopeFilter.test(a.getScope())) { final ArtifactHandler h = a.getArtifactHandler(); @@ -605,9 +611,14 @@ public void setGroupId(String groupId) { } public String getGroupId() { - String groupId = getModel().getGroupId(); - if ((groupId == null) && (getModel().getParent() != null)) { - groupId = getModel().getParent().getGroupId(); + @SuppressWarnings("LocalVariableHidesMemberVariable") // Usually the same value as the field. + Model model = getModel(); + String groupId = model.getGroupId(); + if (groupId == null) { + Parent mp = model.getParent(); + if (mp != null) { + groupId = mp.getGroupId(); + } } return groupId; } @@ -626,8 +637,9 @@ public void setName(String name) { public String getName() { // TODO this should not be allowed to be null. - if (getModel().getName() != null) { - return getModel().getName(); + String name = getModel().getName(); + if (name != null) { + return name; } else { return getArtifactId(); } @@ -638,9 +650,14 @@ public void setVersion(String version) { } public String getVersion() { - String version = getModel().getVersion(); - if ((version == null) && (getModel().getParent() != null)) { - version = getModel().getParent().getVersion(); + @SuppressWarnings("LocalVariableHidesMemberVariable") // Usually the same value as the field. + Model model = getModel(); + String version = model.getVersion(); + if (version == null) { + Parent mp = getModel().getParent(); + if (mp != null) { + version = mp.getVersion(); + } } return version; } @@ -889,17 +906,21 @@ public List getRepositories() { // ---------------------------------------------------------------------- public List getBuildPlugins() { - if (getModel().getBuild() == null) { + Build build = getModel().getBuild(); + if (build == null) { return Collections.emptyList(); } - return Collections.unmodifiableList(getModel().getBuild().getPlugins()); + return Collections.unmodifiableList(build.getPlugins()); } public List getModules() { - if (!getModel().getDelegate().getSubprojects().isEmpty()) { - return getModel().getDelegate().getSubprojects(); + @SuppressWarnings("LocalVariableHidesMemberVariable") // Usually the same value as the field. + Model model = getModel(); + List subprojects = model.getDelegate().getSubprojects(); + if (!subprojects.isEmpty()) { + return subprojects; } - return getModel().getModules(); + return model.getModules(); } public PluginManagement getPluginManagement() { @@ -912,11 +933,12 @@ public PluginManagement getPluginManagement() { } private Build getModelBuild() { - Build build = getModel().getBuild(); + @SuppressWarnings("LocalVariableHidesMemberVariable") // Usually the same value as the field. + Model model = getModel(); + Build build = model.getBuild(); if (build == null) { build = new Build(); - - getModel().setBuild(build); + model.setBuild(build); } return build; } @@ -967,9 +989,13 @@ public List getPluginArtifactRepositories() { } public ArtifactRepository getDistributionManagementArtifactRepository() { - return getArtifact().isSnapshot() && (getSnapshotArtifactRepository() != null) - ? getSnapshotArtifactRepository() - : getReleaseArtifactRepository(); + if (getArtifact().isSnapshot()) { + ArtifactRepository sar = getSnapshotArtifactRepository(); + if (sar != null) { + return sar; + } + } + return getReleaseArtifactRepository(); } public List getPluginRepositories() { @@ -1061,8 +1087,9 @@ public List getAttachedArtifacts() { public Xpp3Dom getGoalConfiguration( String pluginGroupId, String pluginArtifactId, String executionId, String goalId) { Xpp3Dom dom = null; - if (getBuildPlugins() != null) { - for (Plugin plugin : getBuildPlugins()) { + List plugins = getBuildPlugins(); + if (plugins != null) { + for (Plugin plugin : plugins) { if (pluginGroupId.equals(plugin.getGroupId()) && pluginArtifactId.equals(plugin.getArtifactId())) { dom = (Xpp3Dom) plugin.getConfiguration(); if (executionId != null) { @@ -1173,11 +1200,13 @@ public int hashCode() { public List getBuildExtensions() { Build build = getBuild(); - if ((build == null) || (build.getExtensions() == null)) { - return Collections.emptyList(); - } else { - return Collections.unmodifiableList(build.getExtensions()); + if (build != null) { + List extensions = build.getExtensions(); + if (extensions != null) { + return Collections.unmodifiableList(extensions); + } } + return Collections.emptyList(); } public void addProjectReference(@Nonnull MavenProject project) { @@ -1219,16 +1248,11 @@ public Plugin getPlugin(String pluginKey) { */ @Override public String toString() { - StringBuilder sb = new StringBuilder(128); - sb.append("MavenProject: "); - sb.append(getGroupId()); - sb.append(':'); - sb.append(getArtifactId()); - sb.append(':'); - sb.append(getVersion()); - if (getFile() != null) { - sb.append(" @ "); - sb.append(getFile().getPath()); + StringBuilder sb = new StringBuilder(128).append("MavenProject: "); + sb.append(getGroupId()).append(':').append(getArtifactId()).append(':').append(getVersion()); + File f = getFile(); + if (f != null) { + sb.append(" @ ").append(f.getPath()); } return sb.toString(); } @@ -1246,7 +1270,7 @@ public MavenProject clone() { try { clone = (MavenProject) super.clone(); } catch (CloneNotSupportedException e) { - throw new UnsupportedOperationException(e); + throw new AssertionError(e); // Should never happen since this class is cloneable. } clone.deepCopy(this); return clone; @@ -1571,14 +1595,7 @@ protected void setScriptSourceRoots(@Nonnull List scriptSourceRoots) { @Deprecated public void addScriptSourceRoot(@Nullable String path) { - if (path != null) { - path = path.trim(); - if (path.length() != 0) { - if (!getScriptSourceRoots().contains(path)) { - getScriptSourceRoots().add(path); - } - } - } + addPath(getScriptSourceRoots(), path); } @Nonnull @@ -1590,177 +1607,112 @@ public List getScriptSourceRoots() { @Nonnull @Deprecated public List getCompileArtifacts() { - List list = new ArrayList<>(getArtifacts().size()); - for (Artifact a : getArtifacts()) { - // TODO classpath check doesn't belong here - that's the other method - if (a.getArtifactHandler().isAddedToClasspath()) { - // TODO let the scope handler deal with this - if (isCompilePathElement(a.getScope())) { - list.add(a); - } - } - } - return list; + return getClasspathArtifacts(MavenProject::isCompilePathElement); } @Nonnull @Deprecated public List getCompileDependencies() { - Set artifacts = getArtifacts(); - if ((artifacts == null) || artifacts.isEmpty()) { - return Collections.emptyList(); - } - List list = new ArrayList<>(artifacts.size()); - for (Artifact a : getArtifacts()) { - // TODO let the scope handler deal with this - if (isCompilePathElement(a.getScope())) { - Dependency dependency = new Dependency(); - - dependency.setArtifactId(a.getArtifactId()); - dependency.setGroupId(a.getGroupId()); - dependency.setVersion(a.getVersion()); - dependency.setScope(a.getScope()); - dependency.setType(a.getType()); - dependency.setClassifier(a.getClassifier()); - - list.add(dependency); - } - } - return Collections.unmodifiableList(list); + return getDependencies(MavenProject::isCompilePathElement); } @Nonnull @Deprecated public List getTestArtifacts() { - List list = new ArrayList<>(getArtifacts().size()); - for (Artifact a : getArtifacts()) { - // TODO classpath check doesn't belong here - that's the other method - if (a.getArtifactHandler().isAddedToClasspath()) { - list.add(a); - } - } - return list; + return getClasspathArtifacts(MavenProject::isAddedToClasspath); } @Nonnull @Deprecated public List getTestDependencies() { - Set artifacts = getArtifacts(); - if ((artifacts == null) || artifacts.isEmpty()) { - return Collections.emptyList(); - } - List list = new ArrayList<>(artifacts.size()); - for (Artifact a : getArtifacts()) { - Dependency dependency = new Dependency(); - dependency.setArtifactId(a.getArtifactId()); - dependency.setGroupId(a.getGroupId()); - dependency.setVersion(a.getVersion()); - dependency.setScope(a.getScope()); - dependency.setType(a.getType()); - dependency.setClassifier(a.getClassifier()); - - list.add(dependency); - } - return Collections.unmodifiableList(list); + return getDependencies(MavenProject::isTestPathElement); } @Nonnull @Deprecated // used by the Maven ITs public List getRuntimeDependencies() { - Set artifacts = getArtifacts(); - if ((artifacts == null) || artifacts.isEmpty()) { - return Collections.emptyList(); - } - List list = new ArrayList<>(artifacts.size()); - for (Artifact a : getArtifacts()) { - // TODO let the scope handler deal with this - if (isRuntimePathElement(a.getScope())) { - Dependency dependency = new Dependency(); - dependency.setArtifactId(a.getArtifactId()); - dependency.setGroupId(a.getGroupId()); - dependency.setVersion(a.getVersion()); - dependency.setScope(a.getScope()); - dependency.setType(a.getType()); - dependency.setClassifier(a.getClassifier()); - list.add(dependency); - } - } - return Collections.unmodifiableList(list); + return getDependencies(MavenProject::isRuntimePathElement); } @Nonnull @Deprecated public List getRuntimeArtifacts() { - List list = new ArrayList<>(getArtifacts().size()); - for (Artifact a : getArtifacts()) { - // TODO classpath check doesn't belong here - that's the other method - if (a.getArtifactHandler().isAddedToClasspath() && isRuntimePathElement(a.getScope())) { - list.add(a); - } - } - return list; + return getClasspathArtifacts(MavenProject::isRuntimePathElement); } @Nonnull @Deprecated public List getSystemClasspathElements() throws DependencyResolutionRequiredException { - List list = new ArrayList<>(getArtifacts().size()); - String d = getBuild().getOutputDirectory(); - if (d != null) { - list.add(d); - } - for (Artifact a : getArtifacts()) { - if (a.getArtifactHandler().isAddedToClasspath()) { - // TODO let the scope handler deal with this - if (Artifact.SCOPE_SYSTEM.equals(a.getScope())) { - File f = a.getFile(); - if (f != null) { - list.add(f.getPath()); - } - } - } - } - return list; + Stream s1 = Optional.ofNullable(getBuild().getOutputDirectory()).stream(); + Stream s2 = getArtifacts().stream() + .filter(MavenProject::isAddedToClasspath) + .filter(MavenProject::isScopeSystem) + .map((Artifact::getFile)) + .filter(Objects::nonNull) + .map(File::getPath); + return Stream.concat(s1, s2).toList(); } @Nonnull @Deprecated public List getSystemArtifacts() { - List list = new ArrayList<>(getArtifacts().size()); - for (Artifact a : getArtifacts()) { - // TODO classpath check doesn't belong here - that's the other method - if (a.getArtifactHandler().isAddedToClasspath()) { - // TODO let the scope handler deal with this - if (Artifact.SCOPE_SYSTEM.equals(a.getScope())) { - list.add(a); - } - } - } - return list; + return getClasspathArtifacts(MavenProject::isScopeSystem); } @Nonnull @Deprecated public List getSystemDependencies() { - Set artifacts = getArtifacts(); - if ((artifacts == null) || artifacts.isEmpty()) { - return Collections.emptyList(); - } - List list = new ArrayList<>(artifacts.size()); - for (Artifact a : getArtifacts()) { - // TODO let the scope handler deal with this - if (Artifact.SCOPE_SYSTEM.equals(a.getScope())) { - Dependency dependency = new Dependency(); - dependency.setArtifactId(a.getArtifactId()); - dependency.setGroupId(a.getGroupId()); - dependency.setVersion(a.getVersion()); - dependency.setScope(a.getScope()); - dependency.setType(a.getType()); - dependency.setClassifier(a.getClassifier()); - list.add(dependency); - } - } - return Collections.unmodifiableList(list); + return getDependencies(MavenProject::isScopeSystem); + } + + // TODO let the scope handler deal with this + private static boolean isCompilePathElement(Artifact a) { + return isCompilePathElement(a.getScope()); + } + + // TODO let the scope handler deal with this + private static boolean isRuntimePathElement(Artifact a) { + return isRuntimePathElement(a.getScope()); + } + + // TODO let the scope handler deal with this + private static boolean isTestPathElement(Artifact a) { + return isTestPathElement(a.getScope()); + } + + // TODO let the scope handler deal with this + private static boolean isScopeSystem(Artifact a) { + return Artifact.SCOPE_SYSTEM.equals(a.getScope()); + } + + // TODO classpath check doesn't belong here - that's the other method + private static boolean isAddedToClasspath(Artifact a) { + return a.getArtifactHandler().isAddedToClasspath(); + } + + private List getClasspathArtifacts(final Predicate filter) { + return getArtifacts().stream() + .filter(MavenProject::isAddedToClasspath) + .filter(filter) + .toList(); + } + + private List getDependencies(final Predicate filter) { + return getArtifacts().stream() + .filter(filter) + .map(MavenProject::toDependency) + .toList(); + } + + private static Dependency toDependency(Artifact a) { + Dependency dependency = new Dependency(); + dependency.setArtifactId(a.getArtifactId()); + dependency.setGroupId(a.getGroupId()); + dependency.setVersion(a.getVersion()); + dependency.setScope(a.getScope()); + dependency.setType(a.getType()); + dependency.setClassifier(a.getClassifier()); + return dependency; } @Deprecated From de502e4959c642bbbd197e0e78726191c72d4fa6 Mon Sep 17 00:00:00 2001 From: Martin Desruisseaux Date: Sat, 26 Oct 2024 13:28:33 +0200 Subject: [PATCH 3/3] Stronger encapsulation of collection fields in `MavenProject` with immutability and defensive copies. The following rules are applied in this commit (the previous situation was a mix of different practices): MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * All getter methods except `getDependencyArtifacts()` return an empty collection instead of null. * All setter methods except delegates make a defensive copy of the given collection, preserving order. * All getter methods returns an unmodifiable (but not necessarily immutable) collection. * The collections of properties that do not have an `addFoo(…)` method are immutable. * The `clone()` method copies all non-immutable collections, and only them. * `MavenProject(MavenProject)` assigns fields directly without invoking setter methods. --- .../apache/maven/project/MavenProject.java | 359 +++++++++++------- .../maven/project/MavenProjectTest.java | 7 +- 2 files changed, 225 insertions(+), 141 deletions(-) diff --git a/impl/maven-core/src/main/java/org/apache/maven/project/MavenProject.java b/impl/maven-core/src/main/java/org/apache/maven/project/MavenProject.java index e13de5e5de61..82a3f55c1034 100644 --- a/impl/maven-core/src/main/java/org/apache/maven/project/MavenProject.java +++ b/impl/maven-core/src/main/java/org/apache/maven/project/MavenProject.java @@ -23,6 +23,7 @@ import java.io.Writer; import java.nio.file.Path; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashMap; @@ -191,9 +192,10 @@ public class MavenProject implements Cloneable { /** * List of the attached artifacts to this project. + * This is a modifiable collection which needs to be copied by {@link #deepCopy()}. */ @Nonnull - private List attachedArtifacts = new ArrayList<>(); + private List attachedArtifacts; @Nullable private MavenProject executionProject; @@ -207,21 +209,24 @@ public class MavenProject implements Cloneable { /** * Root directories of source codes to compile. + * This is a modifiable collection which needs to be copied by {@link #deepCopy()}. */ @Nonnull - private List compileSourceRoots = new ArrayList<>(); + private List compileSourceRoots; /** * Root directories of test codes to compile. + * This is a modifiable collection which needs to be copied by {@link #deepCopy()}. */ @Nonnull - private List testCompileSourceRoots = new ArrayList<>(); + private List testCompileSourceRoots; /** * Root directories of scriot codes. + * This is a modifiable collection which needs to be copied by {@link #deepCopy()}. */ @Nonnull - private List scriptSourceRoots = new ArrayList<>(); + private List scriptSourceRoots; @Nullable private ArtifactRepository releaseArtifactRepository; @@ -234,13 +239,14 @@ public class MavenProject implements Cloneable { * This is an immutable collection potentially shared by cloned {@code MavenProject} instances. */ @Nonnull - private List activeProfiles = new ArrayList<>(); + private List activeProfiles; /** * The identifiers of all profiles that contributed to this project's effective model. + * This is a modifiable collection which needs to be copied by {@link #deepCopy()}. */ @Nonnull - private Map> injectedProfileIds = new LinkedHashMap<>(); + private Map> injectedProfileIds; /** * Direct dependencies that this project has. @@ -279,9 +285,10 @@ public class MavenProject implements Cloneable { /** * Projects by identifiers. + * This is a modifiable collection which needs to be copied by {@link #deepCopy()}. */ @Nonnull - private Map projectReferences = new HashMap<>(); + private Map projectReferences; private boolean executionRoot; @@ -290,6 +297,7 @@ public class MavenProject implements Cloneable { /** * Key-value pairs providing context. + * This is a modifiable collection which needs to be copied by {@link #deepCopy()}. */ @Nonnull private Map context; @@ -302,19 +310,19 @@ public class MavenProject implements Cloneable { /** * The life cycle phases. + * This is a modifiable collection which needs to be copied by {@link #deepCopy()}. */ @Nonnull - private final Set lifecyclePhases = Collections.synchronizedSet(new LinkedHashSet<>()); + private Set lifecyclePhases; /** * Creates an initially empty project. */ public MavenProject() { - Model model = new Model(); + this(new Model()); model.setGroupId(EMPTY_PROJECT_GROUP_ID); model.setArtifactId(EMPTY_PROJECT_ARTIFACT_ID); model.setVersion(EMPTY_PROJECT_VERSION); - setModel(model); } public MavenProject(org.apache.maven.api.model.Model model) { @@ -327,7 +335,28 @@ public MavenProject(org.apache.maven.api.model.Model model) { * @param model the model to wrap in a maven project */ public MavenProject(@Nonnull Model model) { - setModel(model); + // Do not invoke `setModel(Model)` as escaping `this` is deprecated. + this.model = Objects.requireNonNull(model); + + // Immutable collections. + pluginArtifacts = Set.of(); + remoteArtifactRepositories = List.of(); + pluginArtifactRepositories = List.of(); + collectedProjects = List.of(); + activeProfiles = List.of(); + reportArtifacts = Set.of(); + extensionArtifacts = Set.of(); + managedVersionMap = Map.of(); + + // Mutable collections. + attachedArtifacts = new ArrayList<>(); + compileSourceRoots = new ArrayList<>(); + testCompileSourceRoots = new ArrayList<>(); + scriptSourceRoots = new ArrayList<>(); + injectedProfileIds = new LinkedHashMap<>(); + projectReferences = new HashMap<>(); + context = new HashMap<>(); + lifecyclePhases = Collections.synchronizedSet(new LinkedHashSet<>()); } /** @@ -337,7 +366,76 @@ public MavenProject(@Nonnull Model model) { * @see #clone() */ public MavenProject(@Nonnull MavenProject project) { - deepCopy(project); + // Do not invoke setter methods. See "this-escaped" compiler warning. + model = project.getModel(); + parent = project.getParent(); + file = project.getFile(); + basedir = project.getBasedir(); + rootDirectory = project.getRootDirectory(); + artifactFilter = project.artifactFilter; // This internal property has no getter. + parentArtifact = project.getParentArtifact(); + executionProject = project.executionProject; // Intentionally avoid the getter. + releaseArtifactRepository = project.getReleaseArtifactRepository(); + snapshotArtifactRepository = project.getSnapshotArtifactRepository(); + artifact = project.getArtifact(); + originalModel = project.getOriginalModel(); + executionRoot = project.isExecutionRoot(); + parentFile = project.getParentFile(); + classRealm = project.getClassRealm(); + extensionDependencyFilter = project.getExtensionDependencyFilter(); + + // Immutable collections. + resolvedArtifacts = project.resolvedArtifacts; // This internal property has no getter. + artifacts = project.getArtifacts(); + pluginArtifacts = project.getPluginArtifacts(); + remoteArtifactRepositories = project.getRemoteArtifactRepositories(); + pluginArtifactRepositories = project.getPluginArtifactRepositories(); + remoteProjectRepositories = project.getRemoteProjectRepositories(); + remotePluginRepositories = project.getRemotePluginRepositories(); + collectedProjects = project.getCollectedProjects(); + activeProfiles = project.getActiveProfiles(); + dependencyArtifacts = project.getDependencyArtifacts(); + artifactMap = project.getArtifactMap(); + pluginArtifactMap = project.getPluginArtifactMap(); + reportArtifacts = project.getReportArtifacts(); + reportArtifactMap = project.getReportArtifactMap(); + extensionArtifacts = project.getExtensionArtifacts(); + extensionArtifactMap = project.getExtensionArtifactMap(); + managedVersionMap = project.getManagedVersionMap(); + + // Mutable collections. Will be copied by `deepCopy()`. + attachedArtifacts = project.getAttachedArtifacts(); + compileSourceRoots = project.getCompileSourceRoots(); + testCompileSourceRoots = project.getTestCompileSourceRoots(); + scriptSourceRoots = project.getScriptSourceRoots(); + injectedProfileIds = project.getInjectedProfileIds(); // Mutable, but will be copied by `deepCopy()`. + projectReferences = project.getProjectReferences(); + context = project.context; + lifecyclePhases = project.lifecyclePhases; + deepCopy(); + } + + /** + * Copies in-place the modifiable values of this {@code MavenProject} instance. + * This method should be invoked after a clone or after the copy constructor. + * This method should not invoke any user-overrideable method (e.g., no setter). + */ + private void deepCopy() { + model = model.clone(); + if (originalModel != null) { + originalModel = originalModel.clone(); + } + if (parentFile != null) { + parentFile = parentFile.getAbsoluteFile(); + } + attachedArtifacts = new ArrayList<>(attachedArtifacts); + compileSourceRoots = new ArrayList<>(compileSourceRoots); + testCompileSourceRoots = new ArrayList<>(testCompileSourceRoots); + scriptSourceRoots = new ArrayList<>(scriptSourceRoots); + injectedProfileIds = new LinkedHashMap<>(injectedProfileIds); + projectReferences = new LinkedHashMap<>(projectReferences); + context = new LinkedHashMap<>(context); + lifecyclePhases = Collections.synchronizedSet(new LinkedHashSet<>(lifecyclePhases)); } // TODO: where is the difference with {@code basedir} and {@code rootDirectory}? @@ -466,7 +564,7 @@ private void addPath(List paths, String path) { * @param path the path to add if non-null */ public void addCompileSourceRoot(@Nullable String path) { - addPath(getCompileSourceRoots(), path); + addPath(compileSourceRoots, path); } /** @@ -475,7 +573,7 @@ public void addCompileSourceRoot(@Nullable String path) { * @param path the path to add if non-null */ public void addTestCompileSourceRoot(@Nullable String path) { - addPath(getTestCompileSourceRoots(), path); + addPath(testCompileSourceRoots, path); } /** @@ -483,7 +581,7 @@ public void addTestCompileSourceRoot(@Nullable String path) { */ @Nonnull public List getCompileSourceRoots() { - return compileSourceRoots; + return Collections.unmodifiableList(compileSourceRoots); } /** @@ -491,7 +589,7 @@ public List getCompileSourceRoots() { */ @Nonnull public List getTestCompileSourceRoots() { - return testCompileSourceRoots; + return Collections.unmodifiableList(testCompileSourceRoots); } // TODO let the scope handler deal with this @@ -810,8 +908,43 @@ public void addLicense(License license) { getModel().addLicense(license); } + private static Map copyWithSameOrder(Map items) { + switch (items.size()) { + case 0: + return Map.of(); + case 1: + return Map.copyOf(items); + default: + items = new LinkedHashMap<>(items); + if (items.containsKey(null)) { + throw new NullPointerException("The given map shall not contain the null key."); + } + if (items.containsValue(null)) { + throw new NullPointerException("The given map shall not contain the null values."); + } + return Collections.unmodifiableMap(items); + } + } + + private static Set copyWithSameOrder(Collection items) { + switch (items.size()) { + case 0: + return Set.of(); + case 1: + return Set.copyOf(items); + default: + var copy = new LinkedHashSet<>(items); + if (copy.contains(null)) { + throw new NullPointerException("The given set shall not contain the null element."); + } + return Collections.unmodifiableSet(copy); + } + } + /** * Sets all dependencies that this project has, including transitive ones. + * The given set is copied: changes to the given set after this method call has no effect on this object. + * This copy is for ensuring that {@link #getArtifactMap()} stay consistent with the values given to this method. * *

Side effects

* Invoking this method will also modify the values returned by {@link #getArtifactMap()}. @@ -819,7 +952,7 @@ public void addLicense(License license) { * @param artifacts the new project dependencies */ public void setArtifacts(@Nonnull Set artifacts) { - this.artifacts = artifacts; + this.artifacts = copyWithSameOrder(artifacts); artifactMap = null; // flush the calculated artifactMap } @@ -828,21 +961,18 @@ public void setArtifacts(@Nonnull Set artifacts) { * what phases have run dependencies in some scopes won't be included. e.g. if only compile phase has run, * dependencies with scope test won't be included. * - * @return set of dependencies + * @return unmodifiable set of dependencies */ @Nonnull @SuppressWarnings("ReturnOfCollectionOrArrayField") // The returned set is unmodifiable. public Set getArtifacts() { if (artifacts == null) { if (artifactFilter == null || resolvedArtifacts == null) { - artifacts = new LinkedHashSet<>(); + artifacts = Set.of(); } else { - artifacts = new LinkedHashSet<>(resolvedArtifacts.size() * 2); - for (Artifact artifact : resolvedArtifacts) { - if (artifactFilter.include(artifact)) { - artifacts.add(artifact); - } - } + artifacts = copyWithSameOrder(resolvedArtifacts.stream() + .filter((a) -> artifactFilter.include(a)) + .toList()); } } return artifacts; @@ -853,11 +983,14 @@ public Map getArtifactMap() { if (artifactMap == null) { artifactMap = ArtifactUtils.artifactMapByVersionlessId(getArtifacts()); } - return artifactMap; + return Collections.unmodifiableMap(artifactMap); } /** * Sets all plugins that this project has, including transitive ones. + * The given set is copied: changes to the given set after this method call has no effect on this object. + * This copy is for ensuring that {@link #getPluginArtifactMap()} stay consistent with the values given + * to this method. * *

Side effects

* Invoking this method will also modify the values returned by {@link #getPluginArtifactMap()}. @@ -865,7 +998,7 @@ public Map getArtifactMap() { * @param pluginArtifacts the new project plugins */ public void setPluginArtifacts(@Nonnull Set pluginArtifacts) { - this.pluginArtifacts = pluginArtifacts; + this.pluginArtifacts = copyWithSameOrder(pluginArtifacts); this.pluginArtifactMap = null; } @@ -885,7 +1018,7 @@ public Map getPluginArtifactMap() { if (pluginArtifactMap == null) { pluginArtifactMap = ArtifactUtils.artifactMapByVersionlessId(getPluginArtifacts()); } - return pluginArtifactMap; + return Collections.unmodifiableMap(pluginArtifactMap); } public void setParentArtifact(@Nullable Artifact parentArtifact) { @@ -945,12 +1078,15 @@ private Build getModelBuild() { /** * Sets the artifact repositories of this project. + * The given list is copied: changes to the given list after this method call has no effect on this object. + * This copy is for ensuring that {@link #getRemoteProjectRepositories()} stay consistent with the values + * given to this method. * * @param remoteArtifactRepositories the new artifact repositories */ public void setRemoteArtifactRepositories(@Nonnull List remoteArtifactRepositories) { - this.remoteArtifactRepositories = remoteArtifactRepositories; - this.remoteProjectRepositories = RepositoryUtils.toRepos(getRemoteArtifactRepositories()); + this.remoteArtifactRepositories = List.copyOf(remoteArtifactRepositories); + this.remoteProjectRepositories = null; // Recompute when first requested. } /** @@ -960,31 +1096,29 @@ public void setRemoteArtifactRepositories(@Nonnull List remo @Nonnull @SuppressWarnings("ReturnOfCollectionOrArrayField") // The returned list is unmodifiable. public List getRemoteArtifactRepositories() { - if (remoteArtifactRepositories == null) { - remoteArtifactRepositories = new ArrayList<>(); - } return remoteArtifactRepositories; } /** * Sets the plugin repositories of this project. + * The given list is copied: changes to the given list after this method call has no effect on this object. + * This copy is for ensuring that {@link #getRemotePluginRepositories()} stay consistent with the values + * given to this method. * * @param pluginArtifactRepositories the new artifact repositories */ public void setPluginArtifactRepositories(@Nonnull List pluginArtifactRepositories) { - this.pluginArtifactRepositories = pluginArtifactRepositories; - this.remotePluginRepositories = RepositoryUtils.toRepos(getPluginArtifactRepositories()); + this.pluginArtifactRepositories = List.copyOf(pluginArtifactRepositories); + this.remotePluginRepositories = null; // Recompute when first requested. } /** - * {@return the plugin repositories of this project}. + * {@return the plugin repositories of this project}. The returned list is unmodifiable for ensuring + * that {@link #getRemotePluginRepositories()} stay consistent with the values returned by this method. */ @Nonnull @SuppressWarnings("ReturnOfCollectionOrArrayField") // The returned list is unmodifiable. public List getPluginArtifactRepositories() { - if (pluginArtifactRepositories == null) { - pluginArtifactRepositories = new ArrayList<>(); - } return pluginArtifactRepositories; } @@ -1003,32 +1137,43 @@ public List getPluginRepositories() { } @Nonnull + @SuppressWarnings("ReturnOfCollectionOrArrayField") // The cached list is already unmodifiable. public List getRemoteProjectRepositories() { + if (remoteProjectRepositories == null) { + remoteProjectRepositories = RepositoryUtils.toRepos(getRemoteArtifactRepositories()); + } return remoteProjectRepositories; } @Nonnull + @SuppressWarnings("ReturnOfCollectionOrArrayField") // The cached list is already unmodifiable. public List getRemotePluginRepositories() { + if (remotePluginRepositories == null) { + remotePluginRepositories = RepositoryUtils.toRepos(getPluginArtifactRepositories()); + } return remotePluginRepositories; } /** * Sets the active profiles of this project. + * The given list is copied: changes to the given list after this method call has no effect on this object. * * @param activeProfiles the new active profiles of this project */ public void setActiveProfiles(@Nonnull List activeProfiles) { - this.activeProfiles = activeProfiles; + this.activeProfiles = List.copyOf(activeProfiles); } @Nonnull + @SuppressWarnings("ReturnOfCollectionOrArrayField") // The list is already unmodifiable. public List getActiveProfiles() { return activeProfiles; } public void setInjectedProfileIds(@Nonnull String source, @Nullable List injectedProfileIds) { + Objects.requireNonNull(source); if (injectedProfileIds != null) { - this.injectedProfileIds.put(source, new ArrayList<>(injectedProfileIds)); + this.injectedProfileIds.put(source, List.copyOf(injectedProfileIds)); } else { this.injectedProfileIds.remove(source); } @@ -1045,7 +1190,7 @@ public void setInjectedProfileIds(@Nonnull String source, @Nullable List */ @Nonnull public Map> getInjectedProfileIds() { - return this.injectedProfileIds; + return Collections.unmodifiableMap(injectedProfileIds); } /** @@ -1061,6 +1206,9 @@ public Map> getInjectedProfileIds() { */ @Deprecated public void addAttachedArtifact(@Nonnull Artifact artifact) throws DuplicateArtifactAttachmentException { + if (artifact == null) { + return; // While we document this method as non-null, we observe that some callers provide a null value. + } // if already there we remove it and add again int index = attachedArtifacts.indexOf(artifact); if (index >= 0) { @@ -1078,9 +1226,6 @@ public void addAttachedArtifact(@Nonnull Artifact artifact) throws DuplicateArti */ @Nonnull public List getAttachedArtifacts() { - if (attachedArtifacts == null) { - attachedArtifacts = new ArrayList<>(); - } return Collections.unmodifiableList(attachedArtifacts); } @@ -1122,17 +1267,19 @@ public void setExecutionProject(@Nullable MavenProject executionProject) { } @Nonnull + @SuppressWarnings("ReturnOfCollectionOrArrayField") // The list is already unmodifiable. public List getCollectedProjects() { return collectedProjects; } /** * Sets the collected project. + * The given list is copied: changes to the given list after this method call has no effect on this object. * * @param collectedProjects the collected projects */ public void setCollectedProjects(@Nonnull List collectedProjects) { - this.collectedProjects = collectedProjects; + this.collectedProjects = List.copyOf(collectedProjects); } /** @@ -1145,13 +1292,14 @@ public void setCollectedProjects(@Nonnull List collectedProjects) */ @Deprecated @Nullable // For compatibility with previous behavior. + @SuppressWarnings("ReturnOfCollectionOrArrayField") // The set is already unmodifiable. public Set getDependencyArtifacts() { return dependencyArtifacts; } @Deprecated public void setDependencyArtifacts(@Nonnull Set dependencyArtifacts) { - this.dependencyArtifacts = dependencyArtifacts; + this.dependencyArtifacts = copyWithSameOrder(dependencyArtifacts); } public void setReleaseArtifactRepository(@Nullable ArtifactRepository releaseArtifactRepository) { @@ -1172,10 +1320,11 @@ public Model getOriginalModel() { } public void setManagedVersionMap(@Nonnull Map map) { - managedVersionMap = map; + managedVersionMap = copyWithSameOrder(map); } @Nonnull + @SuppressWarnings("ReturnOfCollectionOrArrayField") // The map is already unmodifiable. public Map getManagedVersionMap() { return managedVersionMap; } @@ -1224,7 +1373,7 @@ public List getFilters() { @Nonnull public Map getProjectReferences() { - return projectReferences; + return Collections.unmodifiableMap(projectReferences); } public boolean isExecutionRoot() { @@ -1236,7 +1385,7 @@ public void setExecutionRoot(boolean executionRoot) { } public String getDefaultGoal() { - return getBuild() != null ? getBuild().getDefaultGoal() : null; + return getBuild().getDefaultGoal(); } public Plugin getPlugin(String pluginKey) { @@ -1272,39 +1421,45 @@ public MavenProject clone() { } catch (CloneNotSupportedException e) { throw new AssertionError(e); // Should never happen since this class is cloneable. } - clone.deepCopy(this); + clone.deepCopy(); return clone; } public void setModel(@Nonnull Model model) { - this.model = model; + this.model = Objects.requireNonNull(model); } /** * Sets the artifacts attached to this project. + * The given list is copied: changes to the given list after this method call has no effect on this object. * * @param attachedArtifacts the new artifacts attached to this project */ protected void setAttachedArtifacts(@Nonnull List attachedArtifacts) { - this.attachedArtifacts = attachedArtifacts; + this.attachedArtifacts.clear(); + this.attachedArtifacts.addAll(attachedArtifacts); } /** * Sets the source root directories of this project. + * The given list is copied: changes to the given list after this method call has no effect on this object. * * @param compileSourceRoots the new source root directories */ protected void setCompileSourceRoots(@Nonnull List compileSourceRoots) { - this.compileSourceRoots = compileSourceRoots; + this.compileSourceRoots.clear(); + this.compileSourceRoots.addAll(compileSourceRoots); } /** * Sets the test source directories of this project. + * The given list is copied: changes to the given list after this method call has no effect on this object. * * @param testCompileSourceRoots the new test source directories */ protected void setTestCompileSourceRoots(@Nonnull List testCompileSourceRoots) { - this.testCompileSourceRoots = testCompileSourceRoots; + this.testCompileSourceRoots.clear(); + this.testCompileSourceRoots.addAll(testCompileSourceRoots); } @Nullable @@ -1317,71 +1472,6 @@ protected ArtifactRepository getSnapshotArtifactRepository() { return snapshotArtifactRepository; } - private void deepCopy(MavenProject project) { - // disown the parent - // copy fields - file = project.file; - basedir = project.basedir; - // don't need a deep copy, they don't get modified or added/removed to/from - but make them unmodifiable to be - // sure! - if (project.getDependencyArtifacts() != null) { - setDependencyArtifacts(Collections.unmodifiableSet(project.getDependencyArtifacts())); - } - if (project.getArtifacts() != null) { - setArtifacts(Collections.unmodifiableSet(project.getArtifacts())); - } - if (project.getParentFile() != null) { - parentFile = new File(project.getParentFile().getAbsolutePath()); - } - if (project.getPluginArtifacts() != null) { - setPluginArtifacts(Collections.unmodifiableSet(project.getPluginArtifacts())); - } - if (project.getReportArtifacts() != null) { - setReportArtifacts(Collections.unmodifiableSet(project.getReportArtifacts())); - } - if (project.getExtensionArtifacts() != null) { - setExtensionArtifacts(Collections.unmodifiableSet(project.getExtensionArtifacts())); - } - setParentArtifact((project.getParentArtifact())); - if (project.getRemoteArtifactRepositories() != null) { - setRemoteArtifactRepositories(Collections.unmodifiableList(project.getRemoteArtifactRepositories())); - } - if (project.getPluginArtifactRepositories() != null) { - setPluginArtifactRepositories(Collections.unmodifiableList(project.getPluginArtifactRepositories())); - } - if (project.getActiveProfiles() != null) { - setActiveProfiles((Collections.unmodifiableList(project.getActiveProfiles()))); - } - if (project.getAttachedArtifacts() != null) { - // clone properties modifiable by plugins in a forked lifecycle - setAttachedArtifacts(new ArrayList<>(project.getAttachedArtifacts())); - } - if (project.getCompileSourceRoots() != null) { - // clone source roots - setCompileSourceRoots((new ArrayList<>(project.getCompileSourceRoots()))); - } - if (project.getTestCompileSourceRoots() != null) { - setTestCompileSourceRoots((new ArrayList<>(project.getTestCompileSourceRoots()))); - } - if (project.getScriptSourceRoots() != null) { - setScriptSourceRoots((new ArrayList<>(project.getScriptSourceRoots()))); - } - if (project.getModel() != null) { - setModel(project.getModel().clone()); - } - if (project.getOriginalModel() != null) { - setOriginalModel(project.getOriginalModel()); - } - setExecutionRoot(project.isExecutionRoot()); - if (project.getArtifact() != null) { - setArtifact(ArtifactUtils.copyArtifact(project.getArtifact())); - } - if (project.getManagedVersionMap() != null) { - setManagedVersionMap(project.getManagedVersionMap()); - } - lifecyclePhases.addAll(project.lifecyclePhases); - } - private static String getProjectReferenceId(String groupId, String artifactId, String version) { StringBuilder buffer = new StringBuilder(128); buffer.append(groupId).append(':').append(artifactId).append(':').append(version); @@ -1397,9 +1487,7 @@ private static String getProjectReferenceId(String groupId, String artifactId, S * @param value the value to associate to the given key, or {@code null} for removing the association */ public void setContextValue(@Nonnull String key, @Nullable Object value) { - if (context == null) { - context = new HashMap<>(); - } + Objects.requireNonNull(key); if (value != null) { context.put(key, value); } else { @@ -1414,10 +1502,7 @@ public void setContextValue(@Nonnull String key, @Nullable Object value) { * @return the associated value, or {@code null} if none */ public Object getContextValue(String key) { - if (context == null) { - return null; - } - return context.get(key); + return context.get(Objects.requireNonNull(key)); } /** @@ -1480,7 +1565,7 @@ public DependencyFilter getExtensionDependencyFilter() { * @hidden */ public void setResolvedArtifacts(@Nullable Set artifacts) { - this.resolvedArtifacts = (artifacts != null) ? artifacts : Collections.emptySet(); + this.resolvedArtifacts = copyWithSameOrder(artifacts); this.artifacts = null; this.artifactMap = null; } @@ -1585,23 +1670,25 @@ public Set createArtifacts(ArtifactFactory artifactFactory, String inh /** * Sets the test script directories of this project. + * The given list is copied: changes to the given list after this method call has no effect on this object. * * @param scriptSourceRoots the new test script directories */ @Deprecated protected void setScriptSourceRoots(@Nonnull List scriptSourceRoots) { - this.scriptSourceRoots = scriptSourceRoots; + this.scriptSourceRoots.clear(); + this.scriptSourceRoots.addAll(scriptSourceRoots); } @Deprecated public void addScriptSourceRoot(@Nullable String path) { - addPath(getScriptSourceRoots(), path); + addPath(scriptSourceRoots, path); } @Nonnull @Deprecated public List getScriptSourceRoots() { - return scriptSourceRoots; + return Collections.unmodifiableList(scriptSourceRoots); } @Nonnull @@ -1727,13 +1814,13 @@ public Reporting getReporting() { @Deprecated public void setReportArtifacts(@Nonnull Set reportArtifacts) { - this.reportArtifacts = reportArtifacts; + this.reportArtifacts = copyWithSameOrder(reportArtifacts); reportArtifactMap = null; } @Deprecated public Set getReportArtifacts() { - return reportArtifacts; + return Collections.unmodifiableSet(reportArtifacts); } @Nonnull @@ -1742,12 +1829,12 @@ public Map getReportArtifactMap() { if (reportArtifactMap == null) { reportArtifactMap = ArtifactUtils.artifactMapByVersionlessId(getReportArtifacts()); } - return reportArtifactMap; + return Collections.unmodifiableMap(reportArtifactMap); } @Deprecated public void setExtensionArtifacts(@Nonnull Set extensionArtifacts) { - this.extensionArtifacts = extensionArtifacts; + this.extensionArtifacts = copyWithSameOrder(extensionArtifacts); extensionArtifactMap = null; } @@ -1764,7 +1851,7 @@ public Map getExtensionArtifactMap() { if (extensionArtifactMap == null) { extensionArtifactMap = ArtifactUtils.artifactMapByVersionlessId(getExtensionArtifacts()); } - return extensionArtifactMap; + return Collections.unmodifiableMap(extensionArtifactMap); } @Deprecated diff --git a/impl/maven-core/src/test/java/org/apache/maven/project/MavenProjectTest.java b/impl/maven-core/src/test/java/org/apache/maven/project/MavenProjectTest.java index 402fd3014fcb..639b943415b5 100644 --- a/impl/maven-core/src/test/java/org/apache/maven/project/MavenProjectTest.java +++ b/impl/maven-core/src/test/java/org/apache/maven/project/MavenProjectTest.java @@ -32,7 +32,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertTrue; class MavenProjectTest extends AbstractMavenProjectTestCase { @@ -173,10 +172,8 @@ void testCloneWithActiveProfile() throws Exception { assertEquals(1, activeProfilesClone.size(), "Expecting 1 active profile"); - assertNotSame( - activeProfilesOrig, - activeProfilesClone, - "The list of active profiles should have been cloned too but is same"); + // Note that the lists may be the same instance when unmodifiable. + assertEquals(activeProfilesOrig, activeProfilesClone); } @Test