jakartaee / rest

Jakarta RESTful Web Services
Other
366 stars 121 forks source link

Define CDI integration and startup for Java SE applications #953

Open spericas opened 3 years ago

spericas commented 3 years ago

There are some options when it comes to starting the CDI container in an SE application. These options can impact the way in which Jakarta REST components are located and initialized, so this issue should cover that as well. For next major release.

jamezp commented 1 year ago

Is there a consideration to require CDI support for SE applications?

jansupol commented 1 year ago

I assume so. Otherwise, there would be needed two injection mechanisms one for SE and one for CDI for EE. The idea for SE would be that the SE app would be started by CDI. When the CDI is started, the REST app is started. At least that is how I understand the intentions of the CDI team.

The more interesting part is the CDI in the client. When the Client is started without the SE application, where is CDI? Yet, the injection is required on the Client side, too.

jamezp commented 1 year ago

I've been curious about how that would be handled on the client side as well. In a true SE client it does seems strange to require the boot of a CDI container first. The performance would likely be affected quite a bit.

jansupol commented 1 year ago

The performance would likely be affected quite a bit.

The CDI/Weld is designed to have just a single container running AFAIK. The implementation contains static fields, and CDI.current() also would struggle to choose from multiple running CDI instances. So the customer would need to start the CDI container manually before starting (multiple) clients and clean up the container when done. It is not only the performance but also the user-friendliness that's in jeopardy here.

jamezp commented 1 year ago

The performance would likely be affected quite a bit.

The CDI/Weld is designed to have just a single container running AFAIK. The implementation contains static fields, and CDI.current() also would struggle to choose from multiple running CDI instances. So the customer would need to start the CDI container manually before starting (multiple) clients and clean up the container when done. It is not only the performance but also the user-friendliness that's in jeopardy here.

That is a really good point. It does seem we should have some kind of wording that CDI will not work in clients in the SE environment unless running in the same JVM as the SeBootstrap.Instance. Or something like that.

mkarg commented 1 year ago

Actually from a user's perspective I would be happy to start the CDI container on my own before starting up SeBootstrap, because it gives me the ability to programmatically register classes and extensions outside of JAX-RS. What I would expect is that JAX-RS is simply one of possibly many CDI extensions.

jamezp commented 1 year ago

That seems reasonable for the SeBootstrap, but I'm not sure how user friendly that would be for a simple client in an SE environment. If the client is part used in the same JVM where the SeBootstrap/CDI container were started that could make sense. For a simple ClientBuilder.newClient() in some simple standalone application, it likely doesn't make much sense.

spericas commented 1 year ago

Just to be clear, and "SE Application" in this context refers to an application that exposes REST resources for others to access, not a "client-only SE Application". And, yes, those would need to start a CDI container, but that can be done automatically in most cases. Developers should have the ability to control their own main() but that should be optional given CDI's discovery mechanism. Many modern microservices frameworks that use Jakarta REST already support this strategy.

jansupol commented 1 year ago

Yes, the couple of last comments is about the client-only SE Application; it is quite unclear how the injection should be supported there, for instance for Configuration in a ClientRequestFilter.

spericas commented 1 year ago

Yes, the couple of last comments is about the client-only SE Application; it is quite unclear how the injection should be supported there, for instance for Configuration in a ClientRequestFilter.

Is that really supported now?

jamezp commented 1 year ago

The @Context injection should work for something like a ClientRequestFilter. Section 10.2.8 shows an example. If there's no more @Context, I'd think we'd need some kind of support for this. Or some new annotation like @ClientResource that could be a CDI qualifier of some sort, but also used in SE with no CDI context.

jansupol commented 1 year ago

Is that really supported now?

I believe we have TCK tests for that.

spericas commented 1 year ago

The @Context injection should work for something like a ClientRequestFilter. Section 10.2.8 shows an example.

That is because a Client can be used in server environment where @Context injection is supported. An application that only uses the Client API is not that.

spericas commented 1 year ago

Is that really supported now?

I believe we have TCK tests for that.

Which one?

jamezp commented 1 year ago

The @Context injection should work for something like a ClientRequestFilter. Section 10.2.8 shows an example.

That is because a Client can be used in server application where @Context injection is supported. An application that only uses the Client API is not that.

That sounds fine, but IMO it should be clarified. There is not specific there that says it can't be used in a standalone client. We may just need to clarify "SE" vs "SeBootstrap" too.

arjantijms commented 1 year ago

Or some new annotation like @ClientResource that could be a CDI qualifier of some sort, but also used in SE with no CDI context.

Setting aside for a moment the idea that a pure client probably doesn't support injection anyway, the interesting thing here is that people are seemingly of the opinion that in SE CDI should be avoided for some reason, but it's perfectly okay to have things like HK2 (for Jersey) active.

What makes CDI by definition worse than (say) HK2? Is it just an urban myth, an ancient believe that we can't erase from our minds, or is there an actual tangible reason?

jamezp commented 1 year ago

I'm actually not of the opinion that SE CDI is a bad thing. RESTEasy 6.2 supports CDI in the SeBootstrap already. My only objection would be to a pure SE client that is some standalone, maybe single launch, app which could add unneeded overhead.

jansupol commented 1 year ago

SE CDI should be avoided for some reason

For server-side SE, either SeBootstrap or starting the Server any proprietary way, CDI is of course the injection framework that is about to be used 4.0. The question, I believe is: Should the Jakarta REST implementation start the CDI container, or should the startup of the CDI container bootup Jakarta REST implementation? I believe we should support the latter (only, by spec)

