Add a multi-fetch operation #4132

imsdu · 2023-08-04T08:10:06Z

imsdu · 2023-08-04T08:22:41Z

...s/archive/src/main/scala/ch/epfl/bluebrain/nexus/delta/plugins/archive/ArchiveDownload.scala

@@ -5,7 +5,7 @@ import akka.util.ByteString
 import cats.implicits._
 import ch.epfl.bluebrain.nexus.delta.plugins.archive.model.ArchiveReference.{FileReference, FileSelfReference, ResourceReference}
 import ch.epfl.bluebrain.nexus.delta.plugins.archive.model.ArchiveRejection._
-import ch.epfl.bluebrain.nexus.delta.plugins.archive.model.ArchiveResourceRepresentation._
+import ch.epfl.bluebrain.nexus.delta.sdk.model.ResourceRepresentation._


I used the same way to chose the output format as archives so I reused (moved and renamed) the class responsible for it

shinyhappydan · 2023-08-04T09:22:01Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/model/ResourceRepresentation.scala

+      case SourceJson.toString      => Right(SourceJson)
+      case CompactedJsonLd.toString => Right(CompactedJsonLd)
+      case ExpandedJsonLd.toString  => Right(ExpandedJsonLd)
+      case NTriples.toString        => Right(NTriples)
+      case NQuads.toString          => Right(NQuads)
+      case Dot.toString             => Right(Dot)
+      case other                    => Left(s"$other is not a valid representation")
+    }


String literals are more appropriate here, as we probably don't want the API to change if we rename one of these (unless we're sure these are covered by tests)

The toString method is overriden.

It is not the most elegant way but I just moved this class from archives to here and it looks good enough

shinyhappydan · 2023-08-04T09:24:51Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/multifetch/MultiFetch.scala

