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

AnnotatedMember.equals() does not work reliably #3187

Closed
klaasdellschaft opened this issue Jun 25, 2021 · 1 comment · Fixed by #3195
Closed

AnnotatedMember.equals() does not work reliably #3187

klaasdellschaft opened this issue Jun 25, 2021 · 1 comment · Fixed by #3195
Milestone

Comments

@klaasdellschaft
Copy link
Contributor

Hi,

I noticed some strange behavior of the current AnnotatedMember.equals() implementations. Following test case for AnnotatedConstructor.equals() currently fails:

public void testAnnotatedConstructorEquality() {
    ObjectMapper mapper = new ObjectMapper();
    DeserializationConfig context = mapper.getDeserializationConfig();
    JavaType beanType = mapper.constructType(SomeBean.class);

    AnnotatedClass instance1 = AnnotatedClassResolver.resolve(context, beanType, context);
    AnnotatedClass instance2 = AnnotatedClassResolver.resolve(context, beanType, context);

    // Successful
    assertEquals(instance1, instance2);
    assertEquals(instance1.getDefaultConstructor().getAnnotated(), instance2.getDefaultConstructor().getAnnotated());
    
    // Fails
    assertEquals(instance1.getDefaultConstructor(), instance2.getDefaultConstructor());
}

Based on the first two successful assertEquals(...) statements, I would have expected that the third assertEquals(...) should be also successful. However, it currently fails.

The reason for this behavior is that AnnotatedConstructor.equals() is currently using == for comparing the two constructors:

public boolean equals(Object o) {
    if (o == this) return true;
    return ClassUtil.hasClass(o, getClass())
            && (((AnnotatedConstructor) o)._constructor == _constructor);
}

However, the implementation of the reflection API in java.lang.Class is always copying / cloning the Field, Method and Constructor instances prior to returning them to the caller (e.g. see Class.copyConstructors()). Thus, each call of Class.getConstructors() will always return new instances.

If you agree that the above test case should be successful (i.e. also assertEquals(instance1.getDefaultConstructor(), instance2.getDefaultConstructor()) should be successful), I would prepare a corresponding pull request that slightly modifies the current implementation of the equals() method for all subclasses of AnnotatedMember that are affected by this problem (i.e. at least AnnotatedField, AnnotatedConstructor and AnnotatedMethod).

@klaasdellschaft klaasdellschaft changed the title Behavior of AnnotatedMember.equals() vs AnnotatedClass.equals() Behavior of AnnotatedMember.equals() Jun 25, 2021
@cowtowncoder
Copy link
Member

@klaasdellschaft I think you are right: I was assuming that instances would be equal.

Now... Jackson itself is probably not using equality check for anything (given there is no known issue) but it does seem like it'd make sense to implement equality checks properly. So I'd be happy to review a PR.
For fields (for example) it'd be simple enough: same name, same class Field is declared in; for methods enough to check name, declaring class, and match of type-erased parameter types.

klaasdellschaft added a commit to klaasdellschaft/jackson-databind that referenced this issue Jul 1, 2021
@cowtowncoder cowtowncoder changed the title Behavior of AnnotatedMember.equals() AnnotatedMember.equals() does not work reliably Jul 3, 2021
@cowtowncoder cowtowncoder added this to the 2.13.0 milestone Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants