ArchCondition to ensure no upper package access incl. solution

[qoomon]

Hie there,
would be great to have the following ArchCondition available out of the box like
noClasses().should().accessClassesThat().resideInAnUpperPackage().check(classes);
however I was not able to create a PR for this, so please feel free to just assimilate my solution below :-D

import static java.lang.String.format;
import static java.lang.System.lineSeparator;
import static java.util.stream.Collectors.toList;

import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses;

import java.util.List;

import org.junit.jupiter.api.Test;

import com.tngtech.archunit.core.domain.Dependency;
import com.tngtech.archunit.core.domain.JavaClass;
import com.tngtech.archunit.core.domain.JavaClasses;
import com.tngtech.archunit.core.importer.ClassFileImporter;
import com.tngtech.archunit.core.importer.ImportOption;
import com.tngtech.archunit.lang.ArchCondition;
import com.tngtech.archunit.lang.ConditionEvents;
import com.tngtech.archunit.lang.SimpleConditionEvent;

public class ArchitectureTest {

    private final JavaClasses classes = new ClassFileImporter()
            .withImportOption(ImportOption.Predefined.DONT_INCLUDE_TESTS)
            .importPackages("foo.project.root.package");

   @Test
    public void ensureNoClassesAccessClassesThatResideInAnUpperPackage() {
        noClasses().should(accessClassesThatResideInAnUpperPackage()).check(classes);
        // requested solution
        // noClasses().should().accessClassesThat().resideInAnUpperPackage().check(classes);
    }

    private ArchCondition<JavaClass> accessClassesThatResideInAnUpperPackage(){
        return new ArchCondition<>("access upper packages") {
            @Override
            public void check(final JavaClass clazz, final ConditionEvents events) {
                final List<Dependency> dependenciesThatResideInAnUpperPackage = clazz.getDirectDependenciesFromSelf().stream()
                        .filter(it -> !it.getTargetClass().getPackageName().isEmpty())
                        .filter(it -> !clazz.getPackageName().equals(it.getTargetClass().getPackageName()))
                        .filter(it -> clazz.getPackageName().startsWith(it.getTargetClass().getPackageName()))
                        .collect(toList());

                final boolean satisfied = !dependenciesThatResideInAnUpperPackage.isEmpty();

                final StringBuilder messageBuilder = new StringBuilder(format("%s to upper package found within class %s",
                        satisfied ? "Access" : "No access",
                        clazz.getName()));
                for (Dependency dependency : dependenciesThatResideInAnUpperPackage) {
                    messageBuilder.append(lineSeparator()).append(dependency.getDescription());
                }

                events.add(new SimpleConditionEvent(clazz, satisfied, messageBuilder.toString()));
            }
        };
    }
}

[codecholeric]

This is an interesting idea, thanks for sharing 😃 I'm not sure if it should go into the core API though (since it feels more specific than the rest), or if it should be included as "library" (like the GeneralCodingRules). I'll think about it though and get back to you!!

[qoomon]

I think it's always kind of code smell to access parent packages as well as having cycles between packages. However looking forward to see it somewhere within your lib. Btw your lib is a great solution to ensure architecture.

[codecholeric]

I don't know if it is always a code smell, for example what if my convention is to always have

somebusiness
  |-- InterfacePublicAPI
  |-- internal
        |-- PublicAPIImpl

Then PublicAPIImpl needs to depend on a parent package to implement the public API InterfacePublicAPI. And I wouldn't call that convention code smell, in the end it's often just a decision. So in my opinion it depends 😉 (which is why in many parts the only way is to provide a flexible tool box that can be taylored to projects).

Or did you actually mean real "access"? For real access this might be a different chapter, it's harder for me to imagine a valid use case there 😉. But then you need to check for javaClass.getAccessesFromSelf() instead of javaClass.getDirectDependenciesFromSelf().

I think your condition from above collects a little too many classes though, because for com.Foo and com.Bar, it is true that Foo.getPackageName().startsWith(Bar.getPackageName()) (equality must be excluded).

[qoomon]

Both of you examples shouldn't lead to a violation, you are totally right. It is a bug in my provided solution. I will adjust that next week.

…

