JavaPackage#getPackageDependenciesFromSelf returns dependencies from both self AND subpackages

[grimsa]

In JavaPacakge:

• getPackageDependenciesFromSelf collects packages of classes returned by getClassDependenciesFromSelf
• getClassDependenciesFromSelf collects dependencies originating from classes returned by getAllClasses
• getAllClasses collects classes from both the current package AND all subpackages (descendants)

This caught me by surprise - I expected getPackageDependenciesFromSelf to only return the package dependencies originating from classes in the current package (i.e. excluding any subpackages).

At first glance it would seem that just replacing getAllClasses with getClasses in getClassDependenciesFrom/ToSelf would make it match my expectation.

If this is a bug, I'd be happy to open a PR with a fix.

[codecholeric]

Thanks for raising this issue! The behavior actually was implemented like this intentionally 🤔 Because that would have been my expectation of the API 😂 I admit that the javadoc does not specify this 😞
The reason for the current behavior is that when considering architecture we usually consider hierarchies, right? Packages model components where subpackages model subcomponents. So if I take my component and ask "what dependencies does it have", then it's the sum of all dependencies of the component plus its subcomponents. At least that was my thinking.
What is your use case for only considering the "flat" dependencies?
In any case, maybe we need both APIs? 🤔 Because dependencies of the flat content + subpackages is definitely a common use case for me 🤷‍♂️ Take for example you want to draw a diagram, then you would also take the package + all subpackages, no? 🤔

[grimsa]

I noticed this when trying to implement a "a package should not depend on its subpackages" rule (reasoning: https://javadevguy.wordpress.com/2017/12/18/happy-packaging/).

What I was trying to do was something like this:

    @ArchTest
    private static ArchRule packageRule = ArchRuleDefinition.all(packages)
            .that(DescribedPredicate.describe(
                    "our company packages",
                    (JavaPackage javaPackage) -> javaPackage.getName().startsWith("com.company")
            ))
            .should(new ArchCondition<>("not depend on their subpackages") {
                @Override
                public void check(JavaPackage javaPackage, ConditionEvents events) {
                    if (javaPackage.getClasses().isEmpty()) {
                        // if package has no classes, then it is not relevant
                        return;
                    }
                    Set<JavaPackage> allSubpackages = javaPackage.getAllSubpackages();
                    String subpackageDependencyNames = javaPackage.getPackageDependenciesFromSelf().stream()
                            .filter(allSubpackages::contains)
                            .map(JavaPackage::getName)
                            .distinct()
                            .collect(Collectors.joining(", "));
                    if (subpackageDependencyNames.isBlank()) {
                        return;
                    }
                    events.add(
                            SimpleConditionEvent.violated(
                                    javaPackage,
                                    "Package %s depends on subpackages: [ %s ]".formatted(javaPackage.getName(), subpackageDependencyNames)
                            )
                    );
                }
            });

This resulted in a number of false positives, where a package was reported to depend on its subpackages even if there was no direct dependency between the package and subpackage.

It is likely that it's useful to have access to both all dependencies and just direct dependencies depending on the context - similar to getSubpackages and getAllSubpackages - though finding easy to understand names for it might be challenging :)

At the same time, this could be worked around by using getClasses on a package, then calling getDirectDependenciesFromSelf() on each class, and then collecting target packages from those.

[grimsa]

I ended up implementing the rule I wanted in a slightly different way (shared here: #921), which did require the methods of JavaPackage discussed here, so my itch can be considered to be scratched :)

In any case, I think I could do something here, and the options would probably be:

1. Update the Javadoc on affected methods to clarify current behavior, OR
2. Introduce new methods and/or possibly rename the old ones (I think doing this in 1.0.0-rc.N would be acceptable) so that ArchUnit has both capabilities (producing package dependencies from both just the current package, as well as from the subtree of packages from the current one).

[codecholeric]

I thought about it, and I think it makes sense to offer both these alternatives in the API. The only problem for me is, to be consistent with other naming it would have to be something like getPackageDependenciesFromSelf() and getAllPackageDependenciesFromSelf(). But, a) the name does not convey the difference very well (in particular the "non-all" version) and b) even worse, it would be breaking the existing API in a very sneaky way by compiling just fine but breaking the semantics 🤔

So I thought more about it and ended up with these two naming ideas:

• getPackageDependenciesFromThisPackage()
• getPackageDependenciesFromThisPackageAndSubpackages()

It would break the naming schema a little, but maybe that's even good. Because in the end it is also a slightly different case. It is an aggregate and in no other place to we have this ambiguity, that it could refer to the flat package or the package plus subpackages.

And I also think it would be way better to break the API in a non-compiling way with 1.0.0, than to sneakily modify an existing method in an incompatible way.

[grimsa]

Opened a PR with the proposed fix.

Though it seems the root problem here might be that the concept of JavaPackage is a little muddled - in some cases it refers strictly to a single package, while in other cases it covers the whole subtree rooted at this package.

Examples of methods considering just the current package: containsClass
Examples of methods considering the whole package subtree: containsPackage, accept

Additionally, I think the term "subpackage" in Java means "direct child", as seen in Java Language Specification:

For example, in the Java SE Platform API:

• The package java has subpackages awt, applet, io, lang, net, and util, but no compilation units.

^ Note that java.lang.annotation, java.lang.reflect, etc. are not mentioned - I interpret that as them not being considered to be "subpackages of java", only "subpackages of java.lang".

Maybe a proper solution would after all be something along the lines of:

• Clarifying the JavaPackage concept to represent strictly a single package (where subpackages would mean direct children only)
• Add other means to consider package trees, e.g. by introducing a new JavaPackageTree concept which would hold the logic for visiting the tree. Then instead of package.getPackageDependenciesFromThisPackageAndSubpackages() it could be explicit: JavaPackageTree.rootedAt(package).getPackageDependenciesFromSelf() (or something along those lines)

This would be a major change though.

[codecholeric]

Yes, I agree with your observations 🤔 Also, that it might be more precise to have two objects. The only think I'm wondering about is, if this then really makes the API that users find clear and easy to use 🤔
Given e.g. as a user I want to define three components by their root package a, b and c and assert dependencies a.. -> b.. and a.. -> c... Wouldn't it be a more natural assumption, that I can do something with the respective package of a, b and c to achieve this? 🤔 Or would I really get the idea that I need to construct a JavaPackageTree from those packages first to make any statements about contained dependencies 🤔 I don't see the clear path how I would end up at the tree as user when I'm coming from this use case. Maybe we could make arriving at the tree easier by adding JavaPackage.asTree(), but still 🤔
But I also see how even subpackages is ambiguous again 🙈