rest icon indicating copy to clipboard operation
rest copied to clipboard

CDI alignment: Servlet Spec beans Qualifier needed.

Open jansupol opened this issue 2 years ago • 8 comments

The Spec, Section 11, says:

A Servlet-based implementation MUST support injection of the following Servlet-defined types: ServletConfig, ServletContext , HttpServletRequest and HttpServletResponse .

In the CDI environment, the implementation cannot provide the @Default CDI beans, the Servlet default beans can be provided by the Servlet Implementation, which leads to a CDI ambiguity. We can see that with Krazo (but any framework can start to provide the beans), which provides @Default HttpServletRequest and ServletContext CDI beans. If the implementation provides the @Default beans, we can see something like

WELD-001409: Ambiguous dependencies for type HttpServletRequest with qualifiers @Default at injection point [UnbackedAnnotatedField] @Inject private org.eclipse.krazo.core.Messages.request at org.eclipse.krazo.core.Messages.request(Messages.java:0) Possible dependencies:

  • WELD%AbstractBuiltInBean%org.eclipse.krazo.cdi.KrazoCdiExtension%HttpServletRequest,
  • org.glassfish.jersey.ext.cdi1x.internal.CdiComponentProvider$Hk2Bean@1c10d8ce

Hence, unless everyone starts to use @Inject @Any, the Jakarta REST should not provide the @Default beans, I believe. For Jakarta REST 4.0, it would be useful to provide a Qualifier that could be used in conjunction with @Inject instead of @Any to be sure to inject the correct instances.

Would any of @Rest, @RestInject, or maybe @Context be a good qualifier? Krazo is using @JaxRsContext for some beans (e.g. HttpServletResponse, UriInfo).

jansupol avatar Oct 06 '22 09:10 jansupol

In the CDI environment, the implementation cannot provide the @Default CDI beans, the Servlet default beans can be provided by the Servlet Implementation, which leads to a CDI ambiguity. We can see that with Krazo (but any framework can start to provide the beans),

As was mentioned in the Jakarta EE platform call yesterday, really the only spec/api that should provide default beans (build-in beans) for these artefacts is the spec/api that owns them. Anything else should not do so.

Since HttpServletRequest comes from the Servlet spec, Servlet should provide the default beans.

arjantijms avatar Oct 06 '22 09:10 arjantijms

Since HttpServletRequest comes from the Servlet spec, Servlet should provide the default beans.

Right. We have basically two options:

  • Provide an independent qualifier to allow compatibility when migrating among implementations
  • Let the implementation to choose their own qualifier

jansupol avatar Oct 06 '22 09:10 jansupol

Well, we the third option:

  • Drop the requirement and let the Servlet implementation provide the beans

jansupol avatar Oct 06 '22 10:10 jansupol

There are some somewhat hacky ways you could achieve this in a CDI extension. However, I'd agree it seems best to drop the requirement. Jakarta REST should only be required to inject Jakarta REST types IMO.

jamezp avatar Oct 06 '22 15:10 jamezp

Indeed, think of it the other way around; Servlet "suddenly" providing beans for injection of Jakarta REST and Jakarta Faces types. Would be rather weird.

arjantijms avatar Oct 06 '22 15:10 arjantijms

And the fourth option:

  • We do not want to be dependent on whether Servlet exposes the beans or not, so we provide a new object (ServletReference perhaps) that would have a reference to the servlet classes. Instead of allowing to inject directly the Servlet beans, we may have our own bean:
interface ServletReference {
   ServletConfig  getServletConfig();
   ServletContext getServletContext();
   HttpServletRequest getHttpServletRequest();
   HttpServletResponse getHttpServletResponse(); 
}

jansupol avatar Oct 06 '22 16:10 jansupol

I am a big fan of separating concerns. It is up to the Servlet API implementation to produce these beans. It is not the job of Jakarta REST anymore. The requirement should be dropped from section 11.

mkarg avatar Oct 08 '22 06:10 mkarg

I also think it is not our responsibility to provide CDI injection for servlet classes. So I'm +1 for dropping the requirement as well.

chkal avatar Oct 16 '22 14:10 chkal