[MNG-8696] PathModularizationCache needs to set cache

[elharo]

…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.

[desruisseaux]

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.

[elharo]

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.

[gnodet]

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.

[elharo]

PTAL. Took @desruisseaux 's suggestion to make the constructor package-private and add another constructor without the PathModularizationCache argument, which delegates to the package-private constructor with a null argument.

[desruisseaux]

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.

[elharo]

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.

[desruisseaux]

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.

[elharo]

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.

[desruisseaux]

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

[elharo]

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

[desruisseaux]

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.

[elharo]

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

[desruisseaux]

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.

[Pankraz76]

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.

[Pankraz76]

might consider init on declaration to give cohesion; default - making first things first.

[Pankraz76]

Suggested change

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

take happy path.

[desruisseaux]

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.

[Pankraz76]

my suggest would not compile. im sorry.

[desruisseaux]

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.

[desruisseaux]

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.

[jira-importer]

Resolve #9351