WOR-1823 part 3: workspace deletion refactoring #3049

blakery · 2024-09-27T23:37:36Z

Ticket: https://broadworkbench.atlassian.net/browse/WOR-1823

Refactors workspace deletion and adds unit tests.

PR checklist

Include the JIRA issue number in the PR description and title
Make sure Swagger is updated if API changes
- ...and Orchestration's Swagger too!
If you changed anything in model/, then you should publish a new official rawls-model and update rawls-model in Orchestration's dependencies.
Get two thumbsworth of PR review
Verify all tests go green, including CI tests
Squash commits and merge to develop (branches are automatically deleted after merging)
Inform other teams of any substantial changes via Slack and/or email

blakery · 2024-09-30T12:47:48Z

core/src/main/scala/org/broadinstitute/dsde/rawls/util/LibraryPermissionsSupport.scala

@@ -43,19 +46,6 @@ trait LibraryPermissionsSupport extends RoleSupport {
      case _                                        => changeMetadataChecker(workspaceId) _
    }

-  def withLibraryAttributeNamespaceCheck[T](attributeNames: Iterable[AttributeName])(op: => T): T = {


This was moved to WorkspaceService, since it is only called there.

blakery · 2024-09-30T12:49:45Z

core/src/main/scala/org/broadinstitute/dsde/rawls/util/LibraryPermissionsSupport.scala

  val samDAO: SamDAO
+  val gcsDAO: GoogleServicesDAO
+  val ctx: RawlsRequestContext


These were being inherited by RoleSupport, but there's no real reason for this trait to be connected to it.

blakery · 2024-09-30T12:50:39Z

core/src/main/scala/org/broadinstitute/dsde/rawls/util/RoleSupport.scala

@@ -28,20 +28,4 @@ trait RoleSupport {
          new RawlsExceptionWithErrorReport(errorReport = ErrorReport(StatusCodes.Forbidden, "You must be an admin."))
        )
    }
