Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename BaseRequest#unmodifiable to copyToImmutable and add doc #2037

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Rename BaseRequest#unmodifiable to copyToImmutable and add doc #2037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/maven-api-core/src/main/java/org/apache/maven/api/services/ArtifactDeployerRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ private static class DefaultArtifactDeployerRequest extends BaseRequest<Session>
int retryFailedDeploymentCount) {
super(session);
this.repository = nonNull(repository, "repository cannot be null");
this.artifacts = unmodifiable(nonNull(artifacts, "artifacts cannot be null"));
this.artifacts = copyToImmutable(nonNull(artifacts, "artifacts cannot be null"));
this.retryFailedDeploymentCount = retryFailedDeploymentCount;
}

Expand Down
2 changes: 1 addition & 1 deletion api/maven-api-core/src/main/java/org/apache/maven/api/services/ArtifactInstallerRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ static class DefaultArtifactInstallerRequest extends BaseRequest<Session> implem

DefaultArtifactInstallerRequest(@Nonnull Session session, @Nonnull Collection<ProducedArtifact> artifacts) {
super(session);
this.artifacts = unmodifiable(nonNull(artifacts, "artifacts cannot be null"));
this.artifacts = copyToImmutable(nonNull(artifacts, "artifacts cannot be null"));
}

@Nonnull
Expand Down
2 changes: 1 addition & 1 deletion api/maven-api-core/src/main/java/org/apache/maven/api/services/ArtifactResolverRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ private static class DefaultArtifactResolverRequest extends BaseRequest<Session>
@Nonnull Collection<? extends ArtifactCoordinates> coordinates,
@Nonnull List<RemoteRepository> repositories) {
super(session);
this.coordinates = unmodifiable(nonNull(coordinates, "coordinates cannot be null"));
this.coordinates = copyToImmutable(nonNull(coordinates, "coordinates cannot be null"));
this.repositories = repositories;
}

Expand Down
18 changes: 12 additions & 6 deletions api/maven-api-core/src/main/java/org/apache/maven/api/services/BaseRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,8 @@
*/
package org.apache.maven.api.services;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;

import org.apache.maven.api.ProtoSession;
import org.apache.maven.api.annotations.Experimental;
Expand Down Expand Up @@ -52,9 +51,16 @@ public static <T> T nonNull(T obj, String message) {
return obj;
}

protected static <T> Collection<T> unmodifiable(Collection<T> obj) {
return obj != null && !obj.isEmpty()
? Collections.unmodifiableCollection(new ArrayList<>(obj))
: Collections.emptyList();
/**
* Creates a defensive immutable copy of a collection or returns an empty immutable list if the input is null or empty.
* This method is useful when creating immutable objects to ensure collection fields cannot be modified by external code.
*
* @param source the source collection to create an immutable copy from
* @param <T> the type of elements in the collection
* @return an unmodifiable view of the collection, or an empty list if the input is null or empty
* @throws NullPointerException if the collection contains null elements
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we do this? In general, lists with null elements are OK, though they might be wrong in a specific case. However if null elements are wrong in the specific case I'd expect the source list to already have checked that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a side effect of using List.copyOf(). The behavior thus surfaces to this helper methods and the javadoc mentions it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, non null elements in list are a burden. Unless there's a specific semantic associated with a null value, I'd rather forbid those. For example if you want to resolve a list of artifacts, passing null values makes absolutely no sense to me.

*/
protected static <T> List<T> copyToImmutable(Collection<T> source) {
return source != null && !source.isEmpty() ? List.copyOf(source) : List.of();
}
}
4 changes: 2 additions & 2 deletions ...maven-api-core/src/main/java/org/apache/maven/api/services/DependencyResolverRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,9 +398,9 @@ static class DefaultDependencyResolverRequest extends BaseRequest<Session>
this.project = project;
this.rootArtifact = rootArtifact;
this.root = root;
this.dependencies = unmodifiable(nonNull(dependencies, "dependencies cannot be null"));
this.dependencies = copyToImmutable(nonNull(dependencies, "dependencies cannot be null"));
this.managedDependencies =
unmodifiable(nonNull(managedDependencies, "managedDependencies cannot be null"));
copyToImmutable(nonNull(managedDependencies, "managedDependencies cannot be null"));
this.verbose = verbose;
this.pathScope = nonNull(pathScope, "pathScope cannot be null");
this.pathTypeFilter = (pathTypeFilter != null) ? pathTypeFilter : (t) -> true;
Expand Down
Loading