openwhisk-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From csantan...@apache.org
Subject [incubator-openwhisk] branch master updated: Change response when entity is empty from 200 to 204. (#2676)
Date Thu, 07 Sep 2017 15:08:46 GMT
This is an automated email from the ASF dual-hosted git repository.

csantanapr pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-openwhisk.git


The following commit(s) were added to refs/heads/master by this push:
     new 4c6688b  Change response when entity is empty from 200 to 204. (#2676)
4c6688b is described below

commit 4c6688bfac2e0939b20b7e1f5fba13973b242959
Author: rodric rabbah <rodric@gmail.com>
AuthorDate: Thu Sep 7 11:08:44 2017 -0400

    Change response when entity is empty from 200 to 204. (#2676)
    
    This allows empty json response with http actions.
    Treat the empty string and a null JSON value as empty content.
    Add empty body tests for http web actions where request specifies an accept header.
    Disable content negotiation on empty response.
    Fix web action status code response in test files.
    Add tests for handling partial result for http actions.
    Update web action doc.
---
 .../scala/whisk/core/controller/RestAPIs.scala     |   3 +-
 .../scala/whisk/core/controller/WebActions.scala   |  31 ++--
 docs/webactions.md                                 |  10 +-
 tests/dat/actions/corsHeaderMod.js                 |   2 +-
 tests/dat/actions/multipleHeaders.js               |   2 +-
 .../core/controller/test/WebActionsApiTests.scala  | 159 +++++++++++++++++++--
 6 files changed, 176 insertions(+), 31 deletions(-)

diff --git a/core/controller/src/main/scala/whisk/core/controller/RestAPIs.scala b/core/controller/src/main/scala/whisk/core/controller/RestAPIs.scala
index d5526b8..acda09d 100644
--- a/core/controller/src/main/scala/whisk/core/controller/RestAPIs.scala
+++ b/core/controller/src/main/scala/whisk/core/controller/RestAPIs.scala
@@ -205,8 +205,7 @@ class RestAPIVersion(config: WhiskConfig, apiPath: String, apiVersion:
String)(
   private val triggers = new TriggersApi(apiPath, apiVersion)
   private val activations = new ActivationsApi(apiPath, apiVersion)
   private val rules = new RulesApi(apiPath, apiVersion)
-  private val WebApiDirectives = new WebApiDirectives()
-  private val web = new WebActionsApi(Seq("web"), this.WebApiDirectives)
+  private val web = new WebActionsApi(Seq("web"), new WebApiDirectives())
 
   class NamespacesApi(val apiPath: String, val apiVersion: String)(
     implicit override val entityStore: EntityStore,
diff --git a/core/controller/src/main/scala/whisk/core/controller/WebActions.scala b/core/controller/src/main/scala/whisk/core/controller/WebActions.scala
index f893c54..10e1f7c 100644
--- a/core/controller/src/main/scala/whisk/core/controller/WebActions.scala
+++ b/core/controller/src/main/scala/whisk/core/controller/WebActions.scala
@@ -43,6 +43,7 @@ import akka.http.scaladsl.model.ContentTypes
 import akka.http.scaladsl.model.FormData
 import akka.http.scaladsl.model.HttpMethods.{DELETE, GET, HEAD, OPTIONS, PATCH, POST, PUT}
 import akka.http.scaladsl.model.HttpCharsets
+import akka.http.scaladsl.model.HttpResponse
 
 import spray.json._
 import spray.json.DefaultJsonProtocol._
@@ -222,6 +223,8 @@ protected[core] object WhiskWebActionsApi extends Directives {
         case _ => throw new Throwable("Invalid header")
       } getOrElse List()
 
+      val body = fields.get("body")
+
       val code = fields.get(rp.statusCode).map {
         case JsNumber(c) =>
           // the following throws an exception if the code is
@@ -229,25 +232,29 @@ protected[core] object WhiskWebActionsApi extends Directives {
           StatusCode.int2StatusCode(c.toIntExact)
 
         case _ => throw new Throwable("Illegal code")
-      } getOrElse (OK)
-
-      fields.get("body") map {
-        case JsString(str) => interpretHttpResponse(code, headers, str, transid)
-        case js            => interpretHttpResponseAsJson(code, headers, js, transid)
-      } getOrElse {
-        respondWithHeaders(removeContentTypeHeader(headers)) {
-          // note that if header defined a content-type, it will be ignored
-          // since the type must be compatible with the data response
-          complete(code, HttpEntity.Empty)
-        }
       }
+
+      body.collect {
+        case JsString(str) if str.nonEmpty   => interpretHttpResponse(code.getOrElse(OK),
headers, str, transid)
+        case JsString(str) /* str.isEmpty */ => respondWithEmptyEntity(code.getOrElse(NoContent),
headers)
+        case js if js != JsNull              => interpretHttpResponseAsJson(code.getOrElse(OK),
headers, js, transid)
+      } getOrElse respondWithEmptyEntity(code.getOrElse(NoContent), headers)
+
     } getOrElse {
-      // either the result was not a JsObject or there was an exception validting the
+      // either the result was not a JsObject or there was an exception validating the
       // response as an http result
       terminate(BadRequest, Messages.invalidMedia(`message/http`))(transid, jsonPrettyPrinter)
     }
   }
 
+  private def respondWithEmptyEntity(code: StatusCode, headers: List[RawHeader]) = {
+    respondWithHeaders(removeContentTypeHeader(headers)) {
+      // note that if header defined a content-type, it will be ignored
+      // since the type must be compatible with the data response
+      complete(HttpResponse(code, entity = HttpEntity.Empty))
+    }
+  }
+
   private def headersFromJson(k: String, v: JsValue): Seq[RawHeader] = v match {
     case JsString(v)  => Seq(RawHeader(k, v))
     case JsBoolean(v) => Seq(RawHeader(k, v.toString))
diff --git a/docs/webactions.md b/docs/webactions.md
index 4be9ef5..b389a8c 100644
--- a/docs/webactions.md
+++ b/docs/webactions.md
@@ -116,12 +116,12 @@ It is important to be aware of the [response size limit](reference.md)
for actio
 An OpenWhisk action that is not a web action requires both authentication and must respond
with a JSON object. In contrast, web actions may be invoked without authentication, and may
be used to implement HTTP handlers that respond with _headers_, _statusCode_, and _body_ content
of different types. The web action must still return a JSON object, but the OpenWhisk system
(namely the `controller`) will treat a web action differently if its result includes one or
more of the following as to [...]
 
 1. `headers`: a JSON object where the keys are header-names and the values are string, number,
or boolean values for those headers (default is no headers). To send multiple values for a
single header, the header's value should be a JSON array of values.
-2. `statusCode`: a valid HTTP status code (default is 200 OK).
-3. `body`: a string which is either plain text or a base64 encoded string for binary data
(default is empty response).
+2. `statusCode`: a valid HTTP status code (default is 200 OK if body is not empty otherwise
204 No Content).
+3. `body`: a string which is either plain text, JSON object or array, or a base64 encoded
string for binary data (default is empty response).
 
-The controller will pass along the action-specified headers, if any, to the HTTP client when
terminating the request/response. Similarly the controller will respond with the given status
code when present. Lastly, the body is passed along as the body of the response. Unless a
`content-type header` is declared in the action result’s `headers`, the body is passed along
as is if it’s a string (or results in an error otherwise). When the `content-type` is defined,
the controller will determi [...]
+The `body` is considered empty if it is `null`, the empty string `""` or undefined.
 
-_Note_: A JSON object or array is treated as binary data and must be base64 encoded as shown
in the example above.
+The controller will pass along the action-specified headers, if any, to the HTTP client when
terminating the request/response. Similarly the controller will respond with the given status
code when present. Lastly, the body is passed along as the body of the response. If a `content-type
header` is not declared in the action result’s `headers`, the body is interpreted as `application/json`
for non-string values, and `text/html` otherwise. When the `content-type` is defined, the
controller  [...]
 
 ## HTTP Context
 
@@ -131,7 +131,7 @@ All web actions, when invoked, receives additional HTTP request details
as param
 2. `__ow_headers` (type: map string to string): A the request headers.
 3. `__ow_path` (type: string): the unmatched path of the request (matching stops after consuming
the action extension).
 4. `__ow_user` (type: string): the namespace identifying the OpenWhisk authenticated subject
-5. `__ow_body` (type: string): the request body entity, as a base64 encoded string when content
is binary, or plain string otherwise
+5. `__ow_body` (type: string): the request body entity, as a base64 encoded string when content
is binary or JSON object/array, or plain string otherwise
 6. `__ow_query` (type: string): the query parameters from the request as an unparsed string
 
 A request may not override any of the named `__ow_` parameters above; doing so will result
in a failed request with status equal to 400 Bad Request.
diff --git a/tests/dat/actions/corsHeaderMod.js b/tests/dat/actions/corsHeaderMod.js
index a658160..df9b704 100644
--- a/tests/dat/actions/corsHeaderMod.js
+++ b/tests/dat/actions/corsHeaderMod.js
@@ -7,6 +7,6 @@ function main() {
             "Location": "openwhisk.org",
             "Set-Cookie": "cookie-cookie-cookie"
         },
-        code: 200
+        statusCode: 200
     }
 }
diff --git a/tests/dat/actions/multipleHeaders.js b/tests/dat/actions/multipleHeaders.js
index 06712c1..f19e236 100644
--- a/tests/dat/actions/multipleHeaders.js
+++ b/tests/dat/actions/multipleHeaders.js
@@ -3,6 +3,6 @@ function main() {
         headers: {
             "Set-Cookie": ["a=b", "c=d"]
         },
-        code: 200
+        statusCode: 200
     }
 }
diff --git a/tests/src/test/scala/whisk/core/controller/test/WebActionsApiTests.scala b/tests/src/test/scala/whisk/core/controller/test/WebActionsApiTests.scala
index fba43f3..f43ed4a 100644
--- a/tests/src/test/scala/whisk/core/controller/test/WebActionsApiTests.scala
+++ b/tests/src/test/scala/whisk/core/controller/test/WebActionsApiTests.scala
@@ -709,16 +709,31 @@ trait WebActionsApiTests extends ControllerTestCommon with BeforeAndAfterEach
wi
     it should s"handle http web action and provide defaults (auth? ${creds.isDefined})" in
{
       implicit val tid = transid()
 
+      def confirmEmptyResponse() = {
+        status should be(NoContent)
+        response.entity shouldBe HttpEntity.Empty
+        withClue(headers) {
+          headers.length shouldBe 0
+        }
+      }
+
       Seq(s"$systemId/proxy/export_c.http").foreach { path =>
-        allowedMethods.foreach { m =>
-          invocationsAllowed += 1
-          actionResult = Some(JsObject())
+        Set(JsObject(), JsObject("body" -> "".toJson), JsObject("body" -> JsNull)).foreach
{ bodyResult =>
+          allowedMethods.foreach { m =>
+            invocationsAllowed += 2
+            actionResult = Some(bodyResult)
 
-          m(s"$testRoutePath/$path") ~> Route.seal(routes(creds)) ~> check {
-            status should be(OK)
-            response.entity shouldBe HttpEntity.Empty
-            withClue(headers) {
-              headers.length shouldBe 0
+            m(s"$testRoutePath/$path") ~> Route.seal(routes(creds)) ~> check {
+              withClue(s"failed for: $bodyResult") {
+                confirmEmptyResponse()
+              }
+            }
+
+            // repeat with accept header, which should be ignored for content-negotiation
+            m(s"$testRoutePath/$path") ~> addHeader("Accept", "application/json") ~>
Route.seal(routes(creds)) ~> check {
+              withClue(s"with accept header, failed for: $bodyResult") {
+                confirmEmptyResponse()
+              }
             }
           }
         }
@@ -755,12 +770,36 @@ trait WebActionsApiTests extends ControllerTestCommon with BeforeAndAfterEach
wi
       implicit val tid = transid()
 
       Seq(s"$systemId/proxy/export_c.http").foreach { path =>
+        Seq(OK, Created).foreach { statusCode =>
+          allowedMethods.foreach { m =>
+            invocationsAllowed += 1
+            actionResult = Some(
+              JsObject(
+                "headers" -> JsObject("content-type" -> "application/json".toJson),
+                webApiDirectives.statusCode -> statusCode.intValue.toJson,
+                "body" -> Base64.getEncoder.encodeToString {
+                  JsObject("field" -> "value".toJson).compactPrint.getBytes
+                }.toJson))
+
+            m(s"$testRoutePath/$path") ~> Route.seal(routes(creds)) ~> check {
+              status should be(statusCode)
+              responseAs[JsObject] shouldBe JsObject("field" -> "value".toJson)
+            }
+          }
+        }
+      }
+    }
+
+    it should s"handle http web action with partially specified result (auth? ${creds.isDefined})"
in {
+      implicit val tid = transid()
+
+      Seq(s"$systemId/proxy/export_c.http").foreach { path =>
+        // omit status code
         allowedMethods.foreach { m =>
           invocationsAllowed += 1
           actionResult = Some(
             JsObject(
               "headers" -> JsObject("content-type" -> "application/json".toJson),
-              webApiDirectives.statusCode -> OK.intValue.toJson,
               "body" -> Base64.getEncoder.encodeToString {
                 JsObject("field" -> "value".toJson).compactPrint.getBytes
               }.toJson))
@@ -770,6 +809,105 @@ trait WebActionsApiTests extends ControllerTestCommon with BeforeAndAfterEach
wi
             responseAs[JsObject] shouldBe JsObject("field" -> "value".toJson)
           }
         }
+
+        // omit status code and headers
+        allowedMethods.foreach { m =>
+          invocationsAllowed += 1
+          actionResult = Some(JsObject("body" -> Base64.getEncoder.encodeToString {
+            JsObject("field" -> "value".toJson).compactPrint.getBytes
+          }.toJson))
+
+          m(s"$testRoutePath/$path") ~> Route.seal(routes(creds)) ~> check {
+            status should be(OK)
+            responseAs[String] shouldBe actionResult.get.fields("body").convertTo[String]
+            contentType shouldBe MediaTypes.`text/html`.withCharset(HttpCharsets.`UTF-8`)
+          }
+        }
+
+        // omit headers only
+        allowedMethods.foreach { m =>
+          invocationsAllowed += 1
+          actionResult = Some(
+            JsObject(
+              webApiDirectives.statusCode -> Created.intValue.toJson,
+              "body" -> Base64.getEncoder.encodeToString {
+                JsObject("field" -> "value".toJson).compactPrint.getBytes
+              }.toJson))
+
+          m(s"$testRoutePath/$path") ~> Route.seal(routes(creds)) ~> check {
+            status should be(Created)
+            responseAs[String] shouldBe actionResult.get.fields("body").convertTo[String]
+            contentType shouldBe MediaTypes.`text/html`.withCharset(HttpCharsets.`UTF-8`)
+          }
+        }
+
+        // omit body and headers
+        Seq(OK, Created, NoContent).foreach { statusCode =>
+          allowedMethods.foreach { m =>
+            invocationsAllowed += 1
+            actionResult = Some(JsObject(webApiDirectives.statusCode -> statusCode.intValue.toJson))
+
+            m(s"$testRoutePath/$path") ~> Route.seal(routes(creds)) ~> check {
+              status should be(statusCode)
+              headers shouldBe empty
+              response.entity shouldBe HttpEntity.Empty
+            }
+          }
+        }
+
+        // omit body but include headers
+        Seq(OK, Created, NoContent).foreach { statusCode =>
+          allowedMethods.foreach { m =>
+            invocationsAllowed += 1
+            actionResult = Some(
+              JsObject(
+                "headers" -> JsObject("Set-Cookie" -> "a=b".toJson, "content-type"
-> "application/json".toJson),
+                webApiDirectives.statusCode -> statusCode.intValue.toJson))
+
+            m(s"$testRoutePath/$path") ~> Route.seal(routes(creds)) ~> check {
+              status should be(statusCode)
+              headers shouldBe List(RawHeader("Set-Cookie", "a=b"))
+              response.entity shouldBe HttpEntity.Empty
+            }
+          }
+        }
+      }
+    }
+
+    it should s"handle http web action with no body when status code is set (auth? ${creds.isDefined})"
in {
+      implicit val tid = transid()
+
+      Seq(s"$systemId/proxy/export_c.http").foreach { path =>
+        // omit body and headers, but add accept header on the request
+        Seq(OK, Created, NoContent).foreach { statusCode =>
+          allowedMethods.foreach { m =>
+            invocationsAllowed += 1
+            actionResult = Some(JsObject(webApiDirectives.statusCode -> statusCode.intValue.toJson))
+
+            m(s"$testRoutePath/$path") ~> addHeader("Accept", "application/json") ~>
Route.seal(routes(creds)) ~> check {
+              status should be(statusCode)
+              headers shouldBe empty
+              response.entity shouldBe HttpEntity.Empty
+            }
+          }
+        }
+
+        // omit body but include headers, and add accept header on the request
+        Seq(OK, Created, NoContent).foreach { statusCode =>
+          allowedMethods.foreach { m =>
+            invocationsAllowed += 1
+            actionResult = Some(
+              JsObject(
+                "headers" -> JsObject("Set-Cookie" -> "a=b".toJson, "content-type"
-> "application/json".toJson),
+                webApiDirectives.statusCode -> statusCode.intValue.toJson))
+
+            m(s"$testRoutePath/$path") ~> addHeader("Accept", "application/json") ~>
Route.seal(routes(creds)) ~> check {
+              status should be(statusCode)
+              headers shouldBe List(RawHeader("Set-Cookie", "a=b"))
+              response.entity shouldBe HttpEntity.Empty
+            }
+          }
+        }
       }
     }
 
@@ -1014,7 +1152,7 @@ trait WebActionsApiTests extends ControllerTestCommon with BeforeAndAfterEach
wi
         s"$systemId/proxy/export_c.xyzz/",
         s"$systemId/proxy/export_c.xyzz/content").foreach { path =>
         allowedMethods.foreach { m =>
-          actionResult = Some(JsObject("statusCode" -> 201.toJson))
+          actionResult = Some(JsObject(webApiDirectives.statusCode -> Created.intValue.toJson))
 
           m(s"$testRoutePath/$path") ~> Route.seal(routes(creds)) ~> check {
             if (webApiDirectives.enforceExtension) {
@@ -1242,6 +1380,7 @@ trait WebActionsApiTests extends ControllerTestCommon with BeforeAndAfterEach
wi
 
             Get(s"$testRoutePath/$path") ~> addHeader("Accept", expectedMediaType.value)
~> Route.seal(routes(creds)) ~> check {
               status should be(OK)
+              contentType shouldBe ContentTypes.`text/html(UTF-8)`
               responseAs[String] shouldBe res
               mediaType shouldBe expectedMediaType
             }

-- 
To stop receiving notification emails like this one, please contact
['"commits@openwhisk.apache.org" <commits@openwhisk.apache.org>'].

Mime
View raw message