eclipse-ee4j / jersey

Eclipse Jersey Project - Read our Wiki:
https://github.com/eclipse-ee4j/jersey/wiki
Other
681 stars 339 forks source link

HttpServletResponse#setStatus(int sc, String sm) no longer called #5682

Closed rw7 closed 1 week ago

rw7 commented 2 weeks ago

This method is deprecated since servlet-api 2.1. This method was dropped from servlet-api 6. Calling it renders jersey 2.x unusable on Tomcat 10.1 with the jakartaee-migration tool: https://github.com/apache/tomcat-jakartaee-migration

This commit is partial cherry-pick of: 4977fecc8704dd2178dacf0ba9590f2463f8accb Author: jansupol jan.supol@oracle.com Date: Mon Feb 21 18:23:20 2022 +0100 Replace removed API in Servlet 6

jansupol commented 2 weeks ago

Sorry, Jersey 2.x is only targeted for Tomcat 9. For Tomcat 10.0, please use Jersey 3.0.x For Tomcat 10.1, please use Jersey 3.1.x For Tomcat 11, please use Jersey 4.0.x

rw7 commented 2 weeks ago

Too bad. I have an application here with a myriad of dependencies. So far, that jakartaee-migration tool dealt miraculously well with all of them, except jersey. Updating to Jersey 3.1.x causes another load of problems with no end in sight.

jansupol commented 2 weeks ago

The migration-tool is used for conversion between Jakarta EE 8 and Jakarta EE 9. But Tomcat 10.1 is Jakarta EE 10, which has the Servlet6. Maybe you can try with Tomcat 10.0.x which has the Servlet 5 (EE9) and would not cause problems with #setStatus.

jansupol commented 2 weeks ago

I assume NoSuchMethodError is thrown in Tomcat 10.1?

rw7 commented 2 weeks ago

Yes.

java.lang.NoSuchMethodError: 'void jakarta.servlet.http.HttpServletResponse.setStatus(int, java.lang.String)'
    at org.glassfish.jersey.servlet.internal.ResponseWriter.writeResponseStatusAndHeaders(ResponseWriter.java:147)
    at org.glassfish.jersey.server.ServerRuntime$Responder$1.getOutputStream(ServerRuntime.java:639)
    at org.glassfish.jersey.message.internal.CommittingOutputStream.commitStream(CommittingOutputStream.java:171)
    at org.glassfish.jersey.message.internal.CommittingOutputStream.flushBuffer(CommittingOutputStream.java:276)
    at org.glassfish.jersey.message.internal.CommittingOutputStream.commit(CommittingOutputStream.java:232)
    at org.glassfish.jersey.message.internal.CommittingOutputStream.close(CommittingOutputStream.java:247)
    at org.glassfish.jersey.message.internal.OutboundMessageContext.close(OutboundMessageContext.java:865)
    at org.glassfish.jersey.server.ContainerResponse.close(ContainerResponse.java:403)
    at org.glassfish.jersey.server.ServerRuntime$Responder.writeResponse(ServerRuntime.java:721)
    at org.glassfish.jersey.server.ServerRuntime$Responder.processResponse(ServerRuntime.java:380)
    at org.glassfish.jersey.server.ServerRuntime$Responder.process(ServerRuntime.java:370)
    at org.glassfish.jersey.server.ServerRuntime$1.run(ServerRuntime.java:259)
    at org.glassfish.jersey.internal.Errors$1.call(Errors.java:248)
    at org.glassfish.jersey.internal.Errors$1.call(Errors.java:244)
    at org.glassfish.jersey.internal.Errors.process(Errors.java:292)
    at org.glassfish.jersey.internal.Errors.process(Errors.java:274)
    at org.glassfish.jersey.internal.Errors.process(Errors.java:244)
    at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:265)
    at org.glassfish.jersey.server.ServerRuntime.process(ServerRuntime.java:235)
    at org.glassfish.jersey.server.ApplicationHandler.handle(ApplicationHandler.java:684)
    at org.glassfish.jersey.servlet.WebComponent.serviceImpl(WebComponent.java:394)
    at org.glassfish.jersey.servlet.WebComponent.service(WebComponent.java:346)
    at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:358)
    at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:311)
    at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:205)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:195)
