Skip to content

[MNG-8696] Hide the cache from DefaultDependencyResolverResult constructor #2347

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

Merged
merged 3 commits into from
Sep 13, 2025
Merged

[MNG-8696] Hide the cache from DefaultDependencyResolverResult constructor #2347

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
811a237
Make package-private the DefaultDependencyResolverResult constructor …
desruisseaux May 18, 2025
f878a62
Spelling and formatting fixes from comments on the pull request.
desruisseaux May 18, 2025
8e773d5
Changes based on comments in the pull request:
desruisseaux May 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
46 changes: 37 additions & 9 deletions impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,21 @@
@Named
@Singleton
public class DefaultDependencyResolver implements DependencyResolver {
/**
* Cache of information about the modules contained in a path element.
* This cache is created when first needed. It may be never created.
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
* This cache is created when first needed. It may be never created.

imho no, as always have return call, on default.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

imho no, as always have return call, on default.

Right. Checking again, a see no code path where the cache would not be created. I will remove the lazy instantiation aspect.

*
* <p><b>TODO:</b> This field should not be in this class, because the cache should be global to the session.
* This field exists here only temporarily, until we clarified where to store session-wide caches.</p>
*
* @see moduleCache()
*/
private PathModularizationCache moduleCache;
Copy link
Contributor

Choose a reason for hiding this comment

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

except of static this is the same outcome.

Suggested change
private PathModularizationCache moduleCache;
private final static PathModularizationCache MODULE_CACHE = new PathModularizationCache();


/**
* Creates an initially empty resolver.
*/
public DefaultDependencyResolver() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

who need this? will create very fragile object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This constructor was already there before, only implicit (when a Java file does not declare any constructor at all). This addition only makes it explicit.

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.

yes right, thanks. But is in use? Encounter compile issues, therefore created and sure about usage?
Might not needed before. Want to make sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

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 think that it is instantiated via reflection, as the class is annotated with @Named and @Singleton from the org.apache.maven.api.di package.

Copy link
Contributor

Choose a reason for hiding this comment

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

test

delete, rebuild, pass.

It was done there before and it is without we having to make up whats already there.

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'm not sure which change is requested here. Is it to remove the constructor? It was added only for making explicit what was implicit before. So this is mostly for documentation purpose.

Demonstration

Save the following file somewhere:

/**
 * A dummy comment.
 */
public class Test {
    /**
     * Another dummy comment.
     */
    public Test() {
    }
}

Compile as below with a recent Java version (tested with Java 24). The -Xdoclint option is for having warnings about the documentation. The -g:none option is for omitting debugging information, because the line numbers will change in this demo.

javac -g:none -Xdoclint:all Test.java

Compilation succeed with no warning. Save the output file:

mv Test.class Test.bak

Remove the constructor (including its comment) and recompile with the same command. Note that we now get the following warning:

Test.java:4: warning: use of default constructor, which does not provide a comment
public class Test {
       ^
1 warning

Compare the output files:

diff Test.bak Test.class

They are identical. Making the implicit constructor explicit changes nothing to the compiled code, but is nevertheless recommended according above warning.

Copy link
Contributor

@Pankraz76 Pankraz76 May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It was added only for making explicit what was implicit before

yes we on the same page. Why we make this? Only if we have 1 constructor that is NON default, only then this change is required, to provide implicit default again. This is not true yet, therefore its "boilerplate" as its there, with or without.

This is a change we can see on compiler lvl, or if rare case we use hibernate having the need for non args constructor like in. https://stackoverflow.com/questions/2935826/why-does-hibernate-require-no-argument-then we see this issue on test lvl.

Please provide stacktrace or reproducer proving this code in mandatory by failing build. Assuming removing the constructor will turn out the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes provided sorry. Then lets merge and local build should fail when removed, right?
Then we simply know riving removal PR. it must fail build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then lets merge and local build should fail when removed, right?

We could make the build fail when the explicit constructor is absent by adding the -Werror compiler options. But Maven is not ready for that yet. For now, this change is just resolving one documentation warning among the many the Maven would have if the -Xdoclint:all option was provided.


@Nonnull
@Override
Expand Down Expand Up @@ -126,7 +141,11 @@ public DependencyResolverResult collect(@Nonnull DependencyResolverRequest reque
final CollectResult result =
session.getRepositorySystem().collectDependencies(systemSession, collectRequest);
return new DefaultDependencyResolverResult(
null, null, result.getExceptions(), session.getNode(result.getRoot(), request.getVerbose()), 0);
null,
moduleCache(),
Copy link
Contributor

Choose a reason for hiding this comment

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

as its dry, and static, inline. Ide shoud give warning on method or constructor that value is always same.

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'm not sure to understand. Inline the call to moduleCache()? This is uneasy, as that method does lazy instantiation.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry wrong assumption. This is called every time as this the only return therefore default.

result.getExceptions(),
session.getNode(result.getRoot(), request.getVerbose()),
0);
} catch (DependencyCollectionException e) {
throw new DependencyResolverException("Unable to collect dependencies", e);
}
Expand Down Expand Up @@ -191,18 +210,14 @@ public DependencyResolverResult resolve(DependencyResolverRequest request)
.map(Artifact::toCoordinates)
.collect(Collectors.toList());
Predicate<PathType> filter = request.getPathTypeFilter();
DefaultDependencyResolverResult resolverResult = new DefaultDependencyResolverResult(
null, moduleCache(), collectorResult.getExceptions(), collectorResult.getRoot(), nodes.size());
if (request.getRequestType() == DependencyResolverRequest.RequestType.FLATTEN) {
DefaultDependencyResolverResult flattenResult = new DefaultDependencyResolverResult(
Copy link
Contributor

Choose a reason for hiding this comment

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

this scope was right. this field concern lives only here. Even could dedicate whole block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change was for removing code duplication. The exact same code was inside the two branches of the if … else statement, with only different variable names.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes see.

Common part can be extracted from 'if'

its same obtain the warning:

image 
    @Override
    public DependencyResolverResult resolve(DependencyResolverRequest request)
            throws DependencyResolverException, ArtifactResolverException {
        InternalSession session =
                InternalSession.from(nonNull(request, "request").getSession());
        try {
            DependencyResolverResult collectorResult = collect(request);
            DependencyResolverResult result = collectorResult;
            List<RemoteRepository> repositories = request.getRepositories() != null
                    ? request.getRepositories()
                    : request.getProject().isPresent()
                    ? session.getService(ProjectManager.class)
                    .getRemoteProjectRepositories(
                            request.getProject().get())
                    : session.getRemoteRepositories();
            if (request.getRequestType()!= DependencyResolverRequest.RequestType.COLLECT) {
                List<Node> nodes = flatten(session, collectorResult.getRoot(), request.getPathScope());
                DefaultDependencyResolverResult resolverResult = new DefaultDependencyResolverResult(
                        null, moduleCache(), collectorResult.getExceptions(), collectorResult.getRoot(), nodes.size());
                if (request.getRequestType() == DependencyResolverRequest.RequestType.FLATTEN) {
                    for (Node node : nodes) {
                        resolverResult.addNode(node);
                    }
                } else {
                    for (Node node : nodes) {
                        Path path = (node.getArtifact() != null)
                                ? session.getService(ArtifactResolver.class).resolve(session, nodes.stream()
                                        .map(Node::getDependency)
                                        .filter(Objects::nonNull)
                                        .map(Artifact::toCoordinates)
                                        .collect(Collectors.toList()), repositories)
                                .getResult(node.getArtifact().toCoordinates())
                                .getPath()
                                : null;
                        try {
                            resolverResult.addDependency(node, node.getDependency(), request.getPathTypeFilter(), path);
                        } catch (IOException e) {
                            throw cannotReadModuleInfo(path, e);
                        }
                    }
                }
                result = resolverResult;
            }
            return result;
        } finally {
            RequestTraceHelper.exit(RequestTraceHelper.enter(session, request));
        }
    }

is the finally an issue inlining exit ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is the finally an issue inlining exit ?

Judging by the method names, it would probably not work. RequestTraceHelper.enter probably needs to be invoked before to start the execution of the code inside the try block. Inlining would cause enter to be invoked too late.

Copy link
Contributor

Choose a reason for hiding this comment

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

thx.

null, null, collectorResult.getExceptions(), collectorResult.getRoot(), nodes.size());
for (Node node : nodes) {
flattenResult.addNode(node);
resolverResult.addNode(node);
}
result = flattenResult;
result = resolverResult;
} else {
PathModularizationCache cache =
new PathModularizationCache(); // TODO: should be project-wide cache.
DefaultDependencyResolverResult resolverResult = new DefaultDependencyResolverResult(
null, cache, collectorResult.getExceptions(), collectorResult.getRoot(), nodes.size());
ArtifactResolverResult artifactResolverResult =
session.getService(ArtifactResolver.class).resolve(session, coordinates, repositories);
for (Node node : nodes) {
Expand All @@ -226,6 +241,19 @@ public DependencyResolverResult resolve(DependencyResolverRequest request)
}
}

/**
* {@return the cache of information about the modules contained in a path element}.
*
* <p><b>TODO:</b> This method should not be in this class, because the cache should be global to the session.
* This method exists here only temporarily, until we clarified where to store session-wide caches.</p>
*/
private PathModularizationCache moduleCache() {
if (moduleCache == null) {
moduleCache = new PathModularizationCache();
}
return moduleCache;
}

private static DependencyResolverException cannotReadModuleInfo(final Path path, final IOException cause) {
return new DependencyResolverException("Cannot read module information of " + path, cause);
}
Expand Down
24 changes: 22 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 @@ -27,6 +27,7 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.function.Predicate;
Expand Down Expand Up @@ -56,6 +57,7 @@ public class DefaultDependencyResolverResult implements DependencyResolverResult
* The corresponding request.
*/
private final DependencyResolverRequest request;

/**
* The exceptions that occurred while building the dependency graph.
*/
Expand Down Expand Up @@ -97,6 +99,24 @@ public class DefaultDependencyResolverResult implements DependencyResolverResult
*/
private final PathModularizationCache cache;

/**
* Creates an initially empty result with a temporary cache.
* Callers should add path elements by calls to {@link #addDependency(Node, Dependency, Predicate, Path)}.
*
* <p><b>WARNING: this constructor may be removed in a future Maven release.</b>
* The reason is because {@code DefaultDependencyResolverResult} needs a cache, which should
* preferably by session-wide. But we have not yet clarified how such caches should be managed.</p>
*
* @param request the corresponding request
* @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);
Copy link
Contributor

Choose a reason for hiding this comment

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

yes this gives dedication and option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this(request, new PathModularizationCache(), exceptions, root, count);
this(request, moduleCache(), exceptions, root, count);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The moduleCache() method is defined in another class. We cannot access that method from here.

}

/**
* Creates an initially empty result. Callers should add path elements by calls
* to {@link #addDependency(Node, Dependency, Predicate, Path)}.
Expand All @@ -107,14 +127,14 @@ public class DefaultDependencyResolverResult implements DependencyResolverResult
* @param root the root node of the dependency graph
* @param count estimated number of dependencies
*/
public DefaultDependencyResolverResult(
DefaultDependencyResolverResult(
DependencyResolverRequest request,
PathModularizationCache cache,
List<Exception> exceptions,
Node root,
int count) {
this.request = request;
this.cache = cache;
this.cache = Objects.requireNonNull(cache);
Copy link
Contributor

Choose a reason for hiding this comment

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

or here

Suggested change
this.cache = Objects.requireNonNull(cache);
this.cache = Objects.requireNonNull(moduleCache());

one of the suggestions is right. as moduleCache() is SSOT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The moduleCache() method is defined in another class. We cannot access that method from here.

However, this is indeed the kind of code that we would have after we clarified how to manage session-wide caches. The moduleCache() method would be in the class managing those caches. We are just not there yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fwiw, session wide informations can be stored in Session.getData().

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