v1.8: Fix verify in advised beans

[antoinemeyer]

fixes #23

I am not entirely happy with that solution, but it does work (until it will not).
Tested on 2.6.15, 2.7.18, 3.0.13, 3.1.12, 3.2.11, 3.3.5.

[inkassso]

While this change does fix the root cause, I don't think the solution is correct.

From what I understand, the only difference now is unwrapping the proxy, creating the spy from the actual service instance instead of the proxy, and injecting that spy directly into the target bean. However, that removes the proxy and with it also any intercepting actions, e.g. Transaction handling, Async execution or similar. I believe that should be kept, i.e. when the spied bean is a proxy, don't overwrite the proxy in the target bean, but instead just overwrite the target in the proxy. See WorkingLoggingServiceTest4_ManualInjectionWithinProxy.java, where I got this working.

In the aspect in the test, there's nothing being executed so it's hard to detect. I propose the aspect should perform a verifiable action, e.g. set a boolean flag that can be asserted in the test.

[antoinemeyer]

While this change does fix the root cause, I don't think the solution is correct.

From what I understand, the only difference now is unwrapping the proxy, creating the spy from the actual service instance instead of the proxy, and injecting that spy directly into the target bean. However, that removes the proxy and with it also any intercepting actions, e.g. Transaction handling, Async execution or similar. I believe that should be kept, i.e. when the spied bean is a proxy, don't overwrite the proxy in the target bean, but instead just overwrite the target in the proxy. See WorkingLoggingServiceTest4_ManualInjectionWithinProxy.java, where I got this working.

In the aspect in the test, there's nothing being executed so it's hard to detect. I propose the aspect should perform a verifiable action, e.g. set a boolean flag that can be asserted in the test.

Thanks for the feedback.
I changed the code so that the original aspect behavior is now maintained.

[antoinemeyer]

Thanks for the feedback. I changed the code so that the original aspect behavior is now maintained.

@inkassso , any last feedback? If not, I'll move forward with this change.

[inkassso]

Looks perfect now! Thanks for the quick fix 💯

[inkassso]

Unrelated to the bug, but I have yet another issue that could be fixed in the same release.

I can't put a comment directly on the line, but please see line BeanUtils.java:78, where the last argument is a lambda deciding whether a bean field matches the type supposed to be spied. It is the equals method, i.e. a strict class comparison, which may cause problems under certain circumstances.

Imagine having ProviderService as an interface and at least one implementation, e.g. RestProviderService. The interface is used in LoggingService like now, but the test specifies

@MockInBean(LoggingService.class)
RestProviderService providerService;

I tried that out and apparently that's an issue because of this strict comparison. The type to spy declared in the test is not exactly the same class as the type used in the target bean. I get the following error in the test:

java.lang.IllegalArgumentException: Cannot find any field for definition:Definition [name=restProviderService, resolvableType=com.github.inkassso.mockinbean.issue23.service.RestProviderService]

However, the type to spy is a subclass of the field type declared in the bean, so that condition only needs to be adjusted accordingly:

field -> field.getType().isAssignableFrom(type)

Already tested it out locally and it works. However, there are edge cases to be considered, e.g. what if the bean declares two fields, one using the interface and one using the subclass (as dirty as it sounds)? Spring definitely allows two beans with the same supertype in the context, in such case it's customary to mark the bean of one subclass as primary, which will be injected into the field declared with the supertype, while the beans of other subclasses will be injected only into fields declared specifically with their respective subtype.

[antoinemeyer]

Unrelated to the bug, but I have yet another issue that could be fixed in the same release.

I can't put a comment directly on the line, but please see line BeanUtils.java:78, where the last argument is a lambda deciding whether a bean field matches the type supposed to be spied. It is the equals method, i.e. a strict class comparison, which may cause problems under certain circumstances.

Imagine having ProviderService as an interface and at least one implementation, e.g. RestProviderService. The interface is used in LoggingService like now, but the test specifies

@MockInBean(LoggingService.class)
RestProviderService providerService;

I tried that out and apparently that's an issue because of this strict comparison. The type to spy declared in the test is not exactly the same class as the type used in the target bean. I get the following error in the test:

java.lang.IllegalArgumentException: Cannot find any field for definition:Definition [name=restProviderService, resolvableType=com.github.inkassso.mockinbean.issue23.service.RestProviderService]

However, the type to spy is a subclass of the field type declared in the bean, so that condition only needs to be adjusted accordingly:

field -> field.getType().isAssignableFrom(type)

Already tested it out locally and it works. However, there are edge cases to be considered, e.g. what if the bean declares two fields, one using the interface and one using the subclass (as dirty as it sounds)? Spring definitely allows two beans with the same supertype in the context, in such case it's customary to mark the bean of one subclass as primary, which will be injected into the field declared with the supertype, while the beans of other subclasses will be injected only into fields declared specifically with their respective subtype.

Ok thank you for reporting this.
I've created issue #30 to address it.

This PR is replaced by #29 to have both fixes in next release.

[antoinemeyer]

Unrelated to the bug, but I have yet another issue that could be fixed in the same release.

@inkassso, I have created #32 to fix that issue. Could you have a look?