Skip to content

[MNG-8696] PathModularizationCache needs to set cache #2219

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

[MNG-8696] PathModularizationCache needs to set cache #2219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
22005d6
PathModularizationCache needs to be public because it's an argument t…
elharo Apr 2, 2025
6d5b0bc
Hide PathModularizationCache from public API
elharo Apr 19, 2025
55c6669
spotless
elharo Apr 19, 2025
3ec4731
remove public constructor
elharo Apr 19, 2025
4150c04
back to public constructor
elharo Apr 19, 2025
fd4dbc4
remove public
elharo Apr 19, 2025
e3ac389
remove TODO
elharo Apr 19, 2025
36050c7
add TODO
elharo Apr 19, 2025
f12ffd4
no null caaches
elharo Apr 19, 2025
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
3 changes: 1 addition & 2 deletions impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultProjectBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,7 @@ public Severity getSeverity() {
public Optional<DependencyResolverResult> getDependencyResolverResult() {
return Optional.ofNullable(res.getDependencyResolutionResult())
.map(r -> new DefaultDependencyResolverResult(
// TODO: this should not be null
null, null, r.getCollectionErrors(), session.getNode(r.getDependencyGraph()), 0));
null, r.getCollectionErrors(), session.getNode(r.getDependencyGraph()), 0));
}
};
} catch (ProjectBuildingException e) {
Expand Down
12 changes: 10 additions & 2 deletions impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,19 +102,27 @@ public class DefaultDependencyResolverResult implements DependencyResolverResult
* to {@link #addDependency(Node, Dependency, Predicate, Path)}.
*
* @param request the corresponding request
* @param cache cache of module information about each dependency
Copy link
Contributor

Choose a reason for hiding this comment

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

In the particular case of this issue, creating the cache at the location where the field is defined is the opposite of the intend, because it makes the PathModularizationCache lifetime strongly coupled with the DefaultDependencyResolverResult lifetime. Such coupling makes PathModularizationCache close to useless. It was done that way in the previous version of this class only as a workaround for the fact that I didn't knew where to store a session-wide cache, but that workaround was intended to be temporary.

More details in a next comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

my suggest would not compile. im sorry.

* @param exceptions the exceptions that occurred while building the dependency graph
* @param root the root node of the dependency graph
* @param count estimated number of dependencies
*/
public DefaultDependencyResolverResult(
DependencyResolverRequest request, List<Exception> exceptions, Node root, int count) {
this(request, new PathModularizationCache(), exceptions, root, count);
}

DefaultDependencyResolverResult(
DependencyResolverRequest request,
PathModularizationCache cache,
List<Exception> exceptions,
Node root,
int count) {
this.request = request;
this.cache = cache;
if (cache == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This if … else block could be replaced by this.cache = Objects.requireNonNull(cache). For consistency, we may do the same for other arguments too except root which is nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's the same. cache is nullable. We need to create a cache if the argument is null, not throw an exception. Null is passed for this argument and we don't want to break those existing usages.

Copy link
Contributor

Choose a reason for hiding this comment

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

The new constructor should replace all the calls to the previous constructor with a null cache. We are sure that the new constructor is invoked outside the package since the old constructor became package-private. So we only need to ensure that the calls inside the package pass a non-null cache, and I think that it is already the case.

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 risky and brittle to allow null caches given the multiple locations that invoke methods on the cache and the lack of test coverage on this class. It is unsafe to allow this field to be null, even if the constructor argument is. Even if it doesn't cause an uncaught NPE now it will in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I agree, which is why my proposal above was to use Objects.requireNonNull.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't want to throw a NullPointerException. We can easily create a cache here..

Copy link
Contributor

Choose a reason for hiding this comment

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

The rational was that a cache created here is of limited use. The intend was to have a session-wide cache, which is why this method was requesting the cache in argument. The creation of a cache in the constructor was only a temporary workaround for the fact that we have not investigated where a session-wide cache should be stored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whatever the intent was, this field is dereferenced without null checks in multiple places. That's dangerous.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I agree that it is dereferenced without null checks, which is why the proposal above was to use Objects.requireNonNull in the constructor. Then, since the constructor become package-private as a result of the change in this pull request, we can easily verify that all invocations of this constructor pass a non-null cache. But anyway, this is not very important. We can leave it as is for now and revisit after we determined where the cache should be stored.

Copy link
Contributor

@Pankraz76 Pankraz76 May 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
if (cache == null) {
if (cache != null) {

take happy path.

this.cache = new PathModularizationCache();
Copy link
Contributor

Choose a reason for hiding this comment

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

could init on declaration to give context for default trading coupling for cohesion. This case is then implied default making use for only dedicated second if.

} else {
this.cache = cache;
}
this.exceptions = exceptions;
this.root = root;
nodes = new ArrayList<>(count);
Expand Down