You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "elharo (via GitHub)" <gi...@apache.org> on 2025/04/02 12:37:26 UTC
[PR] PathModularizationCache needs to be public because it's an argument t… [maven]
elharo opened a new pull request, #2219:
URL: https://github.com/apache/maven/pull/2219
…o a public constructor in DefaultDependencyResolverResult. It only hasn't failed yet because we're only passing null for this argument but there's a TODO to change that.
Alternatively, we could remove PathModularizationCache from the DefaultDependencyResolverResult.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2094581944
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
Review Comment:
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.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2094518296
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
Review Comment:
```suggestion
if (cache != null) {
```
take happy path.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2052469648
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
Review Comment:
No, I don't want to throw a NullPointerException. We can easily create a cache here..
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2051757908
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
Review Comment:
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.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PathModularizationCache needs to be public because it's an argument t… [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #2219:
URL: https://github.com/apache/maven/pull/2219#issuecomment-2774602072
The `DefaultDependencyResolverResult` does not really respect the `@Immutable` contract which is on `Result<REQ>`.
It may be better refactored in a different class that would be package protected, contains the `addDependency` and `addOutputDirectory`, while the `DefaultDependencyResolverResult` would be kept immutable.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2094518427
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
+ this.cache = new PathModularizationCache();
Review Comment:
```suggestion
this.cache = cache;
```
might consider init on declaration to give cohesion; default - making first things first.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
+ this.cache = new PathModularizationCache();
+ } else {
+ this.cache = cache;
+ }
Review Comment:
```suggestion
}
```
one concern.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2051775037
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
Review Comment:
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.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2094518198
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
+ this.cache = new PathModularizationCache();
Review Comment:
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.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2052458318
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
Review Comment:
Yes I agree, which is why my proposal above was to use `Objects.requireNonNull`.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2060125279
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
Review Comment:
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.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2094518296
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
Review Comment:
```suggestion
if (cache != null) {
```
give happy path
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PathModularizationCache needs to be public because it's an argument t… [maven]
Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on PR #2219:
URL: https://github.com/apache/maven/pull/2219#issuecomment-2772558151
Alternatively, we can make the constructor package-private and add another constructor without the `PathModularizationCache` argument, which would delegate to the package-private constructor with a `null` argument.
A few months ago, a started a branch where I tried to reduce the visibility of some Maven internal classes. It required moving some classes to a common package (in this case, we would need to move `DefaultProjectBuilder`). I did not explored further because of conflict with other changes, but generally I would prefer going in the direction of removing some `public` items instead of adding more.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2065789719
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
Review Comment:
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.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2052404632
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
Review Comment:
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.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2094586682
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
Review Comment:
my suggest would not compile. im sorry.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PathModularizationCache needs to be public because it's an argument t… [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #2219:
URL: https://github.com/apache/maven/pull/2219#issuecomment-2772602840
Yes, if we can move more classes into the same packages we can reduce visibility of a lot of things. Excessive proliferation of subpackages is a major antipattern in Java that leads to excessive visibility. It's probably second only to IDEs that mark classes and methods public by default.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2064409578
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
Review Comment:
Whatever the intent was, this field is dereferenced without null checks in multiple places. That's dangerous.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2094518296
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
Review Comment:
```suggestion
if (cache != null) {
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
+ this.cache = new PathModularizationCache();
+ } else {
+ this.cache = cache;
+ }
Review Comment:
```suggestion
}
```
one concern.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
+ this.cache = new PathModularizationCache();
Review Comment:
```suggestion
this.cache = cache;
```
might consider init on declaration to give cohesion; default - making first things first.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
Review Comment:
`private final PathModularizationCache cache = new PathModularizationCache();` L:98
- https://www.reddit.com/r/learnjava/comments/ozgtxm/is_initializing_variables_at_their_declaration/
- https://softwareengineering.stackexchange.com/questions/379039/what-is-the-best-practice-for-initializing-variables-outside-or-inside-of-the-c
-
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on PR #2219:
URL: https://github.com/apache/maven/pull/2219#issuecomment-2889112320
I think that the `DefaultDependencyResolverResult` constructor is creating some confusion. I created a new pull request that I propose as an alternative to this one: #2347. For seeing the purpose, there is two design constraints to consider:
* In order to be useful, `PathModularizationCache` needs to live longer than `DefaultDependencyResolverResult`. Any change that makes the lifetime of those two objects strongly coupled make the cache close to useless.
* At the time I wrote these classes, I didn't know how session-wide caches should be managed in Maven (I still haven't investigated). Therefore, `PathModularizationCache` was instantiated together with `DefaultDependencyResolverResult` *as a temporary workaround, until we learn more about how session-wide caches are managed in Maven.*
The pull #2347 tries to address the same issue as this pull request, but in a way that makes more apparent that `DefaultDependencyResolverResult` and `PathModularizationCache` lifetimes are distinct in principle, and that the current coupling may be temporary.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #2219:
URL: https://github.com/apache/maven/pull/2219#discussion_r2052285482
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -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
* @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) {
Review Comment:
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.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8696] PathModularizationCache needs to set cache [maven]
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo closed pull request #2219: [MNG-8696] PathModularizationCache needs to set cache
URL: https://github.com/apache/maven/pull/2219
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org