Pankraz76 commented on code in PR #2347:
URL: https://github.com/apache/maven/pull/2347#discussion_r2094620239
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -68,6 +68,22 @@
@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.
+ *
+ * <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 clarified where to store
session-wide caches.</p>
+ *
+ * @see #moduleCache()
+ */
+ private PathModularizationCache moduleCache;
Review Comment:
except of static this is the same outcome.
```suggestion
private final static PathModularizationCache MODULE_CACHE = new
PathModularizationCache();
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -226,6 +242,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 clarified where to
store session-wide caches.</p>
+ */
+ private PathModularizationCache moduleCache() {
+ if (moduleCache == null) {
+ moduleCache = new PathModularizationCache();
+ }
+ return moduleCache;
+ }
+
Review Comment:
```suggestion
```
making field final is the same result. static would change to all classes
change same cache. If no collision risk then might be possible.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -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(),
Review Comment:
sorry wrong assumption. This is called every time as this the only return
therefore default.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -68,6 +68,22 @@
@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.
Review Comment:
```suggestion
```
imho no, return case will always call.
<img width="1233" alt="image"
src="https://github.com/user-attachments/assets/bfd2b62e-0a7a-4f86-b40a-25c1ca5489bf";
/>
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -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(
Review Comment:
yes see.
Common part can be extracted from 'if'
its same obtain the warning:
<img width="1115" alt="image"
src="https://github.com/user-attachments/assets/85af7265-c672-40d2-856e-bda91dcc4dc1";
/>
```java
@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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
