How do I execute the number check per JavaClass?

[mettyoung]

I want to create a ArchUnit test to ensure no unnecessary public methods are created in my handlers.

A code violation is as follows:

public class GetAuditLogQueryHandler implements IQueryHandler<GetAuditLogQuery, List<GetAuditLogQueryResponse>> {

  @Override
  public List<GetAuditLogQueryResponse> query(GetAuditLogQuery query) {
  }

  public List<GetAuditLogQueryResponse> query(Long periodId) {
  }
}

I tried writing an assertion as follows:

    ArchRule rule = methods().that().arePublic()
        .and().areDeclaredInClassesThat().implement(IQueryHandler.class)
        .should().containNumberOfElements(DescribedPredicate.equalTo(1));

But instead of ensuring there is only one public method in each class, it counts all public methods in the scanned JavaClasses.

[hankem]

You need to write an ArchRule for classes() if you want to impose a class-level condition like have exactly one public method, which I would implement with a custom predicate:

static imports

import static com.tngtech.archunit.base.DescribedPredicate.describe;
import static com.tngtech.archunit.core.domain.JavaModifier.PUBLIC;
import static com.tngtech.archunit.core.domain.properties.HasModifiers.Predicates.modifier;
import static com.tngtech.archunit.lang.conditions.ArchConditions.have;
import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes;

ArchRule rule = classes()
                .that().implement(IQueryHandler.class)
                .should(have(describe("exactly one public method", javaClass ->
                        javaClass.getMethods().stream().filter(modifier(PUBLIC)).count() == 1
                )));

[mettyoung]

Oh, that's awesome! I thought I need to start with methods() since I was validating the methods; then I encounter the issue of counting the methods in a class (lost the context of class in the process).

However, getMethods() also retrieves the interface method which I overridden. What's the correct way of excluding the super methods?

I tried

javaClass.getMethodCallsToSelf().stream().filter(modifier(PUBLIC)).count() == 1

but it cannot be filtered by modifier public anymore

[hankem]

javaClass.getMethodCallsToSelf is about actual invocations of the method on an instance. Do you want to prevent that additional public methods are declared, or just that they are used?

#1040 will probably introduce an API JavaMethod#isOverridden. It's still being developed, but you can look at the PR to get an inspiration what you could likewise do in a custom predicate.

I thought that the 1 allowed public method was exactly the one from IQueryHandler, so I probably misunderstood what you want to test.

[mettyoung]

Let me share you my objective of the test. I have created IQueryHandler functional interface for all QueryHandler instances to implement. I want to make sure that all QueryHandlers will only implement that method without introducing more public methods.

Regards, Emmett

[hankem]

Sorry, I had initially thought that the overwritten methods wouldn't matter and that the simple rule would be enough.

My new suggestion is unfortunately more complex (but reports violations for individual methods):

@ArchTest
void classes_that_implement_IQueryHandler_should_have_no_other_public_methods(JavaClasses classes) {
    Set<JavaMethod> allowedMethods = Stream.of(IQueryHandler.class, Object.class)
        .map(classes::get)  // Make sure that Object is imported, or this throws an IllegalArgumentException!
        .flatMap(javaClass -> javaClass.getAllMethods().stream().filter(modifier(PUBLIC)))
        .collect(toSet());

    methods()
        .that().arePublic()
        .and().areDeclaredInClassesThat().implement(IQueryHandler.class)
        .should(new ArchCondition<JavaMethod>("implement IQueryHandler or extend from Object") {
            @Override
            public void check(JavaMethod method, ConditionEvents events) {
                boolean notAllowed = allowedMethods.stream().noneMatch(allowedMethod ->
                    method.getName().equals(allowedMethod.getName()) &&
                    method.getParameterTypes().equals(allowedMethod.getParameterTypes())
                );
                if (notAllowed) {
                    String message = createMessage(method, "is public, but does not implement IQueryHandler");
                    events.add(violated(method, message));
                }
            }

        })
        .check(classes);
}

[hankem]

Feature idea: Wouldn't it be nice if JavaMethod had a method hasSameSignatureAs(JavaMethod other), allowing us to simplify:

