METHOD_PARAMETER support in AnnotationsTransformer

[sarxos]

Hello Quarkus Team!

Description

The AnnotationsTransformer has this method to decide if transformer implementation should be used for annotation of a specific target kind:

@Override
public boolean appliesTo(Kind kind) {
	return kind == ...
}

And right now it works with the following target kinds:

• AnnotationTarget.Kind.CLASS
• AnnotationTarget.Kind.METHOD
• AnnotationTarget.Kind.FIELD

Especially, it works just fine when I use it like this:

@Override
public boolean appliesTo(Kind kind) {
	return kind == AnnotationTarget.Kind.CLASS;
	// return kind == AnnotationTarget.Kind.METHOD;
	// return kind == AnnotationTarget.Kind.FIELD;
}

But as you can see above, there is no support for AnnotationTarget.Kind.METHOD_PARAMETER so the following will not work and the transformer will not run at all:

@Override
public boolean appliesTo(Kind kind) {
	return kind == AnnotationTarget.Kind.METHOD_PARAMETER;
}

This is because of AnnotationStore internal implementation, which, when I use:

@Override
public boolean appliesTo(Kind kind) {
	return kind == AnnotationTarget.Kind.METHOD;
}

Will be applied to annotations of both METHOD_PARAMETER and METHOD targets. This is tricky and unclear and in overall cause this weird logic:

• When transformer is applied to METHOD it will transform method and parameter annotations.
• When transformer is applied to METHOD_PARAMETER it will transform no annotations at all.

As @mkouba explained to me in #2493 - this is because Jandex API for method parameters is very limited and in result cause user unfriendly AnnotationsTransformer API.

Implementation ideas

Ok, the idea is here (not tested):

sarxos@d298653

However I have some doubts in regards to if and how this change should be handled. This is something I would like to discuss.

My first doubt is in regards to which direction change in logic should be made. I have two ideas:

1. Make API clear:
   1.a. When transformer is applied to METHOD it will consume method annotations only.
   1.b. When transformer is applied to METHOD_PARAMETER it will consume parameter annotations only.
2. Leave API ambiguous but add METHOD_PARAMETER support:
   2.a. When transformer is applied to METHOD it will consume both method and parameter annotations.
   2.b. When transformer is applied to METHOD_PARAMETER it will consume parameter annotations

I would prefer option 1 since it introduces well defined logic in the public API, at the price of a little bit more complicated internal implementation to w/a Jandex limitations.

Option 2 is so-so, because it adds support for METHOD_PARAMETER but keeps public API ambiguous, but it may be more safe since it's less likely to introduce potential bugs in the framework downstream, i.e. in already existing Quarkus extensions which explicitly transforms parameter annotations.

Please let me know what are your thoughts.

[sarxos]

Hi, I tried to test the code I implemented as a demonstration but it seems not to work, or to put it in more details, it seems that parameters support in Jandex is not complete. There is a code there, but it seems not work well.

The problem is that all the annotations I have on parameters are reported by Jandex as a AnnotationTarget.Kind.METHOD instead of AnnotationTarget.Kind.METHOD_PARAMETER. Do you maybe know the reason behind this? Shall I report this as a bug in Jandex?

[mkouba]

So in order to support AnnotationTarget.Kind.METHOD_PARAMETER in the AnnotationsTransformer we would have to modify all AnnotationStore clients as well and that's the real problem. Because e.g. if you want to obtain annotations for a specific method param of a specific org.jboss.jandex.MethodInfo then there is no way to get the params for this particular method. At least I'm not aware of any way other than filtering the annotations of the MethodInfo - however, this would not work if there is no annotation declared on the param.

[sarxos]

I tried filtering in my demo, but after more investigation it seems to me that no annotation is AnnotationTarget.Kind.METHOD_PARAMETER kind. Even when you put annotation on parameter it's reported by Jandex as AnnotationTarget.Kind.METHOD.

[mkouba]

Do you have a link to your test?

[sarxos]

I apologise, this was a bug in my test. The annotations present on parameters are indeed processed as AnnotationTarget.Kind.METHOD_PARAMETER kind.

In regards to:

So in order to support AnnotationTarget.Kind.METHOD_PARAMETER in the AnnotationsTransformer we would have to modify all AnnotationStore clients as well and that's the real problem.

