Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional field annotation dropped if additional matcher matches the same field #1022

Closed
odrotbohm opened this issue Mar 12, 2021 · 8 comments
Assignees
Milestone

Comments

@odrotbohm
Copy link
Contributor

Assume we want to add annotations to a field and the field is selected by matchers that ultimately end up selecting the same field. In that case, only the latter annotation definition actually ends up in the class file. Here are some sample rules:

  • Field named property -> add @First
  • Field of type String -> add @Second

For a field String property this will end up with only @Second being added. This also fails if the matcher for the second annotation definition is actually the same as the first matcher. Reproducer below:

public class Sample {

  public static void main(String[] args) throws Exception {

    Class<? extends Entity> type = new ByteBuddy()
        .redefine(Entity.class)

        // Adding an annotation based on some criteria
        .field(ElementMatchers.named("property"))
        .annotateField(AnnotationDescription.Builder.ofType(First.class).build())

        // Adding another annotation matching the same field but different criteria
        .field(ElementMatchers.fieldType(String.class)) // Remove this to make it work. Also fails if the matcher above is repeated
        .annotateField(AnnotationDescription.Builder.ofType(Second.class).build())
        
        .make()
        .load(Sample.class.getClassLoader(), ClassLoadingStrategy.Default.CHILD_FIRST)
        .getLoaded();

    // Prints @Second. Expected @First, @Second.
    System.out.println(Arrays.toString(type.getDeclaredField("property").getAnnotations()));
  }

  static class Entity {
    public String property;
  }

  @Retention(RetentionPolicy.RUNTIME)
  @Target(ElementType.FIELD)
  static @interface First {

  }

  @Retention(RetentionPolicy.RUNTIME)
  @Target(ElementType.FIELD)
  static @interface Second {

  }
}
@raphw raphw self-assigned this Mar 12, 2021
@raphw raphw added the question label Mar 12, 2021
@raphw raphw added this to the 1.10.22 milestone Mar 12, 2021
@raphw
Copy link
Owner

raphw commented Mar 12, 2021

You are right, those are not additive. It's applied from bottom to top where the first matching phrase wins. This is, because most interceptions are not additive. To overcome this, you have to add a third clause:

.field(ElementMatchers.named("property"))
.annotateField(AnnotationDescription.Builder.ofType(First.class).build())

.field(ElementMatchers.fieldType(String.class))
.annotateField(AnnotationDescription.Builder.ofType(Second.class).build())

.field(ElementMatchers.fieldType(String.class).and(ElementMatchers.named("property")))
.annotateField(AnnotationDescription.Builder.ofType(First.class).build())
.annotateField(AnnotationDescription.Builder.ofType(Second.class).build())

I see that this can be annoying, I will consider adding an extension to allow for "fall through" applications similar to the agent builder.

@odrotbohm
Copy link
Contributor Author

The problem is that the code adding these two annotations doesn't actually know about each other. It's even living in separate Plugin implementations that process the same types but are ultimately activated based on the classpath arrangement. I don't think it'll be a problem in user projects but in the plugin project itself, I have a test for each plugin, verifying that it's adding the annotations to fields of a given type. I'll probably just skip the test for now.

What irritated me the most was the fact that both annotations are added if you don't use any matchers at all.

@raphw
Copy link
Owner

raphw commented Mar 15, 2021

What do you mean by the last bit? Can you give an example for not using matchers?

@odrotbohm
Copy link
Contributor Author

In the original example, the code adds both annotations if you remove the second matcher.

@raphw
Copy link
Owner

raphw commented Mar 16, 2021

Byte Buddy treats every definition as one unit. The problem is, that Byte Buddy does not require those matchers to be combinable. The visit API is rather meant for such cases. I can see if I can add an "add annotation" mechanism as a visitor, this would probably be the best approach to this.

@raphw
Copy link
Owner

raphw commented Mar 16, 2021

Turns out I already made this a few years back:

builder = builder.visit(MemberAttributeExtension.ForField
  .annotate(...)
  .on(...))

Those are additive, interceptions are not.

@odrotbohm
Copy link
Contributor Author

Thanks Raphael, I'll try to move that model. Will keep you posted.

@odrotbohm
Copy link
Contributor Author

That seems to do the trick. Thanks (again), Raphael! 🙇

@raphw raphw closed this as completed Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants