[GitHub] dubee closed pull request #3588: Make count with skip to work with CouchDB

[GitBox <gi...@apache.org>]

dubee closed pull request #3588: Make count with skip to work with CouchDB
URL: https://github.com/apache/incubator-openwhisk/pull/3588
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/common/scala/src/main/scala/whisk/core/database/CouchDbRestStore.scala b/common/scala/src/main/scala/whisk/core/database/CouchDbRestStore.scala
index a2f63e1c8d..0d97077a25 100644
--- a/common/scala/src/main/scala/whisk/core/database/CouchDbRestStore.scala
+++ b/common/scala/src/main/scala/whisk/core/database/CouchDbRestStore.scala
@@ -258,6 +258,8 @@ class CouchDbRestStore[DocumentAbstraction <: DocumentSerializer](dbProtocol: St
                                      stale: StaleParameter)(implicit transid: TransactionId): Future[List[JsObject]] = {
 
     require(!(reduce && includeDocs), "reduce and includeDocs cannot both be true")
+    require(skip >= 0, "skip should be non negative")
+    require(limit >= 0, "limit should be non negative")
 
     // Apparently you have to do that in addition to setting "descending"
     val (realStartKey, realEndKey) = if (descending) {
@@ -309,25 +311,22 @@ class CouchDbRestStore[DocumentAbstraction <: DocumentSerializer](dbProtocol: St
 
   protected[core] def count(table: String, startKey: List[Any], endKey: List[Any], skip: Int, stale: StaleParameter)(
     implicit transid: TransactionId): Future[Long] = {
+    require(skip >= 0, "skip should be non negative")
 
     val Array(firstPart, secondPart) = table.split("/")
 
     val start = transid.started(this, LoggingMarkers.DATABASE_QUERY, s"[COUNT] '$dbName' searching '$table")
 
     val f = client
-      .executeView(firstPart, secondPart)(
-        startKey = startKey,
-        endKey = endKey,
-        skip = Some(skip),
-        stale = stale,
-        reduce = true)
+      .executeView(firstPart, secondPart)(startKey = startKey, endKey = endKey, stale = stale, reduce = true)
       .map {
         case Right(response) =>
           val rows = response.fields("rows").convertTo[List[JsObject]]
 
-          val out = if (!rows.isEmpty) {
+          val out = if (rows.nonEmpty) {
             assert(rows.length == 1, s"result of reduced view contains more than one value: '$rows'")
-            rows.head.fields("value").convertTo[Long]
+            val count = rows.head.fields("value").convertTo[Long]
+            if (count > skip) count - skip else 0L
           } else 0L
 
           transid.finished(this, start, s"[COUNT] '$dbName' completed: count $out")
diff --git a/common/scala/src/main/scala/whisk/core/database/memory/MemoryArtifactStore.scala b/common/scala/src/main/scala/whisk/core/database/memory/MemoryArtifactStore.scala
index 1f4b2b3426..0b96d06a6a 100644
--- a/common/scala/src/main/scala/whisk/core/database/memory/MemoryArtifactStore.scala
+++ b/common/scala/src/main/scala/whisk/core/database/memory/MemoryArtifactStore.scala
@@ -187,6 +187,9 @@ class MemoryArtifactStore[DocumentAbstraction <: DocumentSerializer](dbName: Str
                                      stale: StaleParameter)(implicit transid: TransactionId): Future[List[JsObject]] = {
     require(!(reduce && includeDocs), "reduce and includeDocs cannot both be true")
     require(!reduce, "Reduce scenario not supported") //TODO Investigate reduce
+    require(skip >= 0, "skip should be non negative")
+    require(limit >= 0, "limit should be non negative")
+
     documentHandler.checkIfTableSupported(table)
 
     val Array(ddoc, viewName) = table.split("/")
diff --git a/tests/src/test/scala/whisk/core/database/test/behavior/ArtifactStoreQueryBehaviors.scala b/tests/src/test/scala/whisk/core/database/test/behavior/ArtifactStoreQueryBehaviors.scala
index 1f6ed9c942..5b63602d48 100644
--- a/tests/src/test/scala/whisk/core/database/test/behavior/ArtifactStoreQueryBehaviors.scala
+++ b/tests/src/test/scala/whisk/core/database/test/behavior/ArtifactStoreQueryBehaviors.scala
@@ -183,6 +183,23 @@ trait ArtifactStoreQueryBehaviors extends ArtifactStoreBehaviorBase {
     result.map(js => JsObject(getJsObject(js, "doc").fields - "_rev")) shouldBe activations.map(_.toDocumentRecord)
   }
 
+  it should "throw exception for negative limits and skip" in {
+    implicit val tid: TransactionId = transid()
+    a[IllegalArgumentException] should be thrownBy query[WhiskActivation](
+      activationStore,
+      WhiskActivation.filtersView.name,
+      List("foo", 0),
+      List("foo", TOP, TOP),
+      limit = -1)
+
+    a[IllegalArgumentException] should be thrownBy query[WhiskActivation](
+      activationStore,
+      WhiskActivation.filtersView.name,
+      List("foo", 0),
+      List("foo", TOP, TOP),
+      skip = -1)
+  }
+
   behavior of s"${storeType}ArtifactStore count"
 
   it should "should match all created activations" in {
@@ -203,8 +220,7 @@ trait ArtifactStoreQueryBehaviors extends ArtifactStoreBehaviorBase {
     result shouldBe 10
   }
 
-  it should "count with skip" ignore {
-    //TODO Skip is not working for CouchDB
+  it should "count with skip" in {
     implicit val tid: TransactionId = transid()
 
     val ns = newNS()
@@ -221,6 +237,25 @@ trait ArtifactStoreQueryBehaviors extends ArtifactStoreBehaviorBase {
       skip = 4)
 
     result shouldBe 10 - 4
+
+    val result2 = count[WhiskActivation](
+      activationStore,
+      WhiskActivation.filtersView.name,
+      List(entityPath, 0),
+      List(entityPath, TOP, TOP),
+      skip = 1000)
+
+    result2 shouldBe 0
+  }
+
+  it should "throw exception for negative skip" in {
+    implicit val tid: TransactionId = transid()
+    a[IllegalArgumentException] should be thrownBy count[WhiskActivation](
+      activationStore,
+      WhiskActivation.filtersView.name,
+      List("foo", 0),
+      List("foo", TOP, TOP),
+      skip = -1)
   }
 
   private def dropRev(js: JsObject): JsObject = {


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services