Fix: Use check instead of checkAll (#1266)

* replaced with in tests where the generator was non-deterministic, fixed URL spec. * updated on Cookie * updated cookie implementation to fix failing tests. There is one breaking change on the Cookie API - Cookie.decodeRequestCookie is returning LIst[String] * removed debug code * in case of empty AsciiString the ByteBuf should be empty * rolled back previous change. According to http spec header SHOULD be present in case the conntent is empty.
zio · May 26, 2022 · 1cb0b4b · 1cb0b4b
1 parent b04e7f5
commit 1cb0b4b
Show file tree

Hide file tree

Showing 10 changed files with 55 additions and 40 deletions.
diff --git a/zio-http/src/main/scala/zhttp/http/Cookie.scala b/zio-http/src/main/scala/zhttp/http/Cookie.scala
@@ -124,8 +124,8 @@ final case class Cookie(
    */
   def encode: String = {
     val c = secret match {
-      case Some(sec) => content + "." + signContent(sec)
-      case None      => content
+      case Some(sec) if sec.nonEmpty => content + "." + signContent(sec)
+      case _                         => content
     }
 
     val cookie = List(
@@ -267,10 +267,20 @@ object Cookie {
           sameSite = Option(sameSite),
         )
       else
-        null
+        Cookie(
+          "",
+          "",
+          expires = Option(expires),
+          maxAge = maxAge,
+          domain = Option(domain),
+          path = Option(path),
+          isSecure = secure,
+          isHttpOnly = httpOnly,
+          sameSite = Option(sameSite),
+        )
 
     secret match {
-      case Some(s) => {
+      case Some(s) if s.nonEmpty => {
         if (decodedCookie != null) {
           val index     = decodedCookie.content.lastIndexOf('.')
           val signature = decodedCookie.content.slice(index + 1, decodedCookie.content.length)
@@ -281,25 +291,27 @@ object Cookie {
           else null
         } else decodedCookie
       }
-      case None    => decodedCookie
+      case _                     => decodedCookie.copy(secret = secret)
     }
 
   }
 
   /**
    * Decodes from `Cookie` header value inside of Request into a cookie
    */
-  def decodeRequestCookie(headerValue: String): Option[List[Cookie]] = {
-    val cookies: Array[String]  = headerValue.split(';').map(_.trim)
-    val x: List[Option[Cookie]] = cookies.toList.map(a => {
-      val (name, content) = splitNameContent(a)
-      if (name.isEmpty && content.isEmpty) None
-      else Some(Cookie(name, content))
-    })
-
-    if (x.contains(None))
-      None
-    else Some(x.map(_.get))
+  def decodeRequestCookie(headerValue: String): List[Cookie] = {
+    if (headerValue.nonEmpty) {
+      val cookies: Array[String]  = headerValue.split(';').map(_.trim)
+      val x: List[Option[Cookie]] = cookies.toList.map(a => {
+        val (name, content) = splitNameContent(a)
+        if (name.isEmpty && content.isEmpty) Some(Cookie("", ""))
+        else Some(Cookie(name, content))
+      })
+
+      if (x.contains(None))
+        List.empty
+      else x.map(_.get)
+    } else List.empty
   }
 
   @inline
@@ -308,7 +320,7 @@ object Cookie {
     if (i >= 0) {
       (str.substring(0, i).trim, str.substring(i + 1).trim)
     } else {
-      (str.trim, null)
+      (str.trim, "")
     }
   }
 

diff --git a/zio-http/src/main/scala/zhttp/http/URL.scala b/zio-http/src/main/scala/zhttp/http/URL.scala
@@ -76,6 +76,13 @@ final case class URL(
     copy(kind = location)
   }
 
+  def isEqual(other: URL): Boolean = {
+    self.kind == other.kind &&
+    self.path == other.path &&
+    (self.queryParams.toSet diff other.queryParams.toSet).isEmpty
+    self.fragment == other.fragment
+  }
+
   private[zhttp] def relative: URL = self.kind match {
     case URL.Location.Relative => self
     case _                     => self.copy(kind = URL.Location.Relative)
@@ -164,4 +171,5 @@ object URL {
       decoded <- Option(uri.getFragment)
     } yield Fragment(raw, decoded)
   }
+
 }
diff --git a/zio-http/src/main/scala/zhttp/http/headers/HeaderGetters.scala b/zio-http/src/main/scala/zhttp/http/headers/HeaderGetters.scala
@@ -163,10 +163,7 @@ trait HeaderGetters[+A] { self =>
 
   final def cookiesDecoded: List[Cookie] =
     headerValues(HeaderNames.cookie).flatMap { header =>
-      Cookie.decodeRequestCookie(header) match {
-        case None       => Nil
-        case Some(list) => list
-      }
+      Cookie.decodeRequestCookie(header)
     }
 
   final def date: Option[CharSequence] =

diff --git a/zio-http/src/main/scala/zhttp/service/EncodeRequest.scala b/zio-http/src/main/scala/zhttp/service/EncodeRequest.scala
@@ -26,9 +26,7 @@ trait EncodeRequest {
       }
 
       val writerIndex = content.writerIndex()
-      if (writerIndex != 0) {
-        headers.set(HttpHeaderNames.CONTENT_LENGTH, writerIndex.toString)
-      }
+      headers.set(HttpHeaderNames.CONTENT_LENGTH, writerIndex.toString)
 
       // TODO: we should also add a default user-agent req header as some APIs might reject requests without it.
       val jReq = new DefaultFullHttpRequest(jVersion, method, path, content)

diff --git a/zio-http/src/test/scala/zhttp/html/DomSpec.scala b/zio-http/src/test/scala/zhttp/html/DomSpec.scala
@@ -1,7 +1,7 @@
 package zhttp.html
 
 import zio.random.Random
-import zio.test.{DefaultRunnableSpec, Gen, assertTrue, checkAll}
+import zio.test.{DefaultRunnableSpec, Gen, assertTrue, check, checkAll}
 
 object DomSpec extends DefaultRunnableSpec {
   def spec = suite("DomSpec") {
@@ -80,7 +80,7 @@ object DomSpec extends DefaultRunnableSpec {
           }
         } +
           testM("not void") {
-            checkAll(tagGen) { name =>
+            check(tagGen) { name =>
               val dom = Dom.element(name)
               assertTrue(dom.encode == s"<${name}></${name}>")
             }

diff --git a/zio-http/src/test/scala/zhttp/http/CookieSpec.scala b/zio-http/src/test/scala/zhttp/http/CookieSpec.scala
@@ -8,7 +8,7 @@ object CookieSpec extends DefaultRunnableSpec {
   def spec = suite("Cookies") {
     suite("response cookies") {
       testM("encode/decode signed/unsigned cookies with secret") {
-        checkAll(HttpGen.cookies) { cookie =>
+        check(HttpGen.cookies) { cookie =>
           val cookieString = cookie.encode
           assert(Cookie.decodeResponseCookie(cookieString, cookie.secret))(isSome(equalTo(cookie))) &&
           assert(Cookie.decodeResponseCookie(cookieString, cookie.secret).map(_.encode))(isSome(equalTo(cookieString)))
@@ -17,13 +17,13 @@ object CookieSpec extends DefaultRunnableSpec {
     } +
       suite("request cookies") {
         testM("encode/decode multiple cookies with ZIO Test Gen") {
-          checkAll(for {
+          check(for {
             name         <- Gen.anyString
             content      <- Gen.anyString
             cookieList   <- Gen.listOf(Gen.const(Cookie(name, content)))
             cookieString <- Gen.const(cookieList.map(x => s"${x.name}=${x.content}").mkString(";"))
           } yield (cookieList, cookieString)) { case (cookies, message) =>
-            assert(Cookie.decodeRequestCookie(message))(isSome(equalTo(cookies)))
+            assert(Cookie.decodeRequestCookie(message))(equalTo(cookies))
           }
         }
       }

diff --git a/zio-http/src/test/scala/zhttp/http/HttpDataSpec.scala b/zio-http/src/test/scala/zhttp/http/HttpDataSpec.scala
@@ -19,7 +19,7 @@ object HttpDataSpec extends DefaultRunnableSpec {
         suite("encode")(
           suite("fromStream") {
             testM("success") {
-              checkAllM(Gen.anyString) { payload =>
+              checkM(Gen.anyString) { payload =>
                 val stringBuffer    = payload.getBytes(HTTP_CHARSET)
                 val responseContent = ZStream.fromIterable(stringBuffer)
                 val res             = HttpData.fromStream(responseContent).toByteBuf.map(_.toString(HTTP_CHARSET))

diff --git a/zio-http/src/test/scala/zhttp/http/URLSpec.scala b/zio-http/src/test/scala/zhttp/http/URLSpec.scala
@@ -51,10 +51,10 @@ object URLSpec extends DefaultRunnableSpec {
 
     suite("asString")(
       testM("using gen") {
-        checkAll(HttpGen.url) { case url =>
+        check(HttpGen.url) { case url =>
           val source  = url.encode
-          val decoded = URL.fromString(source).map(_.encode)
-          assert(decoded)(isRight(equalTo(source)))
+          val decoded = URL.fromString(source)
+          assert(decoded.map(_.isEqual(url)))(isRight(equalTo(true)))
         }
       } +
         test("empty") {

diff --git a/zio-http/src/test/scala/zhttp/service/SSLSpec.scala b/zio-http/src/test/scala/zhttp/service/SSLSpec.scala
@@ -10,7 +10,7 @@ import zio.ZIO
 import zio.duration.durationInt
 import zio.test.Assertion.equalTo
 import zio.test.TestAspect.{ignore, timeout}
-import zio.test.{DefaultRunnableSpec, Gen, assertM, checkAllM}
+import zio.test.{DefaultRunnableSpec, Gen, assertM, checkM}
 
 object SSLSpec extends DefaultRunnableSpec {
   val env = EventLoopGroup.auto() ++ ChannelFactory.auto ++ ServerChannelFactory.auto
@@ -68,7 +68,7 @@ object SSLSpec extends DefaultRunnableSpec {
               assertM(actual)(equalTo(Status.PermanentRedirect))
             } +
             testM("Https request with a large payload should respond with 413") {
-              checkAllM(payload) { payload =>
+              checkM(payload) { payload =>
                 val actual = Client
                   .request(
                     "https://localhost:8073/text",

diff --git a/zio-http/src/test/scala/zhttp/service/ServerSpec.scala b/zio-http/src/test/scala/zhttp/service/ServerSpec.scala
@@ -159,7 +159,7 @@ object ServerSpec extends HttpRunnableSpec {
       Response.text(req.contentLength.getOrElse(-1).toString)
     }
     testM("has content-length") {
-      checkAllM(Gen.alphaNumericString) { string =>
+      checkM(Gen.alphaNumericString) { string =>
         val res = app.deploy.bodyAsString.run(content = HttpData.fromString(string))
         assertM(res)(equalTo(string.length.toString))
       }
@@ -173,7 +173,7 @@ object ServerSpec extends HttpRunnableSpec {
 
   def responseSpec = suite("ResponseSpec") {
     testM("data") {
-      checkAllM(nonEmptyContent) { case (string, data) =>
+      checkM(nonEmptyContent) { case (string, data) =>
         val res = Http.fromData(data).deploy.bodyAsString.run()
         assertM(res)(equalTo(string))
       }
@@ -200,7 +200,7 @@ object ServerSpec extends HttpRunnableSpec {
 
       } +
       testM("header") {
-        checkAllM(HttpGen.header) { case header @ (name, value) =>
+        checkM(HttpGen.header) { case header @ (name, value) =>
           val res = Http.ok.addHeader(header).deploy.headerValue(name).run()
           assertM(res)(isSome(equalTo(value)))
         }
@@ -271,14 +271,14 @@ object ServerSpec extends HttpRunnableSpec {
       val app: Http[Any, Throwable, Request, Response] = Http.collect[Request] { case req =>
         Response(data = HttpData.fromStream(req.bodyAsStream))
       }
-      checkAllM(Gen.alphaNumericString) { c =>
+      checkM(Gen.alphaNumericString) { c =>
         assertM(app.deploy.bodyAsString.run(path = !!, method = Method.POST, content = HttpData.fromString(c)))(
           equalTo(c),
         )
       }
     } +
       testM("FromASCIIString: toHttp") {
-        checkAllM(Gen.anyASCIIString) { payload =>
+        checkM(Gen.anyASCIIString) { payload =>
           val res = HttpData.fromAsciiString(AsciiString.cached(payload)).toHttp.map(_.toString(HTTP_CHARSET))
           assertM(res.run())(equalTo(payload))
         }