From 25cd4811ea0dfd09d701a2290910879837f24cbd Mon Sep 17 00:00:00 2001 From: geoffjentry Date: Thu, 6 Jul 2017 17:35:55 -0400 Subject: [PATCH 1/2] Only gzip metadata if requested. Closes #2419 --- CHANGELOG.md | 1 + README.md | 10 ++++++++ .../cromwell/webservice/CromwellApiService.scala | 3 +-- .../webservice/CromwellApiServiceSpec.scala | 27 ++++++++++++++++------ 4 files changed, 32 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f613fd567..a1f14e69e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Breaking Changes * Request timeouts for HTTP requests on the REST API now return a 503 status code instead of 500. The response for a request timeout is no longer in JSON format. +* The metadata endpoint was defaulting to responding with a gzipped response. This now needs to be explicitly requested with an `Accept-Encoding: gzip` header ## 28 diff --git a/README.md b/README.md index 402c52896..67659dd72 100644 --- a/README.md +++ b/README.md @@ -3439,6 +3439,16 @@ The `call` and `workflow` may optionally contain failures shaped like this: ] ``` +### Compressing the metadata response + +The response from the metadata endpoint can be quite large depending on the workflow. To help with this Cromwell supports gzip encoding the metadata prior to sending it back to the client. In order to enable this, make sure your client is sending the `Accept-Encoding: gzip` header. + +For instance, with cURL: + +``` +$ curl -H "Accept-Encoding: gzip" http://localhost:8000/api/workflows/v1/b3e45584-9450-4e73-9523-fc3ccf749848/metadata +``` + ## POST /api/workflows/:version/:id/abort cURL: diff --git a/engine/src/main/scala/cromwell/webservice/CromwellApiService.scala b/engine/src/main/scala/cromwell/webservice/CromwellApiService.scala index 2d29bb4c5..34830cd8e 100644 --- a/engine/src/main/scala/cromwell/webservice/CromwellApiService.scala +++ b/engine/src/main/scala/cromwell/webservice/CromwellApiService.scala @@ -22,7 +22,6 @@ import cromwell.services.metadata.MetadataService._ import cromwell.webservice.metadata.{MetadataBuilderActor, WorkflowQueryPagination} import cromwell.webservice.metadata.MetadataBuilderActor.{BuiltMetadataResponse, FailedMetadataResponse, MetadataBuilderActorResponse} import WorkflowJsonSupport._ -import akka.http.scaladsl.coding.{Deflate, Gzip, NoCoding} import akka.http.scaladsl.server.Route import cats.data.NonEmptyList import cats.data.Validated.{Invalid, Valid} @@ -87,7 +86,7 @@ trait CromwellApiService { } } } ~ - encodeResponseWith(Gzip, Deflate, NoCoding) { + encodeResponse { path("workflows" / Segment / Segment / "metadata") { (version, possibleWorkflowId) => parameters(('includeKey.*, 'excludeKey.*, 'expandSubWorkflows.as[Boolean].?)) { (includeKeys, excludeKeys, expandSubWorkflowsOption) => val includeKeysOption = NonEmptyList.fromList(includeKeys.toList) diff --git a/engine/src/test/scala/cromwell/webservice/CromwellApiServiceSpec.scala b/engine/src/test/scala/cromwell/webservice/CromwellApiServiceSpec.scala index 654a333aa..e7a70657a 100644 --- a/engine/src/test/scala/cromwell/webservice/CromwellApiServiceSpec.scala +++ b/engine/src/test/scala/cromwell/webservice/CromwellApiServiceSpec.scala @@ -12,6 +12,7 @@ import cromwell.engine.workflow.workflowstore.WorkflowStoreEngineActor.WorkflowA import cromwell.engine.workflow.workflowstore.WorkflowStoreSubmitActor.{WorkflowSubmittedToStore, WorkflowsBatchSubmittedToStore} import cromwell.services.metadata.MetadataService._ import akka.http.scaladsl.model._ +import akka.http.scaladsl.model.headers.{HttpEncodings, `Accept-Encoding`} import akka.http.scaladsl.testkit.{RouteTestTimeout, ScalatestRouteTest} import akka.http.scaladsl.unmarshalling.Unmarshal import akka.stream.ActorMaterializer @@ -20,7 +21,6 @@ import cromwell.util.SampleWdl.HelloWorld import org.scalatest.{AsyncFlatSpec, Matchers} import spray.json._ -import scala.concurrent.Await import scala.concurrent.duration._ class CromwellApiServiceSpec extends AsyncFlatSpec with ScalatestRouteTest with Matchers { @@ -321,8 +321,7 @@ class CromwellApiServiceSpec extends AsyncFlatSpec with ScalatestRouteTest with akkaHttpService.routes ~> check { status should be(StatusCodes.OK) - val decoder: Decoder = Gzip - val result = Await.result(Unmarshal(decoder.decodeMessage(response)).to[JsObject], 1.second) + val result = responseAs[JsObject] result.fields.keys should contain allOf("testKey1", "testKey2") result.fields.keys shouldNot contain("testKey3") result.fields("testKey1") should be(JsString("myValue1")) @@ -330,13 +329,28 @@ class CromwellApiServiceSpec extends AsyncFlatSpec with ScalatestRouteTest with } } + it should "return with gzip encoding when requested" in { + Get(s"/workflows/$version/${CromwellApiServiceSpec.ExistingWorkflowId}/metadata").addHeader(`Accept-Encoding`(HttpEncodings.gzip)) ~> + akkaHttpService.routes ~> + check { + response.headers.find(x => x.name() == "Content-Encoding").get.value should be("gzip") + } + } + + it should "not return with gzip encoding when not requested" in { + Get(s"/workflows/$version/${CromwellApiServiceSpec.ExistingWorkflowId}/metadata") ~> + akkaHttpService.routes ~> + check { + response.headers.find(x => x.name() == "Content-Encoding") shouldBe None + } + } + it should "return with included metadata from the metadata route" in { Get(s"/workflows/$version/${CromwellApiServiceSpec.ExistingWorkflowId}/metadata?includeKey=testKey1&includeKey=testKey2a") ~> akkaHttpService.routes ~> check { status should be(StatusCodes.OK) - val decoder: Decoder = Gzip - val result = Await.result(Unmarshal(decoder.decodeMessage(response)).to[JsObject], 1.second) + val result = responseAs[JsObject] result.fields.keys should contain allOf("testKey1a", "testKey1b", "testKey2a") result.fields.keys should contain noneOf("testKey2b", "testKey3") result.fields("testKey1a") should be(JsString("myValue1a")) @@ -350,8 +364,7 @@ class CromwellApiServiceSpec extends AsyncFlatSpec with ScalatestRouteTest with akkaHttpService.routes ~> check { status should be(StatusCodes.OK) - val decoder: Decoder = Gzip - val result = Await.result(Unmarshal(decoder.decodeMessage(response)).to[JsObject], 1.second) + val result = responseAs[JsObject] result.fields.keys should contain allOf("testKey1a", "testKey1b", "testKey2a") result.fields.keys should contain noneOf("testKey2b", "testKey3") result.fields("testKey1a") should be(JsString("myValue1a")) From 68b62a3d8f1c50a6e96cef713b4015c2a78ff27f Mon Sep 17 00:00:00 2001 From: geoffjentry Date: Fri, 7 Jul 2017 12:52:11 -0400 Subject: [PATCH 2/2] code review --- CHANGELOG.md | 2 +- .../src/test/scala/cromwell/webservice/CromwellApiServiceSpec.scala | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1f14e69e..a1f347059 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ ### Breaking Changes * Request timeouts for HTTP requests on the REST API now return a 503 status code instead of 500. The response for a request timeout is no longer in JSON format. -* The metadata endpoint was defaulting to responding with a gzipped response. This now needs to be explicitly requested with an `Accept-Encoding: gzip` header +* The metadata endpoint no longer returns gzipped responses by default. This now needs to be explicitly requested with an `Accept-Encoding: gzip` header ## 28 diff --git a/engine/src/test/scala/cromwell/webservice/CromwellApiServiceSpec.scala b/engine/src/test/scala/cromwell/webservice/CromwellApiServiceSpec.scala index e7a70657a..5537e64e0 100644 --- a/engine/src/test/scala/cromwell/webservice/CromwellApiServiceSpec.scala +++ b/engine/src/test/scala/cromwell/webservice/CromwellApiServiceSpec.scala @@ -333,7 +333,7 @@ class CromwellApiServiceSpec extends AsyncFlatSpec with ScalatestRouteTest with Get(s"/workflows/$version/${CromwellApiServiceSpec.ExistingWorkflowId}/metadata").addHeader(`Accept-Encoding`(HttpEncodings.gzip)) ~> akkaHttpService.routes ~> check { - response.headers.find(x => x.name() == "Content-Encoding").get.value should be("gzip") + response.headers.find(_.name == "Content-Encoding").get.value should be("gzip") } } @@ -341,7 +341,7 @@ class CromwellApiServiceSpec extends AsyncFlatSpec with ScalatestRouteTest with Get(s"/workflows/$version/${CromwellApiServiceSpec.ExistingWorkflowId}/metadata") ~> akkaHttpService.routes ~> check { - response.headers.find(x => x.name() == "Content-Encoding") shouldBe None + response.headers.find(_.name == "Content-Encoding") shouldBe None } }