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