On Sat, 2 Mar 2019, 11:36 Peter Gafert, ***@***.***> wrote: I don't know if it is always a code smell, for example what if my convention is to always have somebusiness |-- InterfacePublicAPI |-- internal |-- PublicAPIImpl Then PublicAPIImpl needs to depend on a parent package to implement the public API InterfacePublicAPI. And I wouldn't call that convention code smell, in the end it's often just a decision. So in my opinion it depends 😉 (which is why in many parts the only way is to provide a flexible tool box that can be taylored to projects). Or did you actually mean real "access"? For real access this might be a different chapter, it's harder for me to imagine a valid use case there 😉. But then you need to check for javaClass.getAccessesFromSelf() instead of javaClass.getDirectDependenciesFromSelf(). I think your condition from above collects a little too many classes though, because for com.Foo and com.Bar, it is true that Foo.getPackageName().startsWith(Bar.getPackageName()) (equality must be excluded). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#151 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADx6AjPOrAyMauZwWn42YRphwtPaWzQEks5vSlQ7gaJpZM4bYqUN> .

[qoomon]

In other words, what I want is to throw a validation only if a child packages accesses any of its parent packages. On Sat, 2 Mar 2019, 16:05 Bengt Brodersen, <bengt.brodersen@gmail.com> wrote:

…

Both of you examples shouldn't lead to a violation, you are totally right. It is a big in my provided solution. I will adjust that next week. On Sat, 2 Mar 2019, 11:36 Peter Gafert, ***@***.***> wrote: > I don't know if it is always a code smell, for example what if my > convention is to always have > > somebusiness > > |-- InterfacePublicAPI > > |-- internal > > |-- PublicAPIImpl > > > Then PublicAPIImpl needs to depend on a parent package to implement the > public API InterfacePublicAPI. And I wouldn't call that convention code > smell, in the end it's often just a decision. So in my opinion it depends > 😉 (which is why in many parts the only way is to provide a flexible > tool box that can be taylored to projects). > > Or did you actually mean real "access"? For real access this might be a > different chapter, it's harder for me to imagine a valid use case there > 😉. But then you need to check for javaClass.getAccessesFromSelf() > instead of javaClass.getDirectDependenciesFromSelf(). > > I think your condition from above collects a little too many classes > though, because for com.Foo and com.Bar, it is true that > Foo.getPackageName().startsWith(Bar.getPackageName()) (equality must be > excluded). > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#151 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ADx6AjPOrAyMauZwWn42YRphwtPaWzQEks5vSlQ7gaJpZM4bYqUN> > . >

[qoomon]

I've updated my initial solution.

[codecholeric]

I think the line with

dependenciesThatResideInAnUpperPackage = clazz.getDirectDependenciesFromSelf()...

has to be changed to getAccessesFromSelf(), but I can fix that, when I copy it.
I think it could go into a general DependencyRules class within the library package, next to GeneralCodingRules...

[qoomon]

any progress on this?

[codecholeric]

I've refactored the code to be compatible to Java 7 (sigh) and added an example so we can talk about it. The point I'm unsure about is that you can't extend abstract classes from a super package now either, since there is always a constructor call involved.
Just check out the example and tell me what you think 😉 -> 5f7fadc

[qoomon]

Hi, actually that is a structure I want to avoid by this rule. From my perspective it is bad practice to have a structure where and implementation is part of the api package.

[kkocel]

@codecholeric I wonder if there is any blocker with this issue?
According to your doubt - I would say that this is fine - you should not be able to implement classes that reside in the internal package. This was also pointed by @qoomon

[codecholeric]

@kkocel Sorry, there is no real blocker, I just lost track of it 😞
My doubt was not about being able to implement classes in the internal package, but to extend classes in the root package. And I'm still not completely convinced it always is a code smell independently of the circumstances 😉

Consider for example

mycomponent
  |-- interface MyPublicInterface
  |-- class MyPublicInterfaceFactory
  |-- abstract class MyAbstractPublicInterfaceImpl
  |-- subComponentOne
  |       |-- class MySubInterfaceOneExtendingMyAbstractPublicInterfaceImpl
  |-- subComponentTwo
          |-- class MySubInterfaceTwoExtendingMyAbstractPublicInterfaceImpl

So MyPublicInterfaceFactory would only return MyPublicInterface. But the implementations of MyPublicInterface only differ in a small subset of a specific part. So I implement MyAbstractPublicInterfaceImpl containing the basic structure and the subcomponents both offer their extension of MyAbstractPublicInterfaceImpl.
This would then be forbidden by the rule.

However

mycomponent
  |-- interface MyPublicInterface
  |-- class MyPublicInterfaceFactory
  |-- common
  |        |-- abstract class MyAbstractPublicInterfaceImpl
  |-- subComponentOne
  |       |-- class MySubInterfaceOneExtendingMyAbstractPublicInterfaceImpl
  |-- subComponentTwo
          |-- class MySubInterfaceTwoExtendingMyAbstractPublicInterfaceImpl

