Skip to content
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

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

Open
jim-krueger opened this issue Jan 26, 2024 · 17 comments

Comments

@jim-krueger
Copy link
Contributor

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

  • ServletConfig
  • ServletContext
  • HttpServletRequest
  • HttpServletResponse

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

@jamezp
Copy link
Contributor

jamezp commented Jan 29, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

jamezp commented Feb 7, 2024

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

@jansupol
Copy link
Contributor

jansupol commented Feb 8, 2024

@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
Copy link

Azquelt commented Feb 8, 2024

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
Copy link
Contributor

jamezp commented Feb 8, 2024

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
Copy link
Contributor

jansupol commented Feb 8, 2024

As well as in the CDI-managed environment

@arjantijms
Copy link
Contributor

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
Copy link
Contributor

jamezp commented Feb 13, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

jamezp commented Feb 15, 2024

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
Copy link
Contributor

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

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

@jim-krueger
Copy link
Contributor Author

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

@jamezp
Copy link
Contributor

jamezp commented Feb 27, 2024

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
Copy link
Contributor

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

@jamezp
Copy link
Contributor

jamezp commented Feb 29, 2024

@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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants