Add bulk create by copy operation for files #4483

dantb · 2023-11-08T19:05:18Z

Fixes #4400 with a bulk file copy operation.

This PR unfortunately got massive and I'd be happy to pair with someone to go through anything not clear.

Docs still to come

shinyhappydan

Regarding the permissions: I would imagine you should only need files/read on the source and files/write on the destination

...ugins/storage/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/Files.scala

.../src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/routes/FilesRoutes.scala

...ugins/storage/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/Files.scala

.../src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/routes/FilesRoutes.scala

.../main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/routes/CopyFilePayload.scala

...torage/src/test/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/FileFixtures.scala

...s/storage/src/test/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/FilesSpec.scala

.../test/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/routes/FilesRoutesSpec.scala

tests/src/test/scala/ch/epfl/bluebrain/nexus/tests/HttpClient.scala

…age space during copy

docs/src/main/paradox/docs/delta/api/files-api.md

imsdu · 2023-12-05T15:43:53Z

docs/src/main/paradox/docs/delta/api/files-api.md

+  "destinationFilename": "{destinationFilename}",
+  "sourceProjectRef": "{sourceOrg}/{sourceProj}",
+  "sourceFileId": "{sourceFileId}",
+  "sourceTag": "{sourceTagName}",


In some other places, we just pass the id suffixed by the rev/tag (example: multi-fetch)

delta/sdk/src/main/resources/contexts/bulk-operation.json

...ugins/storage/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/Files.scala

imsdu · 2023-12-05T15:56:40Z

...ugins/storage/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/Files.scala

