Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -178,21 +178,31 @@ class HTTPRequestServlet(val req: HttpServletRequest, val provider: HTTPProvider
}
}

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a good idea.


private val _cookies = List(req.cookies :_*)

private val _headers = List(req.headers :_*)

private val _params = List(req.params :_*)

private [this] val _serverPort = req.serverPort


def cookies: List[HTTPCookie] = _cookies

val authType: Box[String] = req.authType

def headers(name: String): List[String] = _headers.filter(_.name == name).map(_.name)

// Just as a sample, to be replaced later
def _newheaders(name: String): List[String] = {
_headers
.find(_.name.equalsIgnoreCase(name))
.map(_.values)
.getOrElse(Nil)
}

def headers: List[HTTPParam] = _headers

val contextPath: String = req.contextPath
Expand Down Expand Up @@ -234,8 +244,14 @@ private class OfflineRequestSnapshot(req: HTTPRequest, val provider: HTTPProvide

val scheme: String = req.scheme

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(_ => 443)
.headOption
.getOrElse(80)
case x => x
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright 2010-2011 WorldWide Conferencing, LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package net.liftweb.http.provider.servlet

import net.liftweb.http.provider._
import net.liftweb.mockweb.WebSpec
import org.mockito.Mockito._
import org.specs2.mock.Mockito


object OfflineRequestSnapshotSpec extends WebSpec with Mockito {

"A snapshot request should" in {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

val mockHttpRequest = mock[HTTPRequest]
val mockProvider = mock[HTTPProvider]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@Bhashit Bhashit Jun 26, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

val headers = HTTPParam("X-SSL", List("true")) :: Nil
when(mockHttpRequest.headers).thenReturn(headers)
when(mockHttpRequest.cookies).thenReturn(Nil)
when(mockHttpRequest.params).thenReturn(Nil)
when(mockHttpRequest.serverPort).thenReturn(80)
val snapshotReq = new OfflineRequestSnapshot(mockHttpRequest, mockProvider)

"have a headers method that returns the list of headers with a given name" in {
snapshotReq.headers("X-SSL") shouldEqual List("true")

// this test shouldn't be successful
snapshotReq.headers("X-SSL") shouldEqual List("X-SSL")
}

"the new headers implementation should work correctly" in {
snapshotReq._newheaders("X-SSL") shouldEqual List("true")
}

"return server-port 443 when the X-SSL header is present" in {
snapshotReq.serverPort shouldEqual 443
}
}

}