a pure client probably doesn't support injection anyway

Section 10.2.8:

Both the client and the server runtime configurations are available for injection via @Context . These configurations are available for injection in providers (client or server) and resource classes (server only).

It does not say anything about a client in server environment. We have this TCK test that expects the Message body provider to be able to do injection in the client: https://github.com/jakartaee/rest/blob/master/jaxrs-tck/src/main/java/ee/jakarta/tck/ws/rs/spec/context/client/JAXRSClientIT.java

Jersey starts an HK2 instance with each WebTarget instance and probably a similar thing do all other vendors. Yeah, the client is slow, especially when not reused.

spericas commented 1 year ago

Section 10.2.8:

Both the client and the server runtime configurations are available for injection via @context . These configurations are available for injection in providers (client or server) and resource classes (server only).

It does not say anything about a client in server environment. We have this TCK test that expects the Message body provider to be able to do injection in the client: https://github.com/jakartaee/rest/blob/master/jaxrs-tck/src/main/java/ee/jakarta/tck/ws/rs/spec/context/client/JAXRSClientIT.java

Jersey starts an HK2 instance with each WebTarget instance and probably a similar thing do all other vendors. Yeah, the client is slow, especially when not reused.

This is an overkill and shouldn't be required. I don't think this was ever intended to be supported this way. We should definitely clarify this. An application that uses just the client API is not really a Jakarta REST application (it does not define either explicitly or implicitly an Application subclass). The client API (unlike the Microprofile RestClient API) does not use IoC nor should it need to support injection --much like dozens of other APIs of its kind.

In summary, in a Jakarta REST SE application we should start a CDI container in some manner; in an application that only uses the Jakarta REST Client API (no Application subclass at all) we should not be required to do so.

jansupol commented 1 year ago

In summary, a Jakarta REST SE application should start a CDI container in some manner; an application that only uses the Jakarta REST Client API (no Application subclass at all) should not be required to do so.

I really believe this should have been the other way around - the CDI should start the Jakarta REST impl along with any other services required by the customer that depend on CDI, such as Jakarta Config

jamezp commented 1 year ago

In summary, a Jakarta REST SE application should start a CDI container in some manner; an application that only uses the Jakarta REST Client API (no Application subclass at all) should not be required to do so.

I really believe this should have been the other way around - the CDI should start the Jakarta REST impl along with any other services required by the customer that depend on CDI, such as Jakarta Config

Do you mean the user should start CDI then start Jakarta REST?

To me it seems reasonable to allow either the user or the Jakarta REST container to start the CDI container. The Jakarta REST implementation could check if CDI is running, and if not start it.

mkarg commented 1 year ago

IMHO Jan is right. From the view of an application programmer what I want to do is write a main method which registers Jakarta REST as a CDI extension, then creates a CDI container, then runs the SeBootstrap, so the REST implementation is triggered to register beans in the existing context.

mkarg commented 1 year ago

What makes CDI by definition worse than (say) HK2? Is it just an urban myth, an ancient believe that we can't erase from our minds, or is there an actual tangible reason?

I don't know of such a myth. I am already writing SE-based JAX-RS applications including CDI every day.

spericas commented 1 year ago

In summary, a Jakarta REST SE application should start a CDI container in some manner; an application that only uses the Jakarta REST Client API (no Application subclass at all) should not be required to do so.

I really believe this should have been the other way around - the CDI should start the Jakarta REST impl along with any other services required by the customer that depend on CDI, such as Jakarta Config

Yes, my apologies, I meant to say "we should start ...". Comment updated.

OndroMih commented 1 year ago

What I would expect as a developer from the client API:

If Rest Client is built without a CDI container, e.g. from a main method:

If Rest Client is built inside a CDI container (in a CDI bean):

I don't see a reason why Rest client should require CDI. If I, as a developer, want to access the context, I would accept that if I want to inject it, I need to run the code in CDI. If I don't want to use CDI, I would accept that I can't inject it, as long as there's some other Java SE mechanism to get it.

Note that, according to the specification in 10.2 Context Types, only Configuration and Providers can be injected to a client via @Context annotation. The test in https://github.com/jakartaee/rest/blob/master/jaxrs-tck/src/main/java/ee/jakarta/tck/ws/rs/spec/context/client/JAXRSClientIT.java actually asserts that these 2 injections work, it doesn't require any other injection.

The only missing piece for me is that there's no Java SE mechanism to get the context. We could require that, if CDI container isn't present, REST Client would still inject the context itself by reflection. This is OK, because only Configuration and Providers need to be injected into the client and these are global for the lifetime of the client, no need to support scopes. The code to use reflection to find and inject into fields, constructors, and methods should be reasonably simple and it's also easy to cover this with a few tests in the TCK. This solution would be also nice if we decide to keep the @Context annotation for backwards compatibility, as it would keep the backwards compatibility also for injection.

Another option would be to introduce a static method to provide the context but we should avoid static methods if possible.

Alternatively, outside CDI container, we could just require that the client injects the context via a constructor, if there's a constructor that accepts Configuration or Providers arguments.

Even better option, without using reflection, would be to introduce a new interface, e.g. ContextAware, with default methods setConfiguration and setProviders, and, if a provider implements it, the context would be set with those methods:

@Provider
class StringBeanEntityProvider implements MessageBodyReader<StringBean>, ContextAware {
  Configuration config;

  public void setConfiguration(Configuration config) {
    this.config = config;
  }

  public StringBean readFrom(...) {
    assert config != null;
    ...
  }
}