would be allowed by the rule. So per se I don't see why case two is better than case one, maybe it is because it clearer identifies what is API and what is internal, but that also depends on my conventions. And what about a structure

mycomponent
  |-- api
  |    |-- interface MyPublicInterface
  |    |-- class MyPublicInterfaceFactory
  |-- abstract class MyAbstractPublicInterfaceImpl
  |-- subComponentOne
  |       |-- class MySubInterfaceOneExtendingMyAbstractPublicInterfaceImpl
  |-- subComponentTwo
          |-- class MySubInterfaceTwoExtendingMyAbstractPublicInterfaceImpl

I.e. my convention says I may only ever access the api package of any component. And I put common stuff in the root package which is inaccessible by other components. This would be forbidden by the rule, but why is it bad in general?

So all I'm saying is, that from my point of view it depends on your conventions. It might make sense to have such a rule, but it might also not make sense. I'm just saying an upper package access is not a code smell for me per se in all contexts.

Also consider

mycomponent
  |-- interface MyPublicInterface
  |-- class MyPublicInterfaceFactory
  |-- internal
          |-- abstract class MyAbstractPublicInterfaceImpl
          |-- subComponentOne
          |       |-- class MySubInterfaceOneExtendingMyAbstractPublicInterfaceImpl
          |-- subComponentTwo
                  |-- class MySubInterfaceTwoExtendingMyAbstractPublicInterfaceImpl

That also would be forbidden even though internal is clearly not part of the public API.

Other than that, I still don't see any blocker to offer this as a library function 😉 The whole point of the library part is to offer rules that might be useful to a wider range of users, but they don't need to be useful in each and every project.

Long story short, does this look good to you? (also you @qoomon) -> #352

[qoomon]

Your second example shouldn't be allowed because the implementations access MyPublicInterface from parent package.
The main smell from my perspective is that packages should depend on each others not on it self, however that's the case if you depend on your parent, and by this implicitly on your own. In my opinion that's at least a little quirky :-D

[kkocel]

@codecholeric I agree that it's not the general rule. For my use case, it would be perfect though.
I want to have classes that are considered internal - something like package scope but allowing inner packages as well - this rule suits perfectly.

[codecholeric]

@qoomon But I thought depending on an interface is fine? At least that's how I interpreted your comments #151 (comment) and #151 (comment) ? Thought it was okay to implement a top level interface in a sub-package?

[kkocel]

@codecholeric just to clarify - this rule will not be triggered on interfaces. However, it will fail in cases when inner classes are implementing a top-level abstract class

[codecholeric]

@kkocel the rule as it is now, yes. But if the second example wouldn't be allowed, then interfaces in upper packages would not be allowed to be implemented, and I don't know if that is widely useful. Because that interfaces from higher packages are implemented in a lower one seems pretty common to me...

[qoomon]

@codecholeric sorry for my bad expression in #151 (comment) I meant no child should depend on any parrent

[codecholeric]

@qoomon, but isn't this a super common use case and pattern? I think this would make the rule way less useful. In particular the example from my comment #151 (comment)
I have seen this very often and really don't consider it a code smell...

[codecholeric]

@kkocel what is your take on this? Would you care about the distinction? Would it still match your use case if interfaces in parent packages could not be implemented anymore in sub-packages?

[qoomon]

@codecholeric I agree it is common, however it doesn't make it reasonable :-)
For example if you want to separate the api (interfaces and objects) and implementation in different jar files you need to include the whole package and exclude all packages that includes implementations and you need to keep those exclude rules updated.
If you stick to the 'never depend on parent package' rule it is rather easy to do stuff like

• If you want to create a jar with everything you just need to pick the mycomponent package
• If you want to create an api package it would be as simple as mycomponent.api

[kkocel]

@codecholeric I use it in a slightly different context - in my Kotlin project I want to limit the scope of internal classes which is a bit too broad. Interface discussion is somewhat orthogonal to my case ;).

[codecholeric]

@qoomon Ah, now I get it, why didn't you explain in the beginning, that you had the splitting into archives in mind 🤣 But yes, then you need dependsOn.. and not accessClasses.. as it is now, right? From that angle I see your point... Then I need to adjust #352 to forbid all dependencies, not only accesses, right?
@kkocel that adjustment works for you as well then, even though you don't care about interfaces for your use case?

[kkocel]

@codecholeric it works for me :)

[qoomon]

@codecholeric exactly, sorry for my bad description of my issue :-D. And thanks again for this great project