From 2dff686fa0b16ac0ffb3f6907837132dc90a55a6 Mon Sep 17 00:00:00 2001 From: Eike Kettner Date: Mon, 14 Dec 2020 23:07:06 +0100 Subject: [PATCH] Introduce unit condition --- .../docspell/backend/ops/OFulltext.scala | 1 + .../scala/docspell/backend/ops/OItem.scala | 2 + .../docspell/backend/ops/OItemSearch.scala | 2 +- .../joex/notify/NotifyDueItemsTask.scala | 2 + .../scala/docspell/store/qb/Condition.scala | 45 +++++++-- .../main/scala/docspell/store/qb/DSL.scala | 8 +- .../main/scala/docspell/store/qb/Select.scala | 19 ++-- .../store/qb/impl/ConditionBuilder.scala | 68 +++++++++++-- .../store/qb/impl/SelectBuilder.scala | 9 +- .../docspell/store/queries/ItemData.scala | 3 +- .../scala/docspell/store/queries/QItem.scala | 2 +- .../store/queries/QMoveAttachment.scala | 4 +- .../docspell/store/qb/QueryBuilderTest.scala | 2 +- .../store/qb/impl/ConditionBuilderTest.scala | 97 +++++++++++++++++++ 14 files changed, 225 insertions(+), 39 deletions(-) create mode 100644 modules/store/src/test/scala/docspell/store/qb/impl/ConditionBuilderTest.scala diff --git a/modules/backend/src/main/scala/docspell/backend/ops/OFulltext.scala b/modules/backend/src/main/scala/docspell/backend/ops/OFulltext.scala index a9fa7716..d7f416d2 100644 --- a/modules/backend/src/main/scala/docspell/backend/ops/OFulltext.scala +++ b/modules/backend/src/main/scala/docspell/backend/ops/OFulltext.scala @@ -3,6 +3,7 @@ package docspell.backend.ops import cats.effect._ import cats.implicits._ import fs2.Stream + import docspell.backend.JobFactory import docspell.backend.ops.OItemSearch._ import docspell.common._ diff --git a/modules/backend/src/main/scala/docspell/backend/ops/OItem.scala b/modules/backend/src/main/scala/docspell/backend/ops/OItem.scala index 889173d3..bcefe0e5 100644 --- a/modules/backend/src/main/scala/docspell/backend/ops/OItem.scala +++ b/modules/backend/src/main/scala/docspell/backend/ops/OItem.scala @@ -4,6 +4,7 @@ import cats.data.NonEmptyList import cats.data.OptionT import cats.effect.{Effect, Resource} import cats.implicits._ + import docspell.backend.JobFactory import docspell.common._ import docspell.ftsclient.FtsClient @@ -12,6 +13,7 @@ import docspell.store.queries.{QAttachment, QItem, QMoveAttachment} import docspell.store.queue.JobQueue import docspell.store.records._ import docspell.store.{AddResult, Store} + import doobie.implicits._ import org.log4s.getLogger diff --git a/modules/backend/src/main/scala/docspell/backend/ops/OItemSearch.scala b/modules/backend/src/main/scala/docspell/backend/ops/OItemSearch.scala index 67d9086f..ccfe6230 100644 --- a/modules/backend/src/main/scala/docspell/backend/ops/OItemSearch.scala +++ b/modules/backend/src/main/scala/docspell/backend/ops/OItemSearch.scala @@ -7,9 +7,9 @@ import fs2.Stream import docspell.backend.ops.OItemSearch._ import docspell.common._ +import docspell.store._ import docspell.store.queries.{QAttachment, QItem} import docspell.store.records._ -import docspell.store._ import bitpeace.{FileMeta, RangeDef} import doobie.implicits._ diff --git a/modules/joex/src/main/scala/docspell/joex/notify/NotifyDueItemsTask.scala b/modules/joex/src/main/scala/docspell/joex/notify/NotifyDueItemsTask.scala index 1819fac1..78ec0882 100644 --- a/modules/joex/src/main/scala/docspell/joex/notify/NotifyDueItemsTask.scala +++ b/modules/joex/src/main/scala/docspell/joex/notify/NotifyDueItemsTask.scala @@ -3,12 +3,14 @@ package docspell.joex.notify import cats.data.OptionT import cats.effect._ import cats.implicits._ + import docspell.backend.ops.OItemSearch.{Batch, ListItem, Query} import docspell.common._ import docspell.joex.mail.EmilHeader import docspell.joex.scheduler.{Context, Task} import docspell.store.queries.QItem import docspell.store.records._ + import emil._ import emil.builder._ import emil.javamail.syntax._ diff --git a/modules/store/src/main/scala/docspell/store/qb/Condition.scala b/modules/store/src/main/scala/docspell/store/qb/Condition.scala index 2a1f3097..0b0c0692 100644 --- a/modules/store/src/main/scala/docspell/store/qb/Condition.scala +++ b/modules/store/src/main/scala/docspell/store/qb/Condition.scala @@ -7,6 +7,9 @@ import doobie._ sealed trait Condition object Condition { + case object UnitCondition extends Condition + + val unit: Condition = UnitCondition case class CompareVal[A](column: Column[A], op: Operator, value: A)(implicit val P: Put[A] @@ -26,36 +29,58 @@ object Condition { case class IsNull(col: Column[_]) extends Condition - case class And(c: Condition, cs: Vector[Condition]) extends Condition { + case class And(inner: NonEmptyList[Condition]) extends Condition { def append(other: Condition): And = other match { - case And(oc, ocs) => - And(c, cs ++ (oc +: ocs)) + case And(otherInner) => + And(inner.concatNel(otherInner)) case _ => - And(c, cs :+ other) + And(inner.append(other)) } } object And { def apply(c: Condition, cs: Condition*): And = - And(c, cs.toVector) + And(NonEmptyList(c, cs.toList)) + object Inner extends InnerCondition { + def unapply(node: Condition): Option[NonEmptyList[Condition]] = + node match { + case n: And => + Option(n.inner) + case _ => + None + } + } } - case class Or(c: Condition, cs: Vector[Condition]) extends Condition { + case class Or(inner: NonEmptyList[Condition]) extends Condition { def append(other: Condition): Or = other match { - case Or(oc, ocs) => - Or(c, cs ++ (oc +: ocs)) + case Or(otherInner) => + Or(inner.concatNel(otherInner)) case _ => - Or(c, cs :+ other) + Or(inner.append(other)) } } object Or { def apply(c: Condition, cs: Condition*): Or = - Or(c, cs.toVector) + Or(NonEmptyList(c, cs.toList)) + + object Inner extends InnerCondition { + def unapply(node: Condition): Option[NonEmptyList[Condition]] = + node match { + case n: Or => + Option(n.inner) + case _ => + None + } + } } case class Not(c: Condition) extends Condition object Not {} + trait InnerCondition { + def unapply(node: Condition): Option[NonEmptyList[Condition]] + } } diff --git a/modules/store/src/main/scala/docspell/store/qb/DSL.scala b/modules/store/src/main/scala/docspell/store/qb/DSL.scala index 97c80a2a..fe3bda42 100644 --- a/modules/store/src/main/scala/docspell/store/qb/DSL.scala +++ b/modules/store/src/main/scala/docspell/store/qb/DSL.scala @@ -97,15 +97,15 @@ trait DSL extends DoobieMeta { case a: Condition.And => cs.foldLeft(a)(_.append(_)) case _ => - Condition.And(c, cs.toVector) + Condition.And(c, cs: _*) } def or(c: Condition, cs: Condition*): Condition = c match { - case Condition.Or(head, tail) => - Condition.Or(head, tail ++ (c +: cs.toVector)) + case o: Condition.Or => + cs.foldLeft(o)(_.append(_)) case _ => - Condition.Or(c, cs.toVector) + Condition.Or(c, cs: _*) } def not(c: Condition): Condition = diff --git a/modules/store/src/main/scala/docspell/store/qb/Select.scala b/modules/store/src/main/scala/docspell/store/qb/Select.scala index 155109a8..ca64d218 100644 --- a/modules/store/src/main/scala/docspell/store/qb/Select.scala +++ b/modules/store/src/main/scala/docspell/store/qb/Select.scala @@ -46,35 +46,35 @@ sealed trait Select { object Select { def apply(projection: Nel[SelectExpr], from: FromExpr) = - SimpleSelect(false, projection, from, None, None) + SimpleSelect(false, projection, from, Condition.unit, None) def apply(projection: SelectExpr, from: FromExpr) = - SimpleSelect(false, Nel.of(projection), from, None, None) + SimpleSelect(false, Nel.of(projection), from, Condition.unit, None) def apply( projection: Nel[SelectExpr], from: FromExpr, where: Condition - ) = SimpleSelect(false, projection, from, Some(where), None) + ) = SimpleSelect(false, projection, from, where, None) def apply( projection: SelectExpr, from: FromExpr, where: Condition - ) = SimpleSelect(false, Nel.of(projection), from, Some(where), None) + ) = SimpleSelect(false, Nel.of(projection), from, where, None) def apply( projection: Nel[SelectExpr], from: FromExpr, where: Condition, groupBy: GroupBy - ) = SimpleSelect(false, projection, from, Some(where), Some(groupBy)) + ) = SimpleSelect(false, projection, from, where, Some(groupBy)) case class SimpleSelect( distinctFlag: Boolean, projection: Nel[SelectExpr], from: FromExpr, - where: Option[Condition], + where: Condition, groupBy: Option[GroupBy] ) extends Select { def group(gb: GroupBy): SimpleSelect = @@ -84,9 +84,10 @@ object Select { copy(distinctFlag = true) def where(c: Option[Condition]): SimpleSelect = - copy(where = c) + where(c.getOrElse(Condition.unit)) + def where(c: Condition): SimpleSelect = - copy(where = Some(c)) + copy(where = c) def appendSelect(e: SelectExpr): SimpleSelect = copy(projection = projection.append(e)) @@ -95,7 +96,7 @@ object Select { copy(from = f(from)) def changeWhere(f: Condition => Condition): SimpleSelect = - copy(where = where.map(f)) + copy(where = f(where)) def orderBy(ob: OrderBy, obs: OrderBy*): Select = Ordered(this, ob, obs.toVector) diff --git a/modules/store/src/main/scala/docspell/store/qb/impl/ConditionBuilder.scala b/modules/store/src/main/scala/docspell/store/qb/impl/ConditionBuilder.scala index cc700b18..3a4e4ede 100644 --- a/modules/store/src/main/scala/docspell/store/qb/impl/ConditionBuilder.scala +++ b/modules/store/src/main/scala/docspell/store/qb/impl/ConditionBuilder.scala @@ -1,5 +1,7 @@ package docspell.store.qb.impl +import cats.data.NonEmptyList + import docspell.store.qb._ import _root_.doobie.implicits._ @@ -12,8 +14,55 @@ object ConditionBuilder { val parenOpen = Fragment.const0("(") val parenClose = Fragment.const0(")") - def build(expr: Condition): Fragment = - expr match { + final def reduce(c: Condition): Condition = + c match { + case Condition.And(inner) => + NonEmptyList.fromList(flatten(inner.toList, Condition.And.Inner)) match { + case Some(rinner) => + if (rinner.tail.isEmpty) reduce(rinner.head) + else Condition.And(rinner.reverse.map(reduce)) + case None => + Condition.unit + } + + case Condition.Or(inner) => + NonEmptyList.fromList(flatten(inner.toList, Condition.Or.Inner)) match { + case Some(rinner) => + if (rinner.tail.isEmpty) reduce(rinner.head) + else Condition.Or(rinner.reverse.map(reduce)) + case None => + Condition.unit + } + + case Condition.Not(Condition.UnitCondition) => + Condition.unit + + case Condition.Not(Condition.Not(inner)) => + reduce(inner) + + case _ => + c + } + + private def flatten( + els: List[Condition], + nodePattern: Condition.InnerCondition, + result: List[Condition] = Nil + ): List[Condition] = + els match { + case Nil => + result + case nodePattern(more) :: tail => + val spliced = flatten(more.toList, nodePattern, result) + flatten(tail, nodePattern, spliced) + case Condition.UnitCondition :: tail => + flatten(tail, nodePattern, result) + case h :: tail => + flatten(tail, nodePattern, h :: result) + } + + final def build(expr: Condition): Fragment = + reduce(expr) match { case c @ Condition.CompareVal(col, op, value) => val opFrag = operator(op) val valFrag = buildValue(value)(c.P) @@ -58,14 +107,14 @@ object ConditionBuilder { case Condition.IsNull(col) => SelectExprBuilder.column(col) ++ fr" is null" - case Condition.And(c, cs) => - val inner = cs.prepended(c).map(build).reduce(_ ++ and ++ _) - if (cs.isEmpty) inner + case Condition.And(ands) => + val inner = ands.map(build).reduceLeft(_ ++ and ++ _) + if (ands.tail.isEmpty) inner else parenOpen ++ inner ++ parenClose - case Condition.Or(c, cs) => - val inner = cs.prepended(c).map(build).reduce(_ ++ or ++ _) - if (cs.isEmpty) inner + case Condition.Or(ors) => + val inner = ors.map(build).reduceLeft(_ ++ or ++ _) + if (ors.tail.isEmpty) inner else parenOpen ++ inner ++ parenClose case Condition.Not(Condition.IsNull(col)) => @@ -73,6 +122,9 @@ object ConditionBuilder { case Condition.Not(c) => fr"NOT" ++ build(c) + + case Condition.UnitCondition => + Fragment.empty } def operator(op: Operator): Fragment = diff --git a/modules/store/src/main/scala/docspell/store/qb/impl/SelectBuilder.scala b/modules/store/src/main/scala/docspell/store/qb/impl/SelectBuilder.scala index 68743737..c6b92a7d 100644 --- a/modules/store/src/main/scala/docspell/store/qb/impl/SelectBuilder.scala +++ b/modules/store/src/main/scala/docspell/store/qb/impl/SelectBuilder.scala @@ -47,7 +47,7 @@ object SelectBuilder { def buildSimple(sq: Select.SimpleSelect): Fragment = { val f0 = sq.projection.map(selectExpr).reduceLeft(_ ++ comma ++ _) val f1 = fromExpr(sq.from) - val f2 = sq.where.map(cond).getOrElse(Fragment.empty) + val f2 = cond(sq.where) val f3 = sq.groupBy.map(groupBy).getOrElse(Fragment.empty) f0 ++ f1 ++ f2 ++ f3 } @@ -70,7 +70,12 @@ object SelectBuilder { FromExprBuilder.build(fr) def cond(c: Condition): Fragment = - fr" WHERE" ++ ConditionBuilder.build(c) + c match { + case Condition.UnitCondition => + Fragment.empty + case _ => + fr" WHERE" ++ ConditionBuilder.build(c) + } def groupBy(gb: GroupBy): Fragment = { val f0 = gb.names.prepended(gb.name).map(selectExpr).reduce(_ ++ comma ++ _) diff --git a/modules/store/src/main/scala/docspell/store/queries/ItemData.scala b/modules/store/src/main/scala/docspell/store/queries/ItemData.scala index 0774f9cb..d96d726e 100644 --- a/modules/store/src/main/scala/docspell/store/queries/ItemData.scala +++ b/modules/store/src/main/scala/docspell/store/queries/ItemData.scala @@ -1,9 +1,10 @@ package docspell.store.queries -import bitpeace.FileMeta import docspell.common._ import docspell.store.records._ +import bitpeace.FileMeta + case class ItemData( item: RItem, corrOrg: Option[ROrganization], diff --git a/modules/store/src/main/scala/docspell/store/queries/QItem.scala b/modules/store/src/main/scala/docspell/store/queries/QItem.scala index 847a4bba..1d82714d 100644 --- a/modules/store/src/main/scala/docspell/store/queries/QItem.scala +++ b/modules/store/src/main/scala/docspell/store/queries/QItem.scala @@ -13,8 +13,8 @@ import docspell.store.qb.DSL._ import docspell.store.qb._ import docspell.store.records._ -import doobie.{Query => _, _} import doobie.implicits._ +import doobie.{Query => _, _} import org.log4s._ object QItem { diff --git a/modules/store/src/main/scala/docspell/store/queries/QMoveAttachment.scala b/modules/store/src/main/scala/docspell/store/queries/QMoveAttachment.scala index c3463cb5..8992065e 100644 --- a/modules/store/src/main/scala/docspell/store/queries/QMoveAttachment.scala +++ b/modules/store/src/main/scala/docspell/store/queries/QMoveAttachment.scala @@ -1,14 +1,14 @@ package docspell.store.queries -import cats.effect._ import cats.data.OptionT +import cats.effect._ import cats.implicits._ import docspell.common._ import docspell.store.records._ -import doobie.{Query => _, _} import doobie.implicits._ +import doobie.{Query => _, _} object QMoveAttachment { def moveAttachmentBefore( diff --git a/modules/store/src/test/scala/docspell/store/qb/QueryBuilderTest.scala b/modules/store/src/test/scala/docspell/store/qb/QueryBuilderTest.scala index 7d5dc64d..edbe8769 100644 --- a/modules/store/src/test/scala/docspell/store/qb/QueryBuilderTest.scala +++ b/modules/store/src/test/scala/docspell/store/qb/QueryBuilderTest.scala @@ -65,7 +65,7 @@ object QueryBuilderTest extends SimpleTestSuite { fail("Unexpected result") } assertEquals(group, None) - assert(where.isDefined) + assert(where != Condition.unit) case _ => fail("Unexpected case") } diff --git a/modules/store/src/test/scala/docspell/store/qb/impl/ConditionBuilderTest.scala b/modules/store/src/test/scala/docspell/store/qb/impl/ConditionBuilderTest.scala new file mode 100644 index 00000000..1e035f71 --- /dev/null +++ b/modules/store/src/test/scala/docspell/store/qb/impl/ConditionBuilderTest.scala @@ -0,0 +1,97 @@ +package docspell.store.qb.impl + +import minitest._ +import docspell.store.qb._ +import docspell.store.qb.DSL._ +import docspell.store.qb.model.{CourseRecord, PersonRecord} + +object ConditionBuilderTest extends SimpleTestSuite { + + val c = CourseRecord.as("c") + val p = PersonRecord.as("p") + + test("reduce ands") { + val cond = + c.lessons > 3 && (c.id === 5L && (p.name === "john" && Condition.unit && p.id === 1L)) + val expected = + and(c.lessons > 3, c.id === 5L, p.name === "john", p.id === 1L) + + assertEquals(ConditionBuilder.reduce(cond), expected) + assertEquals(ConditionBuilder.reduce(expected), expected) + } + + test("reduce ors") { + val cond = + c.lessons > 3 || (c.id === 5L || (p.name === "john" || Condition.unit || p.id === 1L)) + val expected = + or(c.lessons > 3, c.id === 5L, p.name === "john", p.id === 1L) + + assertEquals(ConditionBuilder.reduce(cond), expected) + assertEquals(ConditionBuilder.reduce(expected), expected) + } + + test("mixed and / or") { + val cond = c.lessons > 3 && (p.name === "john" || p.name === "mara") && c.id > 3 + val expected = + and(c.lessons > 3, or(p.name === "john", p.name === "mara"), c.id > 3) + assertEquals(ConditionBuilder.reduce(cond), expected) + assertEquals(ConditionBuilder.reduce(expected), expected) + } + + test("reduce double not") { + val cond = Condition.Not(Condition.Not(c.name === "scala")) + assertEquals(ConditionBuilder.reduce(cond), c.name === "scala") + } + + test("reduce triple not") { + val cond = Condition.Not(Condition.Not(Condition.Not(c.name === "scala"))) + assertEquals(ConditionBuilder.reduce(cond), not(c.name === "scala")) + } + + test("reduce not to unit") { + val cond = Condition.Not(Condition.Not(Condition.Not(Condition.Not(Condition.unit)))) + assertEquals(ConditionBuilder.reduce(cond), Condition.unit) + } + + test("remove units in and/or") { + val cond = + c.name === "scala" && Condition.unit && (c.name === "fp" || Condition.unit) && Condition.unit + assertEquals(ConditionBuilder.reduce(cond), and(c.name === "scala", c.name === "fp")) + } + + test("unwrap single and/ors") { + assertEquals( + ConditionBuilder.reduce(Condition.Or(c.name === "scala")), + c.name === "scala" + ) + assertEquals( + ConditionBuilder.reduce(Condition.And(c.name === "scala")), + c.name === "scala" + ) + + assertEquals( + ConditionBuilder.reduce(Condition.unit && c.name === "scala" && Condition.unit), + c.name === "scala" + ) + assertEquals( + ConditionBuilder.reduce(Condition.unit || c.name === "scala" || Condition.unit), + c.name === "scala" + ) + + assertEquals( + ConditionBuilder.reduce(and(and(and(c.name === "scala"), Condition.unit))), + c.name === "scala" + ) + } + + test("reduce empty and/or") { + assertEquals( + ConditionBuilder.reduce(Condition.unit && Condition.unit && Condition.unit), + Condition.unit + ) + assertEquals( + ConditionBuilder.reduce(Condition.unit || Condition.unit || Condition.unit), + Condition.unit + ) + } +}