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

Mockito mock of interfaces lost annotation information #2640

Closed
udengaardandersent-ELS opened this issue May 11, 2022 · 6 comments · Fixed by #2645
Closed

Mockito mock of interfaces lost annotation information #2640

udengaardandersent-ELS opened this issue May 11, 2022 · 6 comments · Fixed by #2645

Comments

@udengaardandersent-ELS
Copy link

udengaardandersent-ELS commented May 11, 2022

I see some different behaviour after updating mockito, and I think it's a bug.
See code below - the test fails because the mocked interface has lost the expected annotation.

Making mocks of classes, keeps the annotation information.
Making mocks of interfaces, annotation information is lost.

Junit version: junit:junit:4.13.2
mockito version: org.mockito:mockito-core:4.5.1
openJDK 11

package com.company;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import org.hamcrest.MatcherAssert;
import org.hamcrest.core.IsNull;
import org.junit.Test;
import org.mockito.Mockito;

public class TestAnnotation {

	@Retention(RetentionPolicy.RUNTIME)
	@Target(ElementType.TYPE)
	public @interface SomeAnnotation {
	}

	@SomeAnnotation
	public class SomeClass {
	}

	@SomeAnnotation
	public interface SomeInterface {
	}

	@Test
	public void testAnnotation() {
		SomeClass someClass = new SomeClass();
		MatcherAssert.assertThat(someClass.getClass().getAnnotation(SomeAnnotation.class), IsNull.notNullValue());
		SomeClass spyOnSomeClass = Mockito.spy(someClass);
		MatcherAssert.assertThat(spyOnSomeClass.getClass().getAnnotation(SomeAnnotation.class), IsNull.notNullValue());
		SomeClass someClassMock = Mockito.mock(SomeClass.class);
		MatcherAssert.assertThat(someClassMock.getClass().getAnnotation(SomeAnnotation.class), IsNull.notNullValue());

		MatcherAssert.assertThat(SomeInterface.class.getAnnotation(SomeAnnotation.class), IsNull.notNullValue());
		SomeInterface someInterfaceMock = Mockito.mock(SomeInterface.class);
		MatcherAssert.assertThat(someInterfaceMock.getClass().getAnnotation(SomeAnnotation.class), IsNull.notNullValue());
	}
}

The test works on previous versions of mockito like, 4.4.0

@TimvdLippe
Copy link
Contributor

TimvdLippe commented May 11, 2022

4.5.0 included both #2614 and #2613 the other changes in the release (https://github.com/mockito/mockito/releases/tag/v4.5.0) don't appear to be interesting for a breakage here. @raphw Does this ring any bells for you?

@udengaardandersent-ELS
Copy link
Author

udengaardandersent-ELS commented May 12, 2022

I'm not very familiar with the code, but i looked at the pull requests posted

The change appears to be from #2613

These lines seems interesting to me.

.annotateType(
features.stripAnnotations || features.mockedType.isInterface()
? new Annotation[0]
: features.mockedType.getAnnotations())

If the mock is an interface it will always stip annotations. Similar to the option stripAnnotations.

Is this really intentional ?

@TimvdLippe
Copy link
Contributor

TimvdLippe commented May 12, 2022

It wouldn't surprise me if we accidentally broke something here. @udengaardandersent-ELS do you mind working on a PR that includes your testcase as-is and modify the line you linked to make the test pass? (While keeping all the other tests as well of course)

@raphw
Copy link
Member

raphw commented May 14, 2022

This was a deliberate change as we had some caching issues with the previous solution. We can only inherit the annotations of one superclass/supertype. Mockito would create the same mock for:

mocked class: SomeInterface.class
mocked class: Object.class, additional interfaces: SomeInterface.class

If you wanted to inherit annotations, if the second mock already was created and in the cache, the request for the first mock would lack annotations as the first call inherited them from Object (which effectively means nothing). A similar issue would arise if:

mocked class: SomeInterface.class, additional interfaces: OtherInterface.class
mocked class: OtherInterface.class, additional interfaces: SomeInterface.class

Alternatively, we could disable annotation inheritance only for the case with more then one annotation being involved and only inherit from a single annotation if the superclass was Object. My aim here was to make the mock making deterministic and not to depend on order. As the JVM does not inherit annotations from interfaces, droping the feature for interfaces felt like a sound approach.

@chrisgleissner
Copy link

chrisgleissner commented May 17, 2022

After upgrading to Mockito 4.5.1, the Spring @SpyBean annotation on Feign- and Resilience4J-annoyated REST client Java interfaces no longer works as it did with Mockito 4.4.x and earlier. More specifically, Resilience4J no longer registers its AOP proxy for the annotated interface which renders it unusable. This is on the latest Spring Boot 2.6.7.

I think it is related to the problem described by the OP and I would appreciate if the behavior of Mockito 4.4.x for interface annotations could be reestablished on 4.5.x. If there are valid use cases for the new behavior, it could be hidden behind a feature flag and activated on demand.

raphw added a commit that referenced this issue May 19, 2022
…nterface is mocked, including additional interfaces.

Without this restriction, the first presented interface might determine the interfaces that are inherited by a subsequent mock that presents the interfaces in a different order. Also, it does not make semantic sense to decide on a particular interface to inherit annotations from. Fixes #2640.
@raphw
Copy link
Member

raphw commented May 19, 2022

I created a PR that fixes this issue for cases without additional interfaces. We can release a new version after its reviewed and merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants