-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Issue #10356 Update Weld integration #10359
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far so good.... just some nits and questions
jetty-ee10/jetty-ee10-cdi/src/main/java/org/eclipse/jetty/ee10/cdi/CdiSpiDecorator.java
Show resolved
Hide resolved
_createCreationalContext = beanManagerClass.getMethod("createCreationalContext", contextualClass); | ||
_inject = injectionTargetClass.getMethod("inject", Object.class, creationalContextClass); | ||
_dispose = injectionTargetClass.getMethod("dispose", Object.class); | ||
_release = creationalContextClass.getMethod("release", null); | ||
} | ||
catch (Exception e) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should catch Throwable here an then if debug is on, dump the classloader and if any of the classes can be loaded and if so from where.
Class<?> creationalContextClass = classLoader.loadClass("jakarta.enterprise.context.spi.CreationalContext"); | ||
Class<?> contextualClass = classLoader.loadClass("jakarta.enterprise.context.spi.Contextual"); | ||
|
||
MethodHandles.Lookup lookup = MethodHandles.lookup(); | ||
_current = lookup.findStatic(cdiClass, "current", MethodType.methodType(cdiClass)); | ||
_getBeanManager = lookup.findVirtual(cdiClass, "getBeanManager", MethodType.methodType(beanManagerClass)); | ||
_createAnnotatedType = lookup.findVirtual(beanManagerClass, "createAnnotatedType", MethodType.methodType(annotatedTypeClass, Class.class)); | ||
_createInjectionTarget = lookup.findVirtual(beanManagerClass, "createInjectionTarget", MethodType.methodType(injectionTargetClass, annotatedTypeClass)); | ||
_getInjectionTargetFactory = lookup.findVirtual(beanManagerClass, "getInjectionTargetFactory", MethodType.methodType(injectionTargetFactoryClass, annotatedTypeClass)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that method handles still work for ee9. I guess we don't have CDI API on the classpath? But I thought EE9 was using the same transaction API jars as it was already in jakarta namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they do not, I just haven't fixed them yet!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EE9 is now fixed.
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than some niggles LGTM
return new String[]{"jakarta.enterprise.", "jakarta.decorator."}; | ||
return new String[0]; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing end of line
* </dl> | ||
* | ||
* @see AnnotationConfiguration.ServletContainerInitializerOrdering | ||
*/ | ||
public class CdiServletContainerInitializer implements ServletContainerInitializer | ||
{ | ||
public static final String CDI_INTEGRATION_ATTRIBUTE = "org.eclipse.jetty.ee10.cdi"; | ||
public static final String CDI_INTEGRATION_ATTRIBUTE = "org.eclipse.jetty.cdi"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a comment on why this is not scoped to ee10 in the name
@@ -1,4 +1,4 @@ | |||
# Jetty Logging using jetty-slf4j-impl | |||
org.eclipse.jetty.LEVEL=INFO | |||
org.eclipse.jetty.ee10.annotations.LEVEL=DEBUG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe change back to INFO before merging to avoid too much noise per default?
* Issue #10356 Update Weld integration Signed-off-by: Olivier Lamy <[email protected]> Co-authored-by: Olivier Lamy <[email protected]>
Closes #10356
Make jetty-12 work with CDI 4/Weld 5.1.1