Save the server-port of the original request by Bhashit · Pull Request #1865 · lift/framework

Bhashit · 2017-06-17T15:45:51Z

store the server-port of the original request since it's used in a lazy val, and Jetty will recycle the request instance if the lazy val is realized too late

… in a lazy val, and Jetty will recycle the request instance if the lazy val is realized too late

farmdawgnation

Looks good overall. One minor, opportunistic formatting change requested.

farmdawgnation · 2017-06-18T13:50:59Z

-  lazy val serverPort: Int = req.serverPort match {
-    case 80 => headers("X-SSL").flatMap(Helpers.asBoolean _).filter(a => a).map(a => 443).headOption getOrElse 80
+  lazy val serverPort: Int = _serverPort match {
+    case 80 => headers("X-SSL").flatMap(Helpers.asBoolean).filter(identity).map(a => 443).headOption getOrElse 80


While we're here, can we change this to be a bit more readable?

case 80 => headers("X-SSL") .flatMap(Helpers.asBoolean) .filter(identity) .map(a => 443) .headOption .getOrElse(80)

Also, what's the point of the filter?

Formatting done.

And about that filter call: It seems to exist because the headers method returns a List[String] and we want to check if there is any true value.

But, now that I looked into it a bit more, I think there might be something wrong with how the headers method is used here. It doesn't return header values, it just returns the names of the headers, which are never going to be converted to a true value and the result of this entire chain is always 80.

I am not sure if I should push a change here or create a ticket.

farmdawgnation · 2017-06-19T14:30:29Z

And about that filter call: It seems to exist because the headers method returns a List[String] and we want to check if there is any true value.

The filter call is probably there because the headers method returns one type and we need a different type. (e.g. got an Iterator need a List) Can you look into that?

But, now that I looked into it a bit more, I think there might be something wrong with how the headers method is used here. It doesn't return header values, it just returns the names of the headers, which are never going to be converted to a true value and the result of this entire chain is always 80.

Hm, that doesn't look good. I don't have time to look at it right now, but can you build out a unit test demonstrating that? If you're right, I think we should look to fix it here.

Bhashit · 2017-06-19T15:10:12Z

About the filter call, yeah, it is like that. I mean, just the way of checking if there really is a header named X-SSL.

About the headers method: I'll push some changes here later

Bhashit · 2017-06-25T04:55:14Z

@farmdawgnation Added a quick-n-dirty test, just for demo, to show that the current headers implementation is incorrect. Can be run with test-only net.liftweb.http.provider.servlet.OfflineRequestSnapshotSpec.

Also added a demo of a correct implementation. These are throwaway changes, and I'll clean them up if it looks okay to you.

Was also wondering why we are changing the server port of the original request anyway.

Shadowfiend

Left a couple of notes while I was looking around.

Shadowfiend · 2017-06-25T18:18:01Z

 }

-private class OfflineRequestSnapshot(req: HTTPRequest, val provider: HTTPProvider) extends HTTPRequest {
+private [servlet] class OfflineRequestSnapshot(req: HTTPRequest, val provider: HTTPProvider) extends HTTPRequest {


If we're making this private[servlet], I wonder if we just move it outside of HTTPRequestServlet and into its own file altogether? Not necessarily a pre-req for merging, just occurred to me while reading.

Sounds like a good idea.

Shadowfiend · 2017-06-25T18:24:53Z

+
+  "A snapshot request should" in {
+    val mockHttpRequest = mock[HTTPRequest]
+    val mockProvider = mock[HTTPProvider]


This could probably just be new HTTPProvider { val context = _ }---no huge difference, just a preference for using the language instead of mock magic when possible.

Setting the context value to _ results in unbound placeholder parameter. I think that only works for class level vars.

Setting context to null doesn't feel right to me. I also thought about setting it to an instance of net.liftweb.http.LiftFilter, which implements the HTTPProvider trait, but then it would look like it has some purpose. So, considering these two scenarios, I think mock is probably appropriate here?

Yeah, right you are! = null it would have to be.

It's not “right” in the sense that if this were live code it would be a bad idea. Since this is a test, I usually care a lot less.

Think of it this way: with the mock, if someone tried to call something on context, things would blow up (because there's no expectation set up for that mock). So in that sense it's the same as if you set a null there: interacting with the object in question will cause issues. But, we save ourselves whatever weird plumbing and other magic the mocking framework does, instead favoring straightforward language constructs.

Now that you said it, null seems okay. Pushed a change.

Shadowfiend · 2017-06-25T18:29:00Z

+
+object OfflineRequestSnapshotSpec extends WebSpec with Mockito {
+
+  "A snapshot request should" in {


This can be "A snapshot request" should {. Also, I would suggest folding in the name of the class under spec, i.e. "An OfflineRequestSnapshot" should { or even (my personal preference) "OfflineRequestSnapshot" should {. This makes it easier when seeing a failed test to know exactly what class failed.

Absolutely. I just wrote this up as a quick-n-dirty example to show the current implementation didn't work correctly. To be improved if approved. I guess this means I should go ahead and do the cleanup.

Bhashit · 2017-06-27T03:44:12Z

The Codacy issues are about converting private to private [this], and there's one about using that null we discussed about in a test.

Shadowfiend · 2017-06-27T14:40:35Z

Fwiw I do think the private[this] advice is worth following. Not a big deal though.

Bhashit · 2017-06-27T14:47:55Z

Yeah. In fact, I started using private[this] as a default for my current project from today. Ha ha. Will be making that change here as well.

Bhashit · 2017-06-27T15:39:45Z

Found the param method to be incorrect in the same manner that the header method was. So, corrected it as well.

farmdawgnation

Requesting some minor changes in the test because I was confused while reading it, but overall this is looking good.

farmdawgnation · 2017-07-03T14:25:06Z

+
+  private[this] val xSSLHeader = HTTPParam(X_SSL, List("true")) :: Nil
+
+  private[this] def getRequestSnapshot(originalPort: Int, headers: List[HTTPParam] = xSSLHeader, params: List[HTTPParam] = Nil) = {


headers should probably default to Nil otherwise it's not obvious in a casual reading of your tests that the SSL header is included.

farmdawgnation · 2017-07-03T14:25:52Z

+        val port80Req = getRequestSnapshot(originalPort = 80)
+        port80Req.serverPort shouldEqual 443
+      }
+      s"equal to the original request-port when the '$X_SSL' header is absent or not set to the string 'true' (case-insensitive), or if the original port is not 80" in {


This looks ripe for splitting into three separate specifications. :)

Set the default value for the `headers` param in `getRequestSnapshot` to Nil

farmdawgnation

This LGTM. I'll let it sit for a day or two to get any additional feedback from interested parties, and then I'll merge it. 👍

Thanks for the solid contribution!

Bhashit added 2 commits June 17, 2017 21:13

Eagerly store the server-port of the original request since it's used…

c1ae4c7

… in a lazy val, and Jetty will recycle the request instance if the lazy val is realized too late

Make a val private [this] (suggestion from codacy)

4690d9b

farmdawgnation requested changes Jun 18, 2017

View reviewed changes

Formatting

24ecc46

Throwaway test to demo incorrect impl.

d02d5ec

Removed an unneeded comment

dbf0a52

Shadowfiend reviewed Jun 25, 2017

View reviewed changes

Bhashit added 5 commits June 26, 2017 13:17

Move OfflineRequestSnapshot to its own file

6e1822e

Better tests

875a787

Clearer tests. Removed a placeholder/demo implementation

487628d

Replace the mock with a stub implementation

2af50bb

Removed an unused var, mistakenly added

5865ef9

Bhashit added 2 commits June 27, 2017 20:48

Change private to private[this]

9d63cad

Correct the implementation of the 'param' method, and add a test for it

17cc1d8

farmdawgnation requested changes Jul 3, 2017

View reviewed changes

farmdawgnation added this to the 3.2.0-M1 milestone Jul 3, 2017

Separate out the test for the headers into three seperate parts.

46bb3a8

Set the default value for the `headers` param in `getRequestSnapshot` to Nil

farmdawgnation approved these changes Jul 3, 2017

View reviewed changes

farmdawgnation merged commit 30d48e0 into lift:master Jul 10, 2017


		object OfflineRequestSnapshotSpec extends WebSpec with Mockito {

		"A snapshot request should" in {


		private[this] val xSSLHeader = HTTPParam(X_SSL, List("true")) :: Nil

		private[this] def getRequestSnapshot(originalPort: Int, headers: List[HTTPParam] = xSSLHeader, params: List[HTTPParam] = Nil) = {

Uh oh!

Conversation

Bhashit commented Jun 17, 2017

Uh oh!

farmdawgnation left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

farmdawgnation commented Jun 19, 2017

Uh oh!

Bhashit commented Jun 19, 2017

Uh oh!

Bhashit commented Jun 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shadowfiend left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bhashit Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bhashit commented Jun 27, 2017

Uh oh!

Shadowfiend commented Jun 27, 2017

Uh oh!

Bhashit commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bhashit commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

farmdawgnation left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

farmdawgnation left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Bhashit commented Jun 25, 2017 •

edited

Loading

Bhashit Jun 26, 2017 •

edited

Loading

Bhashit commented Jun 27, 2017 •

edited

Loading

Bhashit commented Jun 27, 2017 •

edited

Loading