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

mpro7 · 2022-06-27T11:32:01Z

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Issue Number: DEV-973

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

irinaschubert

You didn't add a test. Is there already one? Or is it the case you mentioned today in the daily that you don't know how to test it?

.gitignore

...pi/src/main/scala/org/knora/webapi/messages/v2/responder/valuemessages/ValueMessagesV2.scala

irinaschubert · 2022-06-28T11:50:20Z

...pi/src/main/scala/org/knora/webapi/messages/v2/responder/valuemessages/ValueMessagesV2.scala

-      comment = getComment(jsonLDObject)
-    )
+    for {
+      isRootNode <- checkIfNodeIsRoot(


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

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.

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).

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.

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

mpro7 · 2022-06-28T12:24:20Z

@irinaschubert this is what I was mentioning on the daily. As an exercise I did an attempt to write tests based on org.scalatest.PrivateMethodTester but I failed. At the end nothing in this package is tested, so I don't want to refactor it, just fix the bug.

subotic · 2022-06-28T15:06:32Z

...pi/src/main/scala/org/knora/webapi/messages/v2/responder/valuemessages/ValueMessagesV2.scala

-      comment = getComment(jsonLDObject)
-    )
+    for {
+      isRootNode <- checkIfNodeIsRoot(


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.

subotic · 2022-06-29T17:53:34Z

webapi/src/main/scala/org/knora/webapi/responders/v2/ValuesResponderV2.scala

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


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?

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.

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

subotic · 2022-06-29T17:59:38Z

webapi/src/main/scala/org/knora/webapi/responders/v2/ResourceUtilV2.scala

+      listNodeResponse <-
+        appActor
+          .ask(
+            SparqlExtendedConstructRequest(


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.

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.

subotic · 2022-06-30T21:31:35Z

webapi/src/main/scala/org/knora/webapi/responders/v2/ResourceUtilV2.scala

   *
   * @param listNodeIri the IRI of the list node.
   */
-  def checkListNodeExists(listNodeIri: IRI, appActor: ActorRef)(implicit
+  def checkListNodeExistsAndHasRootNode(listNodeIri: IRI, appActor: ActorRef)(implicit


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

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

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:

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.

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.

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.

BalduinLandolt

LGTM, just some minor things. (Not sure if that will show up easily because I responded in a resolved conversation, but I also commented on the .gitignore thing)

.gitignore

webapi/src/main/scala/org/knora/webapi/responders/v2/ValuesResponderV2.scala

BalduinLandolt · 2022-07-04T08:10:46Z

webapi/src/test/scala/org/knora/webapi/responders/v2/ValuesResponderV2Spec.scala

+      )
+
+      expectMsgPF(timeout) { case msg: akka.actor.Status.Failure =>
+        assert(msg.cause.isInstanceOf[NotFoundException])


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

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

…ist-iri-as-list-property-value

webapi/src/main/scala/org/knora/webapi/responders/v2/ResourceUtilV2.scala

subotic · 2022-07-05T12:50:24Z

webapi/src/main/scala/org/knora/webapi/responders/v2/ResourcesResponderV2.scala

+                    FastFuture.successful(())
+                  // it does have isRootNode property - it's a root node
+                  case Right(true) =>
+                    FastFuture.failed(


I'm not sure if you need FastFuture.failed when you are throwing.

That was the most tricky part. W/oFastFuture.failed() it didn't compile, but strangely now it seems working. Suspect metals was broken at that time...

webapi/src/main/scala/org/knora/webapi/responders/v2/ResourcesResponderV2.scala

subotic · 2022-07-05T12:56:13Z

webapi/src/main/scala/org/knora/webapi/responders/v2/ResourcesResponderV2.scala

+            _ = checkNode match {
+                  // it doesn't have isRootNode property - it's a child node
+                  case Right(false) =>
+                    FastFuture.successful(())


you are not interested in the value, so simply return (). No need to warp it into a Future only to discard it later.

Same as above - I went for the same solution as I implemented in ValueResponderV2. but w/oFastFuture.successful(()) it didn't compile. Strangely now it seems working. Suspect metals was broken at that time...

subotic

Thanks 👍

irinaschubert

LGTM

webapi/src/main/scala/org/knora/webapi/responders/v2/ResourcesResponderV2.scala

sonarcloud · 2022-07-06T05:26:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

mpro7 added 3 commits June 27, 2022 13:28

add method checking if list is a root node or not

d1c775b

improve ignoring trig files

29998c3

refactor HierarchicalListValueContentV2 object

8455366

mpro7 self-assigned this Jun 28, 2022

cleanup

748feb1

mpro7 changed the title ~~fix(value): make impossible to set list root node as a value~~ fix(value): make impossible to set list root node as a value (DEV-973) Jun 28, 2022

mpro7 requested review from BalduinLandolt, irinaschubert and subotic and removed request for BalduinLandolt and irinaschubert June 28, 2022 09:33

irinaschubert reviewed Jun 28, 2022

View reviewed changes

mpro7 requested a review from irinaschubert June 28, 2022 12:24

subotic requested changes Jun 28, 2022

View reviewed changes

mpro7 added 2 commits June 29, 2022 17:00

add list root checkiing method to ValueResponderV2 + test

5953676

remove list root checking from ValueMessangerV2

6176715

mpro7 requested a review from subotic June 29, 2022 15:08

subotic reviewed Jun 29, 2022

View reviewed changes

add new query to handle 2 checks + merge checking methods

eeafd44

mpro7 requested a review from subotic June 30, 2022 14:18

subotic reviewed Jun 30, 2022

View reviewed changes

BalduinLandolt approved these changes Jul 4, 2022

View reviewed changes

mpro7 added 5 commits July 4, 2022 14:56

move throwing to responder

eae72df

Merge branch 'main' into DEV-973-it-shouldnt-be-possible-to-set-the-l…

4b16ae0

…ist-iri-as-list-property-value

fix formatting & bad merge

35eac06

fix failing test

b74ee41

refactor check method to return value + move error handling to responder

58b4551

mpro7 added 3 commits July 5, 2022 13:09

add UNIT tests for util method

d14daa9

remove unused template

db58026

Merge branch 'main' into DEV-973-it-shouldnt-be-possible-to-set-the-l…

fb35176

…ist-iri-as-list-property-value

mpro7 requested a review from subotic July 5, 2022 12:38

subotic reviewed Jul 5, 2022

View reviewed changes

webapi/src/main/scala/org/knora/webapi/responders/v2/ResourceUtilV2.scala Show resolved Hide resolved

subotic reviewed Jul 5, 2022

View reviewed changes

webapi/src/main/scala/org/knora/webapi/responders/v2/ResourcesResponderV2.scala Show resolved Hide resolved

subotic reviewed Jul 5, 2022

View reviewed changes

review changes

388f4a8

mpro7 requested a review from subotic July 5, 2022 13:47

subotic approved these changes Jul 5, 2022

View reviewed changes

irinaschubert approved these changes Jul 5, 2022

View reviewed changes

cleanup

0ce84d9

mpro7 merged commit 94d2b46 into main Jul 6, 2022
12 checks passed

mpro7 deleted the DEV-973-it-shouldnt-be-possible-to-set-the-list-iri-as-list-property-value branch July 6, 2022 06:17

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

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

mpro7 commented Jun 27, 2022 •

edited

irinaschubert left a comment

irinaschubert Jun 28, 2022

mpro7 Jun 28, 2022

irinaschubert Jun 28, 2022

subotic Jun 28, 2022

mpro7 Jun 29, 2022

mpro7 commented Jun 28, 2022

subotic Jun 28, 2022

subotic Jun 29, 2022

subotic Jun 29, 2022

mpro7 Jun 30, 2022 •

edited

subotic Jun 29, 2022

mpro7 Jun 30, 2022

subotic Jun 30, 2022

subotic Jun 30, 2022

mpro7 Jul 4, 2022 •

edited

BalduinLandolt left a comment

BalduinLandolt Jul 4, 2022

mpro7 Jul 5, 2022

subotic Jul 5, 2022

mpro7 Jul 5, 2022

subotic Jul 5, 2022 •

edited

mpro7 Jul 5, 2022

subotic left a comment

irinaschubert left a comment

sonarcloud bot commented Jul 6, 2022

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

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

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

Conversation

mpro7 commented Jun 27, 2022 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

irinaschubert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpro7 commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpro7 Jun 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpro7 Jul 4, 2022 • edited

Choose a reason for hiding this comment

BalduinLandolt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subotic Jul 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subotic left a comment

Choose a reason for hiding this comment

irinaschubert left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 6, 2022

mpro7 commented Jun 27, 2022 •

edited

mpro7 Jun 30, 2022 •

edited

mpro7 Jul 4, 2022 •

edited

subotic Jul 5, 2022 •

edited