+        request.resources
+          .traverse { input =>
+            for {
+              authorized <- aclCheck.authorizeFor(input.project, resources.read, fetchAllCached)


resources looks like a variable and it's quite confusing 😅

shinyhappydan · 2023-08-04T09:26:24Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/multifetch/MultiFetch.scala

+      override def apply(request: MultiFetchRequest)(implicit
+          caller: Caller
+      ): UIO[MultiFetchResponse] = {
+        val fetchAllCached = aclCheck.fetchAll.memoizeOnSuccess


If you group by project then you don't have to cache everything, which is nice

It avoids to go through the list several times.
Also the acl check is not only done at the project level as it tries to check at the org level and the root level so this should make it a little faster as it only queries the db once

If a cache makes sense because it does the org level check then that's fine, but avoiding going through lists multiple times isn't something we should be concerned with. It would have no performance impact and we should optimise for readability instead

It really depends what you do when you are doing with your lists.
And doing multiple passes can hurt readibility too, there is not a golden rule for this.
Here doing it in a single pass with the traverse also guarantees that the items are kept in order

shinyhappydan · 2023-08-04T09:28:34Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/multifetch/MultiFetch.scala

+              if (authorized) resource.fold[Result](NotFound(input.id, input.project)) { content =>
+                Success(input.id, input.project, content)
+              }


I know this is personal preference, but .getOrElse is much clearer intent than folding on an Option

shinyhappydan · 2023-08-04T09:29:34Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/multifetch/MultiFetch.scala

+              resource   <- if (authorized) fetchResource(input) else UIO.none
+            } yield {
+              if (authorized) resource.fold[Result](NotFound(input.id, input.project)) { content =>
+                Success(input.id, input.project, content)
+              }


This double if(authorized) feels like it could be simplified

shinyhappydan · 2023-08-04T09:33:14Z

...k/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/multifetch/model/MultiFetchResponse.scala

+    val encodeItem = itemEncoder(format)
+    resources.traverse(encodeItem).map { r =>
+      Json.obj(
+        "format"    -> format.asJson,


If the caller specifies the format, I wonder if it's useful to return it

Adding it does not hurt and as users of the API often gets confused between the formats, it can help to answer their questions

I think it hurts in the sense that it's code to maintain and which we test, but if you think it's worth it that's fine

shinyhappydan · 2023-08-04T09:38:28Z

delta/sdk/src/test/scala/ch/epfl/bluebrain/nexus/delta/sdk/multifetch/MultiFetchSuite.scala

+  test("Return the response matching the user acls") {
+
+    val expected = MultiFetchResponse(
+      ResourceRepresentation.NTriples,
+      NonEmptyList.of(
+        Success(Latest(successId), project1, successContent),
+        NotFound(Latest(notFoundId), project1),
+        AuthorizationFailed(Latest(unauthorizedId), project2)
+      )
+    )
+
+    multiFetch(request).assert(expected)
+  }
+
+  test("Return only unauthorized for a user with no access") {
+    val expected = MultiFetchResponse(
+      ResourceRepresentation.NTriples,
+      NonEmptyList.of(
+        AuthorizationFailed(Latest(successId), project1),
+        AuthorizationFailed(Latest(notFoundId), project1),
+        AuthorizationFailed(Latest(unauthorizedId), project2)
+      )
+    )
+
+    multiFetch(request)(Caller.Anonymous).assert(expected)
+  }


Consider having more, less broad tests. For example, returning not found is probably one test. There are more tests but they are more straightforward and point to the defect more directly

Multi-fetch is supposed to return the resources in the order they are requested whether it is a success or not.
That is why I took this approach to validate also this part.

Yeah I'm just saying that these tests are basically testing multiple things

Another way of writing these tests would be to have:

A resource is found

A resource is missing

A resource has no authorization

Resources are in order

Resources are returned as type X

I would agree if the complexity was higher but here it is simple

olivergrabinski · 2023-08-04T08:53:24Z

delta/app/src/main/scala/ch/epfl/bluebrain/nexus/delta/routes/MultiFetchRoutes.scala

+  def apply(identities: Identities, aclCheck: AclCheck, multiFetch: MultiFetch)(implicit
+      baseUri: BaseUri,
+      cr: RemoteContextResolution,
+      ordering: JsonKeyOrdering,
+      s: Scheduler
+  ): Route = new MultiFetchRoutes(identities, aclCheck, multiFetch).routes


Is the purpose of these apply methods to avoid the use of new in the Modules?

They are only used in the tests if I remember correctly.
I was in a auto-mode and stick to what has been done before but yes, their utility is questionable

olivergrabinski · 2023-08-04T10:06:25Z

docs/src/main/paradox/docs/delta/api/multi-fetch.md

+
+The following example shows how to perform a multi-fetch and an example of response
+containing errors (missing permissions and resource not found).
+As a response, a json is returned.


It may be interesting to put emphasis on the fact that this is unlike other endpoints which return json-ld

Simon Dumas added 2 commits August 4, 2023 10:05

Add a multi-fetch operation

52aec95

Fix layout + add mention in the release notes

0aea61d

imsdu commented Aug 4, 2023

View reviewed changes

Docs

800ba2b

shinyhappydan reviewed Aug 4, 2023

View reviewed changes

olivergrabinski reviewed Aug 4, 2023

View reviewed changes

Simon Dumas added 2 commits August 4, 2023 14:39

Address feedback

bb58d34

Update docs

466268b

shinyhappydan approved these changes Aug 4, 2023

View reviewed changes

Merge branch 'master' into 4055-fetch-multi-resources

678ab71

imsdu merged commit 221b199 into BlueBrain:master Aug 7, 2023
6 checks passed

imsdu deleted the 4055-fetch-multi-resources branch August 7, 2023 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a multi-fetch operation #4132

Add a multi-fetch operation #4132

imsdu commented Aug 4, 2023 •

edited

Loading

imsdu Aug 4, 2023 •

edited

Loading

shinyhappydan Aug 4, 2023

imsdu Aug 4, 2023 •

edited

Loading

shinyhappydan Aug 4, 2023

shinyhappydan Aug 4, 2023

imsdu Aug 4, 2023

shinyhappydan Aug 4, 2023

imsdu Aug 4, 2023

shinyhappydan Aug 4, 2023

shinyhappydan Aug 4, 2023

shinyhappydan Aug 4, 2023

imsdu Aug 4, 2023

shinyhappydan Aug 4, 2023

shinyhappydan Aug 4, 2023

imsdu Aug 4, 2023

shinyhappydan Aug 4, 2023

imsdu Aug 4, 2023

olivergrabinski Aug 4, 2023

imsdu Aug 4, 2023

olivergrabinski Aug 4, 2023

Add a multi-fetch operation #4132

Add a multi-fetch operation #4132

Conversation

imsdu commented Aug 4, 2023 • edited Loading

imsdu Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsdu Aug 4, 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

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 commented Aug 4, 2023 •

edited

Loading

imsdu Aug 4, 2023 •

edited

Loading

imsdu Aug 4, 2023 •

edited

Loading