Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AJ-1438: disable most entity APIs for Azure workspaces #2641

Merged
merged 6 commits into from
Dec 10, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.broadinstitute.dsde.rawls.entities

import bio.terra.workspace.model.CloudPlatform
import org.broadinstitute.dsde.rawls.RawlsExceptionWithErrorReport
import org.broadinstitute.dsde.rawls.config.DataRepoEntityProviderConfig
import org.broadinstitute.dsde.rawls.dataaccess.datarepo.DataRepoDAO
Expand All @@ -9,7 +10,7 @@
import org.broadinstitute.dsde.rawls.entities.datarepo.{DataRepoEntityProvider, DataRepoEntityProviderBuilder}
import org.broadinstitute.dsde.rawls.entities.exceptions.DataEntityException
import org.broadinstitute.dsde.rawls.entities.local.{LocalEntityProvider, LocalEntityProviderBuilder}
import org.broadinstitute.dsde.rawls.model.ErrorReport
import org.broadinstitute.dsde.rawls.model.{ErrorReport, WorkspaceType}

import scala.concurrent.{ExecutionContext, Future}
import scala.reflect.runtime.universe._
Expand Down Expand Up @@ -42,6 +43,10 @@

def resolveProvider(requestArguments: EntityRequestArguments): Try[EntityProvider] = {

if (!WorkspaceType.RawlsWorkspace.equals(requestArguments.workspace.workspaceType)) {
throw new DataEntityException(s"This functionality only available to ${CloudPlatform.GCP} workspaces.")

Check warning on line 47 in core/src/main/scala/org/broadinstitute/dsde/rawls/entities/EntityManager.scala

View check run for this annotation

Codecov / codecov/patch

core/src/main/scala/org/broadinstitute/dsde/rawls/entities/EntityManager.scala#L47

Added line #L47 was not covered by tests
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the heart of the PR: throw an error for non-RawlsWorkspace workspaces.

// soon: look up the reference name to ensure it exists.
// for now, this simplistic logic illustrates the approach: choose the right builder for the job.
val targetTag = if (requestArguments.dataReference.isDefined) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -540,44 +540,31 @@ class EntityService(protected val ctx: RawlsRequestContext,
}
}

def copyEntities(entityCopyDef: EntityCopyDefinition,
uri: Uri,
linkExistingEntities: Boolean
): Future[EntityCopyResponse] =
getV2WorkspaceContextAndPermissions(entityCopyDef.destinationWorkspace,
SamWorkspaceActions.write,
Some(WorkspaceAttributeSpecs(all = false))
) flatMap { destWorkspaceContext =>
getV2WorkspaceContextAndPermissions(entityCopyDef.sourceWorkspace,
SamWorkspaceActions.read,
Some(WorkspaceAttributeSpecs(all = false))
) flatMap { sourceWorkspaceContext =>
dataSource.inTransaction { dataAccess =>
for {
sourceAD <- DBIO.from(
samDAO.getResourceAuthDomain(SamResourceTypeNames.workspace, sourceWorkspaceContext.workspaceId, ctx)
)
destAD <- DBIO.from(
samDAO.getResourceAuthDomain(SamResourceTypeNames.workspace, destWorkspaceContext.workspaceId, ctx)
)
result <- authDomainCheck(sourceAD.toSet, destAD.toSet) flatMap { _ =>
val entityNames = entityCopyDef.entityNames
val entityType = entityCopyDef.entityType
val copyResults = traceDBIOWithParent("checkAndCopyEntities", ctx)(s1 =>
dataAccess.entityQuery.checkAndCopyEntities(sourceWorkspaceContext,
destWorkspaceContext,
entityType,
entityNames,
linkExistingEntities,
s1
)
)
copyResults
}
} yield result
}
}
}
def copyEntities(entityCopyDef: EntityCopyDefinition, linkExistingEntities: Boolean): Future[EntityCopyResponse] =
for {
destWsCtx <- getV2WorkspaceContextAndPermissions(entityCopyDef.destinationWorkspace,
SamWorkspaceActions.write,
Some(WorkspaceAttributeSpecs(all = false))
)
sourceWsCtx <- getV2WorkspaceContextAndPermissions(entityCopyDef.sourceWorkspace,
SamWorkspaceActions.read,
Some(WorkspaceAttributeSpecs(all = false))
)
sourceAD <- samDAO.getResourceAuthDomain(SamResourceTypeNames.workspace, sourceWsCtx.workspaceId, ctx)
destAD <- samDAO.getResourceAuthDomain(SamResourceTypeNames.workspace, destWsCtx.workspaceId, ctx)
_ = authDomainCheck(sourceAD.toSet, destAD.toSet)
entityRequestArguments = EntityRequestArguments(destWsCtx, ctx, None, None)
entityProvider <- entityManager.resolveProviderFuture(entityRequestArguments)
entityCopyResponse <- entityProvider
.copyEntities(sourceWsCtx,
destWsCtx,
entityCopyDef.entityType,
entityCopyDef.entityNames,
linkExistingEntities,
ctx
)
.recover(bigQueryRecover)
} yield entityCopyResponse
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While moving the important implementation inside the LocalEntityProvider, I took the opportunity to refactor and simplify this method. It is now one big for-yield instead of a mix of flatMaps and for-yields, AND it removes outbound REST calls from Rawls to Sam from being inside a database transaction.


def batchUpdateEntitiesInternal(workspaceName: WorkspaceName,
entityUpdates: Seq[EntityUpdateDefinition],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@ import org.broadinstitute.dsde.rawls.model.{
AttributeEntityReference,
AttributeValue,
Entity,
EntityCopyDefinition,
EntityCopyResponse,
EntityQuery,
EntityQueryResponse,
EntityTypeMetadata,
RawlsRequestContext,
SubmissionValidationEntityInputs
SubmissionValidationEntityInputs,
Workspace
}

import scala.concurrent.Future
Expand Down Expand Up @@ -70,4 +73,12 @@ trait EntityProvider {
def batchUpdateEntities(entityUpdates: Seq[EntityUpdateDefinition]): Future[Traversable[Entity]]

def batchUpsertEntities(entityUpdates: Seq[EntityUpdateDefinition]): Future[Traversable[Entity]]

def copyEntities(sourceWorkspaceContext: Workspace,
destWorkspaceContext: Workspace,
entityType: String,
entityNames: Seq[String],
linkExistingEntities: Boolean,
parentContext: RawlsRequestContext
): Future[EntityCopyResponse]
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,16 @@
AttributeValueList,
AttributeValueRawJson,
Entity,
EntityCopyDefinition,
EntityCopyResponse,
EntityQuery,
EntityQueryResponse,
EntityTypeMetadata,
ErrorReport,
GoogleProjectId,
RawlsRequestContext,
SubmissionValidationEntityInputs
SubmissionValidationEntityInputs,
Workspace
}
import org.broadinstitute.dsde.rawls.util.CollectionUtils
import org.broadinstitute.dsde.rawls.{RawlsException, RawlsExceptionWithErrorReport}
Expand Down Expand Up @@ -524,4 +527,13 @@

override def batchUpsertEntities(entityUpdates: Seq[EntityUpdateDefinition]): Future[Traversable[Entity]] =
throw new UnsupportedEntityOperationException("batch-upsert entities not supported by this provider.")

override def copyEntities(sourceWorkspaceContext: Workspace,
destWorkspaceContext: Workspace,
entityType: String,
entityNames: Seq[String],
linkExistingEntities: Boolean,
parentContext: RawlsRequestContext
): Future[EntityCopyResponse] =
throw new UnsupportedEntityOperationException("copy entities not supported by this provider.")

Check warning on line 538 in core/src/main/scala/org/broadinstitute/dsde/rawls/entities/datarepo/DataRepoEntityProvider.scala

View check run for this annotation

Codecov / codecov/patch

core/src/main/scala/org/broadinstitute/dsde/rawls/entities/datarepo/DataRepoEntityProvider.scala#L538

Added line #L538 was not covered by tests
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,20 @@ import org.broadinstitute.dsde.rawls.model.{
AttributeEntityReference,
AttributeValue,
Entity,
EntityCopyDefinition,
EntityCopyResponse,
EntityQuery,
EntityQueryResponse,
EntityQueryResultMetadata,
EntityTypeMetadata,
ErrorReport,
RawlsRequestContext,
SamResourceTypeNames,
SamWorkspaceActions,
SubmissionValidationEntityInputs,
SubmissionValidationValue,
Workspace
Workspace,
WorkspaceAttributeSpecs
}
import org.broadinstitute.dsde.rawls.util.TracingUtils._
import org.broadinstitute.dsde.rawls.util.{AttributeSupport, CollectionUtils, EntitySupport}
Expand Down Expand Up @@ -394,4 +399,22 @@ class LocalEntityProvider(requestArguments: EntityRequestArguments,
override def batchUpsertEntities(entityUpdates: Seq[EntityUpdateDefinition]): Future[Traversable[Entity]] =
batchUpdateEntitiesImpl(entityUpdates, upsert = true)

override def copyEntities(sourceWorkspaceContext: Workspace,
destWorkspaceContext: Workspace,
entityType: String,
entityNames: Seq[String],
linkExistingEntities: Boolean,
parentContext: RawlsRequestContext
): Future[EntityCopyResponse] =
traceWithParent("checkAndCopyEntities", parentContext)(_ =>
dataSource.inTransaction { dataAccess =>
dataAccess.entityQuery.checkAndCopyEntities(sourceWorkspaceContext,
destWorkspaceContext,
entityType,
entityNames,
linkExistingEntities,
parentContext
)
}
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,14 @@ trait WorkspaceSupport {
} yield ()

// can't use withClonedAuthDomain because the Auth Domain -> no Auth Domain logic is different
def authDomainCheck(sourceWorkspaceADs: Set[String], destWorkspaceADs: Set[String]): ReadWriteAction[Boolean] =
def authDomainCheck(sourceWorkspaceADs: Set[String], destWorkspaceADs: Set[String]): Boolean =
// if the source has any auth domains, the dest must also *at least* have those auth domains
if (sourceWorkspaceADs.subsetOf(destWorkspaceADs)) DBIO.successful(true)
if (sourceWorkspaceADs.subsetOf(destWorkspaceADs)) true
else {
val missingGroups = sourceWorkspaceADs -- destWorkspaceADs
val errorMsg =
s"Source workspace has an Authorization Domain containing the groups ${missingGroups.mkString(", ")}, which are missing on the destination workspace"
DBIO.failed(new RawlsExceptionWithErrorReport(ErrorReport(StatusCodes.UnprocessableEntity, errorMsg)))
throw new RawlsExceptionWithErrorReport(ErrorReport(StatusCodes.UnprocessableEntity, errorMsg))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This authDomainCheck method has only one usage - inside copyEntities, which I rewrote above. There was no need for authDomainCheck to return its result wrapped in a ReadWriteAction.


// WorkspaceContext helpers
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ trait EntityApiService extends UserInfoDirectives {
entity(as[EntityCopyDefinition]) { copyDefinition =>
complete {
entityServiceConstructor(ctx)
.copyEntities(copyDefinition, request.uri, linkExistingEntitiesBool)
.copyEntities(copyDefinition, linkExistingEntitiesBool)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea why the request URI was passed into copyEntities. It was never used, and I removed it from the argument list for copyEntities.

.map { response =>
if (
response.hardConflicts.isEmpty && (response.softConflicts.isEmpty || linkExistingEntitiesBool)
Expand Down
Loading