+    destFilesAttributes.traverse { destFileAttributes =>
+      for {
+        iri      <- generateId(pc)
+        command   = CreateFile(iri, dest.project, destStorageRef, destStorageTpe, destFileAttributes, c.subject, dest.tag)


It would be interesting to keep track that this file is a copy

Good shout, Cristina mentioned wanting that - this is already massive so I'm thinking do in a follow up?

.../src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/routes/FilesRoutes.scala

… to these

...orage/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/StoragePluginModule.scala

...age/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/batch/BatchCopy.scala

...main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/routes/BatchFilesRoutes.scala

imsdu · 2023-12-14T10:09:40Z

...ch/epfl/bluebrain/nexus/delta/plugins/storage/storages/operations/StorageFileRejection.scala

+  /**
+    * Rejection returned when a storage cannot fetch a file's attributes
+    */
+  sealed abstract class CopyFileRejection(loggedDetails: String) extends StorageFileRejection(loggedDetails)


Could we have it only on the FileRejection side ?

Ahh as in ditch the existing pattern since it just adds more error types than are necessary? I guess this came from monix typed errors in the past

imsdu · 2023-12-14T10:26:27Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/jsonld/BulkOperationResults.scala

+import io.circe.syntax._
+import io.circe.{Encoder, Json, JsonObject}
+
+final case class BulkOperationResults[A](results: Seq[A])


In the end, it is not something that is restrained to bulk operations, we could have a more generic name

I would go the opposite direction and call it CopyFilesResult 😆

Is it not? The only purpose of this type was to encode the bulk-operation context in a contained way

If it is a CopyFileResult, it should go in the storage plugin (and the related context too)

shinyhappydan · 2023-12-14T09:20:07Z

...rnel/src/main/scala/ch/epfl/bluebrain/nexus/delta/kernel/utils/TransactionalFileCopier.scala

    Ref.of[IO, Option[CopyOperationFailed]](None).flatMap { errorRef =>
      files
        .parTraverse { case c @ CopyBetween(source, dest) =>
-          copySingle(source, dest).onError(_ => errorRef.set(Some(CopyOperationFailed(c))))
+          copySingle(source, dest).onError(e => errorRef.set(Some(CopyOperationFailed(c, e))))


Why does the ref get used here rather than modifying the error?

Don't think I understand the question 🤔

Why are we setting a ref when we could just return the error?

I'll refactor this afterwards

...src/test/scala/ch/epfl/bluebrain/nexus/delta/kernel/utils/TransactionalFileCopierSuite.scala

...orage/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/StoragePluginModule.scala

...age/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/batch/BatchCopy.scala

...n/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/storages/operations/disk/package.scala

shinyhappydan · 2023-12-14T10:42:54Z

...rc/test/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/batch/BatchCopySuite.scala

+    val events                         = ListBuffer.empty[Event]
+    val (sourceFileRes, sourceStorage) = genFileResourceAndStorage(sourceFileId, sourceProj.context, diskVal)
+    val (user, aclCheck)               = userAuthorizedOnProjectStorage(sourceStorage.value)
+
+    val batchCopy                = mkBatchCopy(
+      fetchFile = stubbedFetchFile(sourceFileRes, events),
+      fetchStorage = stubbedFetchStorage(sourceStorage, events),
+      aclCheck = aclCheck,
+      stats = stubbedStorageStats(storageStatEntry, events),
+      diskCopy = stubbedDiskCopy(stubbedFileAttr, events)
+    )


This is a big setup

shinyhappydan · 2023-12-14T10:43:20Z

...rc/test/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/batch/BatchCopySuite.scala

+    val destStorage: DiskStorage = genDiskStorage()
+
+    batchCopy.copyFiles(source, destStorage)(caller(user)).map { obtained =>
+      val obtainedEvents = events.toList


I wonder if an abstraction around the events list might be a bit clearer

There is actually a really nice pure FP abstraction for this kinda of testing (no mutable state), but it relies on tagless final / monad transformers so not a good fit here. I'll think about how to make it clearer tho 👍

I don't think the mutable state is the problem, it just needs to read better. I was thinking just a tiny class / enclosure

shinyhappydan · 2023-12-14T10:44:13Z

...c/test/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/batch/BatchFilesSuite.scala

+  test("batch copying should fetch storage, perform copy and evaluate create file commands") {
+    val events                        = ListBuffer.empty[Event]
+    val destProj: Project             = genProject()
+    val (destStorageRef, destStorage) = (genRevision(), genStorage(destProj.ref, diskVal))
+    val fetchFileStorage              = mockFetchFileStorage(destStorageRef, destStorage.storage, events)
+    val stubbedDestAttributes         = genAttributes()
+    val batchCopy                     = BatchCopyMock.withStubbedCopyFiles(events, stubbedDestAttributes)
+    val destFileUUId                  = UUID.randomUUID() // Not testing UUID generation, same for all of them
+
+    val batchFiles: BatchFiles = mkBatchFiles(events, destProj, destFileUUId, fetchFileStorage, batchCopy)
+    implicit val c: Caller     = Caller(genUser(), Set())
+    val (source, destination)  = (genCopyFileSource(), genCopyFileDestination(destProj.ref, destStorage.storage))
+    val obtained               = batchFiles.copyFiles(source, destination).accepted
+
+    val expectedFileIri = destProj.base.iri / destFileUUId.toString
+    val expectedCmds    = stubbedDestAttributes.map(
+      CreateFile(expectedFileIri, destProj.ref, destStorageRef, destStorage.value.tpe, _, c.subject, destination.tag)
+    )
+
+    // resources returned are based on file command evaluation
+    assertEquals(obtained, expectedCmds.map(genFileResourceFromCmd))
+
+    val expectedActiveStorageFetched = ActiveStorageFetched(destination.storage, destProj.ref, destProj.context, c)
+    val expectedBatchCopyCalled      = BatchCopyCalled(source, destStorage.storage, c)
+    val expectedCommandsEvaluated    = expectedCmds.toList.map(FileCommandEvaluated)
+    val expectedEvents               = List(expectedActiveStorageFetched, expectedBatchCopyCalled) ++ expectedCommandsEvaluated
+    assertEquals(events.toList, expectedEvents)
+  }


Not sure what to suggest but this feels like a really big test

shinyhappydan · 2023-12-14T10:47:36Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/jsonld/BulkOperationResults.scala

+import io.circe.syntax._
+import io.circe.{Encoder, Json, JsonObject}
+
+final case class BulkOperationResults[A](results: Seq[A])


I would go the opposite direction and call it CopyFilesResult 😆

imsdu · 2023-12-18T12:33:38Z

...ge/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/batch/BatchFiles.scala

+        for {
+          iri      <- generateId(pc)
+          command   =
+            CreateFile(iri, dest.project, destStorageRef, destStorageTpe, destFileAttributes, c.subject, dest.tag)


Not sure if we want to preserve the original tag.
It would be nice in some way to know that this file has been copied from another

imsdu · 2023-12-18T14:01:35Z

tests/src/test/scala/ch/epfl/bluebrain/nexus/tests/kg/DiskStorageSpec.scala

@@ -8,7 +8,7 @@ import ch.epfl.bluebrain.nexus.tests.iam.types.Permission
 import io.circe.Json
 import org.scalatest.Assertion

-class DiskStorageSpec extends StorageSpec {
+class DiskStorageSpec extends StorageSpec with CopyFilesSpec {


It could be other test classes dedicated for the batch operation ?
Those are already complicated, it may be better to split and just keep the creation of storages in common between them ?

dantb requested review from shinyhappydan, imsdu and olivergrabinski November 8, 2023 19:05

shinyhappydan reviewed Nov 9, 2023

View reviewed changes

...ugins/storage/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/Files.scala Outdated Show resolved Hide resolved

.../src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/routes/FilesRoutes.scala Outdated Show resolved Hide resolved

imsdu reviewed Nov 9, 2023

View reviewed changes

...ugins/storage/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/Files.scala Outdated Show resolved Hide resolved

dantb force-pushed the copy-files branch from 10e0771 to 077d2e9 Compare November 13, 2023 16:16

dantb marked this pull request as ready for review November 14, 2023 12:51

dantb changed the title ~~WIP - Support copying files to another project~~ Add copy operation to files Nov 14, 2023

dantb changed the title ~~Add copy operation to files~~ Add file create by copy operation Nov 14, 2023

dantb changed the title ~~Add file create by copy operation~~ Add create by copy operation for files Nov 14, 2023

imsdu reviewed Nov 17, 2023

View reviewed changes

...ugins/storage/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/Files.scala Outdated Show resolved Hide resolved

shinyhappydan previously approved these changes Nov 17, 2023

View reviewed changes

dantb dismissed shinyhappydan’s stale review via 0450af6 November 19, 2023 19:22

dantb marked this pull request as draft November 21, 2023 12:36

dantb force-pushed the copy-files branch from 755b50b to ddd0c99 Compare November 28, 2023 15:32

dantb added 2 commits November 29, 2023 12:32

Add support for bulk copying files to another project

8eb9a15

Accept list of files in copy route

b30560f

dantb force-pushed the copy-files branch from 6b4dd48 to b30560f Compare November 29, 2023 11:32

dantb added 2 commits December 4, 2023 21:55

Move CopyFiles to kernel, reuse for delta disk storage, validate stor…

f2c6b52

…age space during copy

Return bulk response with context, test route

5c327f1

imsdu reviewed Dec 5, 2023

View reviewed changes

docs/src/main/paradox/docs/delta/api/files-api.md Outdated Show resolved Hide resolved

imsdu reviewed Dec 5, 2023

View reviewed changes

delta/sdk/src/main/resources/contexts/bulk-operation.json Outdated Show resolved Hide resolved

imsdu reviewed Dec 5, 2023

View reviewed changes

...ugins/storage/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/Files.scala Outdated Show resolved Hide resolved

imsdu reviewed Dec 5, 2023

View reviewed changes

.../src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/routes/FilesRoutes.scala Outdated Show resolved Hide resolved

dantb added 4 commits December 7, 2023 09:02

Fix paths when saving copied file attributes

d4a5a10

Refactoring

2a37f77

Add test with source files in different storages

a366c83

Extract bulk file operations into new package

2291cdf

dantb added 9 commits December 8, 2023 11:37

Create BatchFilesRoutes, wire up with bulk file operations

30668e0

Unit test batch files routes

6a4576b

Reduce dependency surface area for BatchFiles + unit test it

cba45f8

Delete previous copy impl, test and fix error mapping in routes

cbe3065

Test rejections are mapped correctly

2477b8f

Create abstractions for file and storage fetching + narrow dependents…

30f8dac

… to these

Test batch copying logic

a6f4fe6

Merge branch 'master' into copy-files

9c9e930

Tidy up

25fe7e4

dantb changed the title ~~Add create by copy operation for files~~ Add bulk create by copy operation for files Dec 13, 2023

Fix test

e0e5b2a

dantb marked this pull request as ready for review December 14, 2023 09:14

imsdu reviewed Dec 14, 2023

View reviewed changes

...orage/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/StoragePluginModule.scala Outdated Show resolved Hide resolved

imsdu reviewed Dec 14, 2023

View reviewed changes

...age/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/batch/BatchCopy.scala Show resolved Hide resolved

imsdu reviewed Dec 14, 2023

View reviewed changes

...main/scala/ch/epfl/bluebrain/nexus/delta/plugins/storage/files/routes/BatchFilesRoutes.scala Outdated Show resolved Hide resolved

imsdu reviewed Dec 14, 2023

View reviewed changes

shinyhappydan approved these changes Dec 14, 2023

View reviewed changes

From comments

a8fe061

imsdu reviewed Dec 18, 2023

View reviewed changes

dantb added 2 commits December 18, 2023 17:21

Merge branch 'master' into copy-files

654e8ac

Merge branch 'master' into copy-files

33205b4

dantb merged commit 50de0ec into BlueBrain:master Dec 18, 2023
8 checks passed

dantb deleted the copy-files branch December 18, 2023 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bulk create by copy operation for files #4483

Add bulk create by copy operation for files #4483

dantb commented Nov 8, 2023 •

edited

Loading

shinyhappydan left a comment

imsdu Dec 5, 2023

imsdu Dec 5, 2023

dantb Dec 13, 2023

imsdu Dec 14, 2023

dantb Dec 18, 2023 •

edited

Loading

imsdu Dec 14, 2023

shinyhappydan Dec 14, 2023

dantb Dec 18, 2023

imsdu Dec 18, 2023 •

edited

Loading

shinyhappydan Dec 14, 2023

dantb Dec 18, 2023

shinyhappydan Dec 18, 2023

dantb Dec 18, 2023

shinyhappydan Dec 14, 2023

shinyhappydan Dec 14, 2023

dantb Dec 18, 2023

shinyhappydan Dec 18, 2023

shinyhappydan Dec 14, 2023

shinyhappydan Dec 14, 2023

imsdu Dec 18, 2023

imsdu Dec 18, 2023 •

edited

Loading

Add bulk create by copy operation for files #4483

Add bulk create by copy operation for files #4483

Conversation

dantb commented Nov 8, 2023 • edited Loading

shinyhappydan left a 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

dantb Dec 18, 2023 • 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

imsdu Dec 18, 2023 • 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

imsdu Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

dantb commented Nov 8, 2023 •

edited

Loading

dantb Dec 18, 2023 •

edited

Loading

imsdu Dec 18, 2023 •

edited

Loading

imsdu Dec 18, 2023 •

edited

Loading