Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 17 commits into from Jul 6, 2022
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d1c775b
add method checking if list is a root node or not
mpro7 Jun 27, 2022
29998c3
improve ignoring trig files
mpro7 Jun 27, 2022
8455366
refactor HierarchicalListValueContentV2 object
mpro7 Jun 27, 2022
748feb1
cleanup
mpro7 Jun 28, 2022
5953676
add list root checkiing method to ValueResponderV2 + test
mpro7 Jun 29, 2022
6176715
remove list root checking from ValueMessangerV2
mpro7 Jun 29, 2022
eeafd44
add new query to handle 2 checks + merge checking methods
mpro7 Jun 30, 2022
eae72df
move throwing to responder
mpro7 Jul 4, 2022
4b16ae0
Merge branch 'main' into DEV-973-it-shouldnt-be-possible-to-set-the-l…
mpro7 Jul 4, 2022
35eac06
fix formatting & bad merge
mpro7 Jul 4, 2022
b74ee41
fix failing test
mpro7 Jul 4, 2022
58b4551
refactor check method to return value + move error handling to responder
mpro7 Jul 5, 2022
d14daa9
add UNIT tests for util method
mpro7 Jul 5, 2022
db58026
remove unused template
mpro7 Jul 5, 2022
fb35176
Merge branch 'main' into DEV-973-it-shouldnt-be-possible-to-set-the-l…
mpro7 Jul 5, 2022
388f4a8
review changes
mpro7 Jul 5, 2022
0ce84d9
cleanup
mpro7 Jul 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Expand Up @@ -36,7 +36,7 @@ sipi/test
*.bak
*.rdb
.sbtrc
/*.trig
**/*.trig
mpro7 marked this conversation as resolved.
Show resolved Hide resolved

dependencies.txt
/client-test-data.zip
Expand Down
6 changes: 3 additions & 3 deletions ...src/main/scala/org/knora/webapi/messages/v2/responder/valuemessages/ValueMessagesV2.scala
View file
Open in desktop
Expand Up @@ -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.feature.FeatureFactoryConfig
import org.knora.webapi.messages.IriConversions._
import org.knora.webapi.messages.OntologyConstants
Expand Down Expand Up @@ -42,7 +43,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]].
Expand Down
50 changes: 49 additions & 1 deletion webapi/src/main/scala/org/knora/webapi/responders/v2/ResourceUtilV2.scala
View file
Open in desktop
Expand Up @@ -9,17 +9,25 @@ 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.feature.FeatureFactoryConfig
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
Expand All @@ -29,6 +37,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
Expand Down Expand Up @@ -132,6 +141,45 @@ object ResourceUtilV2 {
.mapTo[DefaultObjectAccessPermissionsStringResponseADM]
} yield defaultObjectAccessPermissionsResponse.permissionLiteral

/**
* Checks whether a list node is not a list root node, and throws [[BadRequestException]] otherwise.
*
* @param nodeIri the IRI of the list node.
*/
def checkIfNodeIsRoot(nodeIri: IRI, appActor: ActorRef, featureFactoryConfig: FeatureFactoryConfig)(implicit
timeout: Timeout,
executionContext: ExecutionContext
): Future[Unit] = {
implicit val stringFormatter: StringFormatter = StringFormatter.getGeneralInstance

for {
sparqlQuery <-
Future(
org.knora.webapi.messages.twirl.queries.sparql.admin.txt
.getListNode(nodeIri = nodeIri)
.toString()
)

listNodeResponse <-
appActor
.ask(
SparqlExtendedConstructRequest(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at this: https://github.com/dasch-swiss/dsp-api/blob/main/webapi/src/main/twirl/org/knora/webapi/messages/twirl/queries/sparql/admin/checkListRootNodeExistsByIri.scala.txt

What you should do is write an ASK query that will return true or false.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is only one but. With one query for both checks, I can only return one error, this shouldn't be a problem but it's less information for the user.

sparql = sparqlQuery,
featureFactoryConfig = featureFactoryConfig
)
)
.mapTo[SparqlExtendedConstructResponse]

ontoConstAsSmartIri: SmartIri = stringFormatter.toSmartIri(OntologyConstants.KnoraBase.IsRootNode)
statements: Map[SubjectV2, Map[SmartIri, Seq[LiteralV2]]] = listNodeResponse.statements
isRootNode: Boolean = statements.map(_._2.contains(ontoConstAsSmartIri)).head

_ = if (isRootNode) {
throw BadRequestException("Root nodes cannot be set as values.")
}
} yield ()
}

/**
* Checks whether a list node exists, and throws [[NotFoundException]] otherwise.
*
Expand Down
15 changes: 14 additions & 1 deletion webapi/src/main/scala/org/knora/webapi/responders/v2/ValuesResponderV2.scala
View file
Open in desktop
Expand Up @@ -193,13 +193,23 @@ class ValuesResponderV2(responderData: ResponderData) extends Responder(responde
)

// If this is a list value, check that it points to a real list node.

_ <- submittedInternalValueContent match {
case listValue: HierarchicalListValueContentV2 =>
ResourceUtilV2.checkListNodeExists(listValue.valueHasListNode, appActor)
case _ => FastFuture.successful(())
}

// If this is a list value, check if list value is not root list node.
_ <- submittedInternalValueContent match {
case listValue: HierarchicalListValueContentV2 =>
ResourceUtilV2.checkIfNodeIsRoot(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming is a bit misleading. checkIfNodeIsRoot implies that it is going to be true when it is a root node. In this case, you get an exception if it is a root node, i.e., a false. Also, what happens if the node is not a node at all, i.e., it does not exist in the database?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, doing two queries just to check if a node exists and is not a root node is a bit inefficient. You should try to make only one query (a SPARQL ASK), that will return true in the case that it is an existing child node, and a false in the case that it is not existing or a root node.

Copy link
Collaborator Author

@mpro7 mpro7 Jun 30, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How this name is misleading? It says what it does = checks if the list node is root. To make it more precise it might be adjusted to checkIfNodeIsRootAndThrowsErrorIfNot, but IMO this is not really necessary here.

On the other comment you're right, this two queries are not optimal solution. I was actually thinking to use the one I created - which uses inside getListNode. At the end it gets the node, so if it isn't in the database should also return that information. But it seems it isn't that straighfroward, because it's checked somewhere else and this is thrown:

2022-06-30 07:24:18 | ERROR | KnoraExceptionHandler$ | Unable to run route /v2/values
java.util.NoSuchElementException: head of empty list

listValue.valueHasListNode,
appActor,
createValueRequest.featureFactoryConfig
)
case _ => FastFuture.successful(())
}

// Check that the resource class's cardinality for the submitted property allows another value to be added
// for that property.

Expand Down Expand Up @@ -331,6 +341,9 @@ class ValuesResponderV2(responderData: ResponderData) extends Responder(responde
featureFactoryConfig = createValueRequest.featureFactoryConfig,
requestingUser = createValueRequest.requestingUser
)

// _ = println(111, createValueRequest.createValue.resourceIri, resourceInfo.resourceClassIri)

mpro7 marked this conversation as resolved.
Show resolved Hide resolved
} yield CreateValueResponseV2(
valueIri = verifiedValue.newValueIri,
valueType = verifiedValue.value.valueType,
Expand Down
28 changes: 27 additions & 1 deletion webapi/src/test/scala/org/knora/webapi/responders/v2/ValuesResponderV2Spec.scala
View file
Open in desktop
Expand Up @@ -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
Expand Down Expand Up @@ -1780,6 +1780,32 @@ 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
)
),
featureFactoryConfig = defaultFeatureFactoryConfig,
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.

Expand Down