This is a problem only when we go with the option 1. But with the option 2 we have no problems because there are no clients which are explicitly applied to METHOD_PARAMETER since this kind was never supported.

And in regards to:

Because e.g. if you want to obtain annotations for a specific method param of a specific org.jboss.jandex.MethodInfo then there is no way to get the params for this particular method. At least I'm not aware of any way other than filtering the annotations of the MethodInfo - however, this would not work if there is no annotation declared on the param.

Well, this is actually possible but the API it is limited as hell and results in hardly readable code when used. I wrapped it in some nice facade which corresponds to the underlying annotation target in a nicer way, so to get list of parameters from MethodInfo (all of them, not only annotated ones) I have this method:

https://github.com/sarxos/abberwoult/blob/master/abberwoult-deployment/src/main/java/com/github/sarxos/abberwoult/jandex/Reflector.java#L438

Which I can use in the following way (here in a build step):

@BuildStep
AnnotationsTransformerBuildItem doMissingParameterAnnotationsTransformation(Reflector reflector) {

	final DotName dn = DotName.createSimple("com.github.sarxos.abberwoult.annotation.ReceivesTesting$ReceivesActor");
	final ClassRef clazz = reflector.findClass(dn).getOrNull();

	if (clazz != null) {

		for (MethodRef method : clazz.getMethods()) {

			if (!method.getName().equals("onInteger")) {
				continue;
			}

			System.out.println("method ........ " + method.getName());

			for (ParameterRef p : method.getParameters()) {
				System.out.println(" parameter ...... " + p.getPosition());
				System.out.println(" name ........... " + p.getName());
				System.out.println(" annotations .... " + p.getAnnotations());
				System.out.println(" type ........... " + p.getTypeName());
				System.out.println(" --- ");
			}
		}
	}

	return new AnnotationsTransformerBuildItem(new MissingParameterAnnotationsTransformer(reflector));
}

With the class like this:

public class ReceivesTesting {
	public static class ReceivesActor extends SimpleActor implements Utils {
		public void onInteger(@Receives Integer index, Long id) {
			reply(11);
		}
	}
}

It outputs:

method ........ onInteger
 parameter ...... 0
 name ........... index
 annotations .... [@com.github.sarxos.abberwoult.annotation.Receives]
 type ........... java.lang.Integer
 --- 
 parameter ...... 1
 name ........... id
 annotations .... []
 type ........... java.lang.Long
 --- 

[sarxos]

Let me describe the use case which caused me to propose this feature.

I have methods which can receive some values, e.g.:

public void onInteger(@Receives Integer index) { ... }
public void onSomething(@Receives Something some) { ... }

And there is a possibility to receive it from a specific source, e.g. from events stream. Such parameters are annotated with @Event:

public void onSomething(@Receives @Event Something some) { ... }

So what I wanted to achieve is that, similarly to how @Inject is automatically added when missing, a missing @Receives annotation would be automatically added whenever @Event is used on parameter.

So when someone annotate it like this:

public void onSomething(@Event Something some) { ... }

It will automatically become:

public void onSomething(@Receives @Event Something some) { ... }

I found this to be simple for fields and methods since this is well supported, but I found no way to do it for method parameters.

Just for your information. This is not a blocker for me. I can implement the same functionality in a few different ways, but I choose to do this with AnnotationTransformer since it looked to me as the cleanest way, just to discover that it's impossible.

I hoped to implement support for METHOD_PARAMETER on my own but it seems like there is more than what I can see in the AnnotationStore. When I tried to look at the bigger picture I found too much code to grasp in a such short period of time and I do not want to spend too much on this since this feature is not of high importance.

Initially I hoped that it will be enough to use my code, but it seems not to work well and after thinking about time required to implement this and potential drawbacks I'm really considering to drop this feature request... What are your thoughts?

[mkouba]

But with the option 2 we have no problems because there are no clients which are explicitly applied to METHOD_PARAMETER since this kind was never supported.

The "client code" was calling AnnotationStore.getAnnotations() with a MethodInfo as the argument because if you have a MethodInfo there is no way to obtain MethodParameterInfos for all parameters. So it is a problem for both variants.

[sarxos]

Ok, I get your point. Indeed in my w/a I do not work on top of MethodParameterInfo at all because of the reason you described. I will close this for now and try to craft new PR to Jandex to add API for this.