Update realm and schema module to rely on cats-effect #4194

imsdu · 2023-08-23T15:51:45Z

No description provided.

imsdu · 2023-08-28T15:56:48Z

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

@@ -49,6 +50,13 @@ class RealmsRoutes(identities: Identities, realms: Realms, aclCheck: AclCheck)(i
      RealmSearchParams(None, deprecated, rev, createdBy, updatedBy)
    }

+  private def emitFetch(io: IO[RealmResource]): Route = emit(io.attemptNarrow[RealmRejection])


.attemptNarrow allows to fallback to a IO[Either[RealmRejection, A]]

imsdu · 2023-08-28T15:58:24Z

...c/main/scala/ch/epfl/bluebrain/nexus/delta/kernel/effect/migration/MigrateEffectSyntax.scala

+
+import scala.reflect.ClassTag
+
+trait MigrateEffectSyntax {


Allow to go to BIO to IO and IO to BIO

imsdu · 2023-08-28T15:59:04Z

...a/kernel/src/main/scala/ch/epfl/bluebrain/nexus/delta/kernel/kamon/KamonMonitoringCats.scala

+
+import scala.concurrent.duration._
+
+object KamonMonitoringCats {


KamonMonitoring adapted to CE IO

imsdu · 2023-08-28T16:00:01Z

...ain/scala/ch/epfl/bluebrain/nexus/delta/kernel/utils/CatsEffectsClasspathResourceUtils.scala

+import scala.io.{Codec, Source}
+import scala.jdk.CollectionConverters._
+
+trait CatsEffectsClasspathResourceUtils {


ClasspathResourceUtils for CE IO

imsdu · 2023-08-28T16:01:36Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/ce/CatsResponseToJsonLd.scala

+import java.nio.charset.StandardCharsets
+import java.util.Base64
+
+sealed trait CatsResponseToJsonLd {


To serialize both success and error channels in the API
Adapted from ResponseToJsonLd

imsdu · 2023-08-28T16:02:02Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/ce/DeltaDirectives.scala

+
+object DeltaDirectives extends DeltaDirectives
+
+trait DeltaDirectives extends UriDirectives {


Delta directives for CE IO

imsdu · 2023-08-28T16:04:56Z

delta/sdk/src/test/scala/ch/epfl/bluebrain/nexus/delta/sdk/realms/RealmsEvaluateSuite.scala

+  private def openIdAlreadyExists(uri: Uri) =
+    (_: Label, _: Uri) => IO.raiseError(RealmOpenIdConfigAlreadyExists(label, uri))
+
+  group("Evaluate a create command") {


Idea copied from fs2, the output looks like this:

ch.epfl.bluebrain.nexus.delta.sdk.schemas.SchemasImplSuite: + Creating a schema - Succeed with the id present in the payload 0.829s + Creating a schema - Fail with different ids on the payload and passed 0.008s + Updating a schema - Succeed 0.042s + Updating a schema - Fail if the schema does not exist 0.009s + Refreshing a schema - succeed 0.037s + Refreshing a schema - Fail if it does not exist 0.006s

imsdu · 2023-08-28T16:06:07Z

delta/testkit/src/main/scala/ch/epfl/bluebrain/nexus/testkit/ce/CatsEffectAssertions.scala

+import scala.reflect.ClassTag
+import scala.util.control.NonFatal
+
+trait CatsEffectAssertions { self: Assertions =>


Taken from munit cats-effects

I am missing what is blocking us from just using munit-cats-effect directly?

It is not maintained by CE2 and it relies on a outdated version of MUnit

imsdu · 2023-08-31T09:12:00Z

delta/app/src/test/scala/ch/epfl/bluebrain/nexus/delta/routes/ElemRoutesSpec.scala

 import fs2.Stream
 import monix.bio.{Task, UIO}

 import java.time.Instant
 import java.util.UUID

-class ElemRoutesSpec extends BaseRouteSpec with CirceLiteral {
+class ElemRoutesSpec extends BaseRouteSpec with CirceLiteral with IOFromMap {


Temporary, we can change this back later

shinyhappydan · 2023-08-31T09:45:38Z

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

+                          (parameter("rev".as[Int].?) & pathEndOrSingleSlash & entity(as[Json])) {
+                            case (None, source)      =>
+                              // Create a schema with id segment
+                              emitMetadata(Created, schemas.create(id, ref, source).flatTap(index))


I think this is much clearer, thanks

shinyhappydan · 2023-08-31T09:47:00Z

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

-                    operationName(s"$prefixSegment/schemas/{org}/{project}/{id}/source") {
-                      authorizeFor(ref, Read).apply {
-                        implicit val source: Printer = sourcePrinter
-                        val sourceIO                 = schemas.fetch(id, ref).map(_.value.source)
-                        emit(sourceIO.leftWiden[SchemaRejection].rejectOn[SchemaNotFound])
-                      }
+                    authorizeFor(ref, Read).apply {
+                      emitSource(schemas.fetch(id, ref))


It's really nice to hide this printer boilerplate

shinyhappydan · 2023-08-31T09:51:05Z

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

+  private def emitTags(io: IO[SchemaResource]): Route =
+    emit(io.map(_.value.tags).attemptNarrow[SchemaRejection].rejectOn[SchemaNotFound])
+


I don't love the rejectOn being hidden here, although it's probably just nicer if we removed rejectOn :)

It felt better than having it separated from attemptNarrow
And we can't get rid of the rejectOn for now

shinyhappydan · 2023-08-31T09:57:15Z

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

@@ -17,6 +17,15 @@ trait IOUtils {
    clock.realTime(TimeUnit.MILLISECONDS).map(Instant.ofEpochMilli)
 }

+object IOUtils extends IOUtils


Consider calling Clock.now or similar, the important thing here is that you're getting the current time

IOUtils is gonna be replaced with IOInstant progressively

Yes, but IOInstant.get is less readable than something like now or Clock.now where the intention is clearer

If you imported get then you would have to rename it everywhere, so calling it now instead of get helps

shinyhappydan · 2023-08-31T09:58:49Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/realms/Realms.scala

    // format: off
    def create(c: CreateRealm) =
      state.fold {
-        openIdExists(c.label, c.openIdConfig) >> (wellKnown(c.openIdConfig), IOUtils.instant).mapN {
+        openIdExists(c.label, c.openIdConfig) >> (wellKnown(c.openIdConfig), IOInstant.get).mapN {


This could be just now with the rename I suggested earlier and an import of the method

shinyhappydan · 2023-08-31T10:01:40Z

delta/sdk/src/test/scala/ch/epfl/bluebrain/nexus/delta/sdk/realms/RealmsEvaluateSuite.scala

+  group("Fail with realm not found") {
+    List(
+      None -> UpdateRealm(label, 1, name, wellKnownUri, None, None, subject),
+      None -> DeprecateRealm(label, 1, subject)
+    ).foreach { case (state, cmd) =>
+      test(s"for a ${cmd.getClass.getSimpleName} command when the state does not exist") {
+        evaluate(wkResolution, (_, _) => IO.unit)(state, cmd).intercept[RealmNotFound]
+      }
+    }
+  }
+


This isn't a group

shinyhappydan · 2023-08-31T10:04:36Z

...ourcing-psql/src/main/scala/ch/epfl/bluebrain/nexus/delta/sourcing/rejection/Rejection.scala

+
+abstract class Rejection extends Exception with Product with Serializable {
+
+  def reason: String


I think the message in exception should probably be this reason, perhaps even reason should be an argument. It's definitely worth thinking about this now before everything is using it and it's hard to change

The reason is currently brought up all the way to the API to serialize the errors so it is difficult to change reason for message

shinyhappydan · 2023-08-31T10:05:51Z

delta/testkit/src/main/scala/ch/epfl/bluebrain/nexus/testkit/NexusSuite.scala

+  protected def group(name: String)(thunk: => Unit): Unit = {
+    val countBefore     = munitTestsBuffer.size
+    val _               = thunk
+    val countAfter      = munitTestsBuffer.size
+    val countRegistered = countAfter - countBefore
+    val registered      = munitTestsBuffer.toList.drop(countBefore)
+    (0 until countRegistered).foreach(_ => munitTestsBuffer.remove(countBefore))
+    registered.foreach(t => munitTestsBuffer += t.withName(s"$name - ${t.name}"))
+  }


I would just want to know that individual tests can still run in intellij easily? As in, click the test name and then run

I don't think it's possible. It's also not possible to run a group like this. And to run a single test (from the CLI for instance) you need to specify "{groupName} - {testName}" to run an individual test.

I never use this feature maybe with a munit extension it does

It's possible to run a single normal munit test, I just need that to still be possible

Cf. scalameta/munit#633

shinyhappydan · 2023-08-31T10:08:09Z

delta/testkit/src/main/scala/ch/epfl/bluebrain/nexus/testkit/bio/OptionAssertions.scala

+    def assertSome(value: A): Unit =
+      assert(option.contains(value), s"$value was not found in the option, it contains '${option.getOrElse("None")}'")
+    def assertNone(): Unit         =
+      assert(
+        option.isEmpty,
+        s"The option is not empty, it contains ${option.getOrElse(new IllegalStateException("Should not happen))}."))}"
+      )
+
+    def assertSome(): Unit =
+      assert(option.isDefined, s"The option is empty.")


I don't know if I like these methods. It feels like these failure messages won't be helpful

imsdu marked this pull request as draft August 24, 2023 08:20

Simon Dumas added 3 commits August 25, 2023 10:20

Update realm module to rely on cats-effect

d086ca0

Migrate some tests

90e7eb7

Migrate schema module

02ca26d

imsdu force-pushed the 4162-investigate-monix-bio branch from a00b83d to 02ca26d Compare August 25, 2023 09:00

Simon Dumas added 7 commits August 25, 2023 11:08

Update realm routes

49d9efd

Migrate schema tests + route tests

9317331

Remove old test classes

bf6cc0e

Remove the need for monix bio in the routes

2599df1

Refactor tests

71eef05

Use cats effect in routes

16c2df3

Use cats effect in realmroutes

7e4d147

imsdu commented Aug 28, 2023

View reviewed changes

imsdu changed the title ~~Update realm module to rely on cats-effect~~ Update realm and schema module to rely on cats-effect Aug 28, 2023

Simon Dumas added 3 commits August 30, 2023 18:39

Refactor routes

fad6fce

Formatting

f4654c0

Refactoring and polishing

d38c69c

imsdu commented Aug 31, 2023

View reviewed changes

imsdu marked this pull request as ready for review August 31, 2023 09:16

shinyhappydan reviewed Aug 31, 2023

View reviewed changes

Simon Dumas added 2 commits August 31, 2023 13:17

Address feedback

09e03af

Remove group

56924e3

olivergrabinski approved these changes Aug 31, 2023

View reviewed changes

imsdu added 2 commits August 31, 2023 18:12

Merge branch 'master' into 4162-investigate-monix-bio

eb696ff

Merge branch 'master' into 4162-investigate-monix-bio

ef54220

imsdu merged commit dbc2c36 into BlueBrain:master Sep 1, 2023

imsdu deleted the 4162-investigate-monix-bio branch September 1, 2023 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update realm and schema module to rely on cats-effect #4194

Update realm and schema module to rely on cats-effect #4194

imsdu commented Aug 23, 2023

imsdu Aug 28, 2023

imsdu Aug 28, 2023

imsdu Aug 28, 2023

imsdu Aug 28, 2023 •

edited

Loading

imsdu Aug 28, 2023

imsdu Aug 28, 2023

imsdu Aug 28, 2023

imsdu Aug 28, 2023

olivergrabinski Aug 31, 2023

imsdu Aug 31, 2023

imsdu Aug 31, 2023

shinyhappydan Aug 31, 2023

shinyhappydan Aug 31, 2023

shinyhappydan Aug 31, 2023

imsdu Aug 31, 2023 •

edited

Loading

shinyhappydan Aug 31, 2023

imsdu Aug 31, 2023

shinyhappydan Aug 31, 2023

shinyhappydan Aug 31, 2023

shinyhappydan Aug 31, 2023

shinyhappydan Aug 31, 2023

imsdu Aug 31, 2023

shinyhappydan Aug 31, 2023

olivergrabinski Aug 31, 2023

imsdu Aug 31, 2023

shinyhappydan Aug 31, 2023

olivergrabinski Aug 31, 2023

shinyhappydan Aug 31, 2023


		import scala.concurrent.duration._

		object KamonMonitoringCats {


		object DeltaDirectives extends DeltaDirectives

		trait DeltaDirectives extends UriDirectives {

		private def emitTags(io: IO[SchemaResource]): Route =
		emit(io.map(_.value.tags).attemptNarrow[SchemaRejection].rejectOn[SchemaNotFound])


		abstract class Rejection extends Exception with Product with Serializable {

		def reason: String

Update realm and schema module to rely on cats-effect #4194

Update realm and schema module to rely on cats-effect #4194

Conversation

imsdu commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

imsdu Aug 28, 2023 •

edited

Loading

imsdu Aug 31, 2023 •

edited

Loading