eclipse / microprofile

Repository for important documentation - the index to the project / community
Apache License 2.0
676 stars 115 forks source link

Discovery of JAX-RS resources in MicroProfile #50

Open NottyCode opened 6 years ago

NottyCode commented 6 years ago

There was an issue raised against the Open API TCK to resolve how JAX-RS resources get discovered. The issue was closed with a reference to issue #32 based on a discussion on the hangout. I wasn't on the hangout, but reading issue 32 I'm concerned that this issue might be lost so I'm raising this to specifically cover it.

The example sited from section 11.2 of the JAX-RS specification is incomplete for how CDI discovery works, so perhaps the MicroProfile spec could provide a modern and complete example.

kenfinnigan commented 6 years ago

Is this resolved by #49 setting all JAX-RS Resources to be @RequestScoped? In the architecture call we thought it would, but we wanted to verify that

struberg commented 6 years ago

Strong -1 Note that the behaviour IS specified in EE8!

Auto-assigning @RequestScoped would introduce non-compatibility with EE servers! The EE spec says that a class MUST be a CDI bean to be picked up at first. (Except you manually register the resource classes/Application via web.xml) So either have a META-INF/beans.xml in that jar or have a bean-defining annotation. In the later case the Scope is AUTOMATICALLY @Dependent! Yes, that doesn't fit well with JAX-RS, but that's CDI rules.

If we would now define that any JAX-RS resource which has no scope would automatically be @RequestScoped, then this would break CDI rules I fear.

@rmannibucau @antoinesd @mkarg @chkal plz review and also state an opinion. txs!

rmannibucau commented 6 years ago

Same here. Fix it in jaxrs and dont fork it in mp.

mkarg commented 6 years ago

@struberg JAX-RS 2.1 spec says that JAX-RS applications and JAX-RS providers MUST be application scoped and JAX-RS resources MUST be request scoped, so it would be simply logical to make it @RequestScoped by default. We could add this requirement to the JAX-RS 3.0 specification, so it is unambiguously defined and the CDI- and MP-specs can be free of JAX-RS-special-cases. There is a common concesus by the JAX-RS committers anyways to mandate by JAX-RS 3.0 that all JAX-RS resources MUST be CDI beans (yes, this will be backwards-incompatible, that's why we don't do it in JAX-RS 2.2 already).

rmannibucau commented 6 years ago

Hmm, if true it mist be reverted at spec level since it is a breaking change in all impl. Only default scope is ambiguous and is request scope but any scope is valid. Also providers are dependent (of the app) and not application scoped by default.

At least it was like that in 2.0 and i cant believe it has been broken in 2.1.

mkarg commented 6 years ago

As I said, we will break backwards compatibility in JAX-RS 3.x. This already is common sense of the JAX-RS committers. We actually will get rid of the outdated JAX-RS component model in favor of CDI wherever it stands in the way. CDI shall be the core of JAX-RS in future, not just an add-on. That's why we do it in 3.x but not 2.x.

JAX-RS Spec 2.1 chapter 11.2.3: Providers and Application subclasses MUST be singletons or use application scope.. See, I said application scope, not @ApplicationScoped.

struberg commented 6 years ago

If it's backward incompatible in JAX-RS-3.x then you must not do it per JCP rules. And even for JakartaEE it's a bad idea.

But anyway, the initial question was about the situation right now. What is defined right now?

rmannibucau commented 6 years ago

Hmm @RequestScoped for resource by default is against CDI and if the goal is to align on CDI then just say the scope is the cdi one - of the resource bean. This is bckward compatible and works.

Side note: currznt recomlended scope for resource in apps is application scoped so request scoped sounds like a huge regression and will enforce code changes...+ it goes against cdi model

chkal commented 6 years ago

Let me share a few thoughts about this.

Auto-assigning @RequestScoped would introduce non-compatibility with EE servers!

AFAIK Wildfly is already doing it this way today. Please see this mail. Perhaps @asoldano can confirm this?

Only default scope is ambiguous and is request scope but any scope is valid. Also providers are dependent (of the app) and not application scoped by default.

I already brought up the topic of CDI resources and their lifecycle on the JAX-RS mailing list earlier this year. See this mail for details.

In a nutshell the JAX-RS spec states:

My first interpretation was actually that JAX-RS requires CDI resources to be request-scoped. But after some discussions @mkarg provided a great summary:

I think the misunderstanding is that JAX-RS's phrase "per request" is not necessarily @RequestScope. CDI can reach the target of request scoping by many ways, using several annotations or even programmatic solutions. JAX-RS does not enforce "@RequestScoped" literally, only "effectively".

So basically it is fine that CDI resources and providers are dependent-scoped by default. It is up to the JAX-RS implementation to lookup/create instances either once per request or only once for the application.

So this is the current interpretation. But I agree that the spec is rather vague on the concrete integration requirements between CDI and JAX-RS. Especially I'm very unhappy about the term "CDI-style beans". What does this mean exactly? What is the difference between a "CDI-style bean" and a "CDI bean". I would really like to clarify this in JAX-RS.

struberg commented 6 years ago

I think I can shed a light on "CDI-style bean" vs "CDI bean".

[1] https://jaxenter.com/tutorial-introduction-to-cdi-contexts-and-dependency-injection-for-java-ee-jsr-299-104536.html

asoldano commented 6 years ago

Auto-assigning @RequestScoped would introduce non-compatibility with EE servers!

AFAIK Wildfly is already doing it this way today. Please see this mail. Perhaps @asoldano can confirm this?

http://docs.jboss.org/resteasy/docs/3.6.1.Final/userguide/html_single/index.html#CDI

chkal commented 6 years ago

The relevant part from this section:

48.2. Default scopes A CDI bean that does not explicitly define a scope is @Dependent scoped by default. This pseudo scope means that the bean adapts to the lifecycle of the bean it is injected into. Normal scopes (request, session, application) are more suitable for JAX-RS components as they designate component's lifecycle boundaries explicitly. Therefore, the resteasy-cdi module alters the default scoping in the following way:

  • If a JAX-RS root resource does not define a scope explicitly, it is bound to the Request scope.
  • If a JAX-RS Provider or javax.ws.rs.Application subclass does not define a scope explicitly, it is bound to the Application scope.
ljnelson commented 5 years ago

Just a passing note on this: I've seen a couple mentions here and elsewhere about what Java (or Jakarta) EE does or says with regard to the interaction between CDI and JAX-RS. But this is MicroProfile and there is no defined dependency in the MicroProfile effort on the Java (or Jakarta) EE Platform Specification that I am aware of, so surely such mentions are advisory at best. Indeed, at the moment, it seems to me that only the (not really extant, but planned) MicroProfile "umbrella" specification (see #49 and relevant discussion) together with a perhaps clarified section 11.2.3 of the JAX-RS specification governs this interaction.

