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 7 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
15 changes: 8 additions & 7 deletions webapi/src/main/scala/org/knora/webapi/responders/v2/ResourceUtilV2.scala
View file
Open in desktop
Expand Up @@ -9,9 +9,9 @@ import akka.actor.ActorRef
import akka.pattern._
import akka.util.Timeout
import com.typesafe.scalalogging.Logger
import org.knora.webapi.IRI
import dsp.errors.ForbiddenException
import dsp.errors.NotFoundException
import org.knora.webapi.IRI
import org.knora.webapi.messages.SmartIri
import org.knora.webapi.messages.admin.responder.permissionsmessages.DefaultObjectAccessPermissionsStringForPropertyGetADM
import org.knora.webapi.messages.admin.responder.permissionsmessages.DefaultObjectAccessPermissionsStringResponseADM
Expand All @@ -29,6 +29,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 @@ -133,25 +134,25 @@ object ResourceUtilV2 {
} yield defaultObjectAccessPermissionsResponse.permissionLiteral

/**
* Checks whether a list node exists, and throws [[NotFoundException]] otherwise.
* Checks whether a list node exists and has a root node, otherwise throws [[NotFoundException]].
*
* @param listNodeIri the IRI of the list node.
*/
def checkListNodeExists(listNodeIri: IRI, appActor: ActorRef)(implicit
def checkListNodeExistsAndHasRootNode(listNodeIri: IRI, appActor: ActorRef)(implicit
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't we say, that this method should return a boolean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, I'm missing a test for this method.

Copy link
Collaborator Author

@mpro7 mpro7 Jul 4, 2022 •
edited

Choose a reason for hiding this comment

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

That wasn't we but you are trying to force me to do so :) My opinion was different on that. However I've tried and spent some time to find out, that this method has been extracted to ResourceUtilV2 because it's used twice in ValuesResponderV2 as well as once in ResourcesResponderV2, where actually the usage is slightly different and I couldn't make things compile. Changing it from Unit method which throws to one which returns Boolean will also create more code duplication.
All this only strengthen my feelings:

  1. I'm against increasing the scope of tasks, especially bugs. If we have bugs first policy, they should be fixed ASAP. This task is about fixing the bug, which I did applying the convention in the package, I also wrote test.
  2. Changing the way how only checkListNodeExistsAndHasRootNode works w/o touching other methods in ResourceUtilV2 doesn't bring any value, in fact it makes even more confusion. If this is so important - should be moved to refactor task which will solve the problem comprehensively.
  3. At the end I don't mind to refactor code while fixing the bugs, which in fact we all do. But this should be rather only necessary changes which helps in fixing the bug, like in this PR changing checkListNodeExists to checkListNodeExistsAndHasRootNode in order to reduce number of queries and lines of code - one method instead of two.

timeout: Timeout,
executionContext: ExecutionContext
): Future[Unit] =
for {
askString <- Future(
org.knora.webapi.messages.twirl.queries.sparql.admin.txt
.checkListNodeExistsByIri(listNodeIri = listNodeIri)
.checkListNodeExistsAndHasRootNode(listNodeIri = listNodeIri)
.toString
)

checkListNodeExistsResponse <- appActor.ask(SparqlAskRequest(askString)).mapTo[SparqlAskResponse]
response <- appActor.ask(SparqlAskRequest(askString)).mapTo[SparqlAskResponse]

_ = if (!checkListNodeExistsResponse.result) {
throw NotFoundException(s"<$listNodeIri> does not exist or is not a ListNode")
_ = if (!response.result) {
throw NotFoundException(s"<$listNodeIri> does not exist, is not a ListNode or is a root node.")
}
} yield ()

Expand Down
4 changes: 2 additions & 2 deletions webapi/src/main/scala/org/knora/webapi/responders/v2/ResourcesResponderV2.scala
View file
Open in desktop
Expand Up @@ -8,8 +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.feature.FeatureFactoryConfig
import org.knora.webapi.messages.IriConversions._
import org.knora.webapi.messages.OntologyConstants
Expand Down Expand Up @@ -1198,7 +1198,7 @@ class ResourcesResponderV2(responderData: ResponderData) extends ResponderWithSt
Future
.sequence(
listNodesThatShouldExist
.map(listNodeIri => ResourceUtilV2.checkListNodeExists(listNodeIri, appActor))
.map(listNodeIri => ResourceUtilV2.checkListNodeExistsAndHasRootNode(listNodeIri, appActor))
.toSeq
)
.map(_ => ())
Expand Down
15 changes: 8 additions & 7 deletions webapi/src/main/scala/org/knora/webapi/responders/v2/ValuesResponderV2.scala
View file
Open in desktop
Expand Up @@ -7,8 +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.feature.FeatureFactoryConfig
import org.knora.webapi.messages.IriConversions._
import org.knora.webapi.messages.OntologyConstants
Expand Down Expand Up @@ -192,11 +192,10 @@ 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)
ResourceUtilV2.checkListNodeExistsAndHasRootNode(listValue.valueHasListNode, appActor)
case _ => FastFuture.successful(())
}

Expand Down Expand Up @@ -331,6 +330,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 Expand Up @@ -1182,11 +1184,10 @@ 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)
ResourceUtilV2.checkListNodeExistsAndHasRootNode(listValue.valueHasListNode, appActor)
case _ => FastFuture.successful(())
}

Expand Down
24 changes: 24 additions & 0 deletions ...ra/webapi/messages/twirl/queries/sparql/admin/checkListNodeExistsAndHasRootNode.scala.txt
View file
Open in desktop
@@ -0,0 +1,24 @@
@*
* Copyright © 2021 - 2022 Swiss National Data and Service Center for the Humanities and/or DaSCH Service Platform contributors.
* SPDX-License-Identifier: Apache-2.0
*@

@import org.knora.webapi.IRI

@**
* Checks if a list node exists and has root node.
*
* @param listNodeIri the IRI of the list node we want to check.
*@
@(listNodeIri: IRI)

PREFIX xsd: <http://www.w3.org/2001/XMLSchema#>
PREFIX rdf: <http://www.w3.org/1999/02/22-rdf-syntax-ns#>
PREFIX knora-base: <http://www.knora.org/ontology/knora-base#>

ASK {
BIND(IRI("@listNodeIri") AS ?node)

?node rdf:type knora-base:ListNode .
?node knora-base:hasRootNode ?name .
}
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[NotFoundException])
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this really be a "NotFoundException", seems more like a "BadRequest", right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It has been improved with 58b4551 and now depends of outcome I throw BadRequestException or NotFoundException.

}
}

"create a color value" in {
// Add the value.

Expand Down