From e630c18e046c9c1e34602b96e720bebbd6f751b5 Mon Sep 17 00:00:00 2001
From: Tristan Garwood <tristan.garwood@gmail.com>
Date: Wed, 3 Jul 2024 17:19:51 -0400
Subject: [PATCH] PROD-972 ID-1324 Swallow sync deduplication errors in
 endpoint. (#1477)

* PROD-972 ID-1324 Swallow sync deduplication errors in endpoint.

* added test
---
 .../sam/google/GoogleExtensionRoutes.scala    | 14 ++++++++---
 .../sam/dataAccess/MockDirectoryDAO.scala     |  5 ++++
 .../google/GoogleExtensionRoutesSpec.scala    | 25 +++++++++++++++++++
 3 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala
index 3c8acf6c57..467b456eee 100644
--- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala
+++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala
@@ -17,8 +17,8 @@ import org.broadinstitute.dsde.workbench.sam.api.{
   SecurityDirectives,
   ioMarshaller
 }
-import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._
 import org.broadinstitute.dsde.workbench.sam.model._
+import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._
 import org.broadinstitute.dsde.workbench.sam.model.api.SamUser
 import org.broadinstitute.dsde.workbench.sam.service.CloudExtensions
 import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
@@ -245,9 +245,15 @@ trait GoogleExtensionRoutes extends ExtensionRoutes with SamUserDirectives with
               postWithTelemetry(samRequestContext, params: _*) {
                 complete {
                   import SamGoogleModelJsonSupport._
-                  googleGroupSynchronizer.synchronizeGroupMembers(policyId, samRequestContext = samRequestContext).map { syncReport =>
-                    StatusCodes.OK -> syncReport
-                  }
+                  googleGroupSynchronizer
+                    .synchronizeGroupMembers(policyId, samRequestContext = samRequestContext)
+                    .recover {
+                      // If the group sync was already done previously, then no need to return any sync report items, just return 200
+                      case _: GroupAlreadySynchronized => Map.empty[WorkbenchEmail, Seq[SyncReportItem]]
+                    }
+                    .map { syncReport =>
+                      StatusCodes.OK -> syncReport
+                    }
                 }
               } ~
                 getWithTelemetry(samRequestContext, params: _*) {
diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala
index 1d4e62869a..2bc11b176a 100644
--- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala
+++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala
@@ -259,6 +259,11 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench
 
   override def updateSynchronizedDateAndVersion(group: WorkbenchGroup, samRequestContext: SamRequestContext): IO[Unit] = {
     groupSynchronizedDates += group.id -> new Date()
+    groups.get(group.id).foreach {
+      case g: BasicWorkbenchGroup => groups.put(g.id, g.copy(lastSynchronizedVersion = Option(g.lastSynchronizedVersion.getOrElse(0) + 1)))
+      case p: AccessPolicy => groups.put(p.id, p.copy(lastSynchronizedVersion = Option(p.lastSynchronizedVersion.getOrElse(0) + 1)))
+      case _ => ()
+    }
     IO.unit
   }
 
diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala
index 8461ba4214..652f5ee441 100644
--- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala
+++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala
@@ -286,6 +286,31 @@ class GoogleExtensionRoutesSpec extends GoogleExtensionRoutesSpecHelper with Sca
     }
   }
 
+  "GET /api/google/policy/{resourceTypeName}/{resourceId}/{accessPolicyName}/sync" should "200 with empty response when deduping sync requests" in {
+    val resourceTypes = Map(resourceType.name -> resourceType)
+    val (user, samDep, routes) = createTestUser(resourceTypes)
+
+    Post(s"/api/resource/${resourceType.name}/foo") ~> routes.route ~> check {
+      status shouldEqual StatusCodes.NoContent
+      assertResult("") {
+        responseAs[String]
+      }
+    }
+
+    import spray.json.DefaultJsonProtocol._
+    import SamGoogleModelJsonSupport._
+    Post(s"/api/google/resource/${resourceType.name}/foo/${resourceType.ownerRoleName.value}/sync") ~> routes.route ~> check {
+      status shouldEqual StatusCodes.OK
+      responseAs[Map[WorkbenchEmail, Seq[SyncReportItem]]] should not be empty
+    }
+
+    // A duplicate call should return OK and an empty response
+    Post(s"/api/google/resource/${resourceType.name}/foo/${resourceType.ownerRoleName.value}/sync") ~> routes.route ~> check {
+      status shouldEqual StatusCodes.OK
+      responseAs[Map[WorkbenchEmail, Seq[SyncReportItem]]] shouldBe empty
+    }
+  }
+
   "GET /api/google/user/petServiceAccount/{project}/key" should "200 with a new key" in {
     assume(databaseEnabled, databaseEnabledClue)