Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated Jetty dependency and a test broken by Jetty changes. #698

Merged
merged 3 commits into from Apr 11, 2017

Conversation

jakaarl
Copy link
Contributor

@jakaarl jakaarl commented Oct 31, 2016

See issue: #694

pom.xml Outdated
@@ -30,7 +30,7 @@

<properties>
<java.version>1.8</java.version>
<jetty.version>9.3.6.v20151106</jetty.version>
<jetty.version>9.3.13.v20161014</jetty.version>
Copy link

Choose a reason for hiding this comment

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

9.4.0.v20161208 is out now.

@ph-hs
Copy link

ph-hs commented Dec 20, 2016

9.4.0.v20161208 is out now

@jakaarl
Copy link
Contributor Author

jakaarl commented Dec 20, 2016

@ph-hs I noticed, started updating the pull request last night, but there were complications.

@paul-hammant
Copy link

It is interesting that it is only the test that's broken with the Jetty upgrade. In real life, you can do the maven thing and deploy Spark with the 9.4.x of Jetty quite happily.

@paul-hammant
Copy link

paul-hammant commented Dec 20, 2016

The bits that you're stuck on could be, solved by:

-        WebSocketUpgradeFilter webSocketUpgradeFilter =
-                (WebSocketUpgradeFilter) servletContextHandler.getAttribute("org.eclipse.jetty.websocket.server.WebSocketUpgradeFilter");
+        WebSocketUpgradeFilter webSocketUpgradeFilter = (WebSocketUpgradeFilter) servletContextHandler.getServletHandler().getFilter("Jetty_WebSocketUpgradeFilter").getFilter();

and

-        WebSocketServerFactory webSocketServerFactory = webSocketUpgradeFilter.getFactory();
+        WebSocketServerFactory webSocketServerFactory = webSocketUpgradeFilter.getConfiguration().getFactory();

and

-         MappedResource<WebSocketCreator> mappedResource = webSocketUpgradeFilter.getMappings().getMatch("/websocket");
+        MappedResource<WebSocketCreator> mappedResource = webSocketUpgradeFilter.getConfiguration().getMatch("/websocket");

@jakaarl
Copy link
Contributor Author

jakaarl commented Dec 20, 2016

Thanks, I'll take a look tonight. I got burned by not getting the upgrade filter from the attributes and didn't have the time/energy to research into it last night.

@ph-hs
Copy link

ph-hs commented Dec 27, 2016

One of the unrelated tests is now failing:

testDirectoryTraversalProtectionLocal(spark.staticfiles.StaticFilesTest)  Time elapsed: 0.13 sec  <<< FAILURE!
java.lang.AssertionError: expected:<404> but was:<400>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:645)
	at org.junit.Assert.assertEquals(Assert.java:631)
	at spark.staticfiles.StaticFilesTest.testDirectoryTraversalProtectionLocal(StaticFilesTest.java:145)

Needs to be investigated, I think.

@jakaarl
Copy link
Contributor Author

jakaarl commented Jan 17, 2017

Seems the unexpected response comes all the way from Jetty:
WARN org.eclipse.jetty.http.HttpParser - bad HTTP parsed: 400 Bad URI for HttpChannelOverHttp@4db7b425{r=1,c=false,a=IDLE,uri=//localhost:4567/..%5Cspark%5CSpark.class}

@tipsy
Copy link
Contributor

tipsy commented Apr 8, 2017

@jakaarl would be great if you could have a look at this again.
Edit: I changed the behavior of the director-traversal code to return 400, which seems more appropriate anyway. Hopefully I'm not hiding a bug now.
Thanks @jakaarl and @paul-hammant

@tipsy tipsy merged commit 5becfeb into perwendel:master Apr 11, 2017
tipsy added a commit that referenced this pull request Apr 23, 2017
Issue #698 - Upgrade to Jetty 9.4.4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants