Skip to content

Commit

Permalink
do not add parents to simple paths (#2502)
Browse files Browse the repository at this point in the history
  • Loading branch information
paul-butcher authored Dec 19, 2023
1 parent 5683007 commit 6380590
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 62 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ object PathOps {

lazy val isCircular: Boolean =
path.indexOf("/") != -1 && firstNode == lastNode

lazy val isSimple: Boolean =
path.indexOf("/") == -1
}
def concatenatePaths(parentPath: String, childPath: String): String = {
val childRoot = childPath.firstNode
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class PathsService(elasticClient: ElasticClient, index: Index)(
import PathOps._

def getParentPath(path: String): Future[Option[String]] = {
if (path.isCircular) {
if (path.isCircular || path.isSimple) {
Future(None)
} else {
val request: SearchRequest = requestBuilder.parentPath(path)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@ import weco.akka.fixtures.Akka
import weco.catalogue.internal_model.work.generators.WorkGenerators
import scala.concurrent.ExecutionContext.Implicits.global

/**
* Tests covering the PathsService, which fetches data from Elasticsearch
* in order to support the PathConcatenator.
/** Tests covering the PathsService, which fetches data from Elasticsearch in
* order to support the PathConcatenator.
*
* These tests require a running ElasticSearch Instance.
*/
Expand All @@ -33,7 +32,7 @@ class PathsServiceTest
private def service(index: Index) =
new PathsService(
elasticClient = elasticClient,
index = index,
index = index
)

describe("The PathService parentPath getter") {
Expand All @@ -43,11 +42,12 @@ class PathsServiceTest
work(path = "grandparent/parent"),
work(path = "parent/child")
)
withLocalMergedWorksIndex { index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getParentPath("parent/child")) {
_ shouldBe Some("grandparent/parent")
}
withLocalMergedWorksIndex {
index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getParentPath("parent/child")) {
_ shouldBe Some("grandparent/parent")
}
}
}

Expand All @@ -60,11 +60,12 @@ class PathsServiceTest
work(path = "grandparent/parent/brother"),
work(path = "parent/sister")
)
withLocalMergedWorksIndex { index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getParentPath("parent/sister")) {
_ shouldBe Some("grandparent/parent")
}
withLocalMergedWorksIndex {
index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getParentPath("parent/sister")) {
_ shouldBe Some("grandparent/parent")
}
}
}

Expand All @@ -77,11 +78,12 @@ class PathsServiceTest
work(path = "parent"),
work(path = "parent/child")
)
withLocalMergedWorksIndex { index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getParentPath("parent/child")) {
_ shouldBe empty
}
withLocalMergedWorksIndex {
index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getParentPath("parent/child")) {
_ shouldBe empty
}
}
}

Expand All @@ -90,11 +92,30 @@ class PathsServiceTest
work(path = "a/b/c/d/e"),
work(path = "e/f/g/h/i")
)
withLocalMergedWorksIndex { index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getParentPath("e/f/g/h/i")) {
_ shouldBe Some("a/b/c/d/e")
}
withLocalMergedWorksIndex {
index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getParentPath("e/f/g/h/i")) {
_ shouldBe Some("a/b/c/d/e")
}
}
}

it("does not add a parent to a root-only path") {
// When the requested path consists of a single node, it is the root of its own tree,
// and cannot have a parent.
// An earlier iteration of this service would get confused by the trailing slash in
// a path in a different tree.
val works: List[Work[Merged]] = List(
work(path = "parent"),
work(path = "a/completely/different/tree/")
)
withLocalMergedWorksIndex {
index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getParentPath("parent")) {
_ shouldBe empty
}
}
}

Expand All @@ -104,14 +125,15 @@ class PathsServiceTest
work(path = "grandfather/parent"),
work(path = "parent/child")
)
withLocalMergedWorksIndex { index =>
insertIntoElasticsearch(index, works: _*)
withLocalMergedWorksIndex {
index =>
insertIntoElasticsearch(index, works: _*)

val future = service(index).getParentPath("parent/child")
val future = service(index).getParentPath("parent/child")

whenReady(future.failed) {
_ shouldBe a[RuntimeException]
}
whenReady(future.failed) {
_ shouldBe a[RuntimeException]
}
}
}
}
Expand All @@ -124,11 +146,12 @@ class PathsServiceTest
expectedWork,
work(path = "parent/child/grandchild")
)
withLocalMergedWorksIndex { index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getWorkWithPath("parent/child")) {
_ shouldBe expectedWork
}
withLocalMergedWorksIndex {
index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getWorkWithPath("parent/child")) {
_ shouldBe expectedWork
}
}
}

Expand All @@ -137,27 +160,29 @@ class PathsServiceTest
work(path = "parent/child"),
work(path = "parent/child")
)
withLocalMergedWorksIndex { index =>
insertIntoElasticsearch(index, works: _*)

service(index)
.getWorkWithPath("parent/child")
.failed
.futureValue shouldBe a[RuntimeException]
withLocalMergedWorksIndex {
index =>
insertIntoElasticsearch(index, works: _*)

service(index)
.getWorkWithPath("parent/child")
.failed
.futureValue shouldBe a[RuntimeException]
}
}

it("throws an exception if no work can be found") {
val works: List[Work[Merged]] = List(
work(path = "hello/world")
)
withLocalMergedWorksIndex { index =>
insertIntoElasticsearch(index, works: _*)

service(index)
.getWorkWithPath("parent/child")
.failed
.futureValue shouldBe a[RuntimeException]
withLocalMergedWorksIndex {
index =>
insertIntoElasticsearch(index, works: _*)

service(index)
.getWorkWithPath("parent/child")
.failed
.futureValue shouldBe a[RuntimeException]
}
}
}
Expand All @@ -169,26 +194,32 @@ class PathsServiceTest
work(path = "grandparent/parent"),
work(path = "parent/child/grandchild1"),
work(path = "parent/child/grandchild2"),
work(path = "grandparent/parent/child/grandchild3"), // ignored - it has already been resolved up to grandparent
work(path = "child/grandchild3") // ignored - could be child of a different parent
work(path =
"grandparent/parent/child/grandchild3"
), // ignored - it has already been resolved up to grandparent
work(path =
"child/grandchild3"
) // ignored - could be child of a different parent
)
withLocalMergedWorksIndex { index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getChildWorks("grandparent/parent")) {
_ should contain theSameElementsAs List(works(2), works(3))
}
withLocalMergedWorksIndex {
index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getChildWorks("grandparent/parent")) {
_ should contain theSameElementsAs List(works(2), works(3))
}
}
}
it("Returns an empty list if there are no children") {
val works: List[Work[Merged]] = List(
work(path = "grandparent"),
work(path = "grandparent/parent")
)
withLocalMergedWorksIndex { index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getChildWorks("grandparent/parent")) {
_ shouldBe empty
}
withLocalMergedWorksIndex {
index =>
insertIntoElasticsearch(index, works: _*)
whenReady(service(index).getChildWorks("grandparent/parent")) {
_ shouldBe empty
}
}
}
}
Expand Down

0 comments on commit 6380590

Please sign in to comment.