Use ws(s) scheme instead of http(s) when calculating effective websocket request URIs #910

aruediger · 2017-02-28T09:44:10Z

fixes #909

…ive websocket request URIs

akka-ci · 2017-02-28T09:49:40Z

Test PASSed.

aruediger · 2017-02-28T10:02:22Z

akka-http-core/src/main/scala/akka/http/scaladsl/model/Uri.scala

+          case ' ' if replaceSpaces ⇒
+            r ~~ '+'; 1
+          case c if c <= 127 && asciiCompatible ⇒
+            appendEncoded(c.toByte); 1


maybe these braces were present for a reason. 🤔

…ocket request URIs

akka-ci · 2017-03-06T12:51:17Z

Test FAILed.

jrudolph · 2017-03-08T12:40:32Z

Wow, validation ran for 46 hours before it was interrupted. Let's try again...

And, sorry for not getting to review this any sooner, @2Beaucoup.

jrudolph · 2017-03-08T12:40:36Z

PLS BUILD

akka-ci · 2017-03-08T12:52:00Z

Test PASSed.

aruediger · 2017-03-12T20:47:54Z

akka-http-core/src/main/scala/akka/http/scaladsl/model/HttpMessage.scala

+        false
+      }
+      val defaultScheme =
+        if (isWebsocket) (if (securedConnection) "wss" else "ws")


Could also add Uri.websocketScheme.

…scheme

aruediger · 2017-04-20T11:40:22Z

@akka/akka-http-team any opinions on this one?

jlprat

not really into the topic, but it LGTM

jlprat · 2017-04-20T12:54:50Z

akka-http-core/src/test/scala/akka/http/scaladsl/model/HttpMessageSpec.scala

+      test("/segment", "http://example.com/segment", Host("example.com"))
+      test("http://example.com/", "http://example.com/", Host("example.com"))
+      test("http://example.com:8080/", "http://example.com:8080/", Host("example.com", 8080))
+      test("/websocket", "ws://example.com/websocket", Host("example.com"), Upgrade(List(UpgradeProtocol("websocket"))))


Are there any the checks for https if so, you could add one also for wss?

jrudolph

Sorry @2Beaucoup for keeping that one hanging for so long. I agree with the goal of providing an effective URI for all kinds of requests.

As commented it would be nice not to introduce any extra processing to the common path.

jrudolph · 2017-05-08T11:16:26Z

akka-http-core/src/main/scala/akka/http/scaladsl/model/HttpMessage.scala

@@ -365,7 +376,9 @@ object HttpRequest {
        case 0 ⇒ // ok
        case 4 if c(0) == 'h' && c(1) == 't' && c(2) == 't' && c(3) == 'p' ⇒ // ok
        case 5 if c(0) == 'h' && c(1) == 't' && c(2) == 't' && c(3) == 'p' && c(4) == 's' ⇒ // ok
-        case _ ⇒ throw new IllegalArgumentException("""`uri` must have scheme "http", "https" or no scheme""")
+        case 2 if c(0) == 'w' && c(1) == 's' ⇒ // ok
+        case 3 if c(0) == 'w' && c(1) == 's' && c(2) == 's' ⇒ // ok


Scaladoc could be adapted to include "ws" and "wss" as well.

jrudolph · 2017-05-08T11:22:07Z

akka-http-core/src/main/scala/akka/http/scaladsl/model/HttpMessage.scala

+        false
+      }
+      val defaultScheme =
+        if (isWebsocket) (if (securedConnection) "wss" else "ws")


jrudolph · 2017-05-08T11:25:36Z

akka-http-core/src/main/scala/akka/http/scaladsl/model/HttpMessage.scala

        case OptionVal.None                 ⇒ if (defaultHostHeader.isEmpty) fail("is missing a `Host` header") else defaultHostHeader
        case OptionVal.Some(x) if x.isEmpty ⇒ if (defaultHostHeader.isEmpty) fail("an empty `Host` header") else defaultHostHeader
        case OptionVal.Some(x)              ⇒ x
      }
-      uri.toEffectiveHttpRequestUri(host, port, securedConnection)
+      def isWebsocket: Boolean = {


Could we somehow prevent the extra scan through the header list? I'm not sure how critical it is but would be nice to prevent that every request has to go through it. (In the middle-term, we should probably try to evolve the HttpRequest model to include those kind of flags as I started to discuss in #991.)

aruediger

@jrudolph updated. thanks for taking a look!

aruediger · 2017-05-10T07:37:06Z

akka-http-core/src/main/scala/akka/http/scaladsl/model/HttpMessage.scala

@@ -365,7 +376,9 @@ object HttpRequest {
        case 0 ⇒ // ok
        case 4 if c(0) == 'h' && c(1) == 't' && c(2) == 't' && c(3) == 'p' ⇒ // ok
        case 5 if c(0) == 'h' && c(1) == 't' && c(2) == 't' && c(3) == 'p' && c(4) == 's' ⇒ // ok
-        case _ ⇒ throw new IllegalArgumentException("""`uri` must have scheme "http", "https" or no scheme""")
+        case 2 if c(0) == 'w' && c(1) == 's' ⇒ // ok
+        case 3 if c(0) == 'w' && c(1) == 's' && c(2) == 's' ⇒ // ok


akka-ci · 2017-05-10T07:44:57Z

Test FAILed.

jrudolph · 2017-05-16T14:07:07Z

PLS BUILD

jrudolph · 2017-05-16T14:09:24Z

akka-http-core/src/main/scala/akka/http/scaladsl/model/HttpMessage.scala

-      OptionVal.None
-    }
-    val hostHeader: OptionVal[Host] = findHost(headers)
+    @tailrec def findHostAndWsUpgrade(it: Iterator[HttpHeader], host: OptionVal[Host] = OptionVal.None, wsUpgrade: Option[Boolean] = None): (OptionVal[Host], Boolean) =


wsUpgrade can be a Boolean.

This way it returns earlier if Upgrade.hasWebsocket == false but that's rather uncommon. WDYT?

Ah, I see. Yes, let's keep it that way.

jrudolph

One small nitpick, otherwise, LGTM

akka-ci · 2017-05-16T14:15:00Z

Test FAILed.

jrudolph · 2017-05-16T16:10:35Z

PLS BUILD

akka-ci · 2017-05-16T16:22:37Z

Test PASSed.

jrudolph

LGTM

…fective websocket request URIs (akka#910)

+htc #909 use ws(s) scheme instead of http(s) when calculating effect…

b3191eb

…ive websocket request URIs

aruediger force-pushed the 2beaucoup/ws-scheme branch from efbbd5b to b3191eb Compare February 28, 2017 09:44

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels Feb 28, 2017

aruediger commented Feb 28, 2017

View reviewed changes

aruediger requested a review from jrudolph March 2, 2017 10:33

use ws(s): scheme instead of http(s) when calculationg effective webs…

d901d78

…ocket request URIs

akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Mar 4, 2017

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Mar 8, 2017

aruediger commented Mar 12, 2017

View reviewed changes

Merge branch 'master' of github.com:akka/akka-http into 2beaucoup/ws-…

1359da6

…scheme

jlprat reviewed Apr 20, 2017

View reviewed changes

jrudolph reviewed May 8, 2017

View reviewed changes

aruediger added 3 commits May 10, 2017 09:26

iterate just once to find Host and Upgrade to WS header

3f0bb87

add Uri.websocketScheme()

678eeba

fix styling

d568055

aruediger commented May 10, 2017

View reviewed changes

akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels May 10, 2017

jrudolph reviewed May 16, 2017

View reviewed changes

akka-ci added validating PR that is currently being validated by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels May 16, 2017

jrudolph reviewed May 16, 2017

View reviewed changes

akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR that is currently being validated by Jenkins labels May 16, 2017

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels May 16, 2017

jrudolph approved these changes Jun 12, 2017

View reviewed changes

jrudolph merged commit ffe450d into master Jun 12, 2017

jrudolph deleted the 2beaucoup/ws-scheme branch June 12, 2017 14:29

tomrf1 pushed a commit to tomrf1/akka-http that referenced this pull request Aug 13, 2017

+htc akka#909 Use ws(s) scheme instead of http(s) when calculating ef…

3e0d0cf

…fective websocket request URIs (akka#910)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ws(s) scheme instead of http(s) when calculating effective websocket request URIs #910

Use ws(s) scheme instead of http(s) when calculating effective websocket request URIs #910

aruediger commented Feb 28, 2017 •

edited

Loading

akka-ci commented Feb 28, 2017

aruediger Feb 28, 2017

akka-ci commented Mar 6, 2017

jrudolph commented Mar 8, 2017

jrudolph commented Mar 8, 2017

akka-ci commented Mar 8, 2017

aruediger Mar 12, 2017

jrudolph May 8, 2017

aruediger commented Apr 20, 2017

jlprat left a comment

jlprat Apr 20, 2017

jrudolph left a comment

jrudolph May 8, 2017

aruediger May 10, 2017

jrudolph May 8, 2017

jrudolph May 8, 2017

aruediger left a comment

aruediger May 10, 2017

akka-ci commented May 10, 2017

jrudolph commented May 16, 2017

jrudolph May 16, 2017

aruediger May 16, 2017

jrudolph Jun 12, 2017

jrudolph left a comment

akka-ci commented May 16, 2017

jrudolph commented May 16, 2017

akka-ci commented May 16, 2017

jrudolph left a comment

Use ws(s) scheme instead of http(s) when calculating effective websocket request URIs #910

Use ws(s) scheme instead of http(s) when calculating effective websocket request URIs #910

Conversation

aruediger commented Feb 28, 2017 • edited Loading

akka-ci commented Feb 28, 2017

Choose a reason for hiding this comment

akka-ci commented Mar 6, 2017

jrudolph commented Mar 8, 2017

jrudolph commented Mar 8, 2017

akka-ci commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aruediger commented Apr 20, 2017

jlprat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrudolph 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

Choose a reason for hiding this comment

aruediger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented May 10, 2017

jrudolph commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrudolph left a comment

Choose a reason for hiding this comment

akka-ci commented May 16, 2017

jrudolph commented May 16, 2017

akka-ci commented May 16, 2017

jrudolph left a comment

Choose a reason for hiding this comment

aruediger commented Feb 28, 2017 •

edited

Loading