Skip to content

Commit

Permalink
fix: Return 400 instead of 500 in /admin/lists endpoint (DEV-3556) (#…
Browse files Browse the repository at this point in the history 
…3229)

Co-authored-by: Marcin Procyk <marcin.procyk@dasch.swiss>
  • Loading branch information
@seakayone @mpro7
seakayone and mpro7 authored May 2, 2024
1 parent 7800cd3 commit 60029b7
Showing 1 changed file with 86 additions and 149 deletions.
235 changes: 86 additions & 149 deletions webapi/src/main/scala/org/knora/webapi/responders/admin/ListsResponder.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package org.knora.webapi.responders.admin

import zio.IO
import zio.Task
import zio.ZIO
import zio.ZLayer
Expand Down Expand Up @@ -524,20 +525,14 @@ final case class ListsResponder(
case child: ListCreateChildNodeRequest => (child.id, child.projectIri, child.name, child.position)
}

def getPositionOfNewChild(children: Seq[ListChildNodeADM]): Int = {
if (position.exists(_.value > children.size)) {
val givenPosition = position.map(_.value)
throw BadRequestException(
s"Invalid position given ${givenPosition.get}, maximum allowed position is = ${children.size}.",
)
}

val newPosition = if (position.isEmpty || position.exists(_.value.equals(-1))) {
children.size
} else {
position.get.value
def getPositionOfNewChild(children: Seq[ListChildNodeADM]): IO[BadRequestException, Int] = {
val size = children.size
position.map(_.value) match {
case Some(pos) if pos > size =>
ZIO.fail(BadRequestException(s"Invalid position given $pos, maximum allowed position is = $size."))
case Some(pos) if pos >= 0 => ZIO.succeed(pos)
case _ => ZIO.succeed(size)
}
newPosition
}

def getRootNodeIri(parentListNode: ListNodeADM): IRI =
Expand All @@ -552,15 +547,10 @@ final case class ListsResponder(
): Task[(Some[Int], Some[IRI])] =
for {
/* Verify that the list node exists by retrieving the whole node including children one level deep (need for position calculation) */
maybeParentListNode <- listNodeGetADM(nodeIri = parentNodeIri, shallow = true)
(parentListNode, children) = maybeParentListNode match {
case Some(node: ListRootNodeADM) => (node, node.children)
case Some(node: ListChildNodeADM) => (node, node.children)
case _ => throw BadRequestException(s"List node '$parentNodeIri' not found.")
}

// get position of the new child
position = getPositionOfNewChild(children)
parentListNode <- listNodeGetADM(parentNodeIri, shallow = true)
.someOrFail(BadRequestException(s"List node '$parentNodeIri' not found."))
children = parentListNode.children
position <- getPositionOfNewChild(children)

// Is the node supposed to be inserted in a specific position in array of children?
_ <-
Expand Down Expand Up @@ -593,17 +583,12 @@ final case class ListsResponder(
.someOrFail(BadRequestException(s"Project '$projectIri' not found."))

/* verify that the list node name is unique for the project */
projectUniqueNodeName <- listNodeNameIsProjectUnique(
projectIri.value,
name,
)
_ = if (!projectUniqueNodeName) {
val escapedName = name.get.value
val unescapedName = Iri.fromSparqlEncodedString(escapedName)
throw BadRequestException(
s"The node name $unescapedName is already used by a list inside the project ${projectIri.value}.",
)
}
_ <- ZIO.fail {
val unescapedName = Iri.fromSparqlEncodedString(name.map(_.value).getOrElse(""))
BadRequestException(
s"The node name $unescapedName is already used by a list inside the project ${projectIri.value}.",
)
}.whenZIO(listNodeNameIsProjectUnique(projectIri.value, name).negate)

// calculate the data named graph
dataNamedGraph: IRI = ProjectService.projectDataNamedGraphV2(project).value
Expand Down Expand Up @@ -903,34 +888,35 @@ final case class ListsResponder(
* The highest position a node can be placed is to the end of the parents children; that means length of existing
* children + 1
*
* If the node must be added to a new parent, highest valid position is numberOfChildren.
* For example, if the new parent already has 3 children, the highest occupied position is 2, node can be
* placed in position 3. That means the furthest a node can be positioned is being appended to the end of
* children of the new parent.
*
* If node remains in its current parent, the highest valid position is numberOfChildren -1
* That means if the parent node has 4 children, the highest position is 3.
* Nodes are only reorganized within the same parent.
*
* The lowest position a node gets is 0. If -1 is given, node will be appended to the end of children list.
* Values less than -1 are not allowed.
*
* @param parentNode the parent to which the node should belong.
* @param isNewParent identifier that node is added to another parent or not.
* @return [[Unit]]
* fails with a [[BadRequestException]] if given position is out of range.
*/
def isNewPositionValid(parentNode: ListNodeADM, isNewParent: Boolean): Unit = {
def isNewPositionValid(parentNode: ListNodeADM, isNewParent: Boolean): Task[Unit] = {
val numberOfChildren = parentNode.children.size
// If the node must be added to a new parent, highest valid position is numberOfChildren.
// For example, if the new parent already has 3 children, the highest occupied position is 2, node can be
// placed in position 3. That means the furthest a node can be positioned is being appended to the end of
// children of the new parent.
if (isNewParent && changeNodePositionRequest.position > numberOfChildren) {
throw BadRequestException(s"Invalid position given, maximum allowed position is = $numberOfChildren.")
}

// If node remains in its current parent, the highest valid position is numberOfChildren -1
// That means if the parent node has 4 children, the highest position is 3.
// Nodes are only reorganized within the same parent.
if (!isNewParent && changeNodePositionRequest.position > numberOfChildren - 1) {
throw BadRequestException(s"Invalid position given, maximum allowed position is = ${numberOfChildren - 1}.")
}

// The lowest position a node gets is 0. If -1 is given, node will be appended to the end of children list.
// Values less than -1 are not allowed.
if (changeNodePositionRequest.position < -1) {
throw BadRequestException(s"Invalid position given, minimum allowed is -1.")
}
}
ZIO
.fail(s"Invalid position given, maximum allowed position is = $numberOfChildren.")
.when(isNewParent && changeNodePositionRequest.position > numberOfChildren) *>
ZIO
.fail(s"Invalid position given, maximum allowed position is = ${numberOfChildren - 1}.")
.when(!isNewParent && changeNodePositionRequest.position > numberOfChildren - 1) *>
ZIO
.fail(s"Invalid position given, minimum allowed is -1.")
.when(changeNodePositionRequest.position < -1)
}.unit.mapError(BadRequestException.apply)

/**
* Checks that the position of the node is updated and node is sublist of specified parent.
Expand Down Expand Up @@ -988,12 +974,10 @@ final case class ListsResponder(
): Task[Int] =
for {
// get parent node with its immediate children
maybeParentNode <- listNodeGetADM(nodeIri = parentIri, shallow = true)
parentNode =
maybeParentNode.getOrElse(
throw BadRequestException(s"The parent node $parentIri could node be found, report this as a bug."),
)
_ = isNewPositionValid(parentNode, isNewParent = false)
parentNode <-
listNodeGetADM(nodeIri = parentIri, shallow = true)
.someOrFail(BadRequestException(s"The parent node $parentIri could node be found, report this as a bug."))
_ <- isNewPositionValid(parentNode, isNewParent = false)
parentChildren = parentNode.children
currPosition = node.position

Expand Down Expand Up @@ -1065,7 +1049,7 @@ final case class ListsResponder(
// get new parent node with its immediate children
maybeNewParentNode <- listNodeGetADM(nodeIri = newParentIri, shallow = true)
newParent = maybeNewParentNode.get
_ = isNewPositionValid(newParent, isNewParent = true)
_ <- isNewPositionValid(newParent, isNewParent = true)
newSiblings = newParent.children

currentNodePosition = node.position
Expand Down Expand Up @@ -1128,12 +1112,9 @@ final case class ListsResponder(
// get data names graph of the project
dataNamedGraph <- getDataNamedGraph(project.id)
// get node in its current position
maybeNode <- listNodeGetADM(nodeIri.value, shallow = true)
node = maybeNode match {
case Some(node: ListChildNodeADM) => node
case _ =>
throw BadRequestException(s"Update of position is only allowed for child nodes!")
}
node <- listNodeGetADM(nodeIri.value, shallow = true)
.map(_.collect { case child: ListChildNodeADM => child })
.someOrFail(BadRequestException(s"Update of position is only allowed for child nodes!"))

// get node's current parent
currentParentNodeIri <- getParentNodeIRI(nodeIri.value)
Expand Down Expand Up @@ -1208,29 +1189,18 @@ final case class ListsResponder(
* Checks if node itself or any of its children is in use.
*
* @param nodeIri the node's IRI.
* @param nodeChildren the children of the node.
* @throws BadRequestException in case a node or one of its children is in use.
* @param children the children of the node.
*/
def isNodeOrItsChildrenUsed(nodeIri: IRI, nodeChildren: Seq[ListChildNodeADM]): Task[Unit] =
for {
// Is node itself in use?
_ <- isNodeUsed(
nodeIri = nodeIri,
errorFun = throw BadRequestException(s"Node $nodeIri cannot be deleted, because it is in use."),
)

errorCheckFutures: Seq[Task[Unit]] = nodeChildren.map { child =>
isNodeUsed(
nodeIri = child.id,
errorFun = throw BadRequestException(
s"Node $nodeIri cannot be deleted, because its child ${child.id} is in use.",
),
)
}

_ <- ZioHelper.sequence(errorCheckFutures)

} yield ()
def isNodeOrItsChildrenUsed(nodeIri: IRI, children: Seq[ListChildNodeADM]): Task[Unit] = {
def failIfInUse(nodeIri: IRI, failReason: String) = ZIO
.fail(BadRequestException(failReason))
.whenZIO(triplestore.query(Select(sparql.admin.txt.isNodeUsed(nodeIri))).map(_.results.bindings.nonEmpty))
failIfInUse(nodeIri, s"Node $nodeIri cannot be deleted, because it is in use.") *> {
ZIO.foreachDiscard(children) { child =>
failIfInUse(child.id, s"Node $nodeIri cannot be deleted, because its child ${child.id} is in use.")
}
}
}

/**
* Delete a list (root node) or a child node after verifying that neither the node itself nor any of its children
Expand Down Expand Up @@ -1281,12 +1251,9 @@ final case class ListsResponder(
dataNamedGraph: IRI,
): Task[ListNodeADM] =
for {
maybeNode <- listNodeGetADM(nodeIri = parentNodeIri, shallow = false)

parentNode: ListNodeADM =
maybeNode.getOrElse(
throw BadRequestException(s"The parent node of $deletedNodeIri not found, report this as a bug."),
)
parentNode <-
listNodeGetADM(parentNodeIri, shallow = false)
.someOrFail(BadRequestException(s"The parent node of $deletedNodeIri not found, report this as a bug."))

remainingChildren = parentNode.children

Expand Down Expand Up @@ -1420,30 +1387,25 @@ final case class ListsResponder(
dataNamedGraph <- getDataNamedGraph(changeNodeInfoRequest.projectIri)

/* verify that the list name is unique for the project */
nodeNameUnique <- listNodeNameIsProjectUnique(
changeNodeInfoRequest.projectIri.value,
changeNodeInfoRequest.name,
)
_ = if (!nodeNameUnique) {
throw DuplicateValueException(
s"The name ${changeNodeInfoRequest.name.get} is already used by a list inside the project ${changeNodeInfoRequest.projectIri.value}.",
)
}
_ <- ZIO.fail {
val msg =
s"The name ${changeNodeInfoRequest.name.get} is already used by a list inside the project ${changeNodeInfoRequest.projectIri.value}."
DuplicateValueException(msg)
}.whenZIO(
listNodeNameIsProjectUnique(changeNodeInfoRequest.projectIri.value, changeNodeInfoRequest.name).negate,
)

/* Verify that the node with Iri exists. */
maybeNode <- listNodeGetADM(nodeIri = changeNodeInfoRequest.listIri.value, shallow = true)

node = maybeNode.getOrElse(
throw BadRequestException(s"List item with '${changeNodeInfoRequest.listIri}' not found."),
)
node <- listNodeGetADM(changeNodeInfoRequest.listIri.value, shallow = true)
.someOrFail(BadRequestException(s"List item with '${changeNodeInfoRequest.listIri}' not found."))

// Update the list
changeNodeInfoSparqlString: String = sparql.admin.txt
.updateListInfo(
dataNamedGraph = dataNamedGraph,
nodeIri = changeNodeInfoRequest.listIri.value,
hasOldName = node.name.nonEmpty,
isRootNode = maybeNode.exists(_.isInstanceOf[ListRootNodeADM]),
isRootNode = node.isInstanceOf[ListRootNodeADM],
maybeName = changeNodeInfoRequest.name.map(_.value),
projectIri = changeNodeInfoRequest.projectIri.value,
listClassIri = KnoraBase.ListNode,
Expand Down Expand Up @@ -1479,24 +1441,11 @@ final case class ListsResponder(
} yield iriStr

case _ =>
throw BadRequestException(s"Node $nodeIri was not found. Please verify the given IRI.")
ZIO.fail(BadRequestException(s"Node $nodeIri was not found. Please verify the given IRI."))
}
projectIri <- ZIO.fromEither(ProjectIri.from(projectIriStr)).mapError(BadRequestException.apply)
} yield projectIri

/**
* Helper method to check if a node is in use.
*
* @param nodeIri the IRI of the node.
* @param errorFun a function that throws an exception. It will be called if the node is used.
* @return a [[Boolean]].
*/
private def isNodeUsed(nodeIri: IRI, errorFun: => Nothing): Task[Unit] =
for {
isNodeUsedResponse <- triplestore.query(Select(sparql.admin.txt.isNodeUsed(nodeIri)))
_ <- ZIO.when(isNodeUsedResponse.results.bindings.nonEmpty)(ZIO.attempt(errorFun))
} yield ()

/**
* Helper method to get the data named graph of a project.
*
Expand Down Expand Up @@ -1549,39 +1498,27 @@ final case class ListsResponder(
* @param nodeIri the IRI of the node that must be shifted.
* @param newPosition the new position of the child node.
* @param dataNamedGraph the data named graph of the project.
* @throws UpdateNotPerformedException if the position of the node could not be updated.
* @return a [[ListChildNodeADM]].
*/
private def updatePositionOfNode(
nodeIri: IRI,
newPosition: Int,
dataNamedGraph: IRI,
): Task[ListChildNodeADM] = {
// Generate SPARQL for erasing a node.
val sparqlUpdateNodePosition = sparql.admin.txt.updateNodePosition(
dataNamedGraph = dataNamedGraph,
nodeIri = nodeIri,
newPosition = newPosition,
)
): Task[ListChildNodeADM] =
for {
_ <- triplestore.query(Update(sparqlUpdateNodePosition))

_ <- triplestore.query(Update(sparql.admin.txt.updateNodePosition(dataNamedGraph, nodeIri, newPosition)))
/* Verify that the node info was updated */
maybeNode <- listNodeGetADM(nodeIri = nodeIri, shallow = false)

childNode: ListChildNodeADM =
maybeNode
.getOrElse(throw BadRequestException(s"Node with $nodeIri could not be found to update its position."))
.asInstanceOf[ListChildNodeADM]

_ = if (!childNode.position.equals(newPosition)) {
throw UpdateNotPerformedException(
s"The position of the node $nodeIri could not be updated, report this as a possible bug.",
)
}

childNode <- listNodeGetADM(nodeIri = nodeIri, shallow = false)
.someOrFail(BadRequestException(s"Node with $nodeIri could not be found to update its position."))
.map(_.asInstanceOf[ListChildNodeADM])
_ <- ZIO
.fail(
UpdateNotPerformedException(
s"The position of the node $nodeIri could not be updated, report this as a possible bug.",
),
)
.unless(childNode.position.equals(newPosition))
} yield childNode
}

/**
* Helper method to shift nodes between positions startPos and endPos to the left if 'shiftToLeft' is true,
Expand Down

0 comments on commit 60029b7

Please sign in to comment.