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 4 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
80 changes: 67 additions & 13 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 All @@ -25,6 +26,10 @@ import org.knora.webapi.messages.admin.responder.projectsmessages.ProjectADM
import org.knora.webapi.messages.admin.responder.usersmessages.UserADM
import org.knora.webapi.messages.store.sipimessages.GetFileMetadataRequest
import org.knora.webapi.messages.store.sipimessages.GetFileMetadataResponse
import org.knora.webapi.messages.store.triplestoremessages.LiteralV2
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.PermissionUtilADM.EntityPermission
import org.knora.webapi.messages.util._
import org.knora.webapi.messages.util.rdf._
Expand All @@ -42,7 +47,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 Expand Up @@ -2805,12 +2809,8 @@ object HierarchicalListValueContentV2 extends ValueContentReaderV2[HierarchicalL
featureFactoryConfig: FeatureFactoryConfig,
settings: KnoraSettingsImpl,
log: Logger
)(implicit timeout: Timeout, executionContext: ExecutionContext): Future[HierarchicalListValueContentV2] =
Future(fromJsonLDObjectSync(jsonLDObject))

private def fromJsonLDObjectSync(jsonLDObject: JsonLDObject): HierarchicalListValueContentV2 = {
)(implicit timeout: Timeout, executionContext: ExecutionContext): Future[HierarchicalListValueContentV2] = {
implicit val stringFormatter: StringFormatter = StringFormatter.getGeneralInstance

val listValueAsListNode: SmartIri = jsonLDObject.requireIriInObject(
OntologyConstants.KnoraApiV2Complex.ListValueAsListNode,
stringFormatter.toSmartIriWithErr
Expand All @@ -2820,11 +2820,65 @@ object HierarchicalListValueContentV2 extends ValueContentReaderV2[HierarchicalL
throw BadRequestException(s"List node IRI <$listValueAsListNode> is not a Knora data IRI")
}

HierarchicalListValueContentV2(
ontologySchema = ApiV2Complex,
valueHasListNode = listValueAsListNode.toString,
comment = getComment(jsonLDObject)
)
for {
isRootNode <- checkIfNodeIsRoot(
Copy link

Choose a reason for hiding this comment

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

This check seems to be request specific. Did you check if fromJsonLDObject should never allow a root node?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't get your question. In the case of list values, yes it shouldn't be allowed a root node as value. That's actually the clue of this bug.

Copy link

Choose a reason for hiding this comment

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

OK, than that's fine. I thought there might be cases where the value can be a root node (although I don't know what cases there could be).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure, that this check is implemented in the correct place. It should be probably better to move it to here:

dsp-api/webapi/src/main/scala/org/knora/webapi/responders/v2/ValuesResponderV2.scala

Lines 195 to 201 in 3a0902e

// 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(())
}

This way, you will also be able to write the missing tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@subotic Good point, moved checking method to ResourceUtilV2 and added test to ValuesResponderV2Spec.

jsonLDObject = jsonLDObject,
appActor = appActor,
featureFactoryConfig = featureFactoryConfig
)

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

listValueContent <-
Future(
HierarchicalListValueContentV2(
ontologySchema = ApiV2Complex,
valueHasListNode = listValueAsListNode.toString,
comment = getComment(jsonLDObject)
)
)
} yield listValueContent
}

private def checkIfNodeIsRoot(
mpro7 marked this conversation as resolved.
Show resolved Hide resolved
jsonLDObject: JsonLDObject,
appActor: ActorRef,
featureFactoryConfig: FeatureFactoryConfig
)(implicit timeout: Timeout, executionContext: ExecutionContext): Future[Boolean] = {
implicit val stringFormatter: StringFormatter = StringFormatter.getGeneralInstance

val nodeIri = jsonLDObject
.requireIriInObject(
OntologyConstants.KnoraApiV2Complex.ListValueAsListNode,
stringFormatter.toSmartIriWithErr
)
.toString()

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

listNodeResponse <-
appActor
.ask(
SparqlExtendedConstructRequest(
sparql = sparqlQuery,
featureFactoryConfig = featureFactoryConfig
)
)
.mapTo[SparqlExtendedConstructResponse]

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

Expand Down