-
-  def tryIsCurator(userEmail: RawlsUserEmail): Future[Boolean] =
-    gcsDAO.isLibraryCurator(userEmail.value) recoverWith { case t =>


This was only used once, So I just moved it inline.

blakery · 2024-09-30T12:52:47Z

core/src/main/scala/org/broadinstitute/dsde/rawls/util/TracingUtils.scala

+    * @param f the operation to execute within the trace
+    * @return the result of the operation `f`
+    */
+  def trace[T](name: String, parentContext: RawlsRequestContext)(f: RawlsRequestContext => T): T =


Not having this was pushing a lot of different places to use a Future where it wasn't really necessary.

This appears to be equivalent to traceNakedWithParent below

That's using RawlsTracingContext not RawlsRequestContext.

It's arguable that the better approach would be an overload or something, but we have similar divergence between others, such as between traceFutureWithParent and traceFuture.
Also, this is the most common/basic/unembellished instance of this, and therefore worth giving the most basic name. But I didn't feel like renaming usages of traceNakedWithParent in what is already a substantial PR.

blakery · 2024-09-30T13:01:53Z

core/src/main/scala/org/broadinstitute/dsde/rawls/entities/EntityService.scala

@@ -17,19 +17,15 @@ import org.broadinstitute.dsde.rawls.entities.exceptions.{
 import org.broadinstitute.dsde.rawls.expressions.ExpressionEvaluator
 import org.broadinstitute.dsde.rawls.metrics.RawlsInstrumented
 import org.broadinstitute.dsde.rawls.model.AttributeUpdateOperations.{AttributeUpdateOperation, EntityUpdateDefinition}
-import org.broadinstitute.dsde.rawls.model.{


The changes in this file are just from moving an exception definition to the place where it's thrown.

blakery · 2024-09-30T13:03:05Z

core/src/main/scala/org/broadinstitute/dsde/rawls/metrics/MetricsHelper.scala

-                       count: Int = 1,
-                       labels: Map[String, String] = Map.empty,
-                       description: Option[String] = None
-  ): IO[Unit] = {


Unneccesary IO wrapping - only called in one place anyway.

blakery · 2024-09-30T13:04:12Z

core/src/main/scala/org/broadinstitute/dsde/rawls/submissions/SubmissionsRepository.scala

+
+  import dataSource.dataAccess.driver.api._
+
+  def getActiveWorkflowsAndSetStatusToAborted(


This was moved in wholesale from WorkspaceService.

blakery · 2024-09-30T13:05:09Z

core/src/main/scala/org/broadinstitute/dsde/rawls/util/AttributeSupport.scala

   * @throws AttributeUpdateOperationException when adding or removing from an attribute that is not a list
   * @return the updated entity
   */
  def applyOperationsToEntity(entity: Entity, operations: Seq[AttributeUpdateOperation]): Entity =
    entity.copy(attributes = applyAttributeUpdateOperations(entity, operations))
 }
+
+class AttributeUpdateOperationException(message: String) extends RawlsException(message)


This is the only place these are thrown, so it makes sense for them to be defined here.

blakery · 2024-09-30T13:08:23Z

core/src/test/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceServiceUnitTests.scala

-  val defaultRequestContext: RawlsRequestContext =
-    RawlsRequestContext(
-      UserInfo(RawlsUserEmail("test"), OAuth2BearerToken("Bearer 123"), 123, RawlsUserSubjectId("abc"))
+  val ctx: RawlsRequestContext = RawlsRequestContext(


I got tired of having defaultRequestContext making practically every mock require a line break. Sorry about the resulting diff size.

blakery · 2024-09-30T13:10:23Z

core/src/test/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceServiceSpec.scala

@@ -79,7 +84,6 @@ import scala.jdk.DurationConverters.JavaDurationOps
 import scala.language.postfixOps
 import scala.util.Try

-//noinspection NameBooleanParameters,TypeAnnotation,EmptyParenMethodAccessedAsParameterless,ScalaUnnecessaryParentheses,RedundantNewCaseClass,ScalaUnusedSymbol


Nearly all of the changes in this file are fixing ide warnings from removing this.

blakery · 2024-09-30T14:45:58Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

+    // Delete Google Project
+    _ <- traceFutureWithParent("deleteGoogleProject", ctx)(_ => deleteGoogleProject(workspace.googleProjectId))
+    // attempt to delete workspace in WSM, in case thsi is a TDR snapshot - but don't fail on it
+    _ = Try(workspaceManagerDAO.deleteWorkspace(workspace.workspaceIdAsUUID, ctx)).recover {


This is the same behavior we had before, because we would only fail if it was a McWorkspace we were trying to delete, which would never happen, since those are handled by MultiCloudWorkspaceService.

This allows getWorkspace and getWorkspaceById to share the processing logic, without getWorkspaceById calling getWorkspace directly.

…n for consistency

It's arguable we should create a new type of repository for this, but this will work for now. We can always split it out later.

The only permission below WorkspaceAccessLevels.Read is NoAccess, which isn't realistic, since the user has at least read permissions to get this far. Also, not making the additional call to Sam is purely an optimization - so better to just make the call to Sam.

…licative

this should be done by the MC workspace service

there's no longer any point keeping them separate, now that it's more concise

commit a793aa0 Author: Blake Geno <[email protected]> Date: Fri Sep 27 17:00:11 2024 -0400 WOR-1823 part 2: listWorkspaces refactors (#3048) * break out shared processing of workspace response This allows getWorkspace and getWorkspaceById to share the processing logic, without getWorkspaceById calling getWorkspace directly. * refactor getWorkspaceDetails to use for comprehension * remove pointless transaction when getting bucket options * remove inspection supression, fix resulting issues * clean up getBucketUsage and add unit tests * clean up context parameters to reflect actual usage * disabled invalidated tests for getWorkspace(workspaceId) * move getBucketOptions next to getBucketUsage and use for comprehension for consistency * add test for getBucketOptions * move raw datasource calls to repository It's arguable we should create a new type of repository for this, but this will work for now. We can always split it out later. * eliminate some raw usages of datasource * moved admin methods to separate service * only retrieve transfers from queried workspace * move database operations for lock/unlock into repository * fixed unit tests for getWorkspaceById * removed unused method * clean up * moved only method usage of tryIsCurator inline * move only usage of withLibraryAttributeNamespaceCheck to calling service * decoupled LibraryPermissionsSupport from RoleSupport * simplified canShare resolution The only permission below WorkspaceAccessLevels.Read is NoAccess, which isn't realistic, since the user has at least read permissions to get this far. Also, not making the additional call to Sam is purely an optimization - so better to just make the call to Sam. * add method to list submissions, change single query to return an option * add convenience apply method to RawlsExceptionWithErrorReport * refactor list workspaces * add unit tests for listWorkspaces * scalafmt * fixed WorkspaceServiceSpec organization headers, clean up tests * updates from PR review commit 729bb64 Author: Bria Morgan <[email protected]> Date: Fri Sep 27 13:03:32 2024 -0400 AJ-2002 Render local-dev config with gsm instead of vault (#3044) * new render without .ctmpl * use dsde-dev

marctalbott · 2024-09-30T22:01:10Z

core/src/main/scala/org/broadinstitute/dsde/rawls/util/TracingUtils.scala

+    * @param f the operation to execute within the trace
+    * @return the result of the operation `f`
+    */
+  def trace[T](name: String, parentContext: RawlsRequestContext)(f: RawlsRequestContext => T): T =


This appears to be equivalent to traceNakedWithParent below

marctalbott · 2024-09-30T22:15:00Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

+      leonardoService.cleanupResources(workspace.googleProjectId, workspace.workspaceIdAsUUID, ctx)
+    )
+    // Delete Google Project
+    _ <- traceFutureWithParent("deleteGoogleProject", ctx)(_ => deleteGoogleProject(workspace.googleProjectId))


There are technically still v1 workspaces in Rawls and this would bork all other v1 workspaces in a billing project by deleting the billing project's google project. Painful to have to leave it in given our stance on v1 workspaces, but as long as they're still around we probably need to keep the check

If it was a v1 workspace, we'd never have gotten this far.
The first operation is getWorkspaceContextAndPermissions, which only returns v2 workspaces. So calling this endpoint on a v1 workspace would already return a NoSuchWorkspace exception.

Note: IIRC, v1 workspaces are not returned or treated as valid targets for any user-facing operation at this point.

blakery commented Sep 30, 2024

View reviewed changes

blakery force-pushed the WOR-1823-part-3 branch from df30fda to 4b1f1f7 Compare September 30, 2024 12:59

blakery commented Sep 30, 2024

View reviewed changes

blakery marked this pull request as ready for review September 30, 2024 14:52

blakery requested review from a team, cahrens and marctalbott and removed request for a team September 30, 2024 14:52

blakery added 13 commits September 30, 2024 11:27

break out shared processing of workspace response

a85850a

This allows getWorkspace and getWorkspaceById to share the processing logic, without getWorkspaceById calling getWorkspace directly.

refactor getWorkspaceDetails to use for comprehension

5c4d9a2

remove pointless transaction when getting bucket options

7a44f60

remove inspection supression, fix resulting issues

860cc9a

clean up getBucketUsage and add unit tests

ec3c984

clean up context parameters to reflect actual usage

fff57c0

move getBucketOptions next to getBucketUsage and use for comprehensio…

bc2f459

…n for consistency

move raw datasource calls to repository

7b62fdb

It's arguable we should create a new type of repository for this, but this will work for now. We can always split it out later.

eliminate some raw usages of datasource

4a64f2f

moved admin methods to separate service

8ee5087

only retrieve transfers from queried workspace

7956f1e

move database operations for lock/unlock into repository

09cff13

fixed unit tests for getWorkspaceById

f6ce425

blakery added 25 commits September 30, 2024 11:27

moved only method usage of tryIsCurator inline

b280c87

move only usage of withLibraryAttributeNamespaceCheck to calling service

fb66ae0

add method to list submissions, change single query to return an option

4a9082c

refactor list workspaces

9bac0c0

add unit tests for listWorkspaces

21899ff

scalafmt

2be1d09

move sync validation method inline, and remove unnecessary future app…

b727928

…licative

add simple trace method for sync operations using RawlsRequestContext

be34c72

add an apply method for RawlsExceptionWithErrorReport

696ddd3

convert unnecessary usage of a Future Applicative to a sync operation

e4cdad4

rename, reformat, and clean up for more concise tests

5b944c5

refactor outer deleteWorkspace method and add tests

8f932ec

refactor deleteWorkspacesInternal and add tests

f644662

remove unused function

8190331

delete function moved to submissions repo

8a4d4d5

fix disabled annotations and remove unused imports

905e3f1

remove unnecessary cats effect

0a691a4

make service consistent about not handling MC workspace deletion

6b3e91b

this should be done by the MC workspace service

move deleteWorkspaceInternal inline

daf7335

there's no longer any point keeping them separate, now that it's more concise

move attribute exceptions to AttributeSupport

d5e3507

add back method that got overwritten when rebasing

aa4f1dc

added back attempt to delete workspace in WSM

5c5067a

run scalafmt

74fe8a1

blakery force-pushed the WOR-1823-part-3 branch from d52d4a9 to 74fe8a1 Compare September 30, 2024 15:27

cahrens approved these changes Sep 30, 2024

View reviewed changes

marctalbott reviewed Sep 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WOR-1823 part 3: workspace deletion refactoring #3049

WOR-1823 part 3: workspace deletion refactoring #3049

blakery commented Sep 27, 2024 •

edited

Loading

blakery Sep 30, 2024

blakery Sep 30, 2024

blakery Sep 30, 2024

blakery Sep 30, 2024

marctalbott Sep 30, 2024

blakery Oct 1, 2024

blakery Sep 30, 2024

blakery Sep 30, 2024

blakery Sep 30, 2024

blakery Sep 30, 2024

blakery Sep 30, 2024

blakery Sep 30, 2024

blakery Sep 30, 2024

marctalbott Sep 30, 2024

marctalbott Sep 30, 2024

blakery Oct 1, 2024


		import dataSource.dataAccess.driver.api._

		def getActiveWorkflowsAndSetStatusToAborted(

WOR-1823 part 3: workspace deletion refactoring #3049

Are you sure you want to change the base?

WOR-1823 part 3: workspace deletion refactoring #3049

Conversation

blakery commented Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakery commented Sep 27, 2024 •

edited

Loading