[stapler#114] Update to Servlet API 3.1.0 #131

jtnord · 2017-09-20T20:12:42Z

Update stapler to use Servlet API 3.1.0 (and other servlet APIs coming from Java EE 7)

Fixes #114

manual testing using the example maven project
corresponding PR to Jenkins incorporating the change in a real project ([JENKINS-37062] incorporate changes from stapler 1.253 (servlet 3.1) jenkins#3033)

Code could be cleaned up further with the inclusion of #130

Proposed Change Log Entry

updated API to compiled against Java Servlet API 3.1.0, JSP API 2.3.0 and JSTL API 1.2.1
This required a API signature change of org.kohsuke.stapler.compression.FilterServletOutputStream.
- It is recommended that users of this class do not upgrade but replace the implementation as this call may be removed in a future version.

@reviewbybees

This also updates the JSP api to 2.30 as it is also part of Java EE 7 and so will be found in tandem with the servlet API.

Mainly updates jetty and stapler to versions that use servlet 3.1.0 API

jtnord · 2017-09-20T20:15:09Z

core/maven-example/pom.xml

@@ -45,8 +46,8 @@
        <artifactId>maven-compiler-plugin</artifactId>

        <configuration>
-          <source>1.5</source>
-          <target>1.5</target>
+          <source>1.7</source>


minimum target of java ee 7

works for us

I think I remember a discussion somewhere: is stapler stil in-use outside Jenkins? If not, and even if so, why not go to Java 8? It's probably time anyway, as now JDK9 just got GA a few days ago, even JDK8 is legacy now ;-).

It is in a few places (2 or 3 iirc) 7 is required for the servlet spec which is the min requirement here

Stapler is on Java 8 already.

jtnord · 2017-09-20T20:15:37Z

core/maven-example/pom.xml

@@ -13,24 +13,25 @@
    <dependency>
      <groupId>org.kohsuke.stapler</groupId>
      <artifactId>stapler-jsp</artifactId>
-      <version>1.144</version>
+      <!-- TODO update after releasing -->
+      <version>1.253-SNAPSHOT</version>


not part of the build so would need to be fixed later.

ghost · 2017-09-20T20:16:12Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

jtnord · 2017-09-20T20:18:14Z

core/maven-example/pom.xml

        </configuration>
      </plugin>
    </plugins>
  </build>

-  <repositories>


stapler is now on maven central

jtnord · 2017-09-20T20:19:33Z

core/src/main/java/org/kohsuke/stapler/ResponseImpl.java

@@ -305,7 +305,7 @@ public OutputStream getCompressedOutputStream(HttpServletRequest req) throws IOE
        // CompressionFilter not available, so do it on our own.
        // see CompressionFilter for why this is not desirable
        setHeader("Content-Encoding","gzip");
-        return recordOutput(new FilterServletOutputStream(new GZIPOutputStream(super.getOutputStream())));
+        return recordOutput(new FilterServletOutputStream(new GZIPOutputStream(super.getOutputStream()), super.getOutputStream()));


this is pretty ugly - there is no need for stapler to do gzip at all - see #130

currently only 2 uses outside of stapler that I found - I have PRs for both, both of which means this can be completely removed in #130

jtnord · 2017-09-20T20:20:54Z

core/src/main/java/org/kohsuke/stapler/StaplerResponseWrapper.java

@@ -340,4 +341,46 @@ public void setContentType(String type) {
    public void setLocale(Locale loc) {
        getWrapped().setLocale(loc);
    }
+
+    /** {@inheritDoc} */


new APIs in ServletResponse

jtnord · 2017-09-20T20:21:32Z

core/src/main/java/org/kohsuke/stapler/compression/CompressionServletResponse.java

@@ -58,7 +58,7 @@ public ServletOutputStream getOutputStream() throws IOException {
    public void activate() throws IOException {
        if (stream==null) {
            super.setHeader("Content-Encoding", "gzip");
-            stream = new FilterServletOutputStream(new GZIPOutputStream(super.getOutputStream()));
+            stream = new FilterServletOutputStream(new GZIPOutputStream(super.getOutputStream()), stream);


this is pretty ugly - there is no need for stapler to do gzip at all - see #130

jtnord · 2017-09-20T20:21:51Z

core/src/main/java/org/kohsuke/stapler/compression/FilterServletOutputStream.java

+     * @param out the stream that sits above the realStream, performing some filtering.  This must be eventually delegating eventual writes to {@code realStream}.
+     * @param realStream the actual underlying ServletOutputStream from the container.  Used to check the {@link #isReady()} state and to add {@link WriteListener}s. 
+     */
+    public FilterServletOutputStream(OutputStream out, ServletOutputStream realStream) {


this is pretty ugly - there is no need for stapler to do gzip at all - see #130

The usage of this in Jenkins core shows that this is not appropriate there.
I found 2 external uses of this
1 in jenkins core
1 in a propriatary plugin

in both cases it is a mock stream, that does not respond to the client at all and as such has a different use case to this.

Downstream updates from jenkinsci/stapler#131 The stapler API was built using servlet 2.5 and yet Jenkins uses 3.1. This meant some of the code that stapler used was missing new methods that where part of the updated spec and if a plugin happened to call them you would end up with a LinkageError or some other crazyness. This also make Jenkins depend on a dummy version of the old serlet-api maven co-ordinates such that if any plugin gets aa dependency on it transitivly (e.g. Jenkins test harness) they will get a version with no code so the classpath should always be clean (this was more an issue for eclipse than an mvn command)

oleg-nenashev

Generally looks good to me, especially since Jenkins was able to work with Stapler without blowing up.

@jtnord Have you checked compatibility of other projects within the Stapler organization? Maybe they require update before merging

oleg-nenashev · 2017-09-24T10:32:40Z

core/maven-example/pom.xml

@@ -45,8 +46,8 @@
        <artifactId>maven-compiler-plugin</artifactId>

        <configuration>
-          <source>1.5</source>
-          <target>1.5</target>
+          <source>1.7</source>


works for us

oleg-nenashev · 2017-09-24T10:33:41Z

core/maven-example/pom.xml

-
+        <groupId>org.eclipse.jetty</groupId>
+        <artifactId>jetty-maven-plugin</artifactId>
+        <version>9.4.7.v20170914</version>


CC @olamy just in case he is aware about any issues with that

nope. I cannot see any issue with that.

FWIW we are using 9.4.5.v20170502 in various other places in the Jenkins project.

jtnord · 2017-09-25T09:55:16Z

Have you checked compatibility of other projects within the Stapler organization? Maybe they require update before merging

@oleg-nenashev No, updated this repo and the test project only. I did search for the class that had an API signature change and found only 2 occurences of it - neither of which where in the stapler org. If you are aware of any other tests I should do can you outline them? (I ran the example project mvn jetty:run and verified it started up and returned information for various pages)

oleg-nenashev · 2017-09-25T10:14:50Z

@jtnord It should be fine. It would be great if you reference the changed methods in the PR description, so that we could mention it in the changelogs and upgrade guidelines if anything goes wrong.

jtnord · 2017-09-25T11:25:43Z

@oleg-nenashev updated the PR description to add a proposed change-log

oleg-nenashev

🐝

jglick · 2017-09-26T21:59:38Z

core/maven-example/pom.xml

@@ -45,8 +46,8 @@
        <artifactId>maven-compiler-plugin</artifactId>

        <configuration>
-          <source>1.5</source>
-          <target>1.5</target>
+          <source>1.7</source>


Stapler is on Java 8 already.

jglick · 2017-09-26T22:00:54Z

core/maven-example/pom.xml

-
+        <groupId>org.eclipse.jetty</groupId>
+        <artifactId>jetty-maven-plugin</artifactId>
+        <version>9.4.7.v20170914</version>


FWIW we are using 9.4.5.v20170502 in various other places in the Jenkins project.

jglick · 2017-09-26T22:02:57Z

groovy/pom.xml

-      <artifactId>servlet-api</artifactId>
-      <version>2.3</version>
+      <artifactId>javax.servlet-api</artifactId>
+      <version>3.1.0</version>


Please use <dependencyManagement> to avoid repetition.

jtnord added 3 commits September 20, 2017 20:35

[stapler#114] Update POM dependenceies to servet API 3.1.0

a03554c

This also updates the JSP api to 2.30 as it is also part of Java EE 7 and so will be found in tandem with the servlet API.

[Stapler#114] implement new required APIs for Servlet 3.1 API

259cf32

[stapler#114] Update the example for Servelt 3.1.0

ce6bda2

Mainly updates jetty and stapler to versions that use servlet 3.1.0 API

jtnord commented Sep 20, 2017

View reviewed changes

core/maven-example/pom.xml

</configuration>

</plugin>

</plugins>

</build>

<repositories>

Copy link

Member Author

jtnord Sep 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stapler is now on maven central

jtnord commented Sep 20, 2017

View reviewed changes

jglick self-requested a review September 21, 2017 15:22

jtnord mentioned this pull request Sep 22, 2017

[JENKINS-37062] incorporate changes from stapler 1.253 (servlet 3.1) jenkinsci/jenkins#3033

Merged

4 tasks

oleg-nenashev reviewed Sep 24, 2017

View reviewed changes

oleg-nenashev approved these changes Sep 25, 2017

View reviewed changes

jglick approved these changes Sep 26, 2017

View reviewed changes

jglick merged commit dc2afaa into jenkinsci:master Oct 10, 2017

jtnord deleted the servlet_3.x branch October 10, 2017 18:34

jglick mentioned this pull request Oct 25, 2017

Stapler 1.253 jenkinsci/jenkins#3103

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stapler#114] Update to Servlet API 3.1.0 #131

[stapler#114] Update to Servlet API 3.1.0 #131

jtnord commented Sep 20, 2017 •

edited

Loading

jtnord Sep 20, 2017

oleg-nenashev Sep 24, 2017

batmat Sep 25, 2017

jtnord Sep 25, 2017

jglick Sep 26, 2017

jtnord Sep 20, 2017

ghost commented Sep 20, 2017

jtnord Sep 20, 2017

jtnord Sep 20, 2017

jtnord Sep 22, 2017 •

edited

Loading

jtnord Sep 20, 2017

jtnord Sep 20, 2017

jtnord Sep 20, 2017

jtnord Sep 22, 2017

oleg-nenashev left a comment

oleg-nenashev Sep 24, 2017

oleg-nenashev Sep 24, 2017

olamy Sep 24, 2017 •

edited

Loading

jglick Sep 26, 2017

jtnord commented Sep 25, 2017

oleg-nenashev commented Sep 25, 2017

jtnord commented Sep 25, 2017

oleg-nenashev left a comment

jglick Sep 26, 2017

jglick Sep 26, 2017

jglick Sep 26, 2017

[stapler#114] Update to Servlet API 3.1.0 #131

[stapler#114] Update to Servlet API 3.1.0 #131

Conversation

jtnord commented Sep 20, 2017 • edited Loading

Proposed Change Log Entry

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olamy Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord commented Sep 25, 2017

oleg-nenashev commented Sep 25, 2017

jtnord commented Sep 25, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord commented Sep 20, 2017 •

edited

Loading

jtnord Sep 22, 2017 •

edited

Loading

olamy Sep 24, 2017 •

edited

Loading