Make sure bean params are @Typed correctly for CDI lookup in case of subtyping
#29355
Conversation
| // check if the class is a bean param | ||
| if (beanParams.contains(clazz.name().toString()) | ||
| && clazz.declaredAnnotation(ResteasyReactiveDotNames.TYPED) == null) { | ||
| // Stef: we could use createTypedAnnotationInstance which adds all interfaces but I don't think |
There was a problem hiding this comment.
I would use it for the sake of consistency and future debugging :)
There was a problem hiding this comment.
But why would we want to produce the interfaces as beans? Won't that lead to potential ambiguous issues like the one we want to fix? Also, bean params are only ever injected as their declared type by RESTEasy Reactive: never as interfaces. I understand the consistency issue you raise, but I felt this would introduce broadness that could lead into future bugs, and would rather have restricted this fix to what we know we absolutely require, and add the interfaces later if and when there's a use-case for them.
There was a problem hiding this comment.
Originally, the impl I added didn't have interfaces and kept just the impl class and that worked fine for RR and for our tests.
But the issue that made me add it (#29128) shows that user can leverage these classes as beans in various ways that we didn't anticipate. I would generally say that fiddling with types of beans without user knowledge is risky as you might break their injection logic. So the more types you can keep there, the better. It's not like we need to do some extra legwork to make it work either.
There was a problem hiding this comment.
OK, whatever. Lemme try.
This comment has been minimized.
This comment has been minimized.
|
|
||
| @Test | ||
| void shouldDeployWithoutIssues() { | ||
| // we only need to check that it deploys |
There was a problem hiding this comment.
@FroMage you could also check that the bean has all the expected types, see ResourceBeanTypeTest as an example.
But I won't insist :)
|
@manovotn is this one good enough for you (I think we can improve the test later if needed)? I would like to get this one into 2.14.2.Final. |
|
@gsmet I didn't mean to block it, feel free to go ahead with it :) |
Fixes #29227