Declarative contracts

[velo]

This is a preparation work for feign code generation that I'm working on this branch:
https://github.com/velo/feign/tree/apt-generator

With generated client, we will be able to generate clients that will work with graalVM and also don't have java 11 reflective warning. Fixing both:
#935
#944

The key work for the code generation is the new DeclarativeContract. Which would allow to reuse the annotation logic when generating client. So, same contract code used by feign reflectively would be used on APT code generation.

The idea of this PR is to review the design of this new DeclarativeContract

[kdavisk6]

This class has become quite large. What are your thoughts on breaking out the Annotation Processors into their own classes and possibly their own package?

[velo]

This class has become quite large. What are your thoughts on breaking out the Annotation Processors into their own classes and possibly their own package?

ow yes, as a mater of fact, annotation process will go to it's own module, since it will have dependencies to all contracts.

[kdavisk6]

@velo

I have a couple questions regarding the set up. Can you help by providing some more background?

[velo]

note to self:
Extract this class to a new source file before merging. Enough is enough =)

[kdavisk6]

I'm still confused and I think your current approach relies on others having sufficient mental space to interpret what's going on. There are three areas where I'm still not able to fully understand:

1. DO_NOTHING

This says to me that I want to skip processing but I'm doing my best to avoid a if !null check. null checks are not evil and are completely appropriate. However, I do understand that you are going for functional style. I'll reiterate that I think that using Optional is a better approach here. It states clearly that we are not going to process an annotation unless we have a processor registered.

public void processAnnotationsOnClass(MethodMetadata data, Class<?> target) {
  Arrays.stream(target.getAnnotations())
    .forEach(annotation -> getProcessor(annotation)
        .ifPresent(
            annotationProcessor -> annotationProcessor.process(annotation, data)));
}

private Optional<AnnotationProcessor> getProcessor(Annotation annotation) {
  return Optional.ofNullable(this.processors.get(annotation));
}

2. Guarding using a Predicate

I'm still unclear on how this is intended to be used. If its to determine if the processor should be called, we should leave that decision up to processor implementations. Having the Predicate muddies the intention of AnnotationProcessor leaving it unclear what is the expected way to guard against execution. It's still possible to skip processing in the process function with the test method present, bypassing the predicate entirely. If we want to keep it, we'll need to find a way to make it's use crystal clear.

3. Ignoring Parameters

I don't think ignoring is the right verb. I suspect what you are looking for is to identify which parameters are not annotated and how to deal with them. The current implementation relies on the result of processAnnotationsOnParameter to indicate if the parameter was processed. With the annotation processor approach, that's no longer necessary as we will only process parameters that have annotations. We can refactor processAnnotationsOnParameter to process the Parameter, allowing implementations to deal with those special scenarios.

protected void processAnnotationsOnParameter(MethodMetadata methodMetadata,
    Parameter parameter, int index) {
  Annotation[] annotations = parameter.getAnnotations();
  if (annotations != null && annotations.length != 0) {
    for (Annotation annotation : annotations) {
      AnnotationProcessorContext context =
          new AnnotationProcessorContext(methodMetadata, parameter, index);
      this.getAnnotationProcessor(annotation)
          .ifPresent(processor -> processor.process(annotation, context));
    }
  } else {
    /* no annotations, special processing, like request options, etc... */
    if (Request.Options.class.isAssignableFrom(parameter.getType())) {
      /* process */
    }
  }
}

I created a gist that demonstrates these thoughts and my approach. Now, I could be completely off here. If I am, please let me know.

https://gist.github.com/kdavisk6/bb6db782c8fe5284ed0ea3ffb1a5d012

[velo]

1. DO_NOTHING

Ow, there is one left for ParameterAnnotationProcessor, I will remove it. Thanks.

2. Guarding using a Predicate

So, my goal for DeclarativeContract is separate Class + annotation navigation from "how each annotation affects MethodMetadata". Main reason for that is that Annotation Processor Tool does NOT have classes, so, for APT, I need to navigate the annotations using a different API.

This translates into DeclarativeContract having the 3 process processAnnotationsOnClass, ...OnMethod and ...OnParam all being final. So the decision if an annotation should be processed need to be placed on this Predicate so APT can reuse this code.

3. Ignoring Parameters

This is used for JAXRS contract. On jaxrs you can inject things like the @Context HttpRequest request. When JAXRSContract detects @Context it needs to inform feign that this parameter must not be processed. There, my idea of calling it ignored.

Now, if the contract didn't made any decision about a parameter, feign assumes that one is a BODY. That can either be due to no annotation or some annotation that contract don't care about.

gist

AnnotationProcessorContext supporting both param and non-param annotations would lead to index being null some times, I prefer to have 2 possible processors with their own signature... that way, when processing a Parameter you do have the the extra parameter to index and must deal with it.

[kdavisk6]

@velo

Thanks for the explanation and now I understand your approach. I have some reservations about the ignoring approach, it still feels awkward, but it works and I don't want to hold this up any more. 👍

[velo]

Thanks for the explanation and now I understand your approach. I have some reservations about the ignoring approach, it still feels awkward, but it works and I don't want to hold this up any more.

Ok, going to merge it as is.