Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AJ-1490, AJ-423: update workbench-libs and mockserver #1250

Merged
merged 9 commits into from
Dec 4, 2023
Merged

AJ-1490, AJ-423: update workbench-libs and mockserver #1250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5c1154b
update workbench-libs and mockserver
davidangb Dec 2, 2023
19d4a77
okio assembly fix
davidangb Dec 3, 2023
78fa596
remove commented-out line
davidangb Dec 3, 2023
721f7f9
workbench-util to same hash as other workbench-libs deps
davidangb Dec 3, 2023
28c8da8
Revert "workbench-util to same hash as other workbench-libs deps"
davidangb Dec 3, 2023
01587ef
code comment
davidangb Dec 4, 2023
6f3024f
Merge branch 'develop' into da_AJ-1490_upgradeWorkbenchLibs
davidangb Dec 4, 2023
92e996f
Merge branch 'develop' into da_AJ-1490_upgradeWorkbenchLibs
davidangb Dec 4, 2023
a8c6478
Merge branch 'develop' into da_AJ-1490_upgradeWorkbenchLibs
davidangb Dec 4, 2023
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
10 changes: 5 additions & 5 deletions project/Dependencies.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ object Dependencies {
val jacksonV = "2.13.4"
val jacksonHotfixV = "2.13.4.2" // for when only some of the Jackson libs have hotfix releases
val nettyV = "4.1.101.Final"
val workbenchLibsHash = "084d25b"
val workbenchLibsHash = "8ccaa6d" // see https://github.com/broadinstitute/workbench-libs readme for hash values

def excludeGuava(m: ModuleID): ModuleID = m.exclude("com.google.guava", "guava")
val excludeAkkaActor = ExclusionRule(organization = "com.typesafe.akka", name = "akka-actor_2.13")
Expand Down Expand Up @@ -53,10 +53,10 @@ object Dependencies {
exclude("bio.terra", "workspace-manager-client")
excludeAll(excludeAkkaHttp, excludeSprayJson),
excludeGuava("org.broadinstitute.dsde.workbench" %% "workbench-util" % "0.10-128901e"),
"org.broadinstitute.dsde.workbench" %% "workbench-google2" % s"0.25-$workbenchLibsHash",
"org.broadinstitute.dsde.workbench" %% "workbench-oauth2" % s"0.2-$workbenchLibsHash",
"org.broadinstitute.dsde.workbench" %% "workbench-google2" % s"0.35-$workbenchLibsHash",
"org.broadinstitute.dsde.workbench" %% "workbench-oauth2" % s"0.5-$workbenchLibsHash",
"org.broadinstitute.dsde.workbench" %% "sam-client" % "0.1-ef83073",
"org.broadinstitute.dsde.workbench" %% "workbench-notifications" %s"0.3-$workbenchLibsHash",
"org.broadinstitute.dsde.workbench" %% "workbench-notifications" %s"0.6-$workbenchLibsHash",

"com.typesafe.akka" %% "akka-actor" % akkaV,
"com.typesafe.akka" %% "akka-slf4j" % akkaV,
Expand Down Expand Up @@ -95,7 +95,7 @@ object Dependencies {
"com.github.pathikrit" %% "better-files" % "3.9.2",

"org.scalatest" %% "scalatest" % "3.2.17" % "test",
"org.mock-server" % "mockserver-netty" % "3.11" % "test", // TODO: upgrading higher causes failures, need to investigate
"org.mock-server" % "mockserver-netty" % "5.15.0" % "test",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the workbench-libs updates hit a conflict with Orch's older version of mockserver, so I needed to update mockserver too.

// jaxb-api needed by WorkspaceApiServiceSpec.bagitService() method
"javax.xml.bind" % "jaxb-api" % "2.3.1" % "test",
// provides testing mocks
Expand Down
1 change: 1 addition & 0 deletions project/Merging.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ object Merging {
case x if x.endsWith("kotlin-stdlib.kotlin_module") => MergeStrategy.first
case x if x.contains("bouncycastle") => MergeStrategy.first
case x if x.endsWith("kotlin-stdlib-common.kotlin_module") => MergeStrategy.first
case x if x.endsWith("okio.kotlin_module") => MergeStrategy.first
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this fixes an issue in sbt assembly, which we run as part of deployment and tests, but developers typically do not run locally. sbt assembly compiles the fat jar, and hit an error trying to de-duplicate okio-jvm META-INF/okio.kotlin_module and okio META-INF/okio.kotlin_module. This fix takes the same approach as the merge strategy for kotlin-stdlib and otlin-stdlib-common just above in this file.

case x if x.endsWith("arrow-git.properties") => MergeStrategy.concat

// For the following error:
Expand Down
7 changes: 4 additions & 3 deletions src/test/scala/org/broadinstitute/dsde/firecloud/mock/MockWorkspaceServer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import org.mockserver.model.HttpClassCallback.callback
import org.mockserver.model.HttpRequest._
import org.mockserver.model.HttpResponse._
import akka.http.scaladsl.model.StatusCodes._
import com.google.common.net.UrlEscapers
import spray.json._

/**
Expand Down Expand Up @@ -152,7 +153,7 @@ object MockWorkspaceServer {
.withHeader(authHeader)
.withPath(s"${workspaceBasePath}/%s/%s/submissions"
.format(mockValidWorkspace.namespace, mockValidWorkspace.name)))
.callback(
.respond(
callback().
withCallbackClass("org.broadinstitute.dsde.firecloud.mock.ValidSubmissionCallback")
)
Expand All @@ -163,7 +164,7 @@ object MockWorkspaceServer {
.withMethod("POST")
.withPath(s"${workspaceBasePath}/%s/%s/submissions/validate"
.format(mockValidWorkspace.namespace, mockValidWorkspace.name)))
.callback(
.respond(
callback().
withCallbackClass("org.broadinstitute.dsde.firecloud.mock.ValidSubmissionCallback")
)
Expand Down Expand Up @@ -276,7 +277,7 @@ object MockWorkspaceServer {
request()
.withMethod("GET")
.withPath(s"${workspaceBasePath}/%s/%s/submissions/%s/workflows/%s"
.format(mockSpacedWorkspace.namespace, mockSpacedWorkspace.name, mockValidId, mockValidId)))
.format(mockSpacedWorkspace.namespace, UrlEscapers.urlPathSegmentEscaper().escape(mockSpacedWorkspace.name), mockValidId, mockValidId)))
Copy link
Contributor Author

@davidangb davidangb Dec 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

these are syntax/behavior differences in the updated mockserver library, as are almost all the other changes in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you only had to do this URL escaping once in this PR...but it also looks pretty ugly -- if you had to do this 20 times, once for each test case, what would your solution look like?

I'm asking not because I want you to implement that solution (since this is just a one-off) but just curious what it'd look like in Scala :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would take one of two routes:

wrap the escaping in something terser, like StreamingPassthrough.escapePathSegment, so you end up with:

          .withPath(s"${workspaceBasePath}/%s/%s/submissions/%s/workflows/%s"
            .format(mockSpacedWorkspace.namespace, escapePathSegment(mockSpacedWorkspace.name), mockValidId, mockValidId)))

this isn't a big win, but it's nicer.

Alternately, I would make an implicit class that provides methods for strings; then, I could call my custom method directly on the thing I wanted to escape:

    implicit class StringEscaper(s: String) {
      def escape: String = UrlEscapers.urlPathSegmentEscaper().escape(s)
    }

…
          .withPath(s"${workspaceBasePath}/%s/%s/submissions/%s/workflows/%s"
            .format(mockSpacedWorkspace.namespace, mockSpacedWorkspace.name.escape, mockValidId, mockValidId)))

.respond(
response()
.withHeaders(header)
Expand Down
4 changes: 2 additions & 2 deletions src/test/scala/org/broadinstitute/dsde/firecloud/mock/ValidEntityCopyCallback.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ package org.broadinstitute.dsde.firecloud.mock
import org.broadinstitute.dsde.firecloud.mock.MockUtils._
import org.broadinstitute.dsde.firecloud.model.ModelJsonProtocol._
import org.broadinstitute.dsde.rawls.model.EntityCopyDefinition
import org.mockserver.mock.action.ExpectationCallback
import org.mockserver.mock.action.ExpectationResponseCallback
import org.mockserver.model.HttpResponse._
import org.mockserver.model.{HttpRequest, HttpResponse}
import akka.http.scaladsl.model.StatusCodes._
import spray.json._

class ValidEntityCopyCallback extends ExpectationCallback {
class ValidEntityCopyCallback extends ExpectationResponseCallback {

override def handle(httpRequest: HttpRequest): HttpResponse = {

Expand Down
10 changes: 6 additions & 4 deletions src/test/scala/org/broadinstitute/dsde/firecloud/mock/ValidEntityDeleteCallback.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,22 @@ import org.broadinstitute.dsde.firecloud.mock.MockUtils._
import org.broadinstitute.dsde.firecloud.model.EntityId
import org.broadinstitute.dsde.firecloud.model.ModelJsonProtocol._
import spray.json.DefaultJsonProtocol._
import org.mockserver.mock.action.ExpectationCallback
import org.mockserver.mock.action.ExpectationResponseCallback
import org.mockserver.model.HttpResponse._
import org.mockserver.model.{HttpRequest, HttpResponse}
import akka.http.scaladsl.model.StatusCodes._
import spray.json._

class ValidEntityDeleteCallback extends ExpectationCallback {
import scala.util.Try

class ValidEntityDeleteCallback extends ExpectationResponseCallback {

val validEntities = Set(EntityId("sample", "id"), EntityId("sample", "bar"))

override def handle(httpRequest: HttpRequest): HttpResponse = {
val deleteRequest = httpRequest.getBodyAsString.parseJson.convertTo[Set[EntityId]]
val deleteRequest = Try(httpRequest.getBodyAsString.parseJson.convertTo[Set[EntityId]])

if (deleteRequest.subsetOf(validEntities)) {
if (deleteRequest.isSuccess && deleteRequest.get.subsetOf(validEntities)) {
response()
.withHeaders(header)
.withStatusCode(NoContent.intValue)
Expand Down
4 changes: 2 additions & 2 deletions src/test/scala/org/broadinstitute/dsde/firecloud/mock/ValidSubmissionCallback.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ package org.broadinstitute.dsde.firecloud.mock
import org.broadinstitute.dsde.firecloud.mock.MockUtils._
import org.broadinstitute.dsde.firecloud.model.ModelJsonProtocol._
import org.broadinstitute.dsde.firecloud.model.OrchSubmissionRequest
import org.mockserver.mock.action.ExpectationCallback
import org.mockserver.mock.action.ExpectationResponseCallback
import org.mockserver.model.HttpResponse._
import org.mockserver.model.{HttpRequest, HttpResponse}
import akka.http.scaladsl.model.StatusCodes._
import spray.json._

class ValidSubmissionCallback extends ExpectationCallback {
class ValidSubmissionCallback extends ExpectationResponseCallback {

override def handle(httpRequest: HttpRequest): HttpResponse = {

Expand Down
2 changes: 1 addition & 1 deletion src/test/scala/org/broadinstitute/dsde/firecloud/service/PassthroughDirectivesSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ final class PassthroughDirectivesSpec extends BaseServiceSpec with FireCloudDire
override def beforeAll() = {
echoServer = startClientAndServer(echoPort)
echoServer.when(request())
.callback(
.respond(
callback().
withCallbackClass("org.broadinstitute.dsde.firecloud.service.EchoCallback"))
}
Expand Down
13 changes: 7 additions & 6 deletions ...st/scala/org/broadinstitute/dsde/firecloud/service/PassthroughDirectivesSpecSupport.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package org.broadinstitute.dsde.firecloud.service
import org.broadinstitute.dsde.firecloud.mock.MockUtils
import org.broadinstitute.dsde.firecloud.service.PassthroughDirectivesSpec._
import org.broadinstitute.dsde.firecloud.service.PassthroughDirectivesSpecSupport._
import org.mockserver.mock.action.ExpectationCallback
import org.mockserver.mock.action.ExpectationResponseCallback
import org.mockserver.model.{HttpRequest, HttpResponse}
import akka.http.scaladsl.model.StatusCodes.OK
import akka.http.scaladsl.model.Uri
Expand All @@ -12,16 +12,17 @@ import spray.json.DefaultJsonProtocol._

import scala.jdk.CollectionConverters._

class EchoCallback extends ExpectationCallback {
class EchoCallback extends ExpectationResponseCallback {
override def handle(httpRequest: HttpRequest): HttpResponse = {
// translate the mockserver request to a spray Uri
val sprayparams = httpRequest.getQueryStringParameters.asScala.map{p =>
assert(p.getValues.size() <= 1)
p.getName.getValue -> p.getValues.asScala.head.getValue}.toMap
val query: Query = Option(httpRequest.getQueryStringParameters) match {
case None => Query.Empty
case Some(params) => Query(params.getRawParameterString)
}

val sprayuri = Uri(echoUrl)
.withPath(Path(httpRequest.getPath.getValue))
.withQuery(Query(sprayparams))
.withQuery(query)

val requestInfo = RequestInfo(
httpRequest.getMethod.getValue,
Expand Down
4 changes: 2 additions & 2 deletions src/test/scala/org/broadinstitute/dsde/firecloud/webservice/EntityApiServiceSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ class EntityApiServiceSpec extends BaseServiceSpec with EntityApiService with Sp
request()
.withMethod("POST")
.withPath(FireCloudConfig.Rawls.authPrefix + FireCloudConfig.Rawls.workspacesEntitiesCopyPath))
.callback(
.respond(
callback().
withCallbackClass("org.broadinstitute.dsde.firecloud.mock.ValidEntityCopyCallback")
)
Expand Down Expand Up @@ -138,7 +138,7 @@ class EntityApiServiceSpec extends BaseServiceSpec with EntityApiService with Sp
request()
.withMethod("POST")
.withPath(FireCloudConfig.Rawls.authPrefix + FireCloudConfig.Rawls.entitiesPath.format("broad-dsde-dev", "valid") + "/delete"))
.callback(
.respond(
callback().
withCallbackClass("org.broadinstitute.dsde.firecloud.mock.ValidEntityDeleteCallback")
)
Expand Down
8 changes: 4 additions & 4 deletions src/test/scala/org/broadinstitute/dsde/firecloud/webservice/MethodsApiServiceSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import org.broadinstitute.dsde.firecloud.mock.MockUtils
import org.broadinstitute.dsde.firecloud.service.{AgoraPermissionService, BaseServiceSpec, ServiceSpec}
import org.mockserver.integration.ClientAndServer
import org.mockserver.integration.ClientAndServer._
import org.mockserver.mock.action.ExpectationCallback
import org.mockserver.mock.action.ExpectationResponseCallback
import org.mockserver.model.HttpClassCallback.callback
import org.mockserver.model.HttpRequest._
import org.mockserver.model.HttpResponse.response
Expand Down Expand Up @@ -81,7 +81,7 @@ final class MethodsApiServiceSpec extends BaseServiceSpec with ServiceSpec with
testCases foreach { api =>
methodsServer
.when(request().withMethod(api.verb.name).withPath(api.remotePath))
.callback(callback().withCallbackClass("org.broadinstitute.dsde.firecloud.webservice.MethodsApiServiceSpecCallback"))
.respond(callback().withCallbackClass("org.broadinstitute.dsde.firecloud.webservice.MethodsApiServiceSpecCallback"))
}
}

Expand Down Expand Up @@ -128,11 +128,11 @@ final class MethodsApiServiceSpec extends BaseServiceSpec with ServiceSpec with
}
}

final class MethodsApiServiceSpecCallback extends ExpectationCallback {
final class MethodsApiServiceSpecCallback extends ExpectationResponseCallback {
override def handle(httpRequest: HttpRequest): HttpResponse = {
val method:String = httpRequest.getMethod.getValue
val path:String = httpRequest.getPath.getValue
val hasParams:Boolean = !httpRequest.getQueryStringParameters.isEmpty
val hasParams:Boolean = !httpRequest.getQueryStringParameterList.isEmpty

val content = s"$method $path $hasParams"

Expand Down
8 changes: 6 additions & 2 deletions src/test/scala/org/broadinstitute/dsde/firecloud/webservice/WorkspaceApiServiceSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@ import org.broadinstitute.dsde.firecloud.{EntityService, FireCloudConfig}
import org.broadinstitute.dsde.rawls.model.WorkspaceACLJsonSupport._
import org.broadinstitute.dsde.rawls.model._
import org.joda.time.DateTime
import org.mockserver.configuration.Configuration
import org.mockserver.integration.ClientAndServer
import org.mockserver.integration.ClientAndServer._
import org.mockserver.logging.MockServerLogger
import org.mockserver.model.HttpRequest._
import org.mockserver.model.Parameter
import org.mockserver.socket.KeyStoreFactory
import org.mockserver.socket.tls.KeyStoreFactory
import org.scalatest.BeforeAndAfterEach
import spray.json.DefaultJsonProtocol._
import spray.json._
Expand Down Expand Up @@ -299,7 +301,9 @@ class WorkspaceApiServiceSpec extends BaseServiceSpec with WorkspaceApiService w
)

// bagit import requires https urls; set up SSL
HttpsURLConnection.setDefaultSSLSocketFactory(KeyStoreFactory.keyStoreFactory().sslContext().getSocketFactory())
HttpsURLConnection.setDefaultSSLSocketFactory(new KeyStoreFactory(
Configuration.configuration(),
new MockServerLogger()).sslContext().getSocketFactory)

// set up mockserver for all paths defined above
mappings.foreach { entry =>
Expand Down
Loading