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.

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

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

Conversation

mpro7
Copy link
Collaborator

@mpro7 mpro7 commented Jun 27, 2022

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?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

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

@mpro7 mpro7 self-assigned this Jun 28, 2022
@mpro7 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 mpro7 requested review from BalduinLandolt, irinaschubert and subotic and removed request for BalduinLandolt and irinaschubert June 28, 2022 09:33
Copy link

@irinaschubert irinaschubert left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
comment = getComment(jsonLDObject)
)
for {
isRootNode <- checkIfNodeIsRoot(

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.

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:

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

@mpro7
Copy link
Collaborator Author

mpro7 commented Jun 28, 2022

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

@mpro7 mpro7 requested a review from irinaschubert June 28, 2022 12:24
comment = getComment(jsonLDObject)
)
for {
isRootNode <- checkIfNodeIsRoot(
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:

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

@mpro7 mpro7 requested a review from subotic June 29, 2022 15:08
// 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

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

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.

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.

@mpro7 mpro7 requested a review from subotic June 30, 2022 14:18
*
* @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

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.

Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
)

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.

@mpro7 mpro7 requested a review from subotic July 5, 2022 12:38
FastFuture.successful(())
// it does have isRootNode property - it's a root node
case Right(true) =>
FastFuture.failed(
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 if you need FastFuture.failed when you are throwing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

@subotic subotic Jul 5, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@mpro7 mpro7 requested a review from subotic July 5, 2022 13:47
Copy link
Collaborator

@subotic subotic left a comment

Choose a reason for hiding this comment

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

Thanks 👍

Copy link

@irinaschubert irinaschubert left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud
Copy link

sonarcloud bot commented Jul 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mpro7 mpro7 merged commit 94d2b46 into main Jul 6, 2022
@mpro7 mpro7 deleted the DEV-973-it-shouldnt-be-possible-to-set-the-list-iri-as-list-property-value branch July 6, 2022 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants