Jakarta REST 5.0 Epic - Githubissues

spericas commented 1 year ago

/The proposal is carry out all this work in the release-4.0 branch (we can rename this branch later if necessary). After checking out that branch, review presentation in JakartaRest40.pdf for some background info on the 4.0 release.

General

[x] Clean pom files, use dependency and plugin management, verify all versions
[x] Add static dependency with CDI
[x] Update README, LICENSE and review all other top-level files
[ ] Explore better integration with Jakarta Concurrency
[ ] Does Jakarta REST support CDI Lite in addition to CDI Full?
[ ] Are CDI executable methods usable by Jakarta REST implementations? (https://github.com/jakartaee/cdi/pull/639)
[ ] Review copyrights and licenses in all files (especially pom files)

API

[ ] Remove all use of @Context and @Suspended and related classes
[ ] Review support for Application subclasses: zero or more? zero or one?
[ ] If no Application subclasses, use CDI for scanning
[ ] New @Body annotation

Specification

[ ] Remove all uses of @Context and @AsyncResponse and related classes
[ ] Rewrite introduction to describe new API philosophy and integration with CDI
[ ] Define CDI scopes for resources, providers and applications

Examples

[ ] Verify all examples are updated with new API, no @Context etc.
[ ] Add new examples that show CDI integration
[ ] Remove any examples that are not longer relevant

TCKs

[ ] Port all TCK tests to run with new 5.0 API
[ ] New TCK tests to better show integration with CDI

jamezp commented 1 year ago

Is there an issue filed about removing @AsyncResponse? I couldn't find anything in my search, but I may have just missed it.

mkarg commented 1 year ago

Is there an issue filed about removing @AsyncResponse? I couldn't find anything in my search, but I may have just missed it.

I am not aware that we decided to do so.

@spericas Why using a new branch? Why not developing on master branch?

spericas commented 1 year ago

Is there an issue filed about removing @AsyncResponse? I couldn't find anything in my search, but I may have just missed it.

I am not aware that we decided to do so.

There's no longer a need for the annotation if @Entity is introduced as you an see in the PDF file. The param type is sufficient to determine the type of call.

@spericas Why using a new branch? Why not developing on master branch?

We can switch after the TCK work (challenges) is complete.

spericas commented 1 year ago

Is there an issue filed about removing @AsyncResponse? I couldn't find anything in my search, but I may have just missed it.

I don't think so. Note that it's easy to create issues by clicking on one of the epic bullets above ...

jamezp commented 1 year ago

Is there an issue filed about removing @AsyncResponse? I couldn't find anything in my search, but I may have just missed it.

I don't think so. Note that it's easy to create issues by clicking on one of the epic bullets above ...

Sorry for the delay. I'm not sure I fully grok this part yet so I need to do some reading.

Overall though, I think the proposal makes sense. I've got some CDI questions, especially around clients, but that seems more of a fit for CDI specific issue. I need to re-read that issue anyway since it's been a while :)

mkarg commented 1 year ago

Top-level files will be fine when my recently opened PRs are resolved. Checking the issue already, as there are no top-level files to be modified.

Update: PR was merged meanwhile.

WhiteCat22 commented 1 year ago

Hi all, I'm looking for some more information about the new @Entity annotation.

I can't find any discussion around it other than "Use of new annotation @Entity is required" in @spericas presentation PDF.

This seems to just make the method signature more verbose without adding any additional functionality. Is this correct? Or does this annotation impact additional behavior that I am unaware of?

jamezp commented 1 year ago

@WhiteCat22 I could be wrong, but my understanding is we need a way to indicate which part of a method signature is the entity because the @Context and @Suspend annotations are being removed. In the document the following example is used:

@POST
@Path("async")
@Consumes(MediaType.TEXT_PLAIN)
public void putMessageAsync(
  @QueryParam("override") boolean override,
  @Entity String message,
  AsyncResponse ar) {
    Executors.newSingleThreadExecutor().submit(() -> {
        // …
        ar.resume("Done");
    });
}

Since @Suspend is not longer available, we need the @Entity to determine which parameter is the entity.

In some ways it could be considered less verbose because @Context and @Suspend will not be required as arguments for things like Sse, SseEvent, AsyncResponse, etc.

spericas commented 1 year ago

@WhiteCat22 @jamezp Not just that, with CDI taking control of the method call, developers will be able to inject any CDI bean as a parameter, but only one of those injections will correspond to the request entity.

WhiteCat22 commented 1 year ago

Makes sense! Thank you!

WhiteCat22 commented 1 year ago

Is removal of the JAXB dependency related to this issue?

Should Remove all use of @Context and @AsyncResponse and related classes read Remove all use of @Context and @Suspended and related classes?

spericas commented 1 year ago

Is removal of the JAXB dependency related to this issue?

Sort of, it is more related to 4.0 in general, not directly related to the CDI work.

Should Remove all use of @Context and @AsyncResponse and related classes read Remove all use of @Context and @Suspended and related classes?

Fixed, thanks.

WhiteCat22 commented 1 year ago

Hi all, after presenting RESTFulWS-4.0 to the rest of the Open Liberty organization @jim-krueger and I received feedback on the new @Entity annotation. I opened https://github.com/jakartaee/rest/issues/1183 for discussion.

jim-krueger commented 11 months ago

There is action item above for adding TCK for the new Multipart function added in version 3.1. Should we also add an action item to add TCK for the requirement of the default exception mapper in 3.1? Issue 858. I don't believe any new tests were added.

spericas commented 11 months ago

There is action item above for adding TCK for the new Multipart function added in version 3.1. Should we also add an action item to add TCK for the requirement of the default exception mapper in 3.1? Issue 858. I don't believe any new tests were added.

Sure, I'm not sure either if tested in the TCK. Opening an issue for investigation seems like the right step.

jim-krueger commented 11 months ago

Sure, I'm not sure either if tested in the TCK. Opening an issue for investigation seems like the right step.

Agreed. Created Issue 1188 and updated checklist on top.

jim-krueger commented 11 months ago

With the Milestone 1 deadline approaching December 5th. Are there thoughts on what things above must be in the spec/api's for M1 and what things are optional for that deadline?

spericas commented 11 months ago

@jim-krueger I'd say optional would be TCKs and integrations with other specs, mostly everything else should be there. I'd say going over the spec and updating it to reflect the changes in 4.0 would be high priority. If someone can volunteer for that, it would be very helpful.

jim-krueger commented 10 months ago

@spericas I made a first pass at some spec changes for M1. Please give it a review when you can. Thanks. https://github.com/jakartaee/rest/pull/1192

jakartaee / rest

Jakarta REST 5.0 Epic #1158