AnnotationBundleKey equality fails for Parameter Annotations

[jvmccarthy]

First post here, so let me thank you for sharing Jackson with us all. It's a great library and a wonderful asset to the Java community. 👍

AnnotationBundleKey._equals uses == instead of .equals() to compare annotations. This approach does not work with method parameter annotations, which are not cached and not guaranteed to be the same. I was going to submit a PR using .equals(), but it looks like this behavior is intentional from a comment in the test. I'm not sure if this will break anything else but wanted to raise it to your attention.

This issue was discovered because AnnotationBundleKey is used by RestEasy as part of a key to a ConcurrentHashMap in ResteasyJackson2Provider. Because RestEasy used this class with parameter annotations, object equality was failing with the same hash code, leading to lock contention and performance issues. I've submitted a PR with RestEasy to fix the issue there but wanted you to be aware as well.

Here's a snippet which shows the issue.

import com.fasterxml.jackson.jaxrs.cfg.AnnotationBundleKey;

import javax.annotation.Nonnull;
import java.lang.annotation.Annotation;
import java.lang.reflect.Method;

public class AnnotationEqualitySnippet {
  @Nonnull
  public void annotatedMethod(@Nonnull String a) {}

  public static void main(String[] args) throws Exception {
    Method method = AnnotationEqualitySnippet.class.getMethod("annotatedMethod", String.class);

    Annotation[] ann1 = method.getParameterAnnotations()[0];
    Annotation[] ann2 = method.getParameterAnnotations()[0];
    AnnotationBundleKey key1 = new AnnotationBundleKey(ann1, Object.class);
    AnnotationBundleKey key2 = new AnnotationBundleKey(ann2, Object.class);

    System.out.println(key1.equals(key2)); // outputs "false" ("true" expected)
  }
}

[asoldano]

I previously pasted a comment meant to be for the linked RESTEasy PR, sorry about that.

[cowtowncoder]

Interesting... thank you for reporting this. I'll add it on WIP page:

https://github.com/FasterXML/jackson-future-ideas/wiki/Jackson-Work-in-Progress/

and hope to look into this soon as this would seem to have significant performance implications (although I think not correctness?)

However, I worry a bit about cost of full equality check, as many aspects of JDK annotation implementation are less than efficient. I hope equality checking is done properly; given that value types are quite limited, maybe the main concern is with array-valued annotation properties.

[jvmccarthy]

@cowtowncoder thank you for the quick response! The main concern here is that the same annotation is not guaranteed to be the same object reference, as the example above shows. So, it's not safe to check with ==. It works with annotations that are cached, such as method annotations, but that is an implementation detail and not specified. It doesn't work with parameter annotations which are not cached.

[cowtowncoder]

Ok this took a while but yes, I can reproduce the issue. Looks like Class annotations are literally the only things cached, which makes my caching scheme look rather silly. Testing for the win eh?

[cowtowncoder]

Hmmh. So can fix this quite easily, but realized that there is one big unknown: whether the original intent was to be able to only strictly cache readers/writers for specific endpoint (which is actually difficult if not impossible to actually guarantee, wrt JAX-RS API), or if it's ok for readers/writers to be shared across endpoints that look like each other.
Partly I am concerned as there is one existing test that seems to suggest former... although I don't trust the logic there as that seems like it might have passed as a side effect.

Dilemma I have is this: as things are, caching is mostly useless at least on some platforms.
But 2.9.9 patch is quite late in the release for 2.9, so any breakage there is a huge problem.

Given this I think I will do this for 2.10(.0) instead, just to play things safe.

[jvmccarthy]

Thanks @cowtowncoder! Can you tell me why the switch statement has special cases for arrays of length 1-3 (lines 141-155)? Is this a performance optimization over always using the for loop? Just curious. Thanks again!

[cowtowncoder]

Yeah performance optimization. I probably shouldn't have bothered if I don't have time to test -- it's an idiom I've found useful in other contexts, but usually it's at lower level decoders, tight loops.

But it is perf optimization FWTW.