Weld 2335 Having different types between InterceptionFactory and instance to intercept should fail #1599
Conversation
|
Well, sometimes it might make sense to pass a subtype instance - consider this dummy producer: @Produces
void Number produceNumber(InterceptionFactory<Number> factory) {
// MyInterceprotBinding has @Inherited
factory.configure().add(MyInterceprotBinding.Literal.INSTANCE);
return factory.createInterceptedInstance(isDummyConditionTrue ? Long.valueOf("1") : Short.valueOf());
} |
|
First of all, I have to say I feel like this functionality is very limiting. Not being able to use interfaces sounds like a bad practice. Not to mention it's completely counter-intuitive, so we should at the very least point this out somewhere in docs. Ignore the text below, got bad import in the test, sorry for the fuss :-/ |
|
@manovotn this feature is here to apply interceptor binding on an instance dynamically. I agree that the question of supporting parent class is open, even if interceptor binding are only applied on subclass if the annotation is |
|
@mkouba I update the PR to support creation of intercepted instance on child class instance. Technically we could support interfaces (by creating a new AnnotatedType and copying annotations), but is it worth? |
|
@antoinesd I think it is more a spec issue/question for EG. My opinion is that interfaces should not be supported. And I don't think it a bad practice or counter-intuitive - |
| public void testListAdd(BeanManager bm) { | ||
| // resolving via Instance just to make the NPE exception human-readable (direct method injection will blow up with Arq. stack) | ||
| // Invalid producer using an InterceptionFactory for List (interface) and applying it to ArrayList | ||
| List<Object> list = bm.createInstance().select(new TypeLiteral<List<Object>>() {}, Produced.Literal.INSTANCE).get(); |
There was a problem hiding this comment.
Just a minor thing... why no use @Produced Instance<List<Object>> isntance parameter instead of BeanManager bm?
There was a problem hiding this comment.
Because it was the example test I had on my branch and I did it this is way to make the exception human-readable (as the comments suggest). Otherwise you got an NPE coming from arquillian.
I agree it should be either changed now, or at least the comment is now useless.
| // resolving via Instance just to make the NPE exception human-readable (direct method injection will blow up with Arq. stack) | ||
| // Invalid producer using an InterceptionFactory for List (interface) and applying it to ArrayList | ||
| List<Object> list = bm.createInstance().select(new TypeLiteral<List<Object>>() {}, Produced.Literal.INSTANCE).get(); | ||
| Producer.reset(); |
There was a problem hiding this comment.
These lines should be replaced with Assert.fail()...
There was a problem hiding this comment.
+1, since this is now deeded as expected failure
| @@ -112,6 +112,10 @@ public T createInterceptedInstance(T instance) { | |||
| } | |||
| used = true; | |||
|
|
|||
| if(annotatedType.getJavaClass().isInterface()) { | |||
There was a problem hiding this comment.
Please use the predefined formatting style...
|
@antoinesd could you please squash the last 3 commits please? |
add a test on type to have it fails gracefully
|
Merged, thanks! |
As the idea behind InterceptionFactory is to customise the underlying AnnotatedType to add interceptor binding on class or method, it doesn't make sense to pass an instance of a different type than the parameter type used to defined the InterceptionFactory.