Repetitive annotation conjunction

[knittl]

ArchUnit is a wonderful library, thanks! I found one little nuisance though with regard to annotation-based rules. Consider the following Spring controller:

@RestController
public class MyController {
  @GetMapping("{id}/name")
  public String getName(@PathVariable final String id) {
    return "Name of " + id;
  }

  @RequestMapping(value = "{name}/id", method = GET)
  public String getId(@PathVariable final String name) {
    return "Id of " + name;
  }
}

Let's say I want to have a rule that verifies that all my controller methods return Strings (not a real requirement, just an example). Simple:

@ArchTest
static final ArchRule must_return_strings = methods()
        .that().areAnnotatedWith(RequestMapping.class)
        .or().areMetaAnnotatedWith(RequestMapping.class)
        .should().notHaveRawReturnType(String.class);

This works and does what I expect it to do. But it feels repetitive having to specify the annotation twice: once as a direct annotation and a second time as a meta annotation. I find myself very often in this situation, especially when writing rules for a Spring application, since Spring mostly does not distinguish between annotations and meta annotations.

Changing this to:

@ArchTest
static final ArchRule must_return_strings = methods()
        .that().areMetaAnnotatedWith(RequestMapping.class)
        .should().notHaveRawReturnType(String.class);

no longer fails for the getId method if its signature is in violation of the rule. Am I missing something from the docs? Ideally, I'd like to express the rule .that().areAnnotatedOrMetaAnnotatedWith(MyAnnotation.class) without having to repeat myself.

Are there common cases where we need to filter annotation targets by meta annotation only? From my point of view, when filtering for meta annotations, you want to include direct annotations most of the time.

Unfortunately, I cannot come up with a new name which would combine the two conjunctions. Thus my proposal would be a breaking change:

• areMetaAnnotatedWith() should (by default) include meta annotated elements and directly annotated elements (this potentially changes behavior of existing rules)
• Introduce a new conjunction areOnlyMetaAnnotatedWith() which excludes directly annotated elements and only matches elements whose annotations itself are meta-annotated with the specified class.

I'd be happy to work on a PR, but before starting, I wanted to double-check if no such possibility already exists. If such a method already exists, please point me to the docs and close this issue :)

[codecholeric]

Yes, I thought about that in the past, too. I have kept it mainly for backwards compatibility, but I'm not sure if we should not just break it. It feels like "are annotated or meta-annotated" is by far the most common case.
The only use case I could imagine for the other one is to ensure that you don't directly annotate with a certain annotation, but instead use it only via meta-annotation. But for that use case I would actually think that "are meta-annotated ... and are not annotated with ..." would be a way better way to clearly express that this is the important point 🤔
Maybe it's a good idea to break this before 1.0 with a big fat warning in the release notes 🤔 Then the typical use case would be nice to write and the other one (exclude direct annotations) can still be expressed reasonably well...

[knittl]

@codecholeric Oooh, I like your suggestion of expressing this as that().areMetaAnnotatedWith(…).and().areNotAnnotatedWith(…) even more than my proposal to introduce a new method in the DSL!

Would be very interesting if people could chime in which rely on areMetaAnnotatedWith() not returning directly annotated elements (i.e. "areOnlyMetaAnnotatedWith": include only meta-annotated, exclude directly annotated).

Not sure how and when to publish this breaking change though. Is there a schedule for 1.0 already? For the time being, I probably have to define my own predicate to use with the .that(hasAnnotation(RequestMapping.class)) form.

[codecholeric]

I think we can just break it with the respective warning. From what I saw your use case is the typical one, and that one would not be affected by the change (only be written a little more complicated than necessary after the change 😉 )

There is no schedule for 1.0, only a scope 😉 Basically full generic support and method references. Unfortunately this is a little bit more complicated than I would wish, so it's taking me quite a while (since I'm mainly working on it in the evenings / on the weekends)

[knittl]

Alright, thanks a lot 🙂 I appreciate your answer here and your work done on the library so far! Let me know when I can remove the then-redundant rule from my tests :)