rest icon indicating copy to clipboard operation
rest copied to clipboard

Clarification: Can `Application.getSingletons()` return a CDI proxy instance?

Open andymc12 opened this issue 5 years ago • 12 comments

In the last couple of weeks, we've had a few different users open issues as a result of some missing clarification in the spec around whether the objects returned from getSingletons() can be CDI instances or not.

For example:

public class MyApp extends Application {

    @Inject
    MyResource myResource;

    @Inject
    MyProvider myProvider;

    @Override
    public Set<Object> getSingletons() {
        return Stream.of(myResource, myProvider)
                      .collect(Collectors.toCollection(HashSet::new));
    }
}

or:

public class MyApp extends Application {

    @Inject
    MyResource myResource;

    @Inject
    MyProvider myProvider;

    @Override
    public Set<Object> getSingletons() {
        MyResource myResource = CDI.current().select(MyResource.class).get();
        MyProvider myProvider = CDI.current().select(MyProvider.class).get()
        return Stream.of(myResource, myProvider)
                      .collect(Collectors.toCollection(HashSet::new));
    }
}

From reading the spec today (assuming that MyProvider is application scoped - see section 11.2.3), it seems like this should be legal, but in practice it runs into problems with section 11.2.8, which says:

  • Field and property injection of JAX-RS resources MUST be performed prior to the container invoking any @PostConstruct annotated method.
  • Support for constructor injection of JAX-RS resources is OPTIONAL. Portable applications MUST instead use fields or bean properties in conjunction with a @PostConstruct annotated method. Implementations SHOULD warn users about use of non-portable constructor injection.
  • Implementations MUST NOT require use of @Inject or @Resource to trigger injection of JAX-RS annotated fields or properties. Implementations MAY support such usage but SHOULD warn users about non-portability.

The JAX-RS implementation can invoked the getSingletons method, but cannot guarantee that it will perform @Context injection of JAX-RS resources before the @PostConstruct method is invoked. For example, if the scope (of the resource) is @Dependent, then the CDI bean is initialized when constructed, before passing the singletons set back to JAX-RS. Other scopes don't initialize the bean until a method has been called on it - but applications that log the bean would then end up calling the toString() method which initializes it and invokes the @PostConstruct method - prior to JAX-RS injection.

I'm opening this issue (1) to see how (or if) other implementations handle this disparity, and (2) to seek some clarification on what is expected with regard to returning CDI beans (proxies) from the getSingletons method - i.e. should the spec prohibit this?

Thanks!

andymc12 avatar Jan 23 '20 19:01 andymc12

@andymc12 Naturally getSingletons pre-dates all the great CDI machinery and scopes. I agree that there are issues when we let applications provide these instances. Personally, and I believe I mentioned this before, I'd deprecate/remove this method altogether.

Back to your comment, I think (I may need to review this part again) the bullet list in that Section is really applicable to the case in which the JAX-RS implementation participates in the creation of an instance (managed beans, etc) and not the case in which the instances are provided by applications. In any case, it can always be explained better.

spericas avatar Jan 24 '20 13:01 spericas

+1 for deprecating this method in favor of CDI

mkarg avatar Jan 24 '20 20:01 mkarg

Thanks for the feedback. Unless anybody strongly objects, I'll plan to do two things:

  1. Deprecate the getSingletons method.
  2. Write up some clarifying statements to the effect that CDI integration should only be applied to cases where JAX-RS participates in the creation of the instance.

For deprecation, I'll check to see if this is something we can put into 3.0 without requiring a separate release process. If not, it will only go into 3.1.

For the writeup, I think it might make sense to put in the 3.0, 3.1 and even the 2.1 stream so that it would be part of a maintenance release. I would like to see it there to advise current users not to return CDI-managed objects from the getSingletons method in current apps. Wdyt?

andymc12 avatar Jan 24 '20 21:01 andymc12

+1, but maybe we should keep 2.1 as-is?

mkarg avatar Jan 24 '20 21:01 mkarg

@andymc12 I agree with your proposal. It's fine if we deprecate in 3.1.

spericas avatar Jan 27 '20 14:01 spericas

@andymc12

I agree that we should deprecate getSingletons() in 3.0 or 3.1. So +1 for that.

I'm also fine with the clarification. But I'm not completely sure if we should really backport it to 2.1. So +0 for that.

chkal avatar Jan 28 '20 08:01 chkal

Cool, thanks for the feedback. I believe that the consensus is:

  1. Javadoc clarification regarding injection can go into 3.0 and 3.1. #836 addresses this.
  2. Deprecation of the getSingletons method can go into 3.1. #837 was intended to address this, but should go into 3.1-SNAPSHOT instead of master. I'll fix this now.

andymc12 avatar Jan 28 '20 18:01 andymc12

CDI is mandatory since 4.0. JAX-RS 3.1 can run without CDI on a classpath. Deprecating the method would leave JAX-RS users with the deprecated only way of having a singleton. Is it the right thing?

jansupol avatar Jan 29 '20 09:01 jansupol

other implementations handle this disparity

I believe Jersey asks CDI to inject all fields and beans upon creation for every scope if Jersey is to create the instance. Of course, when the application creates the instances, such as in the getSingletons case, Jersey does not inject anything.

Maybe it would make sense to replace Section 11.2.8: The following additional requirements apply when using with The following additional requirements apply when the implementation instantiates

jansupol avatar Jan 29 '20 10:01 jansupol

Personally, and I believe I mentioned this before, I'd deprecate/remove this method altogether.

@spericas While the getSingletons method can be used to create singletons, it also is used to instantiate the resources by the user, to set it as they feel appropriate and to provide the data to the instance which may be hard to get during instantiation by the CDI. How do you plan to replace this functionality?

jansupol avatar Jan 29 '20 12:01 jansupol

CDI is mandatory since 4.0. JAX-RS 3.1 can run without CDI on a classpath. Deprecating the method would leave JAX-RS users with the deprecated only way of having a singleton. Is it the right thing?

Yes, but deprecation does not mean removal. Just a warning that this will change in 4.0.

spericas avatar Jan 29 '20 13:01 spericas

Personally, and I believe I mentioned this before, I'd deprecate/remove this method altogether.

@spericas While the getSingletons method can be used to create singletons, it also is used to instantiate the resources by the user, to set it as they feel appropriate and to provide the data to the instance which may be hard to get during instantiation by the CDI. How do you plan to replace this functionality?

Which is the actual use case that is hard to implement using CDI? You have injection, constructors and @PostConstruct.

spericas avatar Jan 29 '20 13:01 spericas