Do not allow deprecated schemas/ontologies in imports #4391

imsdu · 2023-10-20T08:56:30Z

It is implemented at the resource resolution level as it felt weird to do it at the schema import part.
It is also not excluded that we exclude deprecated resources for other operations.

shinyhappydan

My guess is there's a reason why it makes more sense to do the exclusion in the resolution part, but I think it's a less straightforward system that doing it when importing from the schema. Is the resolution system another one of these features that isn't really used but adding complexity, or is this a more necessary one?

shinyhappydan · 2023-10-20T09:51:26Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/resolvers/ResolverResolution.scala

+    for {
+      resourceOpt <- fetch(ref, projectRef)
+      result      <- resourceOpt.traverse(checkLatestDeprecated(ref, projectRef, _))
+    } yield {
+      result match {
+        case None        => ResolverReport.failed(resolver.id, projectRef -> ResolutionFetchRejection(ref, projectRef)) -> None
+        case Some(true)  =>
+          ResolverReport.failed(resolver.id, projectRef -> ResourceIsDeprecated(ref.original, projectRef)) -> None
+        case Some(false) => ResolverReport.success(resolver.id, projectRef)                                             -> resourceOpt
+      }


I'm not quite sure how to unpick this, but feels like it could be improved? If nothing else, result should be called isDeprecated or similar, checkLatestDeprecated could be more like isLatestRevisionDeprecated

Yep, I forgot to get back to this one, I will see what I can do

shinyhappydan · 2023-10-20T09:54:47Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/resolvers/ResolverResolution.scala

+  private def checkLatestDeprecated(resourceRef: ResourceRef, project: ProjectRef, resource: R) =
+    resourceRef match {
+      case _: ResourceRef.Latest => IO.pure(deprecationCheck(resource))
+      case _                     => fetch(ResourceRef.Latest(resourceRef.original), project).map(_.exists(deprecationCheck(_)))
+    }


Ok so my previous comment was slightly wrong. I would argue here that this should be called something like shouldBeDisallowed, because the current method name suggests the result is whether or not the resource is deprecated. I can see that by putting check you're trying to communicate this, but I think maybe it needs to be taken a step further

shouldDisallowDueToDeprecation?

shinyhappydan · 2023-10-20T09:59:44Z

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

-                 .downField("minCount")
-                 .as[Int]
-                 .toOption shouldEqual Some(2)
+               expectMinCount(json, 2)


This is a nice change

shinyhappydan · 2023-10-20T10:01:06Z

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

+        _ <- deltaClient.delete[Json](s"/schemas/$project/${UrlUtils.encode(baseSchemaId)}?rev=1", Rick) { expectOk }
+        _ <- deltaClient.post[Json](s"/schemas/$project", importingSchemaPayload, Rick) { (json, response) =>
+               response.status shouldEqual StatusCodes.BadRequest
+               `@type`.getOption(json) shouldEqual Some("InvalidSchemaResolution")


Should the error not be specifically that the import was deprecated?

This still feels like the wrong error to receive

This is because we accumulate errors here, we can have an error because an import is deprecated and another because another one is not found at all.

Doesn't the user need the information in order to fix their problem?

The information is there, you can see it in the diff mentioning the report that is included in the payload.
I have also added a check to look for this specific error in the report

imsdu · 2023-10-20T13:55:40Z

My guess is there's a reason why it makes more sense to do the exclusion in the resolution part, but I think it's a less straightforward system that doing it when importing from the schema. Is the resolution system another one of these features that isn't really used but adding complexity, or is this a more necessary one?

Resolution is necessary and is widely used, we rely on it to resolve remote contexts in resources, imports in schemas, there are a couple of endpoints depending on it to help people find resources, ...

…t-deprecated-schemas

shinyhappydan · 2023-10-24T08:35:33Z

My guess is there's a reason why it makes more sense to do the exclusion in the resolution part, but I think it's a less straightforward system that doing it when importing from the schema. Is the resolution system another one of these features that isn't really used but adding complexity, or is this a more necessary one?

Resolution is necessary and is widely used, we rely on it to resolve remote contexts in resources, imports in schemas, there are a couple of endpoints depending on it to help people find resources, ...

If that is the case then isn't it better to have this functionality localised to where it's used, rather than in this more general system

shinyhappydan · 2023-10-24T08:41:11Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/resolvers/ResolverResolution.scala

+    for {
+      resourceOpt <- fetch(ref, projectRef)
+      result      <- resourceOpt.traverse(runDeprecationCheck(ref, projectRef, _))
+    } yield {
+      result match {
+        // The resource has not been found in the project
+        case None        => ResolverReport.failed(resolver.id, projectRef -> ResolutionFetchRejection(ref, projectRef)) -> None
+        // The resource exists but the deprecation check is positive so we reject it
+        case Some(true)  =>
+          ResolverReport.failed(resolver.id, projectRef -> ResourceIsDeprecated(ref.original, projectRef)) -> None
+        // The resource has been successfully resolved
+        case Some(false) => ResolverReport.success(resolver.id, projectRef)                                             -> resourceOpt
+      }


I think by using meaningful variable and method names you don't really need the comments here

I tried and it either had a negative impact on the cross-project resolver part or it was not as clear as adding comments

Something like this?

fetch(ref, projectRef).flatMap { case None => IO.pure(ResolverReport.failed(resolver.id, projectRef -> ResolutionFetchRejection(ref, projectRef)) -> None) case Some(resource) => shouldExcludeDueToDeprecation(ref, projectRef, resource)).map { case Exclude => ResolverReport.failed(resolver.id, projectRef -> ResourceIsDeprecated(ref.original, projectRef)) -> None case Include => ResolverReport.success(resolver.id, projectRef) -> Some(resource) } }

It does not look really clearer and it will have an impact on the cross-project resolver side

I don't fully understand the impact the change would have on cross-project resolver 🤔

Obviously this is all a matter of opinion, but the ways in which I would see it as objectively clearer:

No comments needed

Boolean matching removed for clearer case objects

resourceOpt is only used when it is Some, this is fixed

shouldExcludeDueToDeprecation is clearer than runDeprecationCheck as it is explaining the check

Traversal is maybe not a big deal but it is a complication

I think we can move on for this PR, but the things I have mentioned are things which in general make the codebase we have difficult to understand/navigate. They are all small things by themselves, but they add up.

I'm just posting this to attempt to express how I feel about it, I think it's fine to disagree

shinyhappydan · 2023-10-24T08:42:54Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/resolvers/ResolverResolution.scala

+            resourceOpt  <- fetch(ref, projectRef)
+            resource     <- IO.fromOption(resourceOpt)(ResolutionFetchRejection(ref, projectRef))


I preferred this previously. An option would be to make a method to hide the boilerplate

shinyhappydan · 2023-10-24T08:43:37Z

delta/sdk/src/main/scala/ch/epfl/bluebrain/nexus/delta/sdk/resolvers/ResolverResolution.scala

+            isDeprecated <- runDeprecationCheck(ref, projectRef, resource)
+            _            <- IO.raiseWhen(isDeprecated)(ResourceIsDeprecated(ref.original, projectRef))


Another instance where a method can hide the boilerplate

shinyhappydan · 2023-10-24T08:45:04Z

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

+        _ <- deltaClient.delete[Json](s"/schemas/$project/${UrlUtils.encode(baseSchemaId)}?rev=1", Rick) { expectOk }
+        _ <- deltaClient.post[Json](s"/schemas/$project", importingSchemaPayload, Rick) { (json, response) =>
+               response.status shouldEqual StatusCodes.BadRequest
+               `@type`.getOption(json) shouldEqual Some("InvalidSchemaResolution")


This still feels like the wrong error to receive

imsdu · 2023-10-24T08:59:05Z

My guess is there's a reason why it makes more sense to do the exclusion in the resolution part, but I think it's a less straightforward system that doing it when importing from the schema. Is the resolution system another one of these features that isn't really used but adding complexity, or is this a more necessary one?

Resolution is necessary and is widely used, we rely on it to resolve remote contexts in resources, imports in schemas, there are a couple of endpoints depending on it to help people find resources, ...

If that is the case then isn't it better to have this functionality localised to where it's used, rather than in this more general system

This feature can be interesting in other cases such as context resolution when creating/updating resources
Schema imports does not know how to resolve and fetch schemas and ontologies, it would not be able to fetch the latest version to check the deprecation status
This way, it fits well in the error report returned whenever the resolution of imports fails

shinyhappydan · 2023-10-24T09:03:08Z

This feature can be interesting in other cases such as context resolution when creating/updating resources

If no other feature is using it right now I would say YAGNI

Schema imports does not know how to resolve and fetch schemas and ontologies, it would not be able to fetch

the latest version to check the deprecation status
We could give it the ability to check the deprecation status of the latest revision

It's just an option. Because this feels like quite a complex solution for what on the face of it seems like a simple problem, so I think it's worth making sure we want to pay that price

imsdu · 2023-10-24T09:51:39Z

This feature can be interesting in other cases such as context resolution when creating/updating resources

If no other feature is using it right now I would say YAGNI

I don't see why we would exclude deprecated schemas from imports and not deprecated contexts in resources.

Schema imports does not know how to resolve and fetch schemas and ontologies, it would not be able to fetch

the latest version to check the deprecation status We could give it the ability to check the deprecation status of the latest revision

It's just an option. Because this feels like quite a complex solution for what on the face of it seems like a simple problem, so I think it's worth making sure we want to pay that price

It is not so easy because resolvers are involved and schemas imports role is limited to accumulate imports or errors

Simon Dumas added 3 commits October 20, 2023 10:56

Do not allow deprecated schemas/ontologies in imports

6a8c032

Fix test compilation

618615c

Remove commented lines

f577b92

shinyhappydan reviewed Oct 20, 2023

View reviewed changes

Address feedback

72eef1f

Simon Dumas added 3 commits October 20, 2023 15:57

Merge remote-tracking branch 'upstream/master' into 1855-do-not-impor…

619e392

…t-deprecated-schemas

Address feedback + fix schema error responses

71fa288

Fix resource resolution report json-ld encoding

22780a0

shinyhappydan reviewed Oct 24, 2023

View reviewed changes

Address feedback

ce2f4f2

shinyhappydan approved these changes Oct 25, 2023

View reviewed changes

Merge branch 'master' into 1855-do-not-import-deprecated-schemas

0683626

imsdu merged commit 2f08298 into BlueBrain:master Oct 25, 2023
5 checks passed

imsdu deleted the 1855-do-not-import-deprecated-schemas branch October 25, 2023 11:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not allow deprecated schemas/ontologies in imports #4391

Do not allow deprecated schemas/ontologies in imports #4391

imsdu commented Oct 20, 2023 •

edited

Loading

shinyhappydan left a comment

shinyhappydan Oct 20, 2023

imsdu Oct 20, 2023

shinyhappydan Oct 20, 2023

shinyhappydan Oct 20, 2023

shinyhappydan Oct 20, 2023

shinyhappydan Oct 20, 2023

shinyhappydan Oct 24, 2023

imsdu Oct 24, 2023

shinyhappydan Oct 24, 2023

imsdu Oct 25, 2023

imsdu commented Oct 20, 2023 •

edited

Loading

shinyhappydan commented Oct 24, 2023

shinyhappydan Oct 24, 2023

imsdu Oct 24, 2023

shinyhappydan Oct 24, 2023 •

edited

Loading

imsdu Oct 25, 2023

olivergrabinski Oct 25, 2023

shinyhappydan Oct 25, 2023

shinyhappydan Oct 24, 2023

shinyhappydan Oct 24, 2023

shinyhappydan Oct 24, 2023

imsdu commented Oct 24, 2023

shinyhappydan commented Oct 24, 2023 •

edited

Loading

imsdu commented Oct 24, 2023

		resourceOpt <- fetch(ref, projectRef)
		resource <- IO.fromOption(resourceOpt)(ResolutionFetchRejection(ref, projectRef))

		isDeprecated <- runDeprecationCheck(ref, projectRef, resource)
		_ <- IO.raiseWhen(isDeprecated)(ResourceIsDeprecated(ref.original, projectRef))

Do not allow deprecated schemas/ontologies in imports #4391

Do not allow deprecated schemas/ontologies in imports #4391

Conversation

imsdu commented Oct 20, 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

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 Oct 20, 2023 • edited Loading

shinyhappydan commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shinyhappydan Oct 24, 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

imsdu commented Oct 24, 2023

shinyhappydan commented Oct 24, 2023 • edited Loading

imsdu commented Oct 24, 2023

imsdu commented Oct 20, 2023 •

edited

Loading

imsdu commented Oct 20, 2023 •

edited

Loading

shinyhappydan Oct 24, 2023 •

edited

Loading

shinyhappydan commented Oct 24, 2023 •

edited

Loading