rw7 commented 2 weeks ago

About Tomcat 10.0.x: This is already end-of-life: https://tomcat.apache.org/whichversion.html

Also, Debian does not provide a 10.0.x out of the box, just 9.0.x and 10.1.x.

rw7 commented 2 weeks ago

I'm really don't know much about neither jersey nor servlet api. But when looking at 4977fecc8704dd2178dacf0ba9590f2463f8accb I see: This reasonPhrase was just ommitted, not put anywhere else instead. Also, not a single test had to be adjusted to reflect that changed (actually reduced) functionality. So this reasonPhrase seems to be superfluous, even on servlet api 4 or earlier.

And the function in question was deprecated with servlet api 2.1, the "First official specification" at all, released in 1998: https://en.wikipedia.org/wiki/Jakarta_Servlet

senivam commented 2 weeks ago

in the PR you are doing the Servlet API is from Servelt 4th:

import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;
import javax.servlet.ServletContext;
import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

The Tomcat 10.1 bundles Servlet 6 API:

import jakarta.servlet.Filter;
import jakarta.servlet.FilterChain;
import jakarta.servlet.FilterConfig;
import jakarta.servlet.ServletContext;
import jakarta.servlet.ServletException;
import jakarta.servlet.ServletRequest;
import jakarta.servlet.ServletResponse;
import jakarta.servlet.http.HttpServlet;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;

This won't work anyway even if the PR would be merged. The application should be properly migrated (even manually) with respect to changes that have occurred since the 2.x version. It is important to remember, that Jersey 2.x will not work (without proper hacks) on the Tomcat 10.1. If the migration is at least successful for Jersey 3.0 + Tomcat 10.0, it is possible to use that Tomcat even if it has reached its end of life meanwhile migrating to Jersey 3.1

jansupol commented 2 weeks ago

We know that the reason phrase is very important for the customer use-cases and we had multiple complaints about not using/propagating the reason phrase in EE 10 (as well as in EE 9 Jetty which dropped the reason phrase in the middle of the product lifecycle breaking backward compatibility), so we know we cannot just drop the reason-phrases.

The migration-tool really would not help in the cases where new API methods are used instead of the old ones, or the old methods are dropped. That would be the case not only for the Servlet itself, but also for the 3rd party dependencies brought in to Tomcat.

In the simplest use-cases, the tool might work for Tomcat 10.1, though. In theory, the no-reason method invocations could be wrapped into catch (NoSuchMethodError) as a fallback.

rw7 commented 1 week ago

Just to let you know, it works:

I have my web application running nicely on Tomcat 10.1. without upgrading any of my dependencies. So no jersey 2.x->3.1.x, no Apache Wicket 9->10, etc. I just sent all the dependencies through jakartaee-migration. And of course I had to apply this MR to jersey.

And I had to apply this to jakartaee-migration and use it, otherwise the migration tool migrates too much.

Of course, sooner or later I will upgrade to jersey 3.1.x, as I will upgrade to Apache Wicket 10 etc. But I will tackle these upgrades one by one, in my own pace.

And to make sure I used "Find Usages" in IDEA to look for any usages of any method dropped from servlet api 6. I did not find any such usages, except in jersey what is removed by this PR. Not only in the project I'm converting right now, but also in all other projects my colleagues are currently working on. Makes sense to me, since these methods are deprecated for 26 years.

So I still think, this PR would be good to merge. But if you don't agree, you may just close it.

jansupol commented 1 week ago

We may not drop the ReasonPhrase. No matter the API String status message argument was likely meant for a different message than a reason phrase.

Closing in favor of #5688.