PR Feedback

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala

@@ -768,11 +768,7 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
 
   override def checkStatus(samRequestContext: SamRequestContext): Boolean = {
     writeDbRef.inLocalTransaction { session =>
-      if (session.connection.isValid((2 seconds).toSeconds.intValue())) {
-        true
-      } else {
-        false
-      }
+      session.connection.isValid((2 seconds).toSeconds.intValue())
     }
   }
 }

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/StatusService.scala

@@ -16,12 +16,15 @@ import scala.concurrent.duration._
 import scala.concurrent.{ExecutionContext, Future}
 
 class StatusService(
-    val directoryDAO: DirectoryDAO,
-    val registrationDAO: RegistrationDAO,
-    val cloudExtensions: CloudExtensions,
-    val dbReference: DbReference,
-    initialDelay: FiniteDuration = Duration.Zero,
-    pollInterval: FiniteDuration = 1 minute)(implicit system: ActorSystem, executionContext: ExecutionContext)
+                     val directoryDAO: DirectoryDAO,
+                     // We expect this to be of type LdapRegistrationDAO, because
+                     // the status service specifically cares about checking LDAP's
+                     // status here, not a generic RegistrationDAO
+                     val ldapRegistrationDAO: RegistrationDAO,
+                     val cloudExtensions: CloudExtensions,
+                     val dbReference: DbReference,
+                     initialDelay: FiniteDuration = Duration.Zero,
+                     pollInterval: FiniteDuration = 1 minute)(implicit system: ActorSystem, executionContext: ExecutionContext)
     extends LazyLogging {
   implicit val askTimeout = Timeout(5 seconds)
 
@@ -36,10 +39,10 @@ class StatusService(
   private def checkOpenDJ(): IO[SubsystemStatus] = IO {
     // Since Status calls are ~80% of all Sam calls and are easy to track separately, Status calls are not being traced.
     logger.info("checking opendj connection")
-    if (registrationDAO.getConnectionTarget() != ConnectionType.LDAP) {
+    if (ldapRegistrationDAO.getConnectionTarget() != ConnectionType.LDAP) {
       HealthMonitor.failedStatus("Connection of RegistrationDAO is not to OpenDJ")
     } else {
-      if (registrationDAO.checkStatus(SamRequestContext(None)))
+      if (ldapRegistrationDAO.checkStatus(SamRequestContext(None)))
         HealthMonitor.OkStatus
       else
         HealthMonitor.failedStatus(s"LDAP database connection invalid or timed out checking")

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/StatusRouteSpec.scala

@@ -1,12 +1,16 @@
 package org.broadinstitute.dsde.workbench.sam.api
 
+import java.net.URI
+
 import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._
 import akka.http.scaladsl.model.StatusCodes
 import akka.http.scaladsl.model.headers.OAuth2BearerToken
 import akka.http.scaladsl.testkit.ScalatestRouteTest
+import com.unboundid.ldap.sdk.{LDAPConnection, LDAPConnectionPool}
 import org.broadinstitute.dsde.workbench.model.{UserInfo, WorkbenchEmail, WorkbenchUserId}
 import org.broadinstitute.dsde.workbench.sam.TestSupport
-import org.broadinstitute.dsde.workbench.sam.dataAccess.{MockAccessPolicyDAO, MockDirectoryDAO, MockRegistrationDAO}
+import org.broadinstitute.dsde.workbench.sam.config.DirectoryConfig
+import org.broadinstitute.dsde.workbench.sam.dataAccess.{LdapRegistrationDAO, MockAccessPolicyDAO, MockDirectoryDAO}
 import org.broadinstitute.dsde.workbench.sam.service._
 import org.broadinstitute.dsde.workbench.util.health.StatusJsonSupport._
 import org.broadinstitute.dsde.workbench.util.health.Subsystems.{Database, OpenDJ}
@@ -19,6 +23,7 @@ import org.scalatest.matchers.should.Matchers
 
 class StatusRouteSpec extends AnyFlatSpec with Matchers with ScalatestRouteTest with TestSupport {
 
+
   "GET /version" should "give 200 for ok" in {
     val samRoutes = TestSamRoutes(Map.empty)
     implicit val patienceConfig = PatienceConfig(timeout = 1 second)
@@ -42,8 +47,12 @@ class StatusRouteSpec extends AnyFlatSpec with Matchers with ScalatestRouteTest
   }
 
   it should "give 500 for not ok" in {
+    val directoryConfig: DirectoryConfig = TestSupport.directoryConfig
+    val dirURI = new URI(directoryConfig.directoryUrl)
+    val connectionPool = new LDAPConnectionPool(new LDAPConnection(dirURI.getHost, dirURI.getPort, directoryConfig.user, directoryConfig.password), directoryConfig.connectionPoolSize)
+    connectionPool.close()
     val directoryDAO = new MockDirectoryDAO()
-    val registrationDAO = new MockRegistrationDAO()
+    val registrationDAO = new LdapRegistrationDAO(connectionPool, directoryConfig, TestSupport.blockingEc)
     val policyDAO = new MockAccessPolicyDAO()
 
     val emailDomain = "example.com"

src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala

@@ -1,19 +1,16 @@
 package org.broadinstitute.dsde.workbench.sam.api
 
-import java.net.URI
-
 import akka.actor.ActorSystem
 import akka.http.scaladsl.model.headers.OAuth2BearerToken
 import akka.http.scaladsl.server
 import akka.http.scaladsl.server.Directives.reject
 import akka.stream.Materializer
 import cats.effect.{ContextShift, IO}
-import com.unboundid.ldap.sdk.{LDAPConnection, LDAPConnectionPool}
 import org.broadinstitute.dsde.workbench.google.mock.MockGoogleDirectoryDAO
 import org.broadinstitute.dsde.workbench.model._
 import org.broadinstitute.dsde.workbench.sam.TestSupport
 import org.broadinstitute.dsde.workbench.sam.TestSupport.samRequestContext
-import org.broadinstitute.dsde.workbench.sam.config.{DirectoryConfig, LiquibaseConfig, SwaggerConfig}
+import org.broadinstitute.dsde.workbench.sam.config.{LiquibaseConfig, SwaggerConfig}
 import org.broadinstitute.dsde.workbench.sam.dataAccess.{AccessPolicyDAO, DirectoryDAO, MockAccessPolicyDAO, MockDirectoryDAO, MockRegistrationDAO}
 import org.broadinstitute.dsde.workbench.sam.model.{ResourceActionPattern, ResourceRole, ResourceRoleName, ResourceType, ResourceTypeName, SamResourceActions}
 import org.broadinstitute.dsde.workbench.sam.service._
@@ -24,7 +21,6 @@ import scala.collection.concurrent.TrieMap
 import scala.collection.mutable
 import scala.concurrent.ExecutionContext
 import scala.concurrent.duration.DurationInt
-import scala.util.{Failure, Success, Try}
 
 /**
   * Created by dvoet on 7/14/17.
@@ -75,9 +71,6 @@ object TestSamRoutes {
 
   def apply(resourceTypes: Map[ResourceTypeName, ResourceType], userInfo: UserInfo = defaultUserInfo, policyAccessDAO: Option[AccessPolicyDAO] = None, policies: Option[mutable.Map[WorkbenchGroupIdentity, WorkbenchGroup]] = None)(implicit system: ActorSystem, materializer: Materializer, executionContext: ExecutionContext, contextShift: ContextShift[IO]) = {
     val dbRef = TestSupport.dbRef
-    val directoryConfig: DirectoryConfig = TestSupport.directoryConfig
-    val dirURI = new URI(directoryConfig.directoryUrl)
-    val ldapConnectionPool = new LDAPConnectionPool(new LDAPConnection(dirURI.getHost, dirURI.getPort, directoryConfig.user, directoryConfig.password), directoryConfig.connectionPoolSize)
     val resourceTypesWithAdmin = resourceTypes + (resourceTypeAdmin.name -> resourceTypeAdmin)
     // need to make sure MockDirectoryDAO and MockAccessPolicyDAO share the same groups
     val groups: mutable.Map[WorkbenchGroupIdentity, WorkbenchGroup] = policies.getOrElse(new TrieMap())
@@ -92,20 +85,7 @@ object TestSamRoutes {
         }
       }
     }
-    val registrationDAO = new MockRegistrationDAO() {
-      override def checkStatus(samRequestContext: SamRequestContext): Boolean = {
-        val ldapIsHealthy = Try {
-          ldapConnectionPool.getHealthCheck
-          val connection = ldapConnectionPool.getConnection
-          ldapConnectionPool.getHealthCheck.ensureNewConnectionValid(connection)
-          ldapConnectionPool.releaseConnection(connection)
-        } match {
-          case Success(_) => true
-          case Failure(_) => false
-        }
-        ldapIsHealthy
-      }
-    }
+    val registrationDAO = new MockRegistrationDAO()
     val googleDirectoryDAO = new MockGoogleDirectoryDAO()
     val policyDAO = policyAccessDAO.getOrElse(new MockAccessPolicyDAO(Map.empty[ResourceTypeName, ResourceType], groups))
 

src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala

@@ -299,5 +299,5 @@ class MockDirectoryDAO(private val groups: mutable.Map[WorkbenchGroupIdentity, W
     result.getOrElse(0)
   }
 
-  override def checkStatus(samRequestContext: SamRequestContext): Boolean = false
+  override def checkStatus(samRequestContext: SamRequestContext): Boolean = true
 }