jakartaee / rest

Jakarta RESTful Web Services
Other
351 stars 114 forks source link

Deprecate both @Context and @Suspended #1210

Closed jim-krueger closed 5 months ago

jim-krueger commented 5 months ago

Fixes #1209 Deprecate @Context and @Suspended in release-3.2 in preparation for removal in release-4.0

mkarg commented 5 months ago

This PR is a violation of the committer rules of this project if we take them word-by-word: https://github.com/jakartaee/rest/wiki/Committer-Conventions. There were no three +1 votes, there was no two-weeks voting time. We need to discuss if we set up lower barriers for 3.2 (which I am +1 actually; one way could be to extend the rules by the notion of "working branches", hence release-3.2 would be merged into a "releasing-branch" (like 3.2) later using a separate PR). Until then I would kindly ask you to respect the rules. Thanks.

jim-krueger commented 5 months ago

My apologies Markus. I was thinking of release-3.2 as a "working branch" since we do not have a release plan etc. for it yet and Santiago had requested that we determine if certain issues could be worked through concerning deprecation prior to taking that step. I should have stated that clearly. That being said it sounds like you are not adverse to proceeding this way until such a time as 3.2 is "official", at which time the content of PRs already applied could be confirmed via a vote, with the content removed if found to not be acceptable.

mkarg commented 5 months ago

I am not standing in the way of dealing with release-3.2 as a "working branch" in that sense, and I think nobody else would consider to reject that idea. Nevertheless it would be best to have an official rule. Hence I would encourage you to start a voting on the mailing list to get this new exemption added into our official ruleset.

paulrutter commented 3 months ago

Just came across this PR while moving to 3.1. What is the designated replacement for @Suspended? Is it no longer needed, as using AsyncResponse is enough? See https://github.com/jakartaee/rest/issues/1158#issuecomment-1701361549.

I know that @Context is replaced with @Inject, to align with CDI. Thanks.

@jim-krueger

jamezp commented 3 months ago

@paulrutter Effectively it will be the same, @Inject, but AIUI implied on the method. In other words you simply just remove the @Suspended annotation from your parameter and the implementation will resolve the value.

As an example:

@POST
@Path("async")
@Consumes(MediaType.TEXT_PLAIN)
public void putMessageAsync(
    @QueryParam("override") boolean override,
    String message,
    @Suspended AsyncResponse ar) {
    managedExecutor.submit() -> {
        ar.resume("done");
    });
}

Would become:

@POST
@Path("async")
@Consumes(MediaType.TEXT_PLAIN)
public void putMessageAsync(
    @QueryParam("override") boolean override,
    @Body String message,
    AsyncResponse ar) {
    managedExecutor.submit() -> {
        ar.resume("done");
    });
}
paulrutter commented 3 months ago

Thanks for clarifying, that makes perfect sense.

paulrutter commented 3 months ago

@jim-krueger one more question about @Context being removed, how would the following example look like after it has been removed?

    @Path("/resetpassword")
    @POST
    @Consumes(APPLICATION_FORM_URLENCODED)
    @Produces(APPLICATION_JSON)
    public Response requestResetPassword(@FormParam(USERNAME) final String username,
            @Context final HttpServletRequest request) {
        // do something with the HttpServletRequest
    }

Is it still possible to get a handle on the HttpServletRequest after @Context has been sunset?

jim-krueger commented 3 months ago

@paulrutter This is still a work in progress, but I believe the goal is for it to look like this:

@Path("/resetpassword")
    @POST
    @Consumes(APPLICATION_FORM_URLENCODED)
    @Produces(APPLICATION_JSON)
    public Response requestResetPassword(@FormParam(USERNAME) final String username,
            final HttpServletRequest request) {
        // do something with the HttpServletRequest
    }

You'll want to keep an eye on Issue 1214 as that is where this problem is being tracked.

paulrutter commented 3 months ago

Thanks, will do.