How to check top level classes which are unnecessary public?

[prondzyn]

I have a question. I'm trying to compose rule which find classes that are public but are not accessible anywhere outside of its package. Unfortunately I'm stuck. Do you have any clues for that?

[hankem]

find classes that are public but are not accessible anywhere outside of its package

As public classes are by definition accessible outside of their packages, I assume that you meant accessed... 😉

In this case, the following rule (using getDirectDependenciesToSelf(), checking more general dependencies than getAccessesToSelf(), which would only consider field accesses and method calls) might be a good starting point:

    @ArchTest
    ArchRule not_unnecessarily_public = classes()
            .that().areTopLevelClasses().and(are(onlyUsedFromSamePackage()))
            .should().notBePublic();
    
    static DescribedPredicate<JavaClass> onlyUsedFromSamePackage() {
        return DescribedPredicate.describe("only used from same package", javaClass ->
                javaClass.getDirectDependenciesToSelf().stream().allMatch(dependency ->
                        dependency.getOriginClass().getPackage().equals(javaClass.getPackage())
                )
        );
    }

[prondzyn]

Yes, accessed is the proper word :)

Thanks @hankem. I was testing something similar before, but the case is not so straightforward. I still have several "false positive" findings like:

1. static nested class accessed, so top-level class also need to be public

//this class have to be public because StaticNestedClass is public and accessed outside but not_unnecessarily_public test is caching it
public class TopLevelClass {  
  
  ...

  public static class StaticNestedClass {

    ....
  }
}

2. public constant accessed from outside

//this class have to be public because CONSTANT is public and accessed outside but not_unnecessarily_public test is caching it
public class TopLevelClass {  
  
 public static final String CONSTANT = "Thanks!";
}

3. enum usage

//this class have to be public because FIRST, SECOND ara public and accessed outside but not_unnecessarily_public test is caching them
public enum SomeEnum {
  FIRST, SECOND
}

4. static method usage

//this class have to be public because doSomethingCommon() is public and accessed outside but not_unnecessarily_public test is caching it
public final class Utilities {

  public static String doSomethingCommon() {
    ...
  }
}

How should I improve the not_unnecessarily_public test to avoid above cases?

[timtebeek]

Sounds like you're running up against some of the same limitations that I encountered when trying to detect unused classes and methods. I suspect you can adapt the rules I used there for your purpose as well.

Some of the limitation I ran up against are:

• Method reference is not considered as a dependency Method reference is not considered as a dependency #215
• Expose type parameters of JavaClass Expose type parameters of JavaClass #115
• Inlined string constants

In short you won't be able to completely detect all cases correctly (yet), so it might be best to combine your rule with FreezingArchRule.freeze() to handle any false positives you encounter. Hope this comment helps!

[prondzyn]

Thanks @timtebeek for the tips. I will be following changes in ArchUnit regarding this.

[hankem]

@prondzyn, I agree that my suggested starting point has false positives – some of them, as @timtebeek pointed out (🙏), require new features in ArchUnit, others like §2 in #583 (comment) ("[primitive] constant") unfortunately can't be resolved due to bytecode limitations. For §1 in #583 (comment) ("static nested class"), I believe that one could work out a more sophisticated solution.

Regarding §3 ("enum") and §4 ("static method"): I'd expect that the rule suggested in #583 (comment) would handle those correctly. Can you provide a (counter) example?

[prondzyn]

In my case the "enum" §3 is because one particular enum type is used as generic type like: Set<MyEnumType> , so I understand ArchUnit is not able to handle it because of bytecode limitations.

For "static method" §4 I have for example a util class:

public final class XssStripper {
  public static String stripXss(String param) {
    return param;
  }
}

and call to the method:

  public <T> Optional<String> test(Selector<T> selector) {
    return selector.getFilters()
        .map(XssStripper::stripXss);
  }

[hankem]

Ah, I see. 👍 Thanks! Then I was just mislead by the categorization, and these issues are those noted in #583 (comment):

• §3: Set<MyEnumType> is not specific to enums, but the open issue with generic type parameters (Expose type parameters of JavaClass #115)
• §4: XssStripper::stripXss is not specific to static methods, but the open issue with method references (Method reference is not considered as a dependency #215)

[timtebeek]

With the recent additions in 0.23 I think it should now be possible to create a rule as described in this issue, without false positives. Would that be enough to close this issue?

[codecholeric]

I would also guess that the recent additions should have solved this, so I'm gonna close the issue for now. But feel free to reopen if there is still a problem!