From 865d1f40992c1b78a0da5754056c6d723bef9b00 Mon Sep 17 00:00:00 2001 From: Youri K Date: Fri, 29 Jun 2018 13:43:57 +0200 Subject: [PATCH 1/4] enhance the backend tree validator with some functions the frontend already uses #2830 --- app/models/annotation/nml/NmlParser.scala | 1 + test/backend/NMLUnitTestSuite.scala | 12 ++-- .../tracings/skeleton/TreeValidator.scala | 61 ++++++++++++++++++- 3 files changed, 65 insertions(+), 9 deletions(-) diff --git a/app/models/annotation/nml/NmlParser.scala b/app/models/annotation/nml/NmlParser.scala index 501d86624c4..9942f6681a0 100755 --- a/app/models/annotation/nml/NmlParser.scala +++ b/app/models/annotation/nml/NmlParser.scala @@ -49,6 +49,7 @@ object NmlParser extends LazyLogging with ProtoGeometryImplicits { trees <- extractTrees(data \ "thing", branchPoints, comments) treeGroups = extractTreeGroups(data \ "groups") volumes = extractVolumes(data \ "volume") + _ <- TreeValidator.validateAdditionalInformation(trees, branchPoints, comments, treeGroups) } yield { val dataSetName = parseDataSetName(parameters \ "experiment") val description = parseDescription(parameters \ "experiment") diff --git a/test/backend/NMLUnitTestSuite.scala b/test/backend/NMLUnitTestSuite.scala index 594626ebf6c..48353d0754b 100644 --- a/test/backend/NMLUnitTestSuite.scala +++ b/test/backend/NMLUnitTestSuite.scala @@ -73,16 +73,14 @@ class NMLUnitTestSuite extends FlatSpec with LazyLogging { val wrongTree = dummyTracing.trees(1).copy(comments = Seq(Comment(99, "test"))) val newTracing = dummyTracing.copy(trees = Seq(dummyTracing.trees(0), wrongTree)) - assert(true)//!isParseSuccessful(getParsedTracing(newTracing))) - //TODO: The parser currently doesn't check this + assert(!isParseSuccessful(getParsedTracing(newTracing))) } it should "throw an error for invalid branchPoint state" in { val wrongTree = dummyTracing.trees(1).copy(branchPoints = Seq(BranchPoint(99, 0))) val newTracing = dummyTracing.copy(trees = Seq(dummyTracing.trees(0), wrongTree)) - assert(true)//!isParseSuccessful(getParsedTracing(newTracing))) - //TODO: The parser currently doesn't check this + assert(!isParseSuccessful(getParsedTracing(newTracing))) } it should "throw an error for invalid edge state" in { @@ -131,14 +129,12 @@ class NMLUnitTestSuite extends FlatSpec with LazyLogging { val wrongTree = dummyTracing.trees(1).copy(groupId = Some(9999)) val newTracing = dummyTracing.copy(trees = Seq(dummyTracing.trees(0), wrongTree)) - assert(true)//!isParseSuccessful(getParsedTracing(newTracing))) - //TODO: The parser currently doesn't check this + assert(!isParseSuccessful(getParsedTracing(newTracing))) } it should "throw an error for duplicate groupId state" in { val newTracing = dummyTracing.copy(treeGroups = TreeGroup("Group", 3) +: dummyTracing.treeGroups) - assert(true)//!isParseSuccessful(getParsedTracing(newTracing))) - //TODO: The parser currently doesn't check this + assert(!isParseSuccessful(getParsedTracing(newTracing))) } } diff --git a/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala b/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala index f383f86454f..9f6b720fa70 100644 --- a/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala +++ b/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala @@ -1,6 +1,6 @@ package com.scalableminds.webknossos.datastore.tracings.skeleton -import com.scalableminds.webknossos.datastore.SkeletonTracing.{Edge, Tree} +import com.scalableminds.webknossos.datastore.SkeletonTracing._ import com.scalableminds.util.datastructures.UnionFind import net.liftweb.common.{Box, Failure, Full} import net.liftweb.util.A @@ -9,6 +9,14 @@ import scala.collection.mutable object TreeValidator { + def validateAdditionalInformation(trees: Seq[Tree], branchPoints: Seq[BranchPoint], comments: Seq[Comment], treeGroups: Seq[TreeGroup]): Box[Unit] = + for{ + _ <- checkNoDuplicateTreeGroupIds(treeGroups) + _ <- checkAllTreeGroupIdsUsedExist(trees, treeGroups) + _ <- checkAllNodesUsedInBranchPointsExist(trees, branchPoints) + _ <- checkAllNodesUsedInCommentsExist(trees, comments) + } yield Full(()) + def validateTrees(trees: Seq[Tree]): Box[Unit] = { for { _ <- checkNoDuplicateTreeIds(trees) @@ -102,7 +110,51 @@ object TreeValidator { } } + private def checkNoDuplicateTreeGroupIds(treeGroups: Seq[TreeGroup]) = { + val treeGroupIds = getAllTreeGroupIds(treeGroups, Seq[Int]()) + val distinctTreeGroupIds = treeGroupIds.distinct + if (treeGroupIds.size == distinctTreeGroupIds.size) { + Full(()) + } else { + Failure(s"Duplicate treeGroupIds: ${treeGroupIds.diff(distinctTreeGroupIds).mkString(", ")}") + } + } + + private def checkAllNodesUsedInCommentsExist(trees: Seq[Tree], comments: Seq[Comment]): Box[Unit] = { + val nodesInAllTrees = trees.flatMap(_.nodes.map(_.id)) + val nodesInComments = comments.map(_.nodeId).distinct + + val nodesOnlyInComments = nodesInComments.diff(nodesInAllTrees) + if (nodesOnlyInComments.isEmpty) { + Full(()) + } else { + Failure(s"Some comments refer to non-existent nodes. comments: ${nodesOnlyInComments.mkString(", ")}") + } + } + + private def checkAllNodesUsedInBranchPointsExist(trees: Seq[Tree], branchPoints: Seq[BranchPoint]): Box[Unit] = { + val nodesInAllTrees = trees.flatMap(_.nodes).map(_.id) + val nodesInBranchPoints = branchPoints.map(_.nodeId).distinct + + val nodesOnlyInBranchPoints = nodesInBranchPoints.diff(nodesInAllTrees) + if (nodesOnlyInBranchPoints.isEmpty) { + Full(()) + } else { + Failure(s"Some branchPoints refer to non-existent nodes. branchPoints: ${nodesOnlyInBranchPoints.mkString(", ")}") + } + } + + private def checkAllTreeGroupIdsUsedExist(trees: Seq[Tree], treeGroups: Seq[TreeGroup]) = { + val existingTreeGroups = getAllTreeGroupIds(treeGroups, Seq[Int]()) + val treeGroupsInTrees = trees.flatMap(_.groupId).distinct + val treeGroupsOnlyInTrees = treeGroupsInTrees.diff(existingTreeGroups) + if (treeGroupsOnlyInTrees.isEmpty) { + Full(()) + } else { + Failure(s"Some treeGroups used in trees don't exist. treeGroups: ${treeGroupsOnlyInTrees.mkString(", ")}") + } + } private def foldOverTrees(trees: Seq[Tree])(block: Tree => Box[Unit]) = { trees.foldLeft[Box[Unit]](Full(())){ @@ -113,4 +165,11 @@ object TreeValidator { } } + private def getAllTreeGroupIds(treeGroups: Seq[TreeGroup], ids: Seq[Int]): Seq[Int] = { + treeGroups match { + case (head :: tail) => getAllTreeGroupIds(tail ++ head.children, head.groupId +: ids) + case _ => ids + } + } + } From c4fba6ce945df5fcc55d116c6dce5903a2d43be1 Mon Sep 17 00:00:00 2001 From: Youri K Date: Tue, 3 Jul 2018 17:32:45 +0200 Subject: [PATCH 2/4] implement some of the pr advice #2832 --- app/models/annotation/nml/NmlParser.scala | 5 +++- .../tracings/skeleton/TreeValidator.scala | 24 +++++++------------ 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/app/models/annotation/nml/NmlParser.scala b/app/models/annotation/nml/NmlParser.scala index 9942f6681a0..c6c395d4c71 100755 --- a/app/models/annotation/nml/NmlParser.scala +++ b/app/models/annotation/nml/NmlParser.scala @@ -49,7 +49,10 @@ object NmlParser extends LazyLogging with ProtoGeometryImplicits { trees <- extractTrees(data \ "thing", branchPoints, comments) treeGroups = extractTreeGroups(data \ "groups") volumes = extractVolumes(data \ "volume") - _ <- TreeValidator.validateAdditionalInformation(trees, branchPoints, comments, treeGroups) + _ <- TreeValidator.checkNoDuplicateTreeGroupIds(treeGroups) + _ <- TreeValidator.checkAllTreeGroupIdsUsedExist(trees, treeGroups) + _ <- TreeValidator.checkAllNodesUsedInBranchPointsExist(trees, branchPoints) + _ <- TreeValidator.checkAllNodesUsedInCommentsExist(trees, comments) } yield { val dataSetName = parseDataSetName(parameters \ "experiment") val description = parseDescription(parameters \ "experiment") diff --git a/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala b/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala index 9f6b720fa70..83ef9915702 100644 --- a/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala +++ b/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala @@ -8,15 +8,6 @@ import net.liftweb.util.A import scala.collection.mutable object TreeValidator { - - def validateAdditionalInformation(trees: Seq[Tree], branchPoints: Seq[BranchPoint], comments: Seq[Comment], treeGroups: Seq[TreeGroup]): Box[Unit] = - for{ - _ <- checkNoDuplicateTreeGroupIds(treeGroups) - _ <- checkAllTreeGroupIdsUsedExist(trees, treeGroups) - _ <- checkAllNodesUsedInBranchPointsExist(trees, branchPoints) - _ <- checkAllNodesUsedInCommentsExist(trees, comments) - } yield Full(()) - def validateTrees(trees: Seq[Tree]): Box[Unit] = { for { _ <- checkNoDuplicateTreeIds(trees) @@ -39,7 +30,7 @@ object TreeValidator { } private def checkNoDuplicateNodeIds(trees: Seq[Tree]): Box[Unit] = { - val nodeIds = trees.flatMap(_.nodes).map(_.id) + val nodeIds = getAllNodeIds(trees) val distinctNodeIds = nodeIds.distinct if (nodeIds.size == distinctNodeIds.size) { Full(()) @@ -110,7 +101,7 @@ object TreeValidator { } } - private def checkNoDuplicateTreeGroupIds(treeGroups: Seq[TreeGroup]) = { + def checkNoDuplicateTreeGroupIds(treeGroups: Seq[TreeGroup]) = { val treeGroupIds = getAllTreeGroupIds(treeGroups, Seq[Int]()) val distinctTreeGroupIds = treeGroupIds.distinct if (treeGroupIds.size == distinctTreeGroupIds.size) { @@ -120,8 +111,8 @@ object TreeValidator { } } - private def checkAllNodesUsedInCommentsExist(trees: Seq[Tree], comments: Seq[Comment]): Box[Unit] = { - val nodesInAllTrees = trees.flatMap(_.nodes.map(_.id)) + def checkAllNodesUsedInCommentsExist(trees: Seq[Tree], comments: Seq[Comment]): Box[Unit] = { + val nodesInAllTrees = getAllNodeIds(trees) val nodesInComments = comments.map(_.nodeId).distinct val nodesOnlyInComments = nodesInComments.diff(nodesInAllTrees) @@ -132,8 +123,8 @@ object TreeValidator { } } - private def checkAllNodesUsedInBranchPointsExist(trees: Seq[Tree], branchPoints: Seq[BranchPoint]): Box[Unit] = { - val nodesInAllTrees = trees.flatMap(_.nodes).map(_.id) + def checkAllNodesUsedInBranchPointsExist(trees: Seq[Tree], branchPoints: Seq[BranchPoint]): Box[Unit] = { + val nodesInAllTrees = getAllNodeIds(trees) val nodesInBranchPoints = branchPoints.map(_.nodeId).distinct val nodesOnlyInBranchPoints = nodesInBranchPoints.diff(nodesInAllTrees) @@ -144,7 +135,7 @@ object TreeValidator { } } - private def checkAllTreeGroupIdsUsedExist(trees: Seq[Tree], treeGroups: Seq[TreeGroup]) = { + def checkAllTreeGroupIdsUsedExist(trees: Seq[Tree], treeGroups: Seq[TreeGroup]) = { val existingTreeGroups = getAllTreeGroupIds(treeGroups, Seq[Int]()) val treeGroupsInTrees = trees.flatMap(_.groupId).distinct @@ -172,4 +163,5 @@ object TreeValidator { } } + private def getAllNodeIds(trees: Seq[Tree]) = trees.flatMap(_.nodes).map(_.id) } From 950aa537b89320abd347bf8858eab1a693b50fb5 Mon Sep 17 00:00:00 2001 From: Youri K Date: Thu, 5 Jul 2018 11:19:54 +0200 Subject: [PATCH 3/4] implement pr feedback #2832 --- .../tracings/skeleton/TreeValidator.scala | 57 +++++++++---------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala b/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala index 83ef9915702..58ab82eeee8 100644 --- a/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala +++ b/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala @@ -30,7 +30,7 @@ object TreeValidator { } private def checkNoDuplicateNodeIds(trees: Seq[Tree]): Box[Unit] = { - val nodeIds = getAllNodeIds(trees) + val nodeIds = trees.flatMap(_.nodes).map(_.id) val distinctNodeIds = nodeIds.distinct if (nodeIds.size == distinctNodeIds.size) { Full(()) @@ -101,41 +101,31 @@ object TreeValidator { } } - def checkNoDuplicateTreeGroupIds(treeGroups: Seq[TreeGroup]) = { - val treeGroupIds = getAllTreeGroupIds(treeGroups, Seq[Int]()) - val distinctTreeGroupIds = treeGroupIds.distinct - if (treeGroupIds.size == distinctTreeGroupIds.size) { - Full(()) - } else { - Failure(s"Duplicate treeGroupIds: ${treeGroupIds.diff(distinctTreeGroupIds).mkString(", ")}") + def checkAllNodesUsedInCommentsExist(trees: Seq[Tree], comments: Seq[Comment]): Box[Unit] = { + checkAllNodesUsedExist(trees, comments.map(_.nodeId).distinct) match { + case Some(nodesOnlyInComments) => Failure(s"Some comments refer to non-existent nodes. comments: ${nodesOnlyInComments.mkString(", ")}") + case None => Full(()) } } - def checkAllNodesUsedInCommentsExist(trees: Seq[Tree], comments: Seq[Comment]): Box[Unit] = { - val nodesInAllTrees = getAllNodeIds(trees) - val nodesInComments = comments.map(_.nodeId).distinct + def checkAllNodesUsedInBranchPointsExist(trees: Seq[Tree], branchPoints: Seq[BranchPoint]): Box[Unit] = { + checkAllNodesUsedExist(trees, branchPoints.map(_.nodeId).distinct) match { + case Some(nodesOnlyInBranchPoints) => Failure(s"Some branchPoints refer to non-existent nodes. branchPoints: ${nodesOnlyInBranchPoints.mkString(", ")}") + case None => Full(()) + } + } - val nodesOnlyInComments = nodesInComments.diff(nodesInAllTrees) - if (nodesOnlyInComments.isEmpty) { + def checkNoDuplicateTreeGroupIds(treeGroups: Seq[TreeGroup]): Box[Unit] = { + val treeGroupIds = getAllTreeGroupIds(treeGroups, Seq[Int]()) + val distinctTreeGroupIds = treeGroupIds.distinct + if (treeGroupIds.size == distinctTreeGroupIds.size) { Full(()) } else { - Failure(s"Some comments refer to non-existent nodes. comments: ${nodesOnlyInComments.mkString(", ")}") + Failure(s"Duplicate treeGroupIds: ${treeGroupIds.diff(distinctTreeGroupIds).mkString(", ")}") } } - def checkAllNodesUsedInBranchPointsExist(trees: Seq[Tree], branchPoints: Seq[BranchPoint]): Box[Unit] = { - val nodesInAllTrees = getAllNodeIds(trees) - val nodesInBranchPoints = branchPoints.map(_.nodeId).distinct - - val nodesOnlyInBranchPoints = nodesInBranchPoints.diff(nodesInAllTrees) - if (nodesOnlyInBranchPoints.isEmpty) { - Full(()) - } else { - Failure(s"Some branchPoints refer to non-existent nodes. branchPoints: ${nodesOnlyInBranchPoints.mkString(", ")}") - } - } - - def checkAllTreeGroupIdsUsedExist(trees: Seq[Tree], treeGroups: Seq[TreeGroup]) = { + def checkAllTreeGroupIdsUsedExist(trees: Seq[Tree], treeGroups: Seq[TreeGroup]): Box[Unit] = { val existingTreeGroups = getAllTreeGroupIds(treeGroups, Seq[Int]()) val treeGroupsInTrees = trees.flatMap(_.groupId).distinct @@ -148,7 +138,7 @@ object TreeValidator { } private def foldOverTrees(trees: Seq[Tree])(block: Tree => Box[Unit]) = { - trees.foldLeft[Box[Unit]](Full(())){ + trees.foldLeft[Box[Unit]](Full(())) { case (Full(()), tree) => block(tree) case (f, _) => @@ -163,5 +153,14 @@ object TreeValidator { } } - private def getAllNodeIds(trees: Seq[Tree]) = trees.flatMap(_.nodes).map(_.id) + private def checkAllNodesUsedExist(trees: Seq[Tree], usedNodes: Seq[Int]) = { + val nodesInAllTrees = trees.flatMap(_.nodes).map(_.id) + + val nodesUsedButNonExistent = usedNodes.diff(nodesInAllTrees) + if (nodesUsedButNonExistent.isEmpty) { + None + } else { + Some(nodesUsedButNonExistent) + } + } } From 70b5c56753b6c8bb7086447561b46f8e3a58f212 Mon Sep 17 00:00:00 2001 From: Youri K Date: Thu, 5 Jul 2018 13:34:35 +0200 Subject: [PATCH 4/4] fix merge errors and make code DRYer #2832 --- test/backend/NMLUnitTestSuite.scala | 8 +++---- .../tracings/skeleton/TreeValidator.scala | 23 ++++++------------- 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/test/backend/NMLUnitTestSuite.scala b/test/backend/NMLUnitTestSuite.scala index 2ad24b5a18c..cc9ac799e77 100644 --- a/test/backend/NMLUnitTestSuite.scala +++ b/test/backend/NMLUnitTestSuite.scala @@ -70,14 +70,14 @@ class NMLUnitTestSuite extends FlatSpec { val wrongTree = dummyTracing.trees(1).copy(comments = Seq(Comment(99, "test"))) val newTracing = dummyTracing.copy(trees = Seq(dummyTracing.trees(0), wrongTree)) - assert(!isParseSuccessful(getParsedTracing(newTracing))) + assert(!isParseSuccessful(writeAndParseTracing(newTracing))) } it should "throw an error for a branchPoint with a non-existent nodeId" in { val wrongTree = dummyTracing.trees(1).copy(branchPoints = Seq(BranchPoint(99, 0))) val newTracing = dummyTracing.copy(trees = Seq(dummyTracing.trees(0), wrongTree)) - assert(!isParseSuccessful(getParsedTracing(newTracing))) + assert(!isParseSuccessful(writeAndParseTracing(newTracing))) } it should "throw an error for an edge which is referring to a non-existent node" in { @@ -126,12 +126,12 @@ class NMLUnitTestSuite extends FlatSpec { val wrongTree = dummyTracing.trees(1).copy(groupId = Some(9999)) val newTracing = dummyTracing.copy(trees = Seq(dummyTracing.trees(0), wrongTree)) - assert(!isParseSuccessful(getParsedTracing(newTracing))) + assert(!isParseSuccessful(writeAndParseTracing(newTracing))) } it should "throw an error for duplicate groupId state" in { val newTracing = dummyTracing.copy(treeGroups = TreeGroup("Group", 3) +: dummyTracing.treeGroups) - assert(!isParseSuccessful(getParsedTracing(newTracing))) + assert(!isParseSuccessful(writeAndParseTracing(newTracing))) } } diff --git a/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala b/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala index 58ab82eeee8..2d53788c1e9 100644 --- a/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala +++ b/webknossos-datastore/app/com/scalableminds/webknossos/datastore/tracings/skeleton/TreeValidator.scala @@ -101,19 +101,11 @@ object TreeValidator { } } - def checkAllNodesUsedInCommentsExist(trees: Seq[Tree], comments: Seq[Comment]): Box[Unit] = { - checkAllNodesUsedExist(trees, comments.map(_.nodeId).distinct) match { - case Some(nodesOnlyInComments) => Failure(s"Some comments refer to non-existent nodes. comments: ${nodesOnlyInComments.mkString(", ")}") - case None => Full(()) - } - } + def checkAllNodesUsedInCommentsExist(trees: Seq[Tree], comments: Seq[Comment]): Box[Unit] = + checkAllNodesUsedExist(trees, comments.map(_.nodeId).distinct, "comments") - def checkAllNodesUsedInBranchPointsExist(trees: Seq[Tree], branchPoints: Seq[BranchPoint]): Box[Unit] = { - checkAllNodesUsedExist(trees, branchPoints.map(_.nodeId).distinct) match { - case Some(nodesOnlyInBranchPoints) => Failure(s"Some branchPoints refer to non-existent nodes. branchPoints: ${nodesOnlyInBranchPoints.mkString(", ")}") - case None => Full(()) - } - } + def checkAllNodesUsedInBranchPointsExist(trees: Seq[Tree], branchPoints: Seq[BranchPoint]): Box[Unit] = + checkAllNodesUsedExist(trees, branchPoints.map(_.nodeId).distinct, "branchPoints") def checkNoDuplicateTreeGroupIds(treeGroups: Seq[TreeGroup]): Box[Unit] = { val treeGroupIds = getAllTreeGroupIds(treeGroups, Seq[Int]()) @@ -153,14 +145,13 @@ object TreeValidator { } } - private def checkAllNodesUsedExist(trees: Seq[Tree], usedNodes: Seq[Int]) = { + private def checkAllNodesUsedExist(trees: Seq[Tree], usedNodes: Seq[Int], nodeName: String) = { val nodesInAllTrees = trees.flatMap(_.nodes).map(_.id) val nodesUsedButNonExistent = usedNodes.diff(nodesInAllTrees) if (nodesUsedButNonExistent.isEmpty) { - None + Full(()) } else { - Some(nodesUsedButNonExistent) - } + Failure(s"Some $nodeName refer to non-existent nodes. $nodeName: ${nodesUsedButNonExistent.mkString(", ")}") } } }