fix(SqlExecutionRepository): fixed bug in sql repository in orca-sql …

[juangod-wise]

This fixes two bugs in the selectExecutions method of the orca-sql/src/main/kotlin/com/netflix/spinnaker/orca/sql/pipeline/persistence/SqlExecutionRepository.kt file. These were probably never found because they are only used for some cleanup job: OldPipelineCleanupPollingNotificationAgent.

[juangod-wise]

For the case in which the cursor is null (it always starts as null in the first iteration), the custom where statements that are passed to the function are not used on the query, effectively not filtering the first page.

[juangod-wise]

The pagination is broken, which makes it only show the first page, for example, in the unit tests on orca-sql/src/test/groovy/com/netflix/spinnaker/orca/sql/cleanup/OldPipelineCleanupPollingNotificationAgentSpec.groovy, we have a page size 10, and executions with id: 1, 2, 11, 12, ..., 20

Then, iteration 1 returns the first 10 elements in desc order:

--> 20, ..., 11 (10 elements)

And cursor = 11

Then the second page gets the elements greater than the cursor, which are

--> 20, ..., 12 (9 elements)

Then the stop condition in the PagedIterator is triggered:

isLastPage = currentPage.size < pageSize

Thus, the second page is never returned and only 10 elements are present, when there should be 12.

When we change greater than, for lesser than, we get:

Iteration 1:

--> 20, ..., 11 (10 elements)
Cursor = 11

Iteration 2, elements lesser than 11:

--> 1, 2

Then we get the correct response.

[dbyron-sf]

Isn't SqlExecutionRepository.selectExecutions also called from TaskController.list via SqlExecutionRepository.retrieve ?

[dbyron-sf]

Thanks for this @juangod-wise! I tried reverting back to gt and happily see some test failures. With lt and reverting the first change that checks where for null, the tests still pass. Can you think of a test to add that fails without the where null check?

[dbyron-sf]

I came up with one. OK if I push?

diff --git a/orca-sql/src/test/kotlin/com/netflix/spinnaker/orca/sql/pipeline/persistence/SqlExecutionRepositoryTest.kt b/orca-sql/src/test/kotlin/com/netflix/spinnaker/orca/sql/pipeline/persistence/SqlExecutionRepositoryTest.kt
index d0007e87d..e48209448 100644
--- a/orca-sql/src/test/kotlin/com/netflix/spinnaker/orca/sql/pipeline/persistence/SqlExecutionRepositoryTest.kt
+++ b/orca-sql/src/test/kotlin/com/netflix/spinnaker/orca/sql/pipeline/persistence/SqlExecutionRepositoryTest.kt
@@ -244,6 +244,21 @@ class SqlExecutionRepositoryTest : JUnit5Minutests {
         assertThat(actualPipelineExecution).isEqualTo(pipelineExecution)
       }
     }
+
+    context("retrievePipelinesForApplication") {
+      val pipelineExecution1 = PipelineExecutionImpl(ExecutionType.PIPELINE, "application-1")
+      val pipelineExecution2 = PipelineExecutionImpl(ExecutionType.PIPELINE, "application-2")
+
+      test("correctly use where clause") {
+        // Store pipelines in two different applications
+        sqlExecutionRepository.store(pipelineExecution1)
+        sqlExecutionRepository.store(pipelineExecution2)
+
+        val observable = sqlExecutionRepository.retrievePipelinesForApplication("application-2")
+        val executions = observable.toList().toBlocking().single()
+        assertThat(executions.map(PipelineExecution::getApplication).single()).isEqualTo("application-2")
+      }
+    }
   }
 
   private inner class Fixture {

[juangod-wise]

I came up with one. OK if I push?

Good stuff! Please do!

[juangod-wise]

Isn't SqlExecutionRepository.selectExecutions also called from TaskController.list via SqlExecutionRepository.retrieve ?

Yes it looks like it is also used in TaskController

[dbyron-sf]

I came up with one. OK if I push?

Good stuff! Please do!

42081af

[dbyron-sf]

@Mergifyio backport release-1.32.x release-1.33.x

[mergify]

backport release-1.32.x release-1.33.x

✅ Backports have been created

• #4701 fix(SqlExecutionRepository): fixed bug in sql repository in orca-sql … (backport #4697) has been created for branch release-1.32.x but encountered conflicts
• #4702 fix(SqlExecutionRepository): fixed bug in sql repository in orca-sql … (backport #4697) has been created for branch release-1.33.x but encountered conflicts