fix(value): make impossible to set list root node as a value (DEV-973) (

#2088) * add method checking if list is a root node or not * improve ignoring trig files * refactor HierarchicalListValueContentV2 object * cleanup * add list root checkiing method to ValueResponderV2 + test * remove list root checking from ValueMessangerV2 * add new query to handle 2 checks + merge checking methods * move throwing to responder * fix formatting & bad merge * fix failing test * refactor check method to return value + move error handling to responder * add UNIT tests for util method * remove unused template * review changes * cleanup
dasch-swiss · Jul 6, 2022 · 94d2b46 · 94d2b46
1 parent 48592ad
commit 94d2b46
Show file tree

Hide file tree

Showing 7 changed files with 190 additions and 36 deletions.
diff --git a/.gitignore b/.gitignore
@@ -36,7 +36,7 @@ sipi/test
 *.bak
 *.rdb
 .sbtrc
-/*.trig
+**/*.trig
 
 dependencies.txt
 /client-test-data.zip

diff --git a/...src/main/scala/org/knora/webapi/messages/v2/responder/valuemessages/ValueMessagesV2.scala b/...src/main/scala/org/knora/webapi/messages/v2/responder/valuemessages/ValueMessagesV2.scala
@@ -6,15 +6,16 @@
 package org.knora.webapi.messages.v2.responder.valuemessages
 
 import akka.actor.ActorRef
-import com.typesafe.scalalogging.Logger
 import akka.http.scaladsl.util.FastFuture
 import akka.pattern._
 import akka.util.Timeout
-import org.knora.webapi._
+import com.typesafe.scalalogging.Logger
 import dsp.errors.AssertionException
 import dsp.errors.BadRequestException
 import dsp.errors.NotImplementedException
 import dsp.errors.SipiException
+import dsp.valueobjects.IriErrorMessages
+import org.knora.webapi._
 import org.knora.webapi.messages.IriConversions._
 import org.knora.webapi.messages.OntologyConstants
 import org.knora.webapi.messages.ResponderRequest.KnoraRequestV2
@@ -41,7 +42,6 @@ import java.time.Instant
 import java.util.UUID
 import scala.concurrent.ExecutionContext
 import scala.concurrent.Future
-import dsp.valueobjects.IriErrorMessages
 
 /**
  * A tagging trait for requests handled by [[org.knora.webapi.responders.v2.ValuesResponderV2]].

diff --git a/webapi/src/main/scala/org/knora/webapi/responders/v2/ResourceUtilV2.scala b/webapi/src/main/scala/org/knora/webapi/responders/v2/ResourceUtilV2.scala
@@ -9,17 +9,23 @@ import akka.actor.ActorRef
 import akka.pattern._
 import akka.util.Timeout
 import com.typesafe.scalalogging.Logger
-import org.knora.webapi.IRI
+import dsp.errors.BadRequestException
 import dsp.errors.ForbiddenException
-import dsp.errors.NotFoundException
+import org.knora.webapi.IRI
+import org.knora.webapi.messages.OntologyConstants
 import org.knora.webapi.messages.SmartIri
+import org.knora.webapi.messages.StringFormatter
 import org.knora.webapi.messages.admin.responder.permissionsmessages.DefaultObjectAccessPermissionsStringForPropertyGetADM
 import org.knora.webapi.messages.admin.responder.permissionsmessages.DefaultObjectAccessPermissionsStringResponseADM
 import org.knora.webapi.messages.admin.responder.usersmessages.UserADM
 import org.knora.webapi.messages.store.sipimessages.DeleteTemporaryFileRequest
 import org.knora.webapi.messages.store.sipimessages.MoveTemporaryFileToPermanentStorageRequest
+import org.knora.webapi.messages.store.triplestoremessages.LiteralV2
 import org.knora.webapi.messages.store.triplestoremessages.SparqlAskRequest
 import org.knora.webapi.messages.store.triplestoremessages.SparqlAskResponse
+import org.knora.webapi.messages.store.triplestoremessages.SparqlExtendedConstructRequest
+import org.knora.webapi.messages.store.triplestoremessages.SparqlExtendedConstructResponse
+import org.knora.webapi.messages.store.triplestoremessages.SubjectV2
 import org.knora.webapi.messages.util.KnoraSystemInstances
 import org.knora.webapi.messages.util.PermissionUtilADM
 import org.knora.webapi.messages.util.PermissionUtilADM.EntityPermission
@@ -29,6 +35,7 @@ import org.knora.webapi.messages.v2.responder.resourcemessages.ReadResourceV2
 import org.knora.webapi.messages.v2.responder.valuemessages.FileValueContentV2
 import org.knora.webapi.messages.v2.responder.valuemessages.ReadValueV2
 import org.knora.webapi.messages.v2.responder.valuemessages.ValueContentV2
+
 import scala.concurrent.ExecutionContext
 import scala.concurrent.Future
 import scala.util.Failure
@@ -133,27 +140,49 @@ object ResourceUtilV2 {
     } yield defaultObjectAccessPermissionsResponse.permissionLiteral
 
   /**
-   * Checks whether a list node exists, and throws [[NotFoundException]] otherwise.
+   * Checks whether a list node exists and if is a root node.
    *
-   * @param listNodeIri the IRI of the list node.
+   * @param nodeIri the IRI of the list node.
+   * @param appActor ActorRef
+   * @return Future of Either None for nonexistent, true for root and false for child node.
    */
-  def checkListNodeExists(listNodeIri: IRI, appActor: ActorRef)(implicit
+  def checkListNodeExistsAndIsRootNode(nodeIri: IRI, appActor: ActorRef)(implicit
     timeout: Timeout,
     executionContext: ExecutionContext
-  ): Future[Unit] =
+  ): Future[Either[Option[Nothing], Boolean]] = {
+    implicit val stringFormatter: StringFormatter = StringFormatter.getGeneralInstance
+
     for {
-      askString <- Future(
-                     org.knora.webapi.messages.twirl.queries.sparql.admin.txt
-                       .checkListNodeExistsByIri(listNodeIri = listNodeIri)
-                       .toString
-                   )
-
-      checkListNodeExistsResponse <- appActor.ask(SparqlAskRequest(askString)).mapTo[SparqlAskResponse]
-
-      _ = if (!checkListNodeExistsResponse.result) {
-            throw NotFoundException(s"<$listNodeIri> does not exist or is not a ListNode")
-          }
-    } yield ()
+      sparqlQuery <-
+        Future(
+          org.knora.webapi.messages.twirl.queries.sparql.admin.txt
+            .getListNode(nodeIri = nodeIri)
+            .toString()
+        )
+
+      listNodeResponse <-
+        appActor
+          .ask(
+            SparqlExtendedConstructRequest(
+              sparql = sparqlQuery
+            )
+          )
+          .mapTo[SparqlExtendedConstructResponse]
+
+      statements: Map[SubjectV2, Map[SmartIri, Seq[LiteralV2]]] = listNodeResponse.statements
+
+      maybeList =
+        if (statements.nonEmpty) {
+          val propToCheck: SmartIri = stringFormatter.toSmartIri(OntologyConstants.KnoraBase.IsRootNode)
+          val isRootNode: Boolean   = statements.map(_._2.contains(propToCheck)).head
+
+          Right(isRootNode)
+        } else {
+          Left(None)
+        }
+
+    } yield maybeList
+  }
 
   /**
    * Given a future representing an operation that was supposed to update a value in a triplestore, checks whether

diff --git a/webapi/src/main/scala/org/knora/webapi/responders/v2/ResourcesResponderV2.scala b/webapi/src/main/scala/org/knora/webapi/responders/v2/ResourcesResponderV2.scala
@@ -8,9 +8,8 @@ package org.knora.webapi.responders.v2
 import akka.http.scaladsl.util.FastFuture
 import akka.pattern._
 import akka.stream.Materializer
-import org.knora.webapi._
 import dsp.errors._
-
+import org.knora.webapi._
 import org.knora.webapi.messages.IriConversions._
 import org.knora.webapi.messages.OntologyConstants
 import org.knora.webapi.messages.SmartIri
@@ -1161,9 +1160,26 @@ class ResourcesResponderV2(responderData: ResponderData) extends ResponderWithSt
 
     Future
       .sequence(
-        listNodesThatShouldExist
-          .map(listNodeIri => ResourceUtilV2.checkListNodeExists(listNodeIri, appActor))
-          .toSeq
+        listNodesThatShouldExist.map { listNodeIri =>
+          for {
+            checkNode <- ResourceUtilV2.checkListNodeExistsAndIsRootNode(listNodeIri, appActor)
+
+            _ = checkNode match {
+                  // it doesn't have isRootNode property - it's a child node
+                  case Right(false) => ()
+                  // it does have isRootNode property - it's a root node
+                  case Right(true) =>
+                    throw BadRequestException(
+                      s"<${listNodeIri}> is a root node. Root nodes cannot be set as values."
+                    )
+                  // it deosn't exists or isn't valid list
+                  case Left(_) =>
+                    throw NotFoundException(
+                      s"<${listNodeIri}> does not exist, or is not a ListNode."
+                    )
+                }
+          } yield ()
+        }.toSeq
       )
       .map(_ => ())
   }

diff --git a/webapi/src/main/scala/org/knora/webapi/responders/v2/ValuesResponderV2.scala b/webapi/src/main/scala/org/knora/webapi/responders/v2/ValuesResponderV2.scala
@@ -7,9 +7,8 @@ package org.knora.webapi.responders.v2
 
 import akka.http.scaladsl.util.FastFuture
 import akka.pattern._
-import org.knora.webapi._
 import dsp.errors._
-
+import org.knora.webapi._
 import org.knora.webapi.messages.IriConversions._
 import org.knora.webapi.messages.OntologyConstants
 import org.knora.webapi.messages.SmartIri
@@ -190,11 +189,29 @@ class ValuesResponderV2(responderData: ResponderData) extends Responder(responde
                requestingUser = createValueRequest.requestingUser
              )
 
-        // If this is a list value, check that it points to a real list node.
-
+        // If it is a list value, check that it points to a real list node which is not a root node.
         _ <- submittedInternalValueContent match {
                case listValue: HierarchicalListValueContentV2 =>
-                 ResourceUtilV2.checkListNodeExists(listValue.valueHasListNode, appActor)
+                 for {
+                   checkNode <-
+                     ResourceUtilV2.checkListNodeExistsAndIsRootNode(listValue.valueHasListNode, appActor)
+
+                   _ = checkNode match {
+                         // it doesn't have isRootNode property - it's a child node
+                         case Right(false) => ()
+                         // it does have isRootNode property - it's a root node
+                         case Right(true) =>
+                           throw BadRequestException(
+                             s"<${listValue.valueHasListNode}> is a root node. Root nodes cannot be set as values."
+                           )
+                         // it deosn't exists or isn't valid list
+                         case Left(_) =>
+                           throw NotFoundException(
+                             s"<${listValue.valueHasListNode}> does not exist or is not a ListNode."
+                           )
+                       }
+                 } yield ()
+
                case _ => FastFuture.successful(())
              }
 
@@ -325,6 +342,7 @@ class ValuesResponderV2(responderData: ResponderData) extends Responder(responde
                                             unverifiedValue = unverifiedValue,
                                             requestingUser = createValueRequest.requestingUser
                                           )
+
       } yield CreateValueResponseV2(
         valueIri = verifiedValue.newValueIri,
         valueType = verifiedValue.value.valueType,
@@ -1171,11 +1189,29 @@ class ValuesResponderV2(responderData: ResponderData) extends Responder(responde
                requestingUser = updateValueRequest.requestingUser
              )
 
-        // If this is a list value, check that it points to a real list node.
-
+        // If it is a list value, check that it points to a real list node which is not a root node.
         _ <- submittedInternalValueContent match {
                case listValue: HierarchicalListValueContentV2 =>
-                 ResourceUtilV2.checkListNodeExists(listValue.valueHasListNode, appActor)
+                 for {
+                   checkNode <-
+                     ResourceUtilV2.checkListNodeExistsAndIsRootNode(listValue.valueHasListNode, appActor)
+
+                   _ = checkNode match {
+                         // it doesn't have isRootNode property - it's a child node
+                         case Right(false) => ()
+                         // it does have isRootNode property - it's a root node
+                         case Right(true) =>
+                           throw BadRequestException(
+                             s"<${listValue.valueHasListNode}> is a root node. Root nodes cannot be set as values."
+                           )
+                         // it deosn't exists or isn't valid list
+                         case Left(_) =>
+                           throw NotFoundException(
+                             s"<${listValue.valueHasListNode}> does not exist or is not a ListNode."
+                           )
+                       }
+                 } yield ()
+
                case _ => FastFuture.successful(())
              }
 

diff --git a/webapi/src/test/scala/org/knora/webapi/responders/v2/ResourceUtilV2Spec.scala b/webapi/src/test/scala/org/knora/webapi/responders/v2/ResourceUtilV2Spec.scala
@@ -0,0 +1,48 @@
+/*
+ * Copyright © 2021 - 2022 Swiss National Data and Service Center for the Humanities and/or DaSCH Service Platform contributors.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.knora.webapi.responders.v2
+
+import akka.util.Timeout
+import org.knora.webapi.CoreSpec
+import org.knora.webapi.messages.store.triplestoremessages.RdfDataObject
+
+import scala.concurrent.ExecutionContextExecutor
+
+object ResourceUtilV2Spec {}
+
+class ResourceUtilV2Spec extends CoreSpec() {
+  implicit val timeout: Timeout             = settings.defaultTimeout
+  implicit val ec: ExecutionContextExecutor = system.dispatcher
+
+  override lazy val rdfDataObjects = List(
+    RdfDataObject(path = "test_data/all_data/anything-data.ttl", name = "http://www.knora.org/data/0001/anything")
+  )
+
+  "ResourceUtil" when {
+    "called `checkListNodeExistsAndIsRootNode` method" should {
+      "return FALSE for list child node" in {
+        val nodeIri   = "http://rdfh.ch/lists/0001/otherTreeList01"
+        val checkNode = ResourceUtilV2.checkListNodeExistsAndIsRootNode(nodeIri, appActor)
+
+        checkNode.onComplete(_.get should be(Right(false)))
+      }
+
+      "return TRUE for list root node" in {
+        val nodeIri   = "http://rdfh.ch/lists/0001/otherTreeList"
+        val checkNode = ResourceUtilV2.checkListNodeExistsAndIsRootNode(nodeIri, appActor)
+
+        checkNode.onComplete(_.get should be(Right(true)))
+      }
+
+      "should return NONE for nonexistent list" in {
+        val nodeIri   = "http://rdfh.ch/lists/0001/otherTreeList77"
+        val checkNode = ResourceUtilV2.checkListNodeExistsAndIsRootNode(nodeIri, appActor)
+
+        checkNode.onComplete(_.get should be(Left(None)))
+      }
+    }
+  }
+}
diff --git a/webapi/src/test/scala/org/knora/webapi/responders/v2/ValuesResponderV2Spec.scala b/webapi/src/test/scala/org/knora/webapi/responders/v2/ValuesResponderV2Spec.scala
@@ -6,9 +6,9 @@
 package org.knora.webapi.responders.v2
 
 import akka.testkit.ImplicitSender
+import dsp.errors._
 import org.knora.webapi._
 import org.knora.webapi.config.AppConfig
-import dsp.errors._
 import org.knora.webapi.messages.IriConversions._
 import org.knora.webapi.messages.OntologyConstants
 import org.knora.webapi.messages.SmartIri
@@ -1754,6 +1754,31 @@ class ValuesResponderV2Spec extends CoreSpec() with ImplicitSender {
       }
     }
 
+    "not create a list value that is a root list node" in {
+      val resourceIri           = "http://rdfh.ch/0001/a-blue-thing"
+      val resourceClassIri      = "http://www.knora.org/ontology/0001/anything#BlueThing".toSmartIri
+      val propertyIri: SmartIri = "http://0.0.0.0:3333/ontology/0001/anything/v2#hasListItem".toSmartIri
+      val valueHasListNode      = "http://rdfh.ch/lists/0001/otherTreeList"
+
+      appActor ! CreateValueRequestV2(
+        CreateValueV2(
+          resourceIri = resourceIri,
+          resourceClassIri = resourceClassIri,
+          propertyIri = propertyIri,
+          valueContent = HierarchicalListValueContentV2(
+            ontologySchema = ApiV2Complex,
+            valueHasListNode = valueHasListNode
+          )
+        ),
+        requestingUser = anythingUser1,
+        apiRequestID = UUID.randomUUID
+      )
+
+      expectMsgPF(timeout) { case msg: akka.actor.Status.Failure =>
+        assert(msg.cause.isInstanceOf[BadRequestException])
+      }
+    }
+
     "create a color value" in {
       // Add the value.