WOR-1817: Refactor api service tests (#3033)

* add mock api service and sample tests demonstrating usage

* use real routes from RawlsApiService to test full paths

* move submissions api tests to correct test class

* give MockApiService all required properties

* temporarily add separate test class so existing setup doesn't interfere with usage of new setup patterns

* add test for updating ACL, fix broken test case

* finish replacing workspace api tests

* clean up

* set default mocks to RETURNS_SMART_NULLS

core/src/main/scala/org/broadinstitute/dsde/rawls/webservice/RawlsApiService.scala

@@ -126,7 +126,7 @@ trait RawlsApiService
   implicit val executionContext: ExecutionContext
   implicit val materializer: Materializer
 
-  val baseApiRoutes = (otelContext: Context) =>
+  val baseApiRoutes: Context => Route = (otelContext: Context) =>
     workspaceRoutesV2(otelContext) ~
       workspaceRoutes(otelContext) ~
       entityRoutes(otelContext) ~

core/src/main/scala/org/broadinstitute/dsde/rawls/webservice/WorkspaceApiService.scala

@@ -57,7 +57,7 @@ trait WorkspaceApiService extends UserInfoDirectives {
           }
       } ~
         path("workspaces" / "tags") {
-          parameters('q.?, "limit".as[Int].optional) { (queryString, limit) =>
+          parameters(Symbol("q").?, "limit".as[Int].optional) { (queryString, limit) =>
             get {
               complete {
                 workspaceServiceConstructor(ctx).getTags(queryString, limit)
@@ -79,13 +79,6 @@ trait WorkspaceApiService extends UserInfoDirectives {
           }
         } ~
         path("workspaces" / Segment / Segment) { (workspaceNamespace, workspaceName) =>
-          /* we enforce a 6-character minimum for workspaceNamespace, as part of billing project creation.
-           the previous "mc", "tags", and "id" paths rely on this convention to avoid path-matching conflicts.
-           we might want to change the first Segment above to a regex a la """[^/.]{6,}""".r
-           but note that would be a behavior change: if a user entered fewer than 6 chars it would result in an
-           unmatched path rejection instead of the custom error handling inside WorkspaceService.
-           */
-
           patch {
             entity(as[Array[AttributeUpdateOperation]]) { operations =>
               complete {
@@ -152,12 +145,18 @@ trait WorkspaceApiService extends UserInfoDirectives {
             }
           } ~
             patch {
-              parameter('inviteUsersNotFound.?) { inviteUsersNotFound =>
+              parameter(Symbol("inviteUsersNotFound").?) { inviteUsersNotFound: Option[String] =>
                 entity(as[Set[WorkspaceACLUpdate]]) { aclUpdate =>
+                  val inviteUsersNotFoundValue = inviteUsersNotFound match {
+                    case Some(str) if str.isEmpty => false
+                    case Some(str)                => str.toBoolean
+                    case None                     => false
+                  }
+
                   complete {
                     workspaceServiceConstructor(ctx).updateACL(WorkspaceName(workspaceNamespace, workspaceName),
                                                                aclUpdate,
-                                                               inviteUsersNotFound.getOrElse("false").toBoolean
+                                                               inviteUsersNotFoundValue
                     )
                   }
                 }

core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/MockApiService.scala

@@ -0,0 +1,97 @@
+package org.broadinstitute.dsde.rawls.webservice
+
+import akka.actor.ActorSystem
+import akka.http.scaladsl.model.headers.OAuth2BearerToken
+import akka.http.scaladsl.server
+import akka.http.scaladsl.server.Directives.{provide, _}
+import akka.http.scaladsl.server.Directive1
+import akka.stream.Materializer
+import io.opentelemetry.context.Context
+import org.broadinstitute.dsde.rawls.billing._
+import org.broadinstitute.dsde.rawls.bucketMigration.BucketMigrationService
+import org.broadinstitute.dsde.rawls.dataaccess._
+import org.broadinstitute.dsde.rawls.entities.EntityService
+import org.broadinstitute.dsde.rawls.genomics.GenomicsService
+import org.broadinstitute.dsde.rawls.methods.MethodConfigurationService
+import org.broadinstitute.dsde.rawls.model.{
+  ApplicationVersion,
+  RawlsRequestContext,
+  RawlsUserEmail,
+  RawlsUserSubjectId,
+  UserInfo
+}
+import org.broadinstitute.dsde.rawls.snapshot.SnapshotService
+import org.broadinstitute.dsde.rawls.spendreporting.SpendReportingService
+import org.broadinstitute.dsde.rawls.status.StatusService
+import org.broadinstitute.dsde.rawls.submissions.SubmissionsService
+import org.broadinstitute.dsde.rawls.user.UserService
+import org.broadinstitute.dsde.rawls.workspace.{MultiCloudWorkspaceService, WorkspaceService, WorkspaceSettingService}
+import org.broadinstitute.dsde.workbench.oauth2.OpenIDConnectConfiguration
+import org.mockito.Mockito.RETURNS_SMART_NULLS
+import org.scalatestplus.mockito.MockitoSugar.mock
+
+import scala.concurrent.duration._
+import scala.concurrent.ExecutionContext
+import scala.language.postfixOps
+
+class MockApiService(
+  val userInfo: UserInfo =
+    UserInfo(RawlsUserEmail(""), OAuth2BearerToken("token"), 123, RawlsUserSubjectId("123456789876543212349")),
+  override val openIDConnectConfiguration: OpenIDConnectConfiguration =
+    mock[OpenIDConnectConfiguration](RETURNS_SMART_NULLS),
+  override val samDAO: SamDAO = mock[SamDAO](RETURNS_SMART_NULLS),
+  override val submissionTimeout: FiniteDuration = mock[FiniteDuration](RETURNS_SMART_NULLS),
+  override val workbenchMetricBaseName: String = "workbenchMetricBaseName-test",
+  override val batchUpsertMaxBytes: Long = 1028,
+  override val executionServiceCluster: ExecutionServiceCluster = mock[ExecutionServiceCluster](RETURNS_SMART_NULLS),
+  override val appVersion: ApplicationVersion = mock[ApplicationVersion](RETURNS_SMART_NULLS),
+  override val spendReportingConstructor: RawlsRequestContext => SpendReportingService = _ =>
+    mock[SpendReportingService](RETURNS_SMART_NULLS),
+  override val billingProjectOrchestratorConstructor: RawlsRequestContext => BillingProjectOrchestrator = _ =>
+    mock[BillingProjectOrchestrator](RETURNS_SMART_NULLS),
+  override val entityServiceConstructor: RawlsRequestContext => EntityService = _ =>
+    mock[EntityService](RETURNS_SMART_NULLS),
+  override val genomicsServiceConstructor: RawlsRequestContext => GenomicsService = _ =>
+    mock[GenomicsService](RETURNS_SMART_NULLS),
+  override val snapshotServiceConstructor: RawlsRequestContext => SnapshotService = _ =>
+    mock[SnapshotService](RETURNS_SMART_NULLS),
+  override val statusServiceConstructor: () => StatusService = () => mock[StatusService](RETURNS_SMART_NULLS),
+  override val methodConfigurationServiceConstructor: RawlsRequestContext => MethodConfigurationService = _ =>
+    mock[MethodConfigurationService](RETURNS_SMART_NULLS),
+  override val submissionsServiceConstructor: RawlsRequestContext => SubmissionsService = _ =>
+    mock[SubmissionsService](RETURNS_SMART_NULLS),
+  override val workspaceServiceConstructor: RawlsRequestContext => WorkspaceService = _ =>
+    mock[WorkspaceService](RETURNS_SMART_NULLS),
+  override val multiCloudWorkspaceServiceConstructor: RawlsRequestContext => MultiCloudWorkspaceService = _ =>
+    mock[MultiCloudWorkspaceService](RETURNS_SMART_NULLS),
+  override val workspaceSettingServiceConstructor: RawlsRequestContext => WorkspaceSettingService = _ =>
+    mock[WorkspaceSettingService](RETURNS_SMART_NULLS),
+  override val bucketMigrationServiceConstructor: RawlsRequestContext => BucketMigrationService = _ =>
+    mock[BucketMigrationService](RETURNS_SMART_NULLS),
+  override val userServiceConstructor: RawlsRequestContext => UserService = _ => mock[UserService](RETURNS_SMART_NULLS)
+)(implicit val executionContext: ExecutionContext)
+    extends RawlsApiService
+    with AdminApiService
+    with BillingApiService
+    with BillingApiServiceV2
+    with EntityApiService
+    with NotificationsApiService
+    with SnapshotApiService
+    with StatusApiService
+    with UserApiService
+    with MethodConfigApiService
+    with WorkspaceApiService
+    with SubmissionApiService {
+
+  implicit val system: ActorSystem = ActorSystem("rawls")
+
+  implicit override val materializer: Materializer = Materializer(system)
+
+  override def requireUserInfo(otelContext: Option[Context]): Directive1[UserInfo] = provide(userInfo)
+
+  def testRoutes: server.Route =
+    (handleExceptions(RawlsApiService.exceptionHandler) & handleRejections(RawlsApiService.rejectionHandler)) {
+      baseApiRoutes(Context.root())
+    }
+
+}

core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/SubmissionApiServiceSpec.scala

@@ -1,13 +1,16 @@
 package org.broadinstitute.dsde.rawls.webservice
 
 import akka.actor.{ActorRef, PoisonPill}
+import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport
 import akka.http.scaladsl.model._
 import akka.http.scaladsl.model.headers.OAuth2BearerToken
 import akka.http.scaladsl.server.Route
 import akka.http.scaladsl.server.Route.{seal => sealRoute}
-import akka.http.scaladsl.testkit.RouteTestTimeout
+import akka.http.scaladsl.testkit.{RouteTestTimeout, ScalatestRouteTest}
 import akka.testkit.TestProbe
+import io.opentelemetry.context.Context
 import org.apache.commons.lang3.RandomStringUtils
+import org.broadinstitute.dsde.rawls.{TestExecutionContext, WorkspaceAccessDeniedException}
 import org.broadinstitute.dsde.rawls.dataaccess._
 import org.broadinstitute.dsde.rawls.dataaccess.slick.{ReadWriteAction, TestData, WorkflowAuditStatusRecord}
 import org.broadinstitute.dsde.rawls.google.MockGooglePubSubDAO
@@ -17,15 +20,20 @@ import org.broadinstitute.dsde.rawls.model.ExecutionJsonSupport._
 import org.broadinstitute.dsde.rawls.model.WorkspaceJsonSupport._
 import org.broadinstitute.dsde.rawls.model._
 import org.broadinstitute.dsde.rawls.openam.MockUserInfoDirectives
+import org.broadinstitute.dsde.rawls.submissions.SubmissionsService
+import org.broadinstitute.dsde.rawls.util.MockitoTestUtils
 import org.broadinstitute.dsde.workbench.model.WorkbenchEmail
 import org.joda.time.DateTime
+import org.mockito.Mockito.{verify, when}
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
 import org.scalatest.prop.TableDrivenPropertyChecks
 import org.scalatest.time.{Seconds, Span}
 import spray.json.DefaultJsonProtocol._
 import spray.json._
 
 import java.util.UUID
-import scala.concurrent.ExecutionContext
+import scala.concurrent.{ExecutionContext, Future}
 import scala.concurrent.duration._
 import scala.language.postfixOps
 
@@ -1439,4 +1447,68 @@ class SubmissionApiServiceSpec extends ApiServiceSpec with TableDrivenPropertyCh
         }
     }
   }
+
+  it should "return a 201 when a comment is updated" in {
+    val wsName = testData.wsName
+    val submissionId = UUID.randomUUID().toString
+    val submissionsService = mock[SubmissionsService]
+    val update = UserCommentUpdateOperation("user comment updated")
+    when(submissionsService.updateSubmissionUserComment(wsName, submissionId, update)).thenReturn(Future(1))
+    val service = new MockApiService(submissionsServiceConstructor = _ => submissionsService)
+
+    Patch(
+      s"${wsName.path}/submissions/$submissionId",
+      JsObject(
+        List("userComment" -> "user comment updated".toJson): _*
+      )
+    ) ~>
+      service.testRoutes ~>
+      check {
+        status should be(StatusCodes.NoContent)
+      }
+  }
+
+  it should "return a 404 when no submission was updated" in {
+    val wsName = testData.wsName
+    val submissionId = UUID.randomUUID().toString
+    val submissionsService = mock[SubmissionsService]
+    val update = UserCommentUpdateOperation("user comment updated")
+    when(submissionsService.updateSubmissionUserComment(wsName, submissionId, update)).thenReturn(Future(0))
+    val service = new MockApiService(submissionsServiceConstructor = _ => submissionsService)
+
+    Patch(
+      s"${wsName.path}/submissions/$submissionId",
+      JsObject(
+        List("userComment" -> "user comment updated".toJson): _*
+      )
+    ) ~>
+      service.testRoutes ~>
+      check {
+        status should be(StatusCodes.NotFound)
+      }
+  }
+
+  it should "return 403 when access is denied" in {
+    val wsName = WorkspaceName("ns", "n")
+    val submissionId = UUID.randomUUID().toString
+    val submissionsService = mock[SubmissionsService]
+    val update = UserCommentUpdateOperation("user comment updated")
+    when(submissionsService.updateSubmissionUserComment(wsName, submissionId, update))
+      .thenReturn(Future.failed(WorkspaceAccessDeniedException(wsName)))
+    val service = new MockApiService(submissionsServiceConstructor = _ => submissionsService)
+    Patch(
+      s"${wsName.path}/submissions/$submissionId",
+      JsObject(
+        List("userComment" -> "user comment updated".toJson): _*
+      )
+    ) ~>
+      service.testRoutes ~>
+      check {
+        val response = responseAs[String]
+        status should be(StatusCodes.Forbidden)
+        response should include("insufficient permissions to perform operation on")
+      }
+    verify(submissionsService).updateSubmissionUserComment(wsName, submissionId, update)
+  }
+
 }