From cbe29ffc78c2138c4efc7f4fef893f5bb9c2dc90 Mon Sep 17 00:00:00 2001 From: Rob Title Date: Fri, 9 Jun 2017 14:43:11 -0400 Subject: [PATCH 01/18] GAWB-2056: WorkspaceSubmissionStats should reflect the status of the most recent submission, not the most recent workflow --- .../dataaccess/slick/DriverComponent.scala | 46 ++++++++++++++-- .../dataaccess/slick/WorkspaceComponent.scala | 54 +++++++++++-------- .../slick/WorkspaceComponentSpec.scala | 34 ++++++++++++ 3 files changed, 109 insertions(+), 25 deletions(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponent.scala index deafa19883..4b698de1b2 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponent.scala @@ -4,14 +4,15 @@ import java.nio.ByteOrder import java.sql.Timestamp import java.util.UUID -import akka.util.{ByteString, ByteStringBuilder} +import akka.util.ByteString +import cats._ +import cats.implicits._ +import org.apache.commons.codec.binary.Base64 import org.broadinstitute.dsde.rawls.model._ import org.broadinstitute.dsde.rawls.{RawlsException, RawlsExceptionWithErrorReport} -import org.joda.time.DateTime import slick.driver.JdbcDriver import slick.jdbc.{GetResult, PositionedParameters, SQLActionBuilder, SetParameter} import spray.http.StatusCodes -import org.apache.commons.codec.binary.Base64 import scala.concurrent.ExecutionContext @@ -119,6 +120,45 @@ trait DriverComponent { def renameForHiding(recordCount: Long, name: String): String = { name + "_" + getSufficientlyRandomSuffix(recordCount) } + + // Note: if we used shapeless we could abstract the following functions further by abstracting over the arity. + // But I'm not going to add shapeless just for this. :) + + /** + * Converts a `Seq[(A, B)]` into a `Map[A, B]`, combining the values with a `Monoid[B]` in case of key conflicts. + * + * For example: + * {{{ + * scala> groupPairs(Seq(("a", 1), ("b", 2), ("a", 3))) + * res0: Map[String,Int] = Map(b -> 2, a -> 4) + * }}} + * */ + def groupPairs[A, B: Monoid](pairs: Seq[(A, B)]): Map[A, B] = + pairs.toList.foldMap { case (a, b) => Map(a -> b) } + + /** + * Converts a `Seq[(A, F[B])]` into a `Map[A, F[B]]`, combining the values with the _universal_ monoid for type F. + * This can be useful for when using groupBy with slick aggregate functions such as max, min, avg, etc which return + * an Option. + * + * For example: + * {{{ + * scala> case class Foo(i: Int) + * defined class Foo + * scala> groupPairsK(Seq(("a", Foo(1).some), ("b", Foo(2).some), ("c", Foo(3).some))) + * res9: Map[String,Option[Foo]] = Map(b -> Some(Foo(2)), a -> Some(Foo(1)), c -> Some(Foo(3))) + * }}} + * + */ + def groupPairsK[F[_], A, B](pairs: Seq[(A, F[B])])(implicit M: MonoidK[F]): Map[A, F[B]] = + groupPairs(pairs)(M.algebra[B]) + + def groupTriples[A, B, C: Monoid](trips: Seq[(A, B, C)]): Map[A, Map[B, C]] = + trips.toList.foldMap { case (a, b, c) => Map(a -> Map(b -> c)) } + + def groupTriplesK[F[_], A, B, C](trips: Seq[(A, B, F[C])])(implicit M: MonoidK[F]): Map[A, Map[B, F[C]]] = { + groupTriples(trips)(M.algebra[C]) + } } /** diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala index 9822acd8d3..6a4a2d2c34 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala @@ -3,13 +3,14 @@ package org.broadinstitute.dsde.rawls.dataaccess.slick import java.sql.Timestamp import java.util.{Date, UUID} +import cats.instances.int._ +import cats.instances.option._ import org.broadinstitute.dsde.rawls.RawlsException +import org.broadinstitute.dsde.rawls.dataaccess.SlickWorkspaceContext +import org.broadinstitute.dsde.rawls.model.Attributable.AttributeMap import org.broadinstitute.dsde.rawls.model.WorkspaceAccessLevels.WorkspaceAccessLevel import org.broadinstitute.dsde.rawls.model._ import org.joda.time.DateTime -import org.broadinstitute.dsde.rawls.dataaccess.SlickWorkspaceContext -import org.broadinstitute.dsde.rawls.model.Attributable.AttributeMap - /** * Created by dvoet on 2/4/16. */ @@ -499,21 +500,38 @@ trait WorkspaceComponent { } /** - * gets the submission stats (last workflow failed date, last workflow success date, running submission count) + * gets the submission stats (last submission failed date, last submission success date, running submission count) * for each workspace * * @param workspaceIds the workspace ids to query for * @return WorkspaceSubmissionStats keyed by workspace id */ def listSubmissionSummaryStats(workspaceIds: Seq[UUID]): ReadAction[Map[UUID, WorkspaceSubmissionStats]] = { - // workflow date query: select workspaceId, workflow.status, max(workflow.statusLastChangedDate) ... group by workspaceId, workflow.status - val workflowDatesQuery = for { + // submission date query: + // + // select workspaceId, status, max(submissionDate) + // from ( + // select submission.id, submission.workspaceId, workflow.status, max(submission.submissionDate) + // from submission + // join workflow on workflow.submissionId = s.id + // where submission.workspaceId in (:workspaceIds) + // group by submission.id, submission.workspaceId, workflow.status) v + // group by v.workspaceId, v.status + // + val submissionDatesQuery = for { submissions <- submissionQuery if submissions.workspaceId.inSetBind(workspaceIds) workflows <- workflowQuery if submissions.id === workflows.submissionId - } yield (submissions.workspaceId, workflows.status, workflows.statusLastChangedDate) - - val workflowDatesGroupedQuery = workflowDatesQuery.groupBy { case (wsId, status, _) => (wsId, status) }. - map { case ((wsId, wfStatus), records) => (wsId, wfStatus, records.map { case (_, _, lastChanged) => lastChanged }.max) } + } yield (submissions.id, submissions.workspaceId, workflows.status, submissions.submissionDate) + + val submissionDatesGroupedQuery = submissionDatesQuery.groupBy { case (subId, wsId, status, _) => + (subId, wsId, status) + }.map { case ((subId, wsId, status), records) => + (subId, wsId, status, records.map(_._4).max) + }.groupBy { case (_, wsId, status, _) => + (wsId, status) + }.map { case ((wsId, status), records) => + (wsId, status, records.map(_._4).max) + } // running submission query: select workspaceId, count(1) ... where submissions.status === Submitted group by workspaceId val runningSubmissionsQuery = (for { @@ -521,14 +539,14 @@ trait WorkspaceComponent { } yield submissions).groupBy(_.workspaceId).map { case (wfId, submissions) => (wfId, submissions.length)} for { - workflowDates <- workflowDatesGroupedQuery.result + submissionDates <- submissionDatesGroupedQuery.result runningSubmissions <- runningSubmissionsQuery.result } yield { - val workflowDatesByWorkspaceByStatus: Map[UUID, Map[String, Option[Timestamp]]] = groupByWorkspaceIdThenStatus(workflowDates) - val runningSubmissionCountByWorkspace: Map[UUID, Int] = groupByWorkspaceId(runningSubmissions) + val submissionDatesByWorkspaceByStatus = groupTriplesK(submissionDates) + val runningSubmissionCountByWorkspace = groupPairs(runningSubmissions) workspaceIds.map { wsId => - val (lastFailedDate, lastSuccessDate) = workflowDatesByWorkspaceByStatus.get(wsId) match { + val (lastFailedDate, lastSuccessDate) = submissionDatesByWorkspaceByStatus.get(wsId) match { case None => (None, None) case Some(datesByStatus) => (datesByStatus.getOrElse(WorkflowStatuses.Failed.toString, None), datesByStatus.getOrElse(WorkflowStatuses.Succeeded.toString, None)) @@ -731,14 +749,6 @@ trait WorkspaceComponent { WorkspaceGroups(toGroupMap(realmAclRecs), toGroupMap(accessGroupRecs)) } } - - private def groupByWorkspaceId(runningSubmissions: Seq[(UUID, Int)]): Map[UUID, Int] = { - runningSubmissions.groupBy{ case (wsId, count) => wsId }.mapValues { case Seq((_, count)) => count } - } - - private def groupByWorkspaceIdThenStatus(workflowDates: Seq[(UUID, String, Option[Timestamp])]): Map[UUID, Map[String, Option[Timestamp]]] = { - workflowDates.groupBy { case (wsId, _, _) => wsId }.mapValues(_.groupBy { case (_, status, _) => status }.mapValues { case Seq((_, _, timestamp)) => timestamp }) - } } private case class WorkspaceGroups( diff --git a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala index b2b2de03d6..cf69485af0 100644 --- a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala +++ b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala @@ -114,4 +114,38 @@ class WorkspaceComponentSpec extends TestDriverComponentWithFlatSpecAndMatchers runAndWait(workspaceQuery.delete(workspace.toWorkspaceName)) } } + + it should "list submission summary stats" in withDefaultTestDatabase { + implicit def toWorkspaceId(ws: Workspace): UUID = UUID.fromString(ws.workspaceId) + + val wsIdNoSubmissions: UUID = testData.workspaceNoSubmissions + assertResult(Map(wsIdNoSubmissions -> WorkspaceSubmissionStats(None, None, 0))) { + runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdNoSubmissions))) + } + + val wsIdSuccessfulSubmission: UUID = testData.workspaceSuccessfulSubmission + assertResult(Map(wsIdSuccessfulSubmission -> WorkspaceSubmissionStats(Some(testDate), None, 0))) { + runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdSuccessfulSubmission))) + } + + val wsIdFailedSubmission: UUID = testData.workspaceFailedSubmission + assertResult(Map(wsIdFailedSubmission -> WorkspaceSubmissionStats(None, Some(testDate), 0))) { + runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdFailedSubmission))) + } + + val wsIdSubmittedSubmission: UUID = testData.workspaceSubmittedSubmission + assertResult(Map(wsIdSubmittedSubmission -> WorkspaceSubmissionStats(None, None, 1))) { + runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdSubmittedSubmission))) + } + + val wsIdMixedSubmission: UUID = testData.workspaceMixedSubmissions + assertResult(Map(wsIdMixedSubmission -> WorkspaceSubmissionStats(Some(testDate), Some(testDate), 1))) { + runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdMixedSubmission))) + } + + val wsIdTerminatedSubmission: UUID = testData.workspaceTerminatedSubmissions + assertResult(Map(wsIdTerminatedSubmission -> WorkspaceSubmissionStats(Some(testDate), Some(testDate), 0))) { + runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdTerminatedSubmission))) + } + } } From 21b62b0e185e8a7c03157c7c5bb503769f747169 Mon Sep 17 00:00:00 2001 From: Rob Title Date: Fri, 9 Jun 2017 16:37:29 -0400 Subject: [PATCH 02/18] Fix SQL query logic --- .../dataaccess/slick/WorkspaceComponent.scala | 42 ++++++++++++------- .../slick/WorkspaceComponentSpec.scala | 2 +- 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala index 6a4a2d2c34..c69b0478de 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala @@ -507,30 +507,40 @@ trait WorkspaceComponent { * @return WorkspaceSubmissionStats keyed by workspace id */ def listSubmissionSummaryStats(workspaceIds: Seq[UUID]): ReadAction[Map[UUID, WorkspaceSubmissionStats]] = { + // submission date query: // // select workspaceId, status, max(submissionDate) // from ( - // select submission.id, submission.workspaceId, workflow.status, max(submission.submissionDate) + // select submission.workspaceId, workflow.status, submission.submissionDate, count(1) // from submission - // join workflow on workflow.submissionId = s.id + // join workflow on workflow.submissionId = submission.id // where submission.workspaceId in (:workspaceIds) - // group by submission.id, submission.workspaceId, workflow.status) v - // group by v.workspaceId, v.status + // group by 1, 2, 3) v + // where (status = 'Failure' or (status = 'Succeeded' and count = 1) + // group by 1, 2 // - val submissionDatesQuery = for { + val workflowStatusQuery = for { submissions <- submissionQuery if submissions.workspaceId.inSetBind(workspaceIds) workflows <- workflowQuery if submissions.id === workflows.submissionId - } yield (submissions.id, submissions.workspaceId, workflows.status, submissions.submissionDate) - - val submissionDatesGroupedQuery = submissionDatesQuery.groupBy { case (subId, wsId, status, _) => - (subId, wsId, status) - }.map { case ((subId, wsId, status), records) => - (subId, wsId, status, records.map(_._4).max) - }.groupBy { case (_, wsId, status, _) => - (wsId, status) - }.map { case ((wsId, status), records) => - (wsId, status, records.map(_._4).max) + } yield (submissions.workspaceId, workflows.status, submissions.submissionDate) + + val groupedWorkflowStatusQuery = workflowStatusQuery.groupBy { case (wsId, status, submissionDate) => + (wsId, status, submissionDate) + }.map { case ((wsId, status, submissionDate), records) => + (wsId, status, submissionDate, records.map(_._3).length) + } + + // Note: a submission is successful if it contains _only_ successful workflows. + // A submission is a failure if it contains _any_ failed workflows. + val filteredWorkflowStatusQuery = groupedWorkflowStatusQuery.filter { case (_, status, _, count) => + status === WorkflowStatuses.Failed.toString || (status === WorkflowStatuses.Succeeded.toString && count === 1) + } + + val submissionDateQuery = filteredWorkflowStatusQuery.groupBy { case (workspaceId, status, submissionDate, _) => + (workspaceId, status) + }.map { case ((workspaceId, status), recs) => + (workspaceId, status, recs.map(_._3).max) } // running submission query: select workspaceId, count(1) ... where submissions.status === Submitted group by workspaceId @@ -539,7 +549,7 @@ trait WorkspaceComponent { } yield submissions).groupBy(_.workspaceId).map { case (wfId, submissions) => (wfId, submissions.length)} for { - submissionDates <- submissionDatesGroupedQuery.result + submissionDates <- submissionDateQuery.result runningSubmissions <- runningSubmissionsQuery.result } yield { val submissionDatesByWorkspaceByStatus = groupTriplesK(submissionDates) diff --git a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala index cf69485af0..66374efe74 100644 --- a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala +++ b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala @@ -139,7 +139,7 @@ class WorkspaceComponentSpec extends TestDriverComponentWithFlatSpecAndMatchers } val wsIdMixedSubmission: UUID = testData.workspaceMixedSubmissions - assertResult(Map(wsIdMixedSubmission -> WorkspaceSubmissionStats(Some(testDate), Some(testDate), 1))) { + assertResult(Map(wsIdMixedSubmission -> WorkspaceSubmissionStats(None, Some(testDate), 1))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdMixedSubmission))) } From fe4fcfa1f7f5703431bdd74f85358beec630f6f2 Mon Sep 17 00:00:00 2001 From: Rob Title Date: Fri, 9 Jun 2017 16:45:27 -0400 Subject: [PATCH 03/18] Cleaned up comments a bit --- .../dsde/rawls/dataaccess/slick/DriverComponent.scala | 11 +++++++---- .../rawls/dataaccess/slick/WorkspaceComponent.scala | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponent.scala index 4b698de1b2..f354273053 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponent.scala @@ -121,9 +121,6 @@ trait DriverComponent { name + "_" + getSufficientlyRandomSuffix(recordCount) } - // Note: if we used shapeless we could abstract the following functions further by abstracting over the arity. - // But I'm not going to add shapeless just for this. :) - /** * Converts a `Seq[(A, B)]` into a `Map[A, B]`, combining the values with a `Monoid[B]` in case of key conflicts. * @@ -145,14 +142,20 @@ trait DriverComponent { * {{{ * scala> case class Foo(i: Int) * defined class Foo + * + * scala> groupPairs(Seq(("a", Foo(1).some), ("b", Foo(2).some), ("c", Foo(3).some))) + * << does not compile as there is no Monoid instance for Foo >> + * * scala> groupPairsK(Seq(("a", Foo(1).some), ("b", Foo(2).some), ("c", Foo(3).some))) * res9: Map[String,Option[Foo]] = Map(b -> Some(Foo(2)), a -> Some(Foo(1)), c -> Some(Foo(3))) * }}} - * */ def groupPairsK[F[_], A, B](pairs: Seq[(A, F[B])])(implicit M: MonoidK[F]): Map[A, F[B]] = groupPairs(pairs)(M.algebra[B]) + // Same as above but with triples. + // Note: if we used shapeless we could generalize these functions for any arity. + // But I'm not going to add shapeless just for this. :) def groupTriples[A, B, C: Monoid](trips: Seq[(A, B, C)]): Map[A, Map[B, C]] = trips.toList.foldMap { case (a, b, c) => Map(a -> Map(b -> c)) } diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala index c69b0478de..1a3c1cb5f3 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala @@ -519,7 +519,7 @@ trait WorkspaceComponent { // group by 1, 2, 3) v // where (status = 'Failure' or (status = 'Succeeded' and count = 1) // group by 1, 2 - // + val workflowStatusQuery = for { submissions <- submissionQuery if submissions.workspaceId.inSetBind(workspaceIds) workflows <- workflowQuery if submissions.id === workflows.submissionId From 3ee7afb5d4410d21a012fda9452518292b85ed09 Mon Sep 17 00:00:00 2001 From: Rob Title Date: Fri, 9 Jun 2017 17:05:06 -0400 Subject: [PATCH 04/18] Fix broken workspace api tests due to behavior change --- .../dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala | 1 + .../dsde/rawls/webservice/WorkspaceApiServiceSpec.scala | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala index 66374efe74..60534db4a1 100644 --- a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala +++ b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala @@ -138,6 +138,7 @@ class WorkspaceComponentSpec extends TestDriverComponentWithFlatSpecAndMatchers runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdSubmittedSubmission))) } + // Note: a submission with both a successful and failed workflow is a failure val wsIdMixedSubmission: UUID = testData.workspaceMixedSubmissions assertResult(Map(wsIdMixedSubmission -> WorkspaceSubmissionStats(None, Some(testDate), 1))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdMixedSubmission))) diff --git a/core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/WorkspaceApiServiceSpec.scala b/core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/WorkspaceApiServiceSpec.scala index fdb6232f6e..ee88a240d3 100644 --- a/core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/WorkspaceApiServiceSpec.scala +++ b/core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/WorkspaceApiServiceSpec.scala @@ -384,7 +384,7 @@ class WorkspaceApiServiceSpec extends ApiServiceSpec { } val dateTime = currentTime() assertResult( - WorkspaceListResponse(WorkspaceAccessLevels.Owner, testWorkspaces.workspace.copy(lastModified = dateTime), WorkspaceSubmissionStats(Option(testDate), Option(testDate), 2), Seq(testData.userOwner.userEmail.value)) + WorkspaceListResponse(WorkspaceAccessLevels.Owner, testWorkspaces.workspace.copy(lastModified = dateTime), WorkspaceSubmissionStats(None, Option(testDate), 2), Seq(testData.userOwner.userEmail.value)) ){ val response = responseAs[WorkspaceListResponse] WorkspaceListResponse(response.accessLevel, response.workspace.copy(lastModified = dateTime), response.workspaceSubmissionStats, response.owners) @@ -520,7 +520,7 @@ class WorkspaceApiServiceSpec extends ApiServiceSpec { val dateTime = currentTime() assertResult(Set( - WorkspaceListResponse(WorkspaceAccessLevels.Owner, testWorkspaces.workspace.copy(lastModified = dateTime), WorkspaceSubmissionStats(Option(testDate), Option(testDate), 2), Seq(testData.userOwner.userEmail.value)), + WorkspaceListResponse(WorkspaceAccessLevels.Owner, testWorkspaces.workspace.copy(lastModified = dateTime), WorkspaceSubmissionStats(None, Option(testDate), 2), Seq(testData.userOwner.userEmail.value)), WorkspaceListResponse(WorkspaceAccessLevels.Write, testWorkspaces.workspace2.copy(lastModified = dateTime), WorkspaceSubmissionStats(None, None, 0), Seq.empty), WorkspaceListResponse(WorkspaceAccessLevels.NoAccess, testWorkspaces.workspace3.copy(lastModified = dateTime), WorkspaceSubmissionStats(None, None, 0), Seq.empty) )) { From 5144a528b66a396def7df68f1f567ee58175091e Mon Sep 17 00:00:00 2001 From: Rob Title Date: Fri, 9 Jun 2017 17:21:21 -0400 Subject: [PATCH 05/18] Update swagger doc --- core/src/main/resources/swagger/rawls.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/resources/swagger/rawls.yaml b/core/src/main/resources/swagger/rawls.yaml index 056cc73625..293cad1f06 100644 --- a/core/src/main/resources/swagger/rawls.yaml +++ b/core/src/main/resources/swagger/rawls.yaml @@ -5051,11 +5051,11 @@ definitions: lastSuccessDate: type: string format: date-time - description: The date of the last successful workflow + description: The date of the last successful submission lastFailureDate: type: string format: date-time - description: The date of the last failed workflow + description: The date of the last failed submission runningSubmissionsCount: type: integer description: Count of all the running submissions From 7136d428fcbca1cbcaff9d61343da7cd371691b9 Mon Sep 17 00:00:00 2001 From: Rob Title Date: Fri, 9 Jun 2017 17:59:16 -0400 Subject: [PATCH 06/18] Bah, fix query logic - should use having instead of where --- .../dataaccess/slick/WorkspaceComponent.scala | 31 +++++++------------ .../slick/WorkspaceComponentSpec.scala | 2 +- .../webservice/WorkspaceApiServiceSpec.scala | 4 +-- 3 files changed, 15 insertions(+), 22 deletions(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala index 1a3c1cb5f3..722ff5ddce 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala @@ -512,36 +512,29 @@ trait WorkspaceComponent { // // select workspaceId, status, max(submissionDate) // from ( - // select submission.workspaceId, workflow.status, submission.submissionDate, count(1) + // select distinct submission.workspaceId, workflow.status, submission.submissionDate // from submission // join workflow on workflow.submissionId = submission.id - // where submission.workspaceId in (:workspaceIds) - // group by 1, 2, 3) v - // where (status = 'Failure' or (status = 'Succeeded' and count = 1) + // where submission.workspaceId in (:workspaceIds)) v // group by 1, 2 + // having (status = 'Failure' or (status = 'Succeeded' and count(v.*) = 1)) - val workflowStatusQuery = for { + val workflowStatusQuery = (for { submissions <- submissionQuery if submissions.workspaceId.inSetBind(workspaceIds) workflows <- workflowQuery if submissions.id === workflows.submissionId - } yield (submissions.workspaceId, workflows.status, submissions.submissionDate) + } yield (submissions.workspaceId, workflows.status, submissions.submissionDate)).distinct - val groupedWorkflowStatusQuery = workflowStatusQuery.groupBy { case (wsId, status, submissionDate) => - (wsId, status, submissionDate) - }.map { case ((wsId, status, submissionDate), records) => - (wsId, status, submissionDate, records.map(_._3).length) + val submissionMaxDateQuery = workflowStatusQuery.groupBy { case (workspaceId, status, submissionDate) => + (workspaceId, status) + }.map { case ((workspaceId, status), recs) => + (workspaceId, status, recs.map(_._3).max, recs.length) } // Note: a submission is successful if it contains _only_ successful workflows. // A submission is a failure if it contains _any_ failed workflows. - val filteredWorkflowStatusQuery = groupedWorkflowStatusQuery.filter { case (_, status, _, count) => + val filteredSubmissionMaxDateQuery = submissionMaxDateQuery.filter { case (_, status, _, count) => status === WorkflowStatuses.Failed.toString || (status === WorkflowStatuses.Succeeded.toString && count === 1) - } - - val submissionDateQuery = filteredWorkflowStatusQuery.groupBy { case (workspaceId, status, submissionDate, _) => - (workspaceId, status) - }.map { case ((workspaceId, status), recs) => - (workspaceId, status, recs.map(_._3).max) - } + }.map { case (workspaceId, status, max, _) => (workspaceId, status, max)} // running submission query: select workspaceId, count(1) ... where submissions.status === Submitted group by workspaceId val runningSubmissionsQuery = (for { @@ -549,7 +542,7 @@ trait WorkspaceComponent { } yield submissions).groupBy(_.workspaceId).map { case (wfId, submissions) => (wfId, submissions.length)} for { - submissionDates <- submissionDateQuery.result + submissionDates <- filteredSubmissionMaxDateQuery.result runningSubmissions <- runningSubmissionsQuery.result } yield { val submissionDatesByWorkspaceByStatus = groupTriplesK(submissionDates) diff --git a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala index 60534db4a1..687803412c 100644 --- a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala +++ b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala @@ -140,7 +140,7 @@ class WorkspaceComponentSpec extends TestDriverComponentWithFlatSpecAndMatchers // Note: a submission with both a successful and failed workflow is a failure val wsIdMixedSubmission: UUID = testData.workspaceMixedSubmissions - assertResult(Map(wsIdMixedSubmission -> WorkspaceSubmissionStats(None, Some(testDate), 1))) { + assertResult(Map(wsIdMixedSubmission -> WorkspaceSubmissionStats(Some(testDate), Some(testDate), 1))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdMixedSubmission))) } diff --git a/core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/WorkspaceApiServiceSpec.scala b/core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/WorkspaceApiServiceSpec.scala index ee88a240d3..fdb6232f6e 100644 --- a/core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/WorkspaceApiServiceSpec.scala +++ b/core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/WorkspaceApiServiceSpec.scala @@ -384,7 +384,7 @@ class WorkspaceApiServiceSpec extends ApiServiceSpec { } val dateTime = currentTime() assertResult( - WorkspaceListResponse(WorkspaceAccessLevels.Owner, testWorkspaces.workspace.copy(lastModified = dateTime), WorkspaceSubmissionStats(None, Option(testDate), 2), Seq(testData.userOwner.userEmail.value)) + WorkspaceListResponse(WorkspaceAccessLevels.Owner, testWorkspaces.workspace.copy(lastModified = dateTime), WorkspaceSubmissionStats(Option(testDate), Option(testDate), 2), Seq(testData.userOwner.userEmail.value)) ){ val response = responseAs[WorkspaceListResponse] WorkspaceListResponse(response.accessLevel, response.workspace.copy(lastModified = dateTime), response.workspaceSubmissionStats, response.owners) @@ -520,7 +520,7 @@ class WorkspaceApiServiceSpec extends ApiServiceSpec { val dateTime = currentTime() assertResult(Set( - WorkspaceListResponse(WorkspaceAccessLevels.Owner, testWorkspaces.workspace.copy(lastModified = dateTime), WorkspaceSubmissionStats(None, Option(testDate), 2), Seq(testData.userOwner.userEmail.value)), + WorkspaceListResponse(WorkspaceAccessLevels.Owner, testWorkspaces.workspace.copy(lastModified = dateTime), WorkspaceSubmissionStats(Option(testDate), Option(testDate), 2), Seq(testData.userOwner.userEmail.value)), WorkspaceListResponse(WorkspaceAccessLevels.Write, testWorkspaces.workspace2.copy(lastModified = dateTime), WorkspaceSubmissionStats(None, None, 0), Seq.empty), WorkspaceListResponse(WorkspaceAccessLevels.NoAccess, testWorkspaces.workspace3.copy(lastModified = dateTime), WorkspaceSubmissionStats(None, None, 0), Seq.empty) )) { From 22d8f0b239f7df9d796ffcdd8b5e28332e53ace3 Mon Sep 17 00:00:00 2001 From: Rob Title Date: Tue, 13 Jun 2017 11:11:25 -0400 Subject: [PATCH 07/18] Code review feedback --- .../dataaccess/slick/DriverComponent.scala | 44 ------------------- .../dataaccess/slick/WorkspaceComponent.scala | 25 ++++++++++- .../dsde/rawls/util/CollectionUtils.scala | 19 ++++++++ 3 files changed, 42 insertions(+), 46 deletions(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponent.scala index f354273053..dd1d0877b1 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponent.scala @@ -5,8 +5,6 @@ import java.sql.Timestamp import java.util.UUID import akka.util.ByteString -import cats._ -import cats.implicits._ import org.apache.commons.codec.binary.Base64 import org.broadinstitute.dsde.rawls.model._ import org.broadinstitute.dsde.rawls.{RawlsException, RawlsExceptionWithErrorReport} @@ -120,48 +118,6 @@ trait DriverComponent { def renameForHiding(recordCount: Long, name: String): String = { name + "_" + getSufficientlyRandomSuffix(recordCount) } - - /** - * Converts a `Seq[(A, B)]` into a `Map[A, B]`, combining the values with a `Monoid[B]` in case of key conflicts. - * - * For example: - * {{{ - * scala> groupPairs(Seq(("a", 1), ("b", 2), ("a", 3))) - * res0: Map[String,Int] = Map(b -> 2, a -> 4) - * }}} - * */ - def groupPairs[A, B: Monoid](pairs: Seq[(A, B)]): Map[A, B] = - pairs.toList.foldMap { case (a, b) => Map(a -> b) } - - /** - * Converts a `Seq[(A, F[B])]` into a `Map[A, F[B]]`, combining the values with the _universal_ monoid for type F. - * This can be useful for when using groupBy with slick aggregate functions such as max, min, avg, etc which return - * an Option. - * - * For example: - * {{{ - * scala> case class Foo(i: Int) - * defined class Foo - * - * scala> groupPairs(Seq(("a", Foo(1).some), ("b", Foo(2).some), ("c", Foo(3).some))) - * << does not compile as there is no Monoid instance for Foo >> - * - * scala> groupPairsK(Seq(("a", Foo(1).some), ("b", Foo(2).some), ("c", Foo(3).some))) - * res9: Map[String,Option[Foo]] = Map(b -> Some(Foo(2)), a -> Some(Foo(1)), c -> Some(Foo(3))) - * }}} - */ - def groupPairsK[F[_], A, B](pairs: Seq[(A, F[B])])(implicit M: MonoidK[F]): Map[A, F[B]] = - groupPairs(pairs)(M.algebra[B]) - - // Same as above but with triples. - // Note: if we used shapeless we could generalize these functions for any arity. - // But I'm not going to add shapeless just for this. :) - def groupTriples[A, B, C: Monoid](trips: Seq[(A, B, C)]): Map[A, Map[B, C]] = - trips.toList.foldMap { case (a, b, c) => Map(a -> Map(b -> c)) } - - def groupTriplesK[F[_], A, B, C](trips: Seq[(A, B, F[C])])(implicit M: MonoidK[F]): Map[A, Map[B, F[C]]] = { - groupTriples(trips)(M.algebra[C]) - } } /** diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala index 722ff5ddce..3815ac7524 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala @@ -3,6 +3,7 @@ package org.broadinstitute.dsde.rawls.dataaccess.slick import java.sql.Timestamp import java.util.{Date, UUID} +import cats.{Monoid, MonoidK} import cats.instances.int._ import cats.instances.option._ import org.broadinstitute.dsde.rawls.RawlsException @@ -10,6 +11,7 @@ import org.broadinstitute.dsde.rawls.dataaccess.SlickWorkspaceContext import org.broadinstitute.dsde.rawls.model.Attributable.AttributeMap import org.broadinstitute.dsde.rawls.model.WorkspaceAccessLevels.WorkspaceAccessLevel import org.broadinstitute.dsde.rawls.model._ +import org.broadinstitute.dsde.rawls.util.CollectionUtils import org.joda.time.DateTime /** * Created by dvoet on 2/4/16. @@ -545,8 +547,8 @@ trait WorkspaceComponent { submissionDates <- filteredSubmissionMaxDateQuery.result runningSubmissions <- runningSubmissionsQuery.result } yield { - val submissionDatesByWorkspaceByStatus = groupTriplesK(submissionDates) - val runningSubmissionCountByWorkspace = groupPairs(runningSubmissions) + val submissionDatesByWorkspaceByStatus: Map[UUID, Map[String, Option[Timestamp]]] = groupByWorkspaceIdThenStatus(submissionDates) + val runningSubmissionCountByWorkspace: Map[UUID, Int] = groupByWorkspaceId(runningSubmissions) workspaceIds.map { wsId => val (lastFailedDate, lastSuccessDate) = submissionDatesByWorkspaceByStatus.get(wsId) match { @@ -752,6 +754,25 @@ trait WorkspaceComponent { WorkspaceGroups(toGroupMap(realmAclRecs), toGroupMap(accessGroupRecs)) } } + + private def groupByWorkspaceId(runningSubmissions: Seq[(UUID, Int)]): Map[UUID, Int] = { + CollectionUtils.groupPairs(runningSubmissions.toList) + } + + private def groupByWorkspaceIdThenStatus(workflowDates: Seq[(UUID, String, Option[Timestamp])]): Map[UUID, Map[String, Option[Timestamp]]] = { + // There is no Monoid instance for Option[Timestamp] so we need to bring one into scope. + // However a Monoid for Timestamp doesn't really make sense -- what would it do, add them together? + // We can take advantage of the _universal_ monoid for Option which combines Option values using + // Option.orElse. It's called universal because it works no matter the type inside the Option. + // This is fine in this case because there are guaranteed no key conflicts due to the SQL query + // structure (group by, etc). + // + // TL/DR: The following line brings into scope a Monoid[Option[Timestamp]] which combines values + // using Option.orElse. + + implicit val optionTimestampMonoid: Monoid[Option[Timestamp]] = MonoidK[Option].algebra[Timestamp] + CollectionUtils.groupTriples(workflowDates.toList) + } } private case class WorkspaceGroups( diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/util/CollectionUtils.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/util/CollectionUtils.scala index 1a4a7a8d96..625e21c9a3 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/util/CollectionUtils.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/util/CollectionUtils.scala @@ -1,5 +1,8 @@ package org.broadinstitute.dsde.rawls.util +import cats._ +import cats.implicits._ + object CollectionUtils { //A saner group by than Scala's. @@ -10,4 +13,20 @@ object CollectionUtils { def groupByTuplesFlatten[A, B]( tupleSeq: Seq[(A, Seq[B])] ): Map[A, Seq[B]] = { tupleSeq groupBy { case (a,b) => a } map { case (k, v) => k -> v.flatMap(_._2) } } + + /** + * Converts a `Seq[(A, B)]` into a `Map[A, B]`, combining the values with a `Monoid[B]` in case of key conflicts. + * + * For example: + * {{{ + * scala> groupPairs(Seq(("a", 1), ("b", 2), ("a", 3))) + * res0: Map[String,Int] = Map(b -> 2, a -> 4) + * }}} + * */ + def groupPairs[A, B: Monoid](pairs: List[(A, B)]): Map[A, B] = + pairs.foldMap { case (a, b) => Map(a -> b) } + + // Same as above but with triples + def groupTriples[A, B, C: Monoid](trips: List[(A, B, C)]): Map[A, Map[B, C]] = + trips.foldMap { case (a, b, c) => Map(a -> Map(b -> c)) } } From ee31eb5b882e32c9cd4c5cc6d86d39c4acd7bc2e Mon Sep 17 00:00:00 2001 From: Rob Title Date: Tue, 13 Jun 2017 11:15:15 -0400 Subject: [PATCH 08/18] Use more specific cats imports to help compilation time --- .../broadinstitute/dsde/rawls/util/CollectionUtils.scala | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/util/CollectionUtils.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/util/CollectionUtils.scala index 625e21c9a3..88646e118d 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/util/CollectionUtils.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/util/CollectionUtils.scala @@ -1,7 +1,9 @@ package org.broadinstitute.dsde.rawls.util -import cats._ -import cats.implicits._ +import cats.Monoid +import cats.instances.list._ +import cats.instances.map._ +import cats.syntax.foldable._ object CollectionUtils { From a5120c7b006839e1d3629417abaedeb1c282318a Mon Sep 17 00:00:00 2001 From: Rob Title Date: Tue, 13 Jun 2017 14:00:40 -0400 Subject: [PATCH 09/18] Prefer Seq to List --- .../broadinstitute/dsde/rawls/util/CollectionUtils.scala | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/util/CollectionUtils.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/util/CollectionUtils.scala index 88646e118d..ab439b65ad 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/util/CollectionUtils.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/util/CollectionUtils.scala @@ -25,10 +25,10 @@ object CollectionUtils { * res0: Map[String,Int] = Map(b -> 2, a -> 4) * }}} * */ - def groupPairs[A, B: Monoid](pairs: List[(A, B)]): Map[A, B] = - pairs.foldMap { case (a, b) => Map(a -> b) } + def groupPairs[A, B: Monoid](pairs: Seq[(A, B)]): Map[A, B] = + pairs.toList.foldMap { case (a, b) => Map(a -> b) } // Same as above but with triples - def groupTriples[A, B, C: Monoid](trips: List[(A, B, C)]): Map[A, Map[B, C]] = - trips.foldMap { case (a, b, c) => Map(a -> Map(b -> c)) } + def groupTriples[A, B, C: Monoid](trips: Seq[(A, B, C)]): Map[A, Map[B, C]] = + trips.toList.foldMap { case (a, b, c) => Map(a -> Map(b -> c)) } } From 746feaae5f64d1322fb339f6a5fb36ae06910720 Mon Sep 17 00:00:00 2001 From: Rob Title Date: Tue, 13 Jun 2017 17:35:52 -0400 Subject: [PATCH 10/18] Fix SQL --- .../dataaccess/slick/WorkspaceComponent.scala | 43 ++++++++++++------- .../slick/WorkspaceComponentSpec.scala | 10 ++++- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala index 3815ac7524..b9d43070b0 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala @@ -3,9 +3,9 @@ package org.broadinstitute.dsde.rawls.dataaccess.slick import java.sql.Timestamp import java.util.{Date, UUID} -import cats.{Monoid, MonoidK} import cats.instances.int._ import cats.instances.option._ +import cats.{Monoid, MonoidK} import org.broadinstitute.dsde.rawls.RawlsException import org.broadinstitute.dsde.rawls.dataaccess.SlickWorkspaceContext import org.broadinstitute.dsde.rawls.model.Attributable.AttributeMap @@ -509,34 +509,46 @@ trait WorkspaceComponent { * @return WorkspaceSubmissionStats keyed by workspace id */ def listSubmissionSummaryStats(workspaceIds: Seq[UUID]): ReadAction[Map[UUID, WorkspaceSubmissionStats]] = { - // submission date query: // - // select workspaceId, status, max(submissionDate) + // select workspaceId, status, max(subEndDate) // from ( - // select distinct submission.workspaceId, workflow.status, submission.submissionDate + // select submission.id, submission.workspaceId, workflow.status, max(workflow.statusLastChangedDate) as subEndDate, count(1) // from submission // join workflow on workflow.submissionId = submission.id - // where submission.workspaceId in (:workspaceIds)) v + // where submission.workspaceId in (:workspaceIds) + // group by 1, 2, 3) v + // where (status = 'Failure' or (status = 'Succeeded' and v.count = 1)) // group by 1, 2 - // having (status = 'Failure' or (status = 'Succeeded' and count(v.*) = 1)) + // + // Explanation: + // - inner query returns the most recent workflow status change date, per workflow status and submission + // - outer query returns the most recent workflow status change date where: + // -- the status is Failure; or + // -- the status is Succeeded and that is the only status in the submission - val workflowStatusQuery = (for { + val workflowStatusQuery = for { submissions <- submissionQuery if submissions.workspaceId.inSetBind(workspaceIds) workflows <- workflowQuery if submissions.id === workflows.submissionId - } yield (submissions.workspaceId, workflows.status, submissions.submissionDate)).distinct + } yield (submissions.id, submissions.workspaceId, workflows.status, workflows.statusLastChangedDate) - val submissionMaxDateQuery = workflowStatusQuery.groupBy { case (workspaceId, status, submissionDate) => - (workspaceId, status) - }.map { case ((workspaceId, status), recs) => - (workspaceId, status, recs.map(_._3).max, recs.length) + val groupedWorkflowStatusQuery = workflowStatusQuery.groupBy { case (submissionId, workspaceId, status, _) => + (submissionId, workspaceId, status) + }.map { case ((submissionId, workspaceId, status), recs) => + (submissionId, workspaceId, status, recs.map(_._4).max, recs.length) } // Note: a submission is successful if it contains _only_ successful workflows. // A submission is a failure if it contains _any_ failed workflows. - val filteredSubmissionMaxDateQuery = submissionMaxDateQuery.filter { case (_, status, _, count) => + val filteredGroupedWorkflowStatusQuery = groupedWorkflowStatusQuery.filter { case (_, _, status, _, count) => status === WorkflowStatuses.Failed.toString || (status === WorkflowStatuses.Succeeded.toString && count === 1) - }.map { case (workspaceId, status, max, _) => (workspaceId, status, max)} + } + + val submissionMaxDateQuery = filteredGroupedWorkflowStatusQuery.groupBy { case (_, workspaceId, status, _, _) => + (workspaceId, status) + }.map { case ((workspaceId, status), recs) => + (workspaceId, status, recs.map(_._4).max) + } // running submission query: select workspaceId, count(1) ... where submissions.status === Submitted group by workspaceId val runningSubmissionsQuery = (for { @@ -544,9 +556,10 @@ trait WorkspaceComponent { } yield submissions).groupBy(_.workspaceId).map { case (wfId, submissions) => (wfId, submissions.length)} for { - submissionDates <- filteredSubmissionMaxDateQuery.result + submissionDates <- submissionMaxDateQuery.result runningSubmissions <- runningSubmissionsQuery.result } yield { + println("submission dates: " + submissionDates) val submissionDatesByWorkspaceByStatus: Map[UUID, Map[String, Option[Timestamp]]] = groupByWorkspaceIdThenStatus(submissionDates) val runningSubmissionCountByWorkspace: Map[UUID, Int] = groupByWorkspaceId(runningSubmissions) diff --git a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala index 687803412c..232df14817 100644 --- a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala +++ b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala @@ -118,32 +118,38 @@ class WorkspaceComponentSpec extends TestDriverComponentWithFlatSpecAndMatchers it should "list submission summary stats" in withDefaultTestDatabase { implicit def toWorkspaceId(ws: Workspace): UUID = UUID.fromString(ws.workspaceId) + // no submissions val wsIdNoSubmissions: UUID = testData.workspaceNoSubmissions assertResult(Map(wsIdNoSubmissions -> WorkspaceSubmissionStats(None, None, 0))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdNoSubmissions))) } + // 1 successful submission, 0 failed, 0 running val wsIdSuccessfulSubmission: UUID = testData.workspaceSuccessfulSubmission assertResult(Map(wsIdSuccessfulSubmission -> WorkspaceSubmissionStats(Some(testDate), None, 0))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdSuccessfulSubmission))) } + // 0 successful submissions, 1 failed, 0 running val wsIdFailedSubmission: UUID = testData.workspaceFailedSubmission assertResult(Map(wsIdFailedSubmission -> WorkspaceSubmissionStats(None, Some(testDate), 0))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdFailedSubmission))) } + // 0 successful submissions, 0 failed, 1 running val wsIdSubmittedSubmission: UUID = testData.workspaceSubmittedSubmission assertResult(Map(wsIdSubmittedSubmission -> WorkspaceSubmissionStats(None, None, 1))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdSubmittedSubmission))) } - // Note: a submission with both a successful and failed workflow is a failure + // 0 successful submissions, 1 failed, 1 running + val wsIdMixedSubmission: UUID = testData.workspaceMixedSubmissions - assertResult(Map(wsIdMixedSubmission -> WorkspaceSubmissionStats(Some(testDate), Some(testDate), 1))) { + assertResult(Map(wsIdMixedSubmission -> WorkspaceSubmissionStats(None, Some(testDate), 1))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdMixedSubmission))) } + // 1 successful submissions, 1 failed, 0 running val wsIdTerminatedSubmission: UUID = testData.workspaceTerminatedSubmissions assertResult(Map(wsIdTerminatedSubmission -> WorkspaceSubmissionStats(Some(testDate), Some(testDate), 0))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdTerminatedSubmission))) From f819423d173374cd15515dbea49758b6184b5fbd Mon Sep 17 00:00:00 2001 From: Rob Title Date: Wed, 14 Jun 2017 12:02:02 -0400 Subject: [PATCH 11/18] Add interleaved submission test case: it passes --- .../slick/TestDriverComponent.scala | 28 +++++++++++++++++++ .../slick/WorkspaceComponentSpec.scala | 19 ++++++++----- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/TestDriverComponent.scala b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/TestDriverComponent.scala index 830557589d..beec9d9007 100644 --- a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/TestDriverComponent.scala +++ b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/TestDriverComponent.scala @@ -210,6 +210,7 @@ trait TestDriverComponent extends DriverComponent with DataAccess { val wsName7 = WorkspaceName("myNamespace", "myWorkspacewithRealmsMethodConfigsAbortedSubmission") val wsName8 = WorkspaceName("myNamespace", "myWorkspacewithRealmsMethodConfigsAbortedSuccessfulSubmission") val wsName9 = WorkspaceName("myNamespace", "myWorkspaceToTestGrantPermissions") + val wsInterleaved = WorkspaceName("myNamespace", "myWorkspaceToTestInterleavedSubmissions") val workspaceToTestGrantId = UUID.randomUUID() val nestedProjectGroup = makeRawlsGroup("nested project group", Set(userOwner)) @@ -297,6 +298,9 @@ trait TestDriverComponent extends DriverComponent with DataAccess { // Workspace with realms, with aborted and successful submissions val (workspaceTerminatedSubmissions, workspaceTerminatedSubmissionsGroups) = makeWorkspace(billingProject, wsName8.name, Option(realm), UUID.randomUUID().toString, "aBucket", currentTime(), currentTime(), "testUser", wsAttrs, false) + // Workspace with a successful submission that had another submission run and fail while it was running + val (workspaceInterleavedSubmissions, workspaceInterleavedSubmissionsGroups) = makeWorkspace(billingProject, wsInterleaved.name, Option(realm), UUID.randomUUID().toString, "aBucket", currentTime(), currentTime(), "testUser", wsAttrs, false) + // Standard workspace to test grant permissions val (workspaceToTestGrant, workspaceToTestGrantGroups) = makeWorkspaceToTestGrant(billingProject, wsName9.name, None, workspaceToTestGrantId.toString, "aBucket", currentTime(), currentTime(), "testUser", wsAttrs, false) @@ -481,6 +485,16 @@ trait TestDriverComponent extends DriverComponent with DataAccess { Workflow(Option("workflowSubmitted2"), WorkflowStatuses.Submitted, testDate, sample6.toReference, inputResolutions) ), SubmissionStatuses.Submitted, false) + //two submissions interleaved in time + val t1 = new DateTime(2017, 1, 1, 5, 10) + val t2 = new DateTime(2017, 1, 1, 5, 15) + val t3 = new DateTime(2017, 1, 1, 5, 20) + val t4 = new DateTime(2017, 1, 1, 5, 30) + val outerSubmission = Submission(UUID.randomUUID().toString(), t1, userOwner, methodConfig.namespace, methodConfig.name, indiv1.toReference, + Seq(Workflow(Option("workflowSuccessful1"), WorkflowStatuses.Succeeded, t4, sample1.toReference, inputResolutions)), SubmissionStatuses.Done, false) + val innerSubmission = Submission(UUID.randomUUID().toString(), t2, userOwner, methodConfig.namespace, methodConfig.name, indiv1.toReference, + Seq(Workflow(Option("workflowFailed1"), WorkflowStatuses.Failed, t3, sample1.toReference, inputResolutions)), SubmissionStatuses.Done, false) + def createWorkspaceGoogleGroups(gcsDAO: GoogleServicesDAO): Unit = { val groups = billingProject.groups.values ++ testProject1.groups.values ++ @@ -495,6 +509,7 @@ trait TestDriverComponent extends DriverComponent with DataAccess { workspaceSubmittedSubmissionGroups ++ workspaceMixedSubmissionsGroups ++ workspaceTerminatedSubmissionsGroups ++ + workspaceInterleavedSubmissionsGroups ++ controlledWorkspaceGroups ++ Seq(realm.membersGroup, realm.adminsGroup, realm2.membersGroup, realm2.adminsGroup) @@ -514,6 +529,7 @@ trait TestDriverComponent extends DriverComponent with DataAccess { workspaceSubmittedSubmission, workspaceMixedSubmissions, workspaceTerminatedSubmissions, + workspaceInterleavedSubmissions, workspaceToTestGrant) val saveAllWorkspacesAction = DBIO.sequence(allWorkspaces.map(workspaceQuery.save)) @@ -549,6 +565,7 @@ trait TestDriverComponent extends DriverComponent with DataAccess { DBIO.sequence(workspaceSubmittedSubmissionGroups.map(rawlsGroupQuery.save).toSeq), DBIO.sequence(workspaceMixedSubmissionsGroups.map(rawlsGroupQuery.save).toSeq), DBIO.sequence(workspaceTerminatedSubmissionsGroups.map(rawlsGroupQuery.save).toSeq), + DBIO.sequence(workspaceInterleavedSubmissionsGroups.map(rawlsGroupQuery.save).toSeq), DBIO.sequence(workspaceToTestGrantGroups.map(rawlsGroupQuery.save).toSeq), managedGroupQuery.createManagedGroup(realm), managedGroupQuery.createManagedGroup(realm2), @@ -647,6 +664,17 @@ trait TestDriverComponent extends DriverComponent with DataAccess { submissionQuery.create(context, submissionMixed), updateWorkflowExecutionServiceKey("unittestdefault") ) + }), + withWorkspaceContext(workspaceInterleavedSubmissions)({ context => + DBIO.seq( + entityQuery.save(context, Seq(aliquot1, aliquot2, sample1, sample2, sample3, sample4, sample5, sample6, sample7, sample8, pair1, pair2, ps1, sset1, sset2, sset3, sset4, sset_empty, indiv1, indiv2)), + + methodConfigurationQuery.create(context, methodConfig), + + submissionQuery.create(context, outerSubmission), + submissionQuery.create(context, innerSubmission), + updateWorkflowExecutionServiceKey("unittestdefault") + ) }) ) } diff --git a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala index 232df14817..fd8e9bd1b2 100644 --- a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala +++ b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponentSpec.scala @@ -118,41 +118,46 @@ class WorkspaceComponentSpec extends TestDriverComponentWithFlatSpecAndMatchers it should "list submission summary stats" in withDefaultTestDatabase { implicit def toWorkspaceId(ws: Workspace): UUID = UUID.fromString(ws.workspaceId) - // no submissions + // no submissions: 0 successful, 0 failed, 0 running val wsIdNoSubmissions: UUID = testData.workspaceNoSubmissions assertResult(Map(wsIdNoSubmissions -> WorkspaceSubmissionStats(None, None, 0))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdNoSubmissions))) } - // 1 successful submission, 0 failed, 0 running + // successful submission: 1 successful, 0 failed, 0 running val wsIdSuccessfulSubmission: UUID = testData.workspaceSuccessfulSubmission assertResult(Map(wsIdSuccessfulSubmission -> WorkspaceSubmissionStats(Some(testDate), None, 0))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdSuccessfulSubmission))) } - // 0 successful submissions, 1 failed, 0 running + // failed submission: 0 successful, 1 failed, 0 running val wsIdFailedSubmission: UUID = testData.workspaceFailedSubmission assertResult(Map(wsIdFailedSubmission -> WorkspaceSubmissionStats(None, Some(testDate), 0))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdFailedSubmission))) } - // 0 successful submissions, 0 failed, 1 running + // submitted submissions: 0 successful, 0 failed, 1 running val wsIdSubmittedSubmission: UUID = testData.workspaceSubmittedSubmission assertResult(Map(wsIdSubmittedSubmission -> WorkspaceSubmissionStats(None, None, 1))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdSubmittedSubmission))) } - // 0 successful submissions, 1 failed, 1 running - + // mixed submissions: 0 successful, 1 failed, 1 running val wsIdMixedSubmission: UUID = testData.workspaceMixedSubmissions assertResult(Map(wsIdMixedSubmission -> WorkspaceSubmissionStats(None, Some(testDate), 1))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdMixedSubmission))) } - // 1 successful submissions, 1 failed, 0 running + // terminated submissions: 1 successful, 1 failed, 0 running val wsIdTerminatedSubmission: UUID = testData.workspaceTerminatedSubmissions assertResult(Map(wsIdTerminatedSubmission -> WorkspaceSubmissionStats(Some(testDate), Some(testDate), 0))) { runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdTerminatedSubmission))) } + + // interleaved submissions: 1 successful, 1 failed, 0 running + val wsIdInterleavedSubmissions: UUID = testData.workspaceInterleavedSubmissions + assertResult(Map(wsIdInterleavedSubmissions -> WorkspaceSubmissionStats(Option(testData.t4), Option(testData.t3), 0))) { + runAndWait(workspaceQuery.listSubmissionSummaryStats(Seq(wsIdInterleavedSubmissions))) + } } } From 2a7610e6a6b3e58f9ee2f260c44fd8309b558643 Mon Sep 17 00:00:00 2001 From: Rob Title Date: Wed, 14 Jun 2017 21:53:29 -0400 Subject: [PATCH 12/18] Another attempt at fixing the query. Seems good, will do more testing. --- .../dataaccess/slick/WorkspaceComponent.scala | 57 ++++++++++--------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala index b9d43070b0..05e32356c9 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala @@ -511,43 +511,45 @@ trait WorkspaceComponent { def listSubmissionSummaryStats(workspaceIds: Seq[UUID]): ReadAction[Map[UUID, WorkspaceSubmissionStats]] = { // submission date query: // - // select workspaceId, status, max(subEndDate) + // select workspaceId, case when subFailed > 0 then 'Failed' else 'Succeeded' end as status, max(subEndDate) as subEndDate // from ( - // select submission.id, submission.workspaceId, workflow.status, max(workflow.statusLastChangedDate) as subEndDate, count(1) + // select submission.id, submission.workspaceId, max(case when w.status = 'Failed' then 1 else 0 end) as subFailed, max(w.status_last_changed) as subEndDate // from submission // join workflow on workflow.submissionId = submission.id // where submission.workspaceId in (:workspaceIds) - // group by 1, 2, 3) v - // where (status = 'Failure' or (status = 'Succeeded' and v.count = 1)) + // and status in ('Succeeded', 'Failed') + // group by 1, 2) v // group by 1, 2 // // Explanation: - // - inner query returns the most recent workflow status change date, per workflow status and submission - // - outer query returns the most recent workflow status change date where: - // -- the status is Failure; or - // -- the status is Succeeded and that is the only status in the submission - - val workflowStatusQuery = for { - submissions <- submissionQuery if submissions.workspaceId.inSetBind(workspaceIds) - workflows <- workflowQuery if submissions.id === workflows.submissionId - } yield (submissions.id, submissions.workspaceId, workflows.status, workflows.statusLastChangedDate) - - val groupedWorkflowStatusQuery = workflowStatusQuery.groupBy { case (submissionId, workspaceId, status, _) => - (submissionId, workspaceId, status) - }.map { case ((submissionId, workspaceId, status), recs) => - (submissionId, workspaceId, status, recs.map(_._4).max, recs.length) - } - - // Note: a submission is successful if it contains _only_ successful workflows. - // A submission is a failure if it contains _any_ failed workflows. - val filteredGroupedWorkflowStatusQuery = groupedWorkflowStatusQuery.filter { case (_, _, status, _, count) => - status === WorkflowStatuses.Failed.toString || (status === WorkflowStatuses.Succeeded.toString && count === 1) + // - inner query gets 2 things per (workspace, submission): + // -- whether or not the submission contains any failed workflows + // -- the most recent workflow status change date + // - outer query gets the most recent workflow status change date per (workspace, submissionStatus), where submissionStatus is: + // -- Failed if the submission contains failed workflows + // -- Succeeded if the submission does not contain failed workflows + + val innerQuery = ( + for { + submissions <- submissionQuery if submissions.workspaceId.inSetBind(workspaceIds) + workflows <- workflowQuery if submissions.id === workflows.submissionId + } yield (submissions.id, submissions.workspaceId, workflows.status, workflows.statusLastChangedDate) + ).filter { case (_, _, status, _) => + status === WorkflowStatuses.Failed.toString || status === WorkflowStatuses.Succeeded.toString + }.map { case (submissionId, workspaceId, status, statusLastChangedDate) => + (submissionId, workspaceId, Case If (status === WorkflowStatuses.Failed.toString) Then 1 Else 0, statusLastChangedDate) + }.groupBy { case (submissionId, workspaceId, _, _) => + (submissionId, workspaceId) + }.map { case ((submissionId, workspaceId), recs) => + (submissionId, workspaceId, recs.map(_._3).max, recs.map(_._4).max) } - val submissionMaxDateQuery = filteredGroupedWorkflowStatusQuery.groupBy { case (_, workspaceId, status, _, _) => + val outerQuery = innerQuery.map { case (_, workspaceId, numFailures, maxDate) => + (workspaceId, Case If (numFailures > 0) Then WorkflowStatuses.Failed.toString Else WorkflowStatuses.Succeeded.toString, maxDate) + }.groupBy { case (workspaceId, status, _) => (workspaceId, status) }.map { case ((workspaceId, status), recs) => - (workspaceId, status, recs.map(_._4).max) + (workspaceId, status, recs.map(_._3).max) } // running submission query: select workspaceId, count(1) ... where submissions.status === Submitted group by workspaceId @@ -556,10 +558,9 @@ trait WorkspaceComponent { } yield submissions).groupBy(_.workspaceId).map { case (wfId, submissions) => (wfId, submissions.length)} for { - submissionDates <- submissionMaxDateQuery.result + submissionDates <- outerQuery.result runningSubmissions <- runningSubmissionsQuery.result } yield { - println("submission dates: " + submissionDates) val submissionDatesByWorkspaceByStatus: Map[UUID, Map[String, Option[Timestamp]]] = groupByWorkspaceIdThenStatus(submissionDates) val runningSubmissionCountByWorkspace: Map[UUID, Int] = groupByWorkspaceId(runningSubmissions) From 7927a2291356910d661ca4292814aaaca88270a0 Mon Sep 17 00:00:00 2001 From: Rob Title Date: Wed, 14 Jun 2017 22:01:50 -0400 Subject: [PATCH 13/18] minor: clearer variable names --- .../dsde/rawls/dataaccess/slick/WorkspaceComponent.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala index 05e32356c9..ac69c1afae 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala @@ -529,7 +529,7 @@ trait WorkspaceComponent { // -- Failed if the submission contains failed workflows // -- Succeeded if the submission does not contain failed workflows - val innerQuery = ( + val innerSubmissionDateQuery = ( for { submissions <- submissionQuery if submissions.workspaceId.inSetBind(workspaceIds) workflows <- workflowQuery if submissions.id === workflows.submissionId @@ -544,7 +544,7 @@ trait WorkspaceComponent { (submissionId, workspaceId, recs.map(_._3).max, recs.map(_._4).max) } - val outerQuery = innerQuery.map { case (_, workspaceId, numFailures, maxDate) => + val outerSubmissionDateQuery = innerSubmissionDateQuery.map { case (_, workspaceId, numFailures, maxDate) => (workspaceId, Case If (numFailures > 0) Then WorkflowStatuses.Failed.toString Else WorkflowStatuses.Succeeded.toString, maxDate) }.groupBy { case (workspaceId, status, _) => (workspaceId, status) @@ -558,7 +558,7 @@ trait WorkspaceComponent { } yield submissions).groupBy(_.workspaceId).map { case (wfId, submissions) => (wfId, submissions.length)} for { - submissionDates <- outerQuery.result + submissionDates <- outerSubmissionDateQuery.result runningSubmissions <- runningSubmissionsQuery.result } yield { val submissionDatesByWorkspaceByStatus: Map[UUID, Map[String, Option[Timestamp]]] = groupByWorkspaceIdThenStatus(submissionDates) From 103a4dae4aeedd63e1ad83ff7e7d88a9f40f1c7f Mon Sep 17 00:00:00 2001 From: Rob Title Date: Thu, 15 Jun 2017 08:42:11 -0400 Subject: [PATCH 14/18] Fix unit test failure due to adding test workflows --- .../dsde/rawls/dataaccess/slick/DriverComponentSpec.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponentSpec.scala b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponentSpec.scala index 5ae3fb3b9b..eb827aaf3a 100644 --- a/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponentSpec.scala +++ b/core/src/test/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponentSpec.scala @@ -69,7 +69,7 @@ class DriverComponentSpec extends TestDriverComponentWithFlatSpecAndMatchers wit val queryRecords = runAndWait(query.as[WorkflowRecord]) // first check that we're not just comparing empty seqs - assertResult(24) { queryRecords.length } + assertResult(26) { queryRecords.length } assertResult(queryRecords) { runAndWait(concatSqlActions(select, where1, reduceSqlActionsWithDelim(statuses), where2).as[WorkflowRecord]) From bc1aebfaafc412187e51fdee8b559d1bccc11ca0 Mon Sep 17 00:00:00 2001 From: Rob Title Date: Fri, 16 Jun 2017 10:03:54 -0400 Subject: [PATCH 15/18] Change .max to .count --- .../dsde/rawls/dataaccess/slick/WorkspaceComponent.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala index ac69c1afae..52c2950ea0 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala @@ -541,7 +541,7 @@ trait WorkspaceComponent { }.groupBy { case (submissionId, workspaceId, _, _) => (submissionId, workspaceId) }.map { case ((submissionId, workspaceId), recs) => - (submissionId, workspaceId, recs.map(_._3).max, recs.map(_._4).max) + (submissionId, workspaceId, recs.map(_._3).count, recs.map(_._4).max) } val outerSubmissionDateQuery = innerSubmissionDateQuery.map { case (_, workspaceId, numFailures, maxDate) => From 3553256b9a2fa492351ca4918b1aa9c77468d124 Mon Sep 17 00:00:00 2001 From: Rob Title Date: Fri, 16 Jun 2017 10:16:15 -0400 Subject: [PATCH 16/18] Oops, meant .count --- .../dsde/rawls/dataaccess/slick/WorkspaceComponent.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala index 52c2950ea0..5c3ff6dce1 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala @@ -541,7 +541,7 @@ trait WorkspaceComponent { }.groupBy { case (submissionId, workspaceId, _, _) => (submissionId, workspaceId) }.map { case ((submissionId, workspaceId), recs) => - (submissionId, workspaceId, recs.map(_._3).count, recs.map(_._4).max) + (submissionId, workspaceId, recs.map(_._3).sum, recs.map(_._4).max) } val outerSubmissionDateQuery = innerSubmissionDateQuery.map { case (_, workspaceId, numFailures, maxDate) => From bb205b38b0e3c9d1a5721442d7d81b5ebeaa6c86 Mon Sep 17 00:00:00 2001 From: Rob Title Date: Fri, 16 Jun 2017 13:45:22 -0400 Subject: [PATCH 17/18] Final code review comments --- .../dataaccess/slick/WorkspaceComponent.scala | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala index 5c3ff6dce1..91ddd230e6 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala @@ -774,18 +774,22 @@ trait WorkspaceComponent { } private def groupByWorkspaceIdThenStatus(workflowDates: Seq[(UUID, String, Option[Timestamp])]): Map[UUID, Map[String, Option[Timestamp]]] = { - // There is no Monoid instance for Option[Timestamp] so we need to bring one into scope. - // However a Monoid for Timestamp doesn't really make sense -- what would it do, add them together? - // We can take advantage of the _universal_ monoid for Option which combines Option values using - // Option.orElse. It's called universal because it works no matter the type inside the Option. - // This is fine in this case because there are guaranteed no key conflicts due to the SQL query - // structure (group by, etc). + // The function groupTriples, called below, transforms a Seq((T1, T2, T3)) to a Map(T1 -> Map(T2 -> T3)). + // It does this by calling foldMap, which in turn requires a monoid for T3. In our case, T3 is an Option[Timestamp], + // so we need to provide an implicit monoid for Option[Timestamp]. // - // TL/DR: The following line brings into scope a Monoid[Option[Timestamp]] which combines values - // using Option.orElse. + // There isn't really a sane monoid implementation for Timestamp (what would you do, add them?). Thankfully + // it turns out that the UUID/String pairs in workflowDates are always unique, so it doesn't matter what the + // monoid does because it'll never be used to combine two Option[Timestamp]s. It just needs to be provided in + // order to make the compiler happy. + // + // To do this, we use the universal monoid for Option, MonoidK[Option]. Note that the inner Option takes no type + // parameter: MonoidK doesn't care about the type inside Option, it just calls orElse on the Option for its "combine" + // operator. Finally, the call to algebra[Timestamp] turns a MonoidK[Option] into a Monoid[Option[Timestamp]] by + // leaving the monoid implementation alone (so it still calls orElse) and poking the Timestamp type into the Option. implicit val optionTimestampMonoid: Monoid[Option[Timestamp]] = MonoidK[Option].algebra[Timestamp] - CollectionUtils.groupTriples(workflowDates.toList) + CollectionUtils.groupTriples(workflowDates) } } From 00df98b512db1cd1e7b703ba1296cd662747f97a Mon Sep 17 00:00:00 2001 From: Rob Title Date: Fri, 16 Jun 2017 13:45:59 -0400 Subject: [PATCH 18/18] Remove another unnecessary .toList --- .../dsde/rawls/dataaccess/slick/WorkspaceComponent.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala index 91ddd230e6..d7e37b73aa 100644 --- a/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala +++ b/core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala @@ -770,7 +770,7 @@ trait WorkspaceComponent { } private def groupByWorkspaceId(runningSubmissions: Seq[(UUID, Int)]): Map[UUID, Int] = { - CollectionUtils.groupPairs(runningSubmissions.toList) + CollectionUtils.groupPairs(runningSubmissions) } private def groupByWorkspaceIdThenStatus(workflowDates: Seq[(UUID, String, Option[Timestamp])]): Map[UUID, Map[String, Option[Timestamp]]] = {