From c4093a0af7436787e81a8032d3b654273c8ebede Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Fri, 25 Oct 2024 12:21:54 -0400 Subject: [PATCH] AN-214 Redirect workflow existence queries from metadata to summary (#7575) --- .../database/slick/MetadataSlickDatabase.scala | 5 ----- .../slick/tables/MetadataEntryComponent.scala | 16 ---------------- .../database/sql/MetadataSqlDatabase.scala | 2 -- .../webservice/routes/CromwellApiService.scala | 18 ++---------------- .../routes/MetadataRouteSupport.scala | 2 +- .../routes/wes/WesRouteSupport.scala | 12 ++++++++---- .../routes/CromwellApiServiceSpec.scala | 14 +++++--------- .../routes/MetadataRouteSupportSpec.scala | 2 +- .../services/metadata/MetadataService.scala | 1 - .../metadata/impl/MetadataDatabaseAccess.scala | 3 --- .../metadata/impl/MetadataServiceActor.scala | 11 ----------- .../metadata/impl/WriteMetadataActorSpec.scala | 3 --- 12 files changed, 17 insertions(+), 72 deletions(-) diff --git a/database/sql/src/main/scala/cromwell/database/slick/MetadataSlickDatabase.scala b/database/sql/src/main/scala/cromwell/database/slick/MetadataSlickDatabase.scala index fbab736ae26..9f18505efcf 100644 --- a/database/sql/src/main/scala/cromwell/database/slick/MetadataSlickDatabase.scala +++ b/database/sql/src/main/scala/cromwell/database/slick/MetadataSlickDatabase.scala @@ -130,11 +130,6 @@ class MetadataSlickDatabase(originalDatabaseConfig: Config) } yield () } - override def metadataEntryExists(workflowExecutionUuid: String)(implicit ec: ExecutionContext): Future[Boolean] = { - val action = dataAccess.metadataEntryExistsForWorkflowExecutionUuid(workflowExecutionUuid).result - runTransaction(action) - } - override def metadataSummaryEntryExists( workflowExecutionUuid: String )(implicit ec: ExecutionContext): Future[Boolean] = { diff --git a/database/sql/src/main/scala/cromwell/database/slick/tables/MetadataEntryComponent.scala b/database/sql/src/main/scala/cromwell/database/slick/tables/MetadataEntryComponent.scala index 3ba54ee2760..0ca87f4e2d3 100644 --- a/database/sql/src/main/scala/cromwell/database/slick/tables/MetadataEntryComponent.scala +++ b/database/sql/src/main/scala/cromwell/database/slick/tables/MetadataEntryComponent.scala @@ -95,22 +95,6 @@ trait MetadataEntryComponent { }.size ) - val metadataEntryExistsForWorkflowExecutionUuid = Compiled((workflowExecutionUuid: Rep[String]) => - (for { - metadataEntry <- metadataEntries - if metadataEntry.workflowExecutionUuid === workflowExecutionUuid - } yield metadataEntry).exists - ) - - def metadataEntryExistsForWorkflowExecutionUuid(workflowId: Rep[String], key: Rep[String]): Rep[Boolean] = - metadataEntries - .filter(metadataEntry => - metadataEntry.workflowExecutionUuid === workflowId && - metadataEntry.metadataKey === key && - metadataEntry.metadataValue.isDefined - ) - .exists - val metadataEntriesForWorkflowExecutionUuidAndMetadataKey = Compiled((workflowExecutionUuid: Rep[String], metadataKey: Rep[String]) => (for { diff --git a/database/sql/src/main/scala/cromwell/database/sql/MetadataSqlDatabase.scala b/database/sql/src/main/scala/cromwell/database/sql/MetadataSqlDatabase.scala index 5746af4f501..85465f05b1d 100644 --- a/database/sql/src/main/scala/cromwell/database/sql/MetadataSqlDatabase.scala +++ b/database/sql/src/main/scala/cromwell/database/sql/MetadataSqlDatabase.scala @@ -37,8 +37,6 @@ trait MetadataSqlDatabase extends SqlDatabase { labelMetadataKey: String )(implicit ec: ExecutionContext): Future[Unit] - def metadataEntryExists(workflowExecutionUuid: String)(implicit ec: ExecutionContext): Future[Boolean] - def metadataSummaryEntryExists(workflowExecutionUuid: String)(implicit ec: ExecutionContext): Future[Boolean] def queryMetadataEntries(workflowExecutionUuid: String, timeout: Duration)(implicit diff --git a/engine/src/main/scala/cromwell/webservice/routes/CromwellApiService.scala b/engine/src/main/scala/cromwell/webservice/routes/CromwellApiService.scala index d75eed4ccd8..ce44b53fc87 100644 --- a/engine/src/main/scala/cromwell/webservice/routes/CromwellApiService.scala +++ b/engine/src/main/scala/cromwell/webservice/routes/CromwellApiService.scala @@ -123,7 +123,7 @@ trait CromwellApiService } ~ path("workflows" / Segment / Segment / "timing") { (_, possibleWorkflowId) => instrumentRequest { - onComplete(validateWorkflowIdInMetadata(possibleWorkflowId, serviceRegistryActor)) { + onComplete(validateWorkflowIdInMetadataSummaries(possibleWorkflowId, serviceRegistryActor)) { case Success(workflowId) => completeTimingRouteResponse(metadataLookupForTimingRoute(workflowId)) case Failure(e: UnrecognizedWorkflowException) => e.failRequest(StatusCodes.NotFound) case Failure(e: InvalidWorkflowException) => e.failRequest(StatusCodes.BadRequest) @@ -159,7 +159,7 @@ trait CromwellApiService path("workflows" / Segment / Segment / "releaseHold") { (_, possibleWorkflowId) => post { instrumentRequest { - val response = validateWorkflowIdInMetadata(possibleWorkflowId, serviceRegistryActor) flatMap { + val response = validateWorkflowIdInMetadataSummaries(possibleWorkflowId, serviceRegistryActor) flatMap { workflowId => workflowStoreActor .ask(WorkflowStoreActor.WorkflowOnHoldToSubmittedCommand(workflowId)) @@ -319,20 +319,6 @@ object CromwellApiService { case e: Exception => e.errorRequest(StatusCodes.InternalServerError) } - def validateWorkflowIdInMetadata(possibleWorkflowId: String, serviceRegistryActor: ActorRef)(implicit - timeout: Timeout, - executor: ExecutionContext - ): Future[WorkflowId] = - Try(WorkflowId.fromString(possibleWorkflowId)) match { - case Success(w) => - serviceRegistryActor.ask(ValidateWorkflowIdInMetadata(w)).mapTo[WorkflowValidationResponse] flatMap { - case RecognizedWorkflowId => Future.successful(w) - case UnrecognizedWorkflowId => validateWorkflowIdInMetadataSummaries(possibleWorkflowId, serviceRegistryActor) - case FailedToCheckWorkflowId(t) => Future.failed(t) - } - case Failure(_) => Future.failed(InvalidWorkflowException(possibleWorkflowId)) - } - def validateWorkflowIdInMetadataSummaries(possibleWorkflowId: String, serviceRegistryActor: ActorRef)(implicit timeout: Timeout, executor: ExecutionContext diff --git a/engine/src/main/scala/cromwell/webservice/routes/MetadataRouteSupport.scala b/engine/src/main/scala/cromwell/webservice/routes/MetadataRouteSupport.scala index ee2b4cdd483..0f87b1a2226 100644 --- a/engine/src/main/scala/cromwell/webservice/routes/MetadataRouteSupport.scala +++ b/engine/src/main/scala/cromwell/webservice/routes/MetadataRouteSupport.scala @@ -237,7 +237,7 @@ object MetadataRouteSupport { case FailedToGetArchiveStatusAndEndTime(e) => Future.failed(e) } - validateWorkflowIdInMetadata(possibleWorkflowId, serviceRegistryActor) flatMap { id => + validateWorkflowIdInMetadataSummaries(possibleWorkflowId, serviceRegistryActor) flatMap { id => /* for requests made to one of /metadata, /logs or /outputs endpoints, perform an additional check to see if metadata for the workflow has been archived and deleted or not (as they interact with metadata table) diff --git a/engine/src/main/scala/cromwell/webservice/routes/wes/WesRouteSupport.scala b/engine/src/main/scala/cromwell/webservice/routes/wes/WesRouteSupport.scala index bd5977ae853..c18ed91d007 100644 --- a/engine/src/main/scala/cromwell/webservice/routes/wes/WesRouteSupport.scala +++ b/engine/src/main/scala/cromwell/webservice/routes/wes/WesRouteSupport.scala @@ -27,7 +27,10 @@ import cromwell.services.{FailedMetadataJsonResponse, SuccessfulMetadataJsonResp import cromwell.webservice.PartialWorkflowSources import cromwell.webservice.WebServiceUtils.{completeResponse, materializeFormData, EnhancedThrowable} import cromwell.webservice.routes.CromwellApiService -import cromwell.webservice.routes.CromwellApiService.{validateWorkflowIdInMetadata, UnrecognizedWorkflowException} +import cromwell.webservice.routes.CromwellApiService.{ + validateWorkflowIdInMetadataSummaries, + UnrecognizedWorkflowException +} import cromwell.webservice.routes.MetadataRouteSupport.{metadataBuilderActorRequest, metadataQueryRequest} import cromwell.webservice.routes.wes.WesResponseJsonSupport._ import cromwell.webservice.routes.wes.WesRouteSupport.{respondWithWesError, _} @@ -94,9 +97,10 @@ trait WesRouteSupport extends HttpInstrumentation { } }, path("runs" / Segment / "status") { possibleWorkflowId => - val response = validateWorkflowIdInMetadata(possibleWorkflowId, serviceRegistryActor).flatMap(w => - serviceRegistryActor.ask(GetStatus(w)).mapTo[MetadataServiceResponse] - ) + val response = + validateWorkflowIdInMetadataSummaries(possibleWorkflowId, serviceRegistryActor).flatMap(w => + serviceRegistryActor.ask(GetStatus(w)).mapTo[MetadataServiceResponse] + ) // WES can also return a 401 or a 403 but that requires user auth knowledge which Cromwell doesn't currently have onComplete(response) { case Success(SuccessfulMetadataJsonResponse(_, jsObject)) => diff --git a/engine/src/test/scala/cromwell/webservice/routes/CromwellApiServiceSpec.scala b/engine/src/test/scala/cromwell/webservice/routes/CromwellApiServiceSpec.scala index 7900c02a0c9..d6c46d0beff 100644 --- a/engine/src/test/scala/cromwell/webservice/routes/CromwellApiServiceSpec.scala +++ b/engine/src/test/scala/cromwell/webservice/routes/CromwellApiServiceSpec.scala @@ -574,15 +574,13 @@ object CromwellApiServiceSpec { val SucceededWorkflowId = WorkflowId.fromString("0cb43b8c-0259-4a19-b7fe-921ced326738") val FailedWorkflowId = WorkflowId.fromString("df501790-cef5-4df7-9b48-8760533e3136") val SummarizedWorkflowId = WorkflowId.fromString("f0000000-0000-0000-0000-000000000000") + val UnsummarizedWorkflowId = WorkflowId.fromString("00001111-aaaa-bbbb-cccc-ddddffffeeee") val WorkflowIdExistingOnlyInSummaryTable = WorkflowId.fromString("f0000000-0000-0000-0000-000000000011") val ArchivedWorkflowId = WorkflowId.fromString("c4c6339c-2145-47fb-acc5-b5cb8d2809f5") val ArchivedAndDeletedWorkflowId = WorkflowId.fromString("abc1234d-2145-47fb-acc5-b5cb8d2809f5") val wesWorkflowId = WorkflowId.randomId() - val SummarizedWorkflowIds = Set( - SummarizedWorkflowId, - WorkflowIdExistingOnlyInSummaryTable, - ArchivedWorkflowId, - ArchivedAndDeletedWorkflowId + val UnsummarizedWorkflowIds = Set( + UnsummarizedWorkflowId ) val RecognizedWorkflowIds = Set( ExistingWorkflowId, @@ -593,6 +591,7 @@ object CromwellApiServiceSpec { SucceededWorkflowId, FailedWorkflowId, SummarizedWorkflowId, + WorkflowIdExistingOnlyInSummaryTable, ArchivedWorkflowId, ArchivedAndDeletedWorkflowId, wesWorkflowId @@ -684,11 +683,8 @@ object CromwellApiServiceSpec { None ) sender() ! response - case ValidateWorkflowIdInMetadata(id) => - if (RecognizedWorkflowIds.contains(id)) sender() ! MetadataService.RecognizedWorkflowId - else sender() ! MetadataService.UnrecognizedWorkflowId case ValidateWorkflowIdInMetadataSummaries(id) => - if (SummarizedWorkflowIds.contains(id)) sender() ! MetadataService.RecognizedWorkflowId + if (RecognizedWorkflowIds.contains(id)) sender() ! MetadataService.RecognizedWorkflowId else sender() ! MetadataService.UnrecognizedWorkflowId case FetchWorkflowMetadataArchiveStatusAndEndTime(id) => id match { diff --git a/engine/src/test/scala/cromwell/webservice/routes/MetadataRouteSupportSpec.scala b/engine/src/test/scala/cromwell/webservice/routes/MetadataRouteSupportSpec.scala index 2cd8b64517a..31d3f5098c7 100644 --- a/engine/src/test/scala/cromwell/webservice/routes/MetadataRouteSupportSpec.scala +++ b/engine/src/test/scala/cromwell/webservice/routes/MetadataRouteSupportSpec.scala @@ -573,7 +573,7 @@ class MetadataRouteSupportSpec extends AsyncFlatSpec with ScalatestRouteTest wit |} """.stripMargin - val unsummarizedId = CromwellApiServiceSpec.ExistingWorkflowId + val unsummarizedId = CromwellApiServiceSpec.UnsummarizedWorkflowId Patch(s"/workflows/$version/$unsummarizedId/labels", HttpEntity(ContentTypes.`application/json`, validLabelsJson) ) ~> diff --git a/services/src/main/scala/cromwell/services/metadata/MetadataService.scala b/services/src/main/scala/cromwell/services/metadata/MetadataService.scala index 6c8bd4c7f59..6c50f7fad31 100644 --- a/services/src/main/scala/cromwell/services/metadata/MetadataService.scala +++ b/services/src/main/scala/cromwell/services/metadata/MetadataService.scala @@ -130,7 +130,6 @@ object MetadataService { case object RefreshSummary extends MetadataServiceAction case object SendMetadataTableSizeMetrics extends MetadataServiceAction - final case class ValidateWorkflowIdInMetadata(possibleWorkflowId: WorkflowId) extends MetadataServiceAction final case class ValidateWorkflowIdInMetadataSummaries(possibleWorkflowId: WorkflowId) extends MetadataServiceAction final case class FetchWorkflowMetadataArchiveStatusAndEndTime(workflowId: WorkflowId) extends MetadataServiceAction final case class FetchFailedJobsMetadataWithWorkflowId(workflowId: WorkflowId) extends BuildWorkflowMetadataJsonAction diff --git a/services/src/main/scala/cromwell/services/metadata/impl/MetadataDatabaseAccess.scala b/services/src/main/scala/cromwell/services/metadata/impl/MetadataDatabaseAccess.scala index e0b547370d3..28da7cff228 100644 --- a/services/src/main/scala/cromwell/services/metadata/impl/MetadataDatabaseAccess.scala +++ b/services/src/main/scala/cromwell/services/metadata/impl/MetadataDatabaseAccess.scala @@ -303,9 +303,6 @@ trait MetadataDatabaseAccess { _ map { case (id, labelsForId) => WorkflowId.fromString(id) -> labelsForId } } - def workflowWithIdExistsInMetadata(possibleWorkflowId: String)(implicit ec: ExecutionContext): Future[Boolean] = - metadataDatabaseInterface.metadataEntryExists(possibleWorkflowId) - def workflowWithIdExistsInMetadataSummaries(possibleWorkflowId: String)(implicit ec: ExecutionContext ): Future[Boolean] = diff --git a/services/src/main/scala/cromwell/services/metadata/impl/MetadataServiceActor.scala b/services/src/main/scala/cromwell/services/metadata/impl/MetadataServiceActor.scala index bcf6346b601..245a61ade3d 100644 --- a/services/src/main/scala/cromwell/services/metadata/impl/MetadataServiceActor.scala +++ b/services/src/main/scala/cromwell/services/metadata/impl/MetadataServiceActor.scala @@ -194,16 +194,6 @@ case class MetadataServiceActor(serviceConfig: Config, globalConfig: Config, ser None } - private def validateWorkflowIdInMetadata(possibleWorkflowId: WorkflowId, sender: ActorRef): Unit = - workflowWithIdExistsInMetadata(possibleWorkflowId.toString) onComplete { - case Success(true) => sender ! RecognizedWorkflowId - case Success(false) => sender ! UnrecognizedWorkflowId - case Failure(e) => - sender ! FailedToCheckWorkflowId( - new RuntimeException(s"Failed lookup attempt for workflow ID $possibleWorkflowId", e) - ) - } - private def validateWorkflowIdInMetadataSummaries(possibleWorkflowId: WorkflowId, sender: ActorRef): Unit = workflowWithIdExistsInMetadataSummaries(possibleWorkflowId.toString) onComplete { case Success(true) => sender ! RecognizedWorkflowId @@ -258,7 +248,6 @@ case class MetadataServiceActor(serviceConfig: Config, globalConfig: Config, ser case action: PutMetadataActionAndRespond => writeActor forward action // Assume that listen messages are directed to the write metadata actor case listen: Listen => writeActor forward listen - case v: ValidateWorkflowIdInMetadata => validateWorkflowIdInMetadata(v.possibleWorkflowId, sender()) case v: ValidateWorkflowIdInMetadataSummaries => validateWorkflowIdInMetadataSummaries(v.possibleWorkflowId, sender()) case g: FetchWorkflowMetadataArchiveStatusAndEndTime => diff --git a/services/src/test/scala/cromwell/services/metadata/impl/WriteMetadataActorSpec.scala b/services/src/test/scala/cromwell/services/metadata/impl/WriteMetadataActorSpec.scala index 007712dfb9f..c64a36cffb1 100644 --- a/services/src/test/scala/cromwell/services/metadata/impl/WriteMetadataActorSpec.scala +++ b/services/src/test/scala/cromwell/services/metadata/impl/WriteMetadataActorSpec.scala @@ -372,9 +372,6 @@ class WriteMetadataActorSpec extends TestKitSuite with AnyFlatSpecLike with Matc Future.failed(WriteMetadataActorSpec.IntermittentException) } - override def metadataEntryExists(workflowExecutionUuid: String)(implicit ec: ExecutionContext): Nothing = - notImplemented() - override def metadataSummaryEntryExists(workflowExecutionUuid: String)(implicit ec: ExecutionContext): Nothing = notImplemented()