jakartaee / rest

Jakarta RESTful Web Services
Other
351 stars 114 forks source link

Support injection via CDI to indicate a dependency on a Servlet-defined resource #1214

Open jim-krueger opened 5 months ago

jim-krueger commented 5 months ago

The following Servlet-defined types are currently supported via @Context injection and will need to be supported via CDI going forward:

See: https://jakarta.ee/specifications/restful-ws/3.1/jakarta-restful-ws-spec-3.1#servlet_container

jamezp commented 5 months ago

IMO we shouldn't add anything specific here for the REST spec. In fact, we should probably deprecate these injection types. If servlet is available, the CDI spec requires servlet containers make these types injectable https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0#additional_builtin_beans. The one exception here is the ServletConfig.

jim-krueger commented 4 months ago

The one exception here is the ServletConfig

Looks like jakarta.servlet.http.HttpServletResponse is another exception. @Azquelt Does CDI have any equivalents for these?

jamezp commented 4 months ago

Good catch. I would guess most implementations do allow it to be injected, but you're correct that it's not listed.

jansupol commented 4 months ago

@arjantijms Can you, please, shed light on what CDI beans are provided by Servlet 6.1 (Remember we discussed some changes) and what provides the Glassfish Servlet impl?

Azquelt commented 4 months ago

Looks like jakarta.servlet.http.HttpServletResponse is another exception. @Azquelt Does CDI have any equivalents for these?

No, the only beans I'm aware of provided by CDI are in the spec section James linked before. However, note that these requirements will move to the web profile spec for EE 11, so the REST spec may want to restate them anyway.

If the REST implementation already has a way to get hold of these objects, I would have thought that making them available for injection should be fairly straight forward with a producer method.

jamezp commented 4 months ago

If we do include those as required in the spec, we should be specific they are only available in a servlet environment. The Jakarta EE Core Profile does not include servlet.

jansupol commented 4 months ago

As well as in the CDI-managed environment

arjantijms commented 4 months ago

Build-in beans should really be provided by the technology that owns (defines/exports) these types. In this case, it should be Servlet, and only Servlet, that should define that those types are injectable.

It's rather late in the cycle, but it has been discussed before, and maybe we can still get in the Servlet spec.

jamezp commented 4 months ago

Build-in beans should really be provided by the technology that owns (defines/exports) these types. In this case, it should be Servlet, and only Servlet, that should define that those types are injectable.

It's rather late in the cycle, but it has been discussed before, and maybe we can still get in the Servlet spec.

I agree with this. Even if we can't get it into the servlet spec, could we deprecate/remove the ability to inject it in the REST spec? I know it's a breaking change. I can say in Wildfly the only one that can't currently be injected is the HttpServletResponse. TBH I'm not quite sure what that would be needed for anyway.

arjantijms commented 4 months ago

could we deprecate/remove the ability to inject it in the REST spec?

That would be better indeed. REST would otherwise have to inject only the types not already injected via CDI itself, otherwise they would clash. That would then cause one group of Servlet types to be injected by CDI itself, another group by REST, while all the while Servlet itself should do it.

jansupol commented 4 months ago

Even if we can't get it into the servlet spec, could we deprecate/remove the ability to inject it in the REST spec? I know it's a breaking change

Can we say that it is only injectable by @Context in the non-cdi environment? That would keep the backward compatibility.

jamezp commented 4 months ago

Even if we can't get it into the servlet spec, could we deprecate/remove the ability to inject it in the REST spec? I know it's a breaking change

Can we say that it is only injectable by @Context in the non-cdi environment? That would keep the backward compatibility.

+1 from me, this makes the most sense

jansupol commented 4 months ago

I'm not quite sure what that would be needed for anyway.

I have seen it to be used quite heavily, unfortunately.

jim-krueger commented 4 months ago

I've updated https://github.com/jakartaee/platform/issues/837 to request support for these interfaces as part of the work being done in the platform for EE11.

jamezp commented 4 months ago

I think they're is going to be an issue with injecting the HttpServletResponse. It's too soon in the CDI lifecycle to be injected via @RequestScoped and there is no other scope that would really fit.

spericas commented 4 months ago

@jamezp Could you elaborate on your comment? The "too soon" part in particular?

jamezp commented 4 months ago

@jamezp Could you elaborate on your comment? The "too soon" part in particular?

Sure. Note this is Weld specific, but I would assume other implementations behave the same or similar. Weld creates the request scope in the ServletContextListener.contextIntialized() to fire the request scope event. The response has more than likely not been created at this point as the request is just starting.

One idea @WhiteCat22 and I were discussing was possibly something like injecting a ServletService (not a great name, but for here it works). This would have a method like CompletionStage<HttpServletResponse> getHttpServletResponse(). However, there's a question on where this would live.

The Jakarta REST spec does not currently have a hard dependency on Jakarta Servlet and we likely don't want it given Jakarta EE Core does not include servlet. It's also kind of a weird API for the servlet spec too. It's also just a bit odd in general to have to get the response like that, but I don't have a lot of other ideas :)