Add resource trial routes #4274

imsdu · 2023-09-14T16:34:11Z

docs/src/main/paradox/docs/delta/api/practice.md

olivergrabinski · 2023-09-19T12:29:36Z

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

+    new ResourcesPracticeRoutes(
+      identities,
+      aclCheck,
+      (project, source, caller) => schemas.createDryRun(project, source)(caller).toBIO[SchemaRejection],


Having this as a private method in ResourcesPracticeRoutes would allow getting rid of the constructor all together? Not super important

The idea is to make unit-testing easier because Schemas comes with a lot of dependencies (doobie, SHACL engine, ...)

Using functions like this makes general navigation of the code harder, as well as this constructor being unnecessary. It should already be easy to use mock implementations in a unit test

Interface segregation is a good way of handling this sort of situation and will mean you won't need the dependencies you mention

I am not sure we want to do a remake between OOP and FP in the comments here.
As long as we have the apply method here, I don't think it hurts navigability.
The FP way allows better expressivity and better flexibility than introducing a multitude of interfaces.
On the flexibility side, what happens if in another case, you want to generate a schema but only care about one of the fields or just if it succeeds ? Or something else ?
You would have to introduce new interfaces or have your mocks to know about the result before dropping the info.
(Those are just simple examples, there could be others)

Every time I want to see what code actually run when we are passing one of these functions, I have to navigate multiple steps and potentially multiple branches. That is definitely harder than just clicking on show implementations

In this instance, using a function instead of an interface gives us nothing and costs a lot, which is why I'm saying we should change it here

Also I don't believe there is a conflict between OOP and FP here, I'm not sure how using an interface makes something not functional?

Interesting discussion! In these kinds of situations we've found single-method traits to be quite a good compromise in the past. They essentially act as just named functions, sort of like a type alias but first-class. Navigation is still easy and Scala's de-sugaring makes them simple to stub.

For example if a dependent expects a trait MyTrait { def foo(i: Int): String }, you can pass it as a stub like (i: Int) -> "hello" in tests and the compiler will understand it needs to do the whole new MyTrait { ... } bit for you, which is nice. In Scala 3 there's even less boilerplate needed (better type inference).

Not sure I understand this bit you want to generate a schema but only care about one of the fields or just if it succeeds ? - what would that look like? (Just trying figure out what I might do in that situation)

As an aside, in Haskell there are a number of approaches to this type of dependency injection. You might do a type alias, newtype wrapper round the function, or a type class (if doing tagless final). Otherwise type signatures become very hard to read when containing lots of functions passed as arguments. The single-method trait isn't that different from the type class pattern as I see it. (Well really all of these approaches are pretty similar 😛 )

The "method dictionary" pattern can be quite neat too - you basically have a bunch of newtype wrapped functions stored in some Env data type, and access them by the record accessor (just the thing inside the newtype). For convenience you might also have syntax to pull out the functions and run them in one fell swoop. In this case that may require quite a big refactor though so I'm not sure.

The only downside with SAM (the example (i: Int) -> "hello" from above) is that Intellij doesn't really understand that it's an implementation of the interface from a code-navigation perspective. I suppose mostly we would only use SAM in tests, where it isn't so necessary to have the navigation functionality

In production code it's just nice to easily see what the dependency actually does

I would love to be able to use a SAM here and we should use them where we can.
The Schemas module is complex and contains a collection of operations though (with like 3 operations to create a schema) and the part which is mentioned is only used in this context (so far).
As it is a matter that goes further than the context of this PR, we should rather discuss that when we are all in the office

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

shinyhappydan · 2023-09-19T12:32:04Z

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

+/**
+  * The resource practice routes allowing to do read-only operations on resource
+  */


I would question whether "practise" makes sense in this context

Also it's not really that you're doing read-only operations, it's that you are performing dry-runs of write operations?

They are not, the two operations do not go through the state machine because they don't result in an event and a new state to be persisted in the primary store

Maybe there is another way to phrase it, but I think this comment is misleading

shinyhappydan · 2023-09-19T12:35:07Z

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

+  private def generate(project: ProjectRef, input: GenerationInput)(implicit caller: Caller) =
+    input.schema match {
+      case ExistingSchema(schemaId) =>
+        emit(resourcesPractice.generate(project, schemaId, input.resource).flatMap(_.asJson))
+      case NewSchema(schemaSource)  =>
+        emit(
+          generateSchema(project, schemaSource, caller).flatMap { schema =>
+            resourcesPractice.generate(project, schema, input.resource).flatMap(_.asJson)
+          }
+        )
+    }


It feels like this is too much logic for routes, it could go int the ResourcesPractise instead

Those are 10 lines of code here without complexity where:

Most of it is related to how we handle the input

ResourcesPractis(c)e is about generating resources, not schemas

I think this function is unrelated to parsing or response-writing so it should be in a service rather than routes, but that's just my opinion I guess

Any time the user calls an API they are ostensibly performing a single action. The user is saying they want to generate using a generated schema. Perhaps that affects they way you design the ResourcesPractise, but it shouldn't really be a concern for the routes

I think this function is unrelated to parsing or response-writing so it should be in a service rather than routes, but that's just my opinion I guess

It is related to parsing as GenerationInput only lives in the scope of the routes and reflects the payload submitted to the user.

Any time the user calls an API they are ostensibly performing a single action. The user is saying they want to generate using a generated schema. Perhaps that affects they way you design the ResourcesPractise, but it shouldn't really be a concern for the routes

We are talking here about a pattern matching and a flatMap operation and they live in a private method that does not pollute the route declaration.
I prefer this solution than changing ResourcePractise or including yet another layer

All I can say is that I see unnecessary logic in the routes a lot and it would be better if it weren't there

shinyhappydan · 2023-09-19T12:47:35Z

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

+    "generate a resource without passing a schema" in {
+      val payload = json"""{ "resource": $validSource }"""
+      Get(s"/v1/practice/resources/$projectRef/", payload.toEntity) ~> asAlice ~> routes ~> check {
+        response.status shouldEqual StatusCodes.OK
+        response.asJson shouldEqual jsonContentOf("practice/resource-without-schema.json")
+      }
+    }


Why should passing this payload return this result? If this test fails we should know what the code is intended to do

shinyhappydan · 2023-09-19T12:50:25Z

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

+      val unconstrainedEncoded = UrlUtils.encode(schemas.resources.toString)
+      Get(s"/v1/resources/$projectRef/$unconstrainedEncoded/myId/validate") ~> asAlice ~> routes ~> check {


You could use a name like unconstrained here, or maybe unconstrainedSchema if that makes more sense

shinyhappydan · 2023-09-19T12:53:42Z

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

+    "fail to create a resource against a schema that does not exist" in {
+      Put(
+        "/v1/resources/myorg/myproject/pretendschema/wrong",
+        payload.removeKeys(keywords.id).replaceKeyWithValue("number", "wrong").toEntity


Why is this line needed?

shinyhappydan · 2023-09-19T12:57:41Z

.../sdk/src/test/scala/ch/epfl/bluebrain/nexus/delta/sdk/resources/ResourcesPracticeSuite.scala

-    practice.generate(projectRef, resourceSchema, source).map(_.value).assert(expectedData)
+    for {
+      generated <- practice.generate(projectRef, resourceSchema, source)
+      _          = assertEquals(generated.schema, None)
+      _          = assertEquals(generated.attempt.map(_.value), Right(expectedData))
+    } yield ()


Having to re-write the existing test when you add a second assertion suggests that it's probably better to write this way initially

shinyhappydan · 2023-09-21T15:51:52Z

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

-        payload.removeKeys(keywords.id).replaceKeyWithValue("number", "wrong").toEntity
+        payloadWithoutId.replaceKeyWithValue("number", "wrong").toEntity


After looking at this for a while I can see now that if you set the number field to be a string, presumably that breaks validation. It's not very obvious though, because there is nothing to suggest that is what the schema checks. With this and other tests, it's possible to make it clearer

I tried to make it a bit clearer, note that this was some existing test code, that I only refactored a bit

…e-routes' into 4102-introduce-resources-practice-routes

crisely09 · 2023-09-22T08:20:54Z

does this mean that we can perform dry-runs now?

Simon Dumas added 3 commits September 14, 2023 18:33

Add resource practice routes

df704bc

Scala doc + formatting

9183371

Add documentation for new endpoint/update release notes

e00938f

olivergrabinski suggested changes Sep 19, 2023

View reviewed changes

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

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

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

olivergrabinski reviewed Sep 19, 2023

View reviewed changes

shinyhappydan requested changes Sep 19, 2023

View reviewed changes

Simon Dumas added 4 commits September 20, 2023 14:13

Address feedback

7393abc

Fix doc links

cc7fbd4

Fix unit test

3dc1c36

Rename practice to trial

d84c0c1

imsdu changed the title ~~Add resource practice routes~~ Add resource trial routes Sep 20, 2023

Merge branch 'master' into 4102-introduce-resources-practice-routes

0a85d8b

shinyhappydan reviewed Sep 21, 2023

View reviewed changes

Simon Dumas added 4 commits September 22, 2023 09:33

Remove unnecessary change

a6da305

Merge remote-tracking branch 'origin/4102-introduce-resources-practic…

a34befa

…e-routes' into 4102-introduce-resources-practice-routes

Oops

3d0e2b8

Improving test clarity

7a1e554

imsdu merged commit 16483bc into BlueBrain:master Sep 22, 2023
6 checks passed

imsdu deleted the 4102-introduce-resources-practice-routes branch September 22, 2023 08:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resource trial routes #4274

Add resource trial routes #4274

imsdu commented Sep 14, 2023 •

edited

Loading

olivergrabinski Sep 19, 2023

imsdu Sep 20, 2023

shinyhappydan Sep 20, 2023

imsdu Sep 20, 2023

shinyhappydan Sep 20, 2023

shinyhappydan Sep 20, 2023

dantb Sep 21, 2023

shinyhappydan Sep 21, 2023

imsdu Sep 21, 2023

shinyhappydan Sep 19, 2023

imsdu Sep 20, 2023

shinyhappydan Sep 20, 2023

shinyhappydan Sep 19, 2023

imsdu Sep 20, 2023

shinyhappydan Sep 20, 2023

shinyhappydan Sep 20, 2023

imsdu Sep 20, 2023 •

edited

Loading

shinyhappydan Sep 20, 2023

shinyhappydan Sep 19, 2023

shinyhappydan Sep 19, 2023

shinyhappydan Sep 19, 2023

shinyhappydan Sep 19, 2023

shinyhappydan Sep 21, 2023

imsdu Sep 22, 2023

crisely09 commented Sep 22, 2023

		val unconstrainedEncoded = UrlUtils.encode(schemas.resources.toString)
		Get(s"/v1/resources/$projectRef/$unconstrainedEncoded/myId/validate") ~> asAlice ~> routes ~> check {

		payload.removeKeys(keywords.id).replaceKeyWithValue("number", "wrong").toEntity
		payloadWithoutId.replaceKeyWithValue("number", "wrong").toEntity

Add resource trial routes #4274

Add resource trial routes #4274

Conversation

imsdu commented Sep 14, 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

imsdu Sep 20, 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

crisely09 commented Sep 22, 2023

imsdu commented Sep 14, 2023 •

edited

Loading

imsdu Sep 20, 2023 •

edited

Loading