(Slightly related issue: https://github.com/eclipse-ee4j/jaxrs-api/issues/698.)

rmannibucau commented 5 years ago

@ljnelson the point was that if MP diverges from EE it is just a small library like a tons we find on the net. The only strength of MP is to give eagerly future EE features in a "close enough" shape to enable costless migrations (sed more or less). In that context it can't do anything ans must take care it will converge in case of any ambiguity.

ljnelson commented 5 years ago

@rmannibucau I don't have an opinion on this one way or the other. I just wanted to point out that as of November 9, 2018 there is no platform specification for MicroProfile that defines behaviors arising from the interaction of its component specifications. There will, IMHO, need to be if there is a desire to standardize such behaviors.

kenfinnigan commented 5 years ago

In today's MP Architecture call we discussed again the resolution to #49 in light of the above comments.

As there was no agreement this was the appropriate approach, it was agreed that further discussion on this issue was warranted

kenfinnigan commented 5 years ago

Would it be worth considering marking all JAX-RS Resources as @ApplicationScoped if no other scope is defined?

rmannibucau commented 5 years ago

CDI spec defines them as @Dependent already

kenfinnigan commented 5 years ago

Do you have a reference where it refers to that? My understanding any definition of that was at the Java EE specification level, not an individual spec.

Even so, there's nothing saying we couldn't prefer a different scope for MicroProfile.

rmannibucau commented 5 years ago

In cdi spec no expicit defined scope implies dependent, mp only uses cdi for scanning so all classses are cdi beans including jaxrs resources, put them together since you dont want to break user or go against jakartaee and you have your default well specified - even more than ee which opened the door to vendor specific mecanism preventing the portability in some (> cdi) cases.

Also this wouldnt be compatible with some config, jwtauth and failsafe feature so likely better to stick with already spec-ed defaults.

kenfinnigan commented 5 years ago

I didn't think JAX-RS Resource classes were automatically CDI beans unless a scope is defined.

I don't see any issues with MicroProfile diverging with what JavaEE/JakartaEE might define, if we think it's the right approach.

rmannibucau commented 5 years ago

Well for me it is a clear blocker since it means Microprofile does not respect the specifications it is based upon which should like not being consistent with itself and pretty much not usable (and doing reusable libraries will be impossible). So at the end Microprofile libs and code couldn't be deployed in a EE container without a huge impact. Lastly it does not bring any feature to the user even if I agree the default would have been better for perfs - not done cause the proxying rules are too restrictive to be general.

That said it is trivial to do with a CDI extension which just change the scope of any bean with @Path so maybe do it as an extension and that's it?

kenfinnigan commented 5 years ago

You appear to argue against a change and then say it's easy to do, so I'm confused.

All I'm saying is that if we as a community feel something other than @Dependent should be the scope for JAX-RS Resources, then there's nothing saying we can't do that

rmannibucau commented 5 years ago

@kenfinnigan it is easy to provide as a custom extension user can import or not, but shouldn't be in the platform IMO

kenfinnigan commented 5 years ago

Ok, that's your opinion, but I'd like to have the wider community comment before we close this

rdebusscher commented 5 years ago

I thought JAX-RS resources are non-contextual instances. http://www.next-presso.com/2017/06/non-contextual-instances-in-cdi/

Not created by CDI container (and thus not injectable somewhere else) but you can inject beans into it.

The JAX-RS spec also says "By default JAX-RS resource classes are per-request and all providers are singletons" which is comparable to @RequestScoped (but not the same as they aren't actual CDI beans)

kenfinnigan commented 5 years ago

I believe that's the current state.

I'm interested in seeing whether the community thinks @ApplicationScoped is a good default for JAX-RS Resources in MicroProfile

rdebusscher commented 5 years ago

There are conflicting situations when MicroPrpofile is implemented within Java EE container. What should the scope be then if both environments have different 'defaults'.

So if we should pick a CDI scope for MP, it should be @RequestScoped.

rmannibucau commented 5 years ago

If it is, it defeats goal of Microprofile (and JakartaEE) to make CDI centric all specs (which is very good for users an done of the reason of spring success in early days, IoC must be unique). Better to stick to what is in MP and therefore get instances thanks to CDI otherwise you have to reimplement a JAXRS container which supports all CDI features until the decorators which is pretty much complicated to understand and useless.

That said even in this case the default can't be @ApplicationScoped if it is not a CDI bean and the common default is to be "per call" (~ request scoped but using more a @Dependent like impl since it enable more programming models as mentionned earlier).

kenfinnigan commented 5 years ago

Right now I don't believe Java EE/Jakarta EE has any default for JAX-RS Resources that are not annotated as CDI beans, as you mentioned @rdebusscher.

This is one of the areas where MicroProfile needs a platform specification, as any "glue" that might be defined within a Java EE/Jakarta EE platform specification is not present in MicroProfile.

What reason is there that it can't be @ApplicationScoped?

Yes the default JAX-RS handling is per request creation, but in an era where we're trying to be more performant and not create instances without reason why would we stick with @RequestScoped?

rmannibucau commented 5 years ago

@kenfinnigan the reason is simple: whatever server using cdi or not will have a request related instantiation by default until you use spring which will get a singleton default. If MP forks from its own spec then its value is pretty null since you can't rely on the spec and MP would need to redefine CDI, JAX-RS, JSON-P, and JSON-B. Is it what you would like? rephrase all spec MP is supposed to use to change them for the details you disagree (and I can understand you do ;))?

kenfinnigan commented 5 years ago

A request related instantiation from JAX-RS is default, yes.

Changing that doesn't break CDI or JAX-RS, it layers atop the two.

rmannibucau commented 5 years ago

@kenfinnigan this is not true. The key feature of reusing specs is to be able to leverage their ecosystem. If you change defaults you break all the ecosystem "under" your own platform which means you could only leverage microprofile ecosystem which is nothing today compared to the jaxrs/cdi ecosystem and will likely not change since jakarta should get some cloud specs as well.

kenfinnigan commented 5 years ago

I disagree, but we all have different opinions

ljnelson commented 5 years ago

I have many, many thoughts on this. I'll "start simple".

First, I assume that MicroProfile is completely within its rights to do whatever it wants under section 2.3.3 of the JAX-RS 2.1 specification. We're talking not about what is possible, but what is desirable.

Second, I assume that the tension exhibited here is around what to do with (a) root resource classes that (b) are not annotated with any CDI annotations and (c) are not under the control of an Application, correct? (If an Application returns a POJO in the return value of its getSingletons() method, then obviously that POJO resource must be used "as is", i.e. it is an unmanaged instance from the standpoint of CDI injection.)

I'd like to have these assumptions confirmed or denied before I write more on the subject so I don't accidentally throw a lot of words on the page that turn out to be irrelevant.

rmannibucau commented 5 years ago

Point 2 is right Point 1 is not cause you reference the vendor extension point ignoring cdi references which delegate to cdi all the definitiin and MP is exactly there.

chkal commented 5 years ago

IMO @ApplicationScoped is a weird default for JAX-RS resources. Especially as you can use parameter bindings on resource fields like this:

@ApplicationScoped
@Path("hello/{name}")
public class HelloResource {

  @PathParam("name")
  private String name;

  @GET
  public String get() {
    return "Hello " + name;
  }

}

This would require quite some "magic" to work correctly.

mkarg commented 5 years ago

Quoting JAX-RS 2.1 specification chapter 3.1.1 Lifecycle and Environment: "By default a new resource class instance is created for each request to that resource". So @ApplicationScoped is not just a weird default, it is actually wrong default.

chkal commented 5 years ago

I would really be interested in the reasoning behind the idea of defining a new default. What problem are we trying to solve?

Emily-Jiang commented 5 years ago

I thought this a bit further. CDI spec does not say anything about JAX-RS, which is not a bad thing. Java EE spec only requires CDI-managed JAX-RS resources support injection and interceptor support. Since this minimal requirements, some application server provides more support, e.g automatically making JAX-RS as CDI beans to support injection. IIRC, we want to reach consensus among application servers. Should we spec clearly on the interaction between CDI and JAX-RS? In MP, we have 2 options:

  1. we could leave as it is and let Jakarta EE to clarify or JAX-RS 3.0 to fix this as per @chkal
  2. Fix Java EE spec and changes JAX-RS to force each JAX-RS resources to be managed by CDI via the notion of defaulting JAX-RS resources with a bean defining annotations such as ‘RequestScoped’. Since JAX-RS 3.0 is also addressing this issue. I think it is much safer to leave to Jakarta EE to fix this. Option 1 is safer and it will work better in the long run. Option 2 might bring more confusion and introduce potential conflicts as per Romain’s concern.
struberg commented 5 years ago

We need to go back a few meters and probably look where all this discussion started. I have the feeling that the completely side-tracked again.

The JAX-RS spec has a few discovery methods. Most of them do not apply to MicroProfile. The only one being a 'Managed Bean'. Within a jar without any META-INF/beans.xml this is only the case if it has a bean defining annotation as per the CDI spec. Which means you need to give this class a proper CDI scope. Like e.g. @ApplicationScoped or @RequestScoped. If you do have a beans.xml (explicit bean archive case), then almost every class gets picked up as CDI bean. In this case the default scope is @Dependent.

This is what the spec says right now. AND THIS IS PERFECTLY FINE - NO NEED TO CHANGE ANYTHING!

The only problem which happened is that Liberty seems to pick up classes without any CDI annotation nor beans.xml, as long as they have a @Path annotation. That's not by itself a problem, but someone wrote the TCK tests in some mp subproject to require this behaviour - which is simply not portable!

Of course Liberty can continue to pick those classes up. But we should stop writing non-portable TCK tests! For those we should either provide a proper META-INF/beans.xml or annotate the JAX-RS resources with some proper CDI scope. Case closed.

rmannibucau commented 5 years ago

If it helps: this is a common EE behavior to scan all classes outside CDI too but saner for MP to keep it vendor specific since impls diverge in the way they handle it - and it is fine.

ljnelson commented 5 years ago

Point 1 is not cause you reference the vendor extension point ignoring cdi references which delegate to cdi all the definitiin and MP is exactly there.

I am having a hard time understanding this sentence; I apologize.

Doesn't JAX-RS 2.1 section 2.3.3 give a non-Servlet-based vendor the freedom to "host a JAX-RS application in other types of container [sic]" in pretty much any way they see fit, given that (a) "such facilities are outside the scope of this specification" and (b) automatic discovery of root resource classes and provider classes is not required of non-Servlet-based vendors by the JAX-RS specification?

If indeed automatic discovery of root resource classes and provider classes is required of only Servlet-based implementations (2.3.2) but MicroProfile is not such a thing (2.3.3), and if MicroProfile is not Jakarta EE (it's not), and if "an implementation MAY offer other resource class lifecycles" (3.1.1) then if MicroProfile is going to integrate CDI with JAX-RS, can't MicroProfile do it any way it likes? That is my read of @kenfinnigan's proposal to have the (currently nonexistent but necessary) MicroProfile umbrella specification treat resource classes as though they were annotated with @ApplicationScoped for purposes of CDI integration. I am deliberately withholding judgment in this discussion so far on whether that's a good idea or a bad one—it seems like it must be a valid idea, however.

If you believe that MicroProfile is actually constrained in this regard, i.e. that MicroProfile cannot technically integrate JAX-RS 2.1 with CDI 2.0 in any way it likes, could you explain why, without referring to Jakarta EE?

rmannibucau commented 5 years ago

MP is not under 2.3.3 in jaxrs spec but all parts - there are multiple - referencing and relying on CDI. I dont refer about EE but only CDI and JAX RS saying scope is defined. EE just enforces it.

ljnelson commented 5 years ago

MP is not under 2.3.3 in jaxrs spec but all parts - there are multiple - referencing and relying on CDI.

Right; section 11.2.3 in particular, which says that "in a product that supports CDI" (that's MicroProfile) an implementation of JAX-RS "MUST support the use of CDI-style Beans [sic] as root resource classes, providers and Application subclasses. Providers and Application subclasses MUST be singletons or use application scope."

So fair enough, in the case of an Application subclass and any provider class that is instantiated by this implementation, or by the CDI-supporting product the implementation is subsumed under, such a class must be forced by the product or the implementation to be a singleton or to behave as though annotated with ApplicationScoped.

But there is no such restriction on root resource classes, right? So can't MicroProfile do what it wants here? I believe that is why Ken's proposal is not on the face of it invalid.

I dont refer about EE but only CDI and JAX RS saying scope is defined. EE just enforces it.

Could you point in the JAX-RS 2.1 specification where in fact the scope of a root resource class is defined in the context of a JAX-RS 2.1 implementation "belonging to" a product supporting CDI? I didn't find it in section 11.2.3 or section 11.2.8 which are the only two I could find that talk about "CDI-style Beans" (whatever those are, but that's a subject for a different day 😄 ).

rmannibucau commented 5 years ago

You answered yourself actually: the scope is not defined so it inherits from CDI @Dependent. Also please keep in mind it is like that for users (libraries and app writers) since JAX-RS 1.0 and works well. It also guarantees microprogike works (metrics and jwt auth are in my mind).

Now as Mark stated this is not this issue topic so let's agree cdi resources are discovered and other discovery mecanism are vendor specific and let's move on on features with some more value ;).

ljnelson commented 5 years ago

You answered yourself actually: the scope is not defined so it inherits from CDI @dependent.

I think the issue that Ken is raising (and I might still be misinterpreting him) is that an umbrella specification can still change this default without breaking anything in any of its subordinate specifications.

I'll detach from the discussion so as not to add more noise!

kenfinnigan commented 5 years ago

Thanks @ljnelson, you've represented my thoughts very well.

CDI specification defines that beans without a scope are @Dependent.

JAX-RS says Resources that are not added via the Application class will be instantiated per request. Those added via Application are singletons.

As others have said above, I don't see anything in either CDI or JAX-RS specifications that prevents MicroProfile defining a different scope than @Dependent for JAX-RS Resources.

Though the issue of discovering JAX-RS Resources in MicroProfile may have begun due to an issue in a TCK, that doesn't mean it's not a valid issue to be discussed.

I don't consider an argument that it's always been done that way as a reason to keep doing it that way when there are alternatives that could suit better.

With MicroProfile we're attempting to develop pieces that are appropriate for microservices and cloud development. For that reason, I don't see treating every JAX-RS Resource as @Dependent or even @RequestScoped as appropriate. In an age where memory is critical, with us trying to eek out more throughput for the same memory utilization, can we honestly say that instantiating an instance per request is the best choice?

In addition, defining @ApplicationScoped as a default would not only improve memory utilization, but also force developers to be better at developing thread safe code.

rmannibucau commented 5 years ago

....microprofile itself is a reason to not change scopes, please check other specs as mentionned. Also except perf which are not impacted enough to discuss something breaking the platform, it breaks usages. MP can choose to be unstable but if you fo ensure to do as much comm on that as all other communications to ensure people know it cant be chosen gor professional projects.

Contrarly to what you say it is in jaxrs spec that cdi default scope is dependent if you read it strictly: the bean is looked up per request, with no explicit scope it is dependent and therefore behaves as request scope without the proxying constraints. Easy and written as you point out.

Finally app scope would make default not thread safe anymore - and break existing code and ecosystem - so I fear you are not true at all on that too.

kenfinnigan commented 5 years ago

MicroProfile is planning to break backwards compatibility once a year, why should that be limited to the specifications that MicroProfile creates? There's no reason it can't be applied to how other specifications interact within MicroProfile.

As I pointed out in my last message, I don't think it's a bad thing to push developers to be thread safe. It improves the programming model and is more performant than just creating state everywhere. Yes it requires some forethought, but in my view that's not a bad thing.

At any rate, you can have your opinion that it shouldn't change, just as I can have a differing opinion. Neither is wrong or right, it's all about particular choices and tradeoffs.

None of this should preclude a discussion being had as to whether the wider community thinks such a change is appropriate or not.