-                boolean notAllowed = allowedMethods.stream().noneMatch(allowedMethod ->
-                    method.getName().equals(allowedMethod.getName()) &&
-                    method.getParameterTypes().equals(allowedMethod.getParameterTypes())
-                );
-                if (notAllowed) {
+                if (allowedMethods.stream().noneMatch(method::hasSameSignatureAs)) {

?

[mettyoung]

Thank you. I faced issues with your code on fetching allowedMethods. I tried revising and sharing what worked:

@RunWith(ArchUnitRunner.class)
@AnalyzeClasses(packages = "com.mettyoung.application.query",
    importOptions = {
        ImportOption.Predefined.DoNotIncludeJars.class,
        ImportOption.Predefined.DoNotIncludeTests.class
    })
public class QueryHandlersArchTest {

  @ArchTest
  void classes_that_implement_IQueryHandler_should_have_no_other_public_methods(JavaClasses classes) {
    Set<JavaMethod> allowedMethods = classes.stream()
        .filter(clazz -> clazz.isAssignableTo(IQueryHandler.class))
        .flatMap(clazz -> clazz.getAllMethods().stream().filter(modifier(PUBLIC)))
        .collect(Collectors.toSet());

    methods()
        .that().arePublic()
        .and().areDeclaredInClassesThat().implement(IQueryHandler.class)
        .should(new ArchCondition<JavaMethod>("implement IQueryHandler or extend from Object") {
          @Override
          public void check(JavaMethod method, ConditionEvents events) {
            boolean notAllowed = allowedMethods.stream().noneMatch(allowedMethod ->
                method.getName().equals(allowedMethod.getName()) &&
                    method.getParameterTypes().equals(allowedMethod.getParameterTypes())
            );
            if (notAllowed) {
              String message = createMessage(method, "is public, but does not implement IQueryHandler");
              events.add(violated(method, message));
            }
          }
        })
        .check(classes);
  }
}

[hankem]

I faced issues with your code on fetching allowedMethods.

Which issues did you face?

   Set<JavaMethod> allowedMethods = classes.stream()
       .filter(clazz -> clazz.isAssignableTo(IQueryHandler.class))
       .flatMap(clazz -> clazz.getAllMethods().stream().filter(modifier(PUBLIC)))
       .collect(Collectors.toSet());

Will your rule ever catch violations in this way? You now allow every public method not only of IQueryHandler, but also of every class that implements IQueryHandler – even those that I thought you wanted to prevent.

[mettyoung]

I encountered this error

java.lang.IllegalArgumentException: JavaClasses do not contain JavaClass of type com.bytedance.cg.gcrm.salesplanning.common.cqrs.IQueryHandler

	at com.tngtech.archunit.thirdparty.com.google.common.base.Preconditions.checkArgument(Preconditions.java:453)

[hankem]

I had added the warning // Make sure that Object is imported, or this throws an IllegalArgumentException! thinking that you might not import the java.lang package, but I didn't realize that IQueryHandler could likewise not be part of your import... 🤣

Can you try my suggestion of https://github.com/TNG/ArchUnit/issues/1103#issuecomment-1515321539 again when you @AnalyzeClasses in

packages = {"com.mettyoung.application.query", "java.lang", THE_PACKAGE_OF_IQueryHandler}

(hoping that the import doesn't take too long in this case)?

Because I still think that your way of collecting allowedMethods also includes what should actually be violations. (Did your rule ever fail?)

[mettyoung]

Hi, I've updated it but it is still throwing the same error. I don't understand why we have to get Object.class from JavaClasses

[hankem]

I don't understand why we have to get Object.class from JavaClasses

I thought that it should be allowed to override equals(Object), hashCode() or toString() inherited from java.lang.Object. That's why I suggested to add Object's public methods to the Set of allowedMethods.

[hankem]

it is still throwing the same error

Are you sure that you've imported the right IQueryHandler class?

The error message above mention

com.XXXXXXXXX.XX.XXXX.XXXXXXXXXXXXX.XXXXXX.XXXX.IQueryHandler

whereas the last screenshot shows

com.mettyoung.common.cqrs

(Furthermore, when using ImportOption.Predefined.DoNotIncludeJars, IQueryHandler would not be imported, i.e. lead to the JavaClasses do not contain JavaClass error, in case it was actually included as a jar to the project where you get the error.)

[mettyoung]

Hi @hankem , you were correct. I was importing the wrong package while I was copying pasting the code with censorship.

After following your above comment, I'm afraid I still face a different issue in conditional matching.

Furthermore, I do not understand why we need to get the allowedMethods from Object class. We don't need methods like notify and equals right?

I also find the fault in method.getParameterTypes().equals(allowedMethod.getParameterTypes()) because the method.getParameterTypes() returns the subclass while allowedMethod.getParameterTypes() return the generic type.

P.S. Apologies for the request, but could you remove the company name from your previous post? It was a missed from my side to censor the company name in the screenshot.