jakartaee / rest

Jakarta RESTful Web Services
Other
353 stars 114 forks source link

CDI alignment: Servlet Spec beans Qualifier needed. #1135

Open jansupol opened 1 year ago

jansupol commented 1 year ago

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).

arjantijms commented 1 year ago

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.

jansupol commented 1 year ago

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

Right. We have basically two options:

jansupol commented 1 year ago

Well, we the third option:

jamezp commented 1 year ago

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.

arjantijms commented 1 year ago

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.

jansupol commented 1 year ago

And the fourth option:

interface ServletReference {
   ServletConfig  getServletConfig();
   ServletContext getServletContext();
   HttpServletRequest getHttpServletRequest();
   HttpServletResponse getHttpServletResponse(); 
}
mkarg commented 1 year ago

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.

chkal commented 1 year ago

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.