fix(jdbc): label filtering now working properly with key-value match …

…on same object
kestra-io · Dec 21, 2023 · 533f4a1 · 533f4a1
1 parent c75fa9b
commit 533f4a1
Show file tree

Hide file tree

Showing 12 changed files with 38 additions and 50 deletions.
diff --git a/core/src/test/java/io/kestra/core/repositories/AbstractExecutionRepositoryTest.java b/core/src/test/java/io/kestra/core/repositories/AbstractExecutionRepositoryTest.java
@@ -110,7 +110,14 @@ protected void inject(String executionTriggerId) {
                 .build();
         }
 
-        executionRepository.save(builder(State.Type.RUNNING, null).labels(List.of(new Label("key", "value"))).trigger(executionTrigger).build());
+        executionRepository.save(builder(State.Type.RUNNING, null)
+            .labels(List.of(
+                new Label("key", "value"),
+                new Label("key2", "value2")
+            ))
+            .trigger(executionTrigger)
+            .build()
+        );
         for (int i = 1; i < 28; i++) {
             executionRepository.save(builder(
                 i < 5 ? State.Type.RUNNING : (i < 8 ? State.Type.FAILED : State.Type.SUCCESS),
@@ -132,6 +139,9 @@ protected void find() {
 
         executions = executionRepository.find(Pageable.from(1, 10),  null, null, null, null, null, null, null, Map.of("key", "value"), null);
         assertThat(executions.getTotal(), is(1L));
+
+        executions = executionRepository.find(Pageable.from(1, 10),  null, null, null, null, null, null, null, Map.of("key", "value2"), null);
+        assertThat(executions.getTotal(), is(0L));
     }
 
     @Test

diff --git a/core/src/test/java/io/kestra/core/repositories/AbstractFlowRepositoryTest.java b/core/src/test/java/io/kestra/core/repositories/AbstractFlowRepositoryTest.java
@@ -25,6 +25,7 @@
 import io.kestra.core.utils.TestsUtils;
 import io.micronaut.context.event.ApplicationEventListener;
 import io.micronaut.data.model.Pageable;
+import io.micronaut.data.model.Sort;
 import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
 import jakarta.inject.Inject;
 import jakarta.inject.Named;
@@ -36,11 +37,7 @@
 import java.io.IOException;
 import java.net.URISyntaxException;
 import java.time.Duration;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
+import java.util.*;
 import java.util.concurrent.TimeoutException;
 import javax.validation.ConstraintViolationException;
 
@@ -254,13 +251,19 @@ void findByNamespaceWithSource() {
 
     @Test
     void find() {
-        List<Flow> save = flowRepository.find(Pageable.from(1, 10),null, null, "io.kestra.tests", Collections.emptyMap());
-        assertThat((long) save.size(), is(10L));
+        List<Flow> save = flowRepository.find(Pageable.from(1, (int) Helpers.FLOWS_COUNT - 1, Sort.UNSORTED), null, null, null, null);
+        assertThat((long) save.size(), is(Helpers.FLOWS_COUNT - 1));
+
+        save = flowRepository.find(Pageable.from(1, (int) Helpers.FLOWS_COUNT + 1, Sort.UNSORTED), null, null, null, null);
+        assertThat((long) save.size(), is(Helpers.FLOWS_COUNT));
 
         save = flowRepository.find(Pageable.from(1),null, null, "io.kestra.tests.minimal.bis", Collections.emptyMap());
         assertThat((long) save.size(), is(1L));
 
-        save = flowRepository.find(Pageable.from(1),null, null, "io.kestra.tests", Map.of("key1", "value1"));
+        save = flowRepository.find(Pageable.from(1, 100, Sort.UNSORTED), null, null, null, Map.of("country", "FR"));
+        assertThat(save.size(), is(1));
+
+        save = flowRepository.find(Pageable.from(1),null, null, "io.kestra.tests", Map.of("key2", "value2"));
         assertThat((long) save.size(), is(1L));
 
         save = flowRepository.find(Pageable.from(1),null, null, "io.kestra.tests", Map.of("key1", "value2"));

diff --git a/jdbc-h2/src/main/java/io/kestra/repository/h2/H2ExecutionRepositoryService.java b/jdbc-h2/src/main/java/io/kestra/repository/h2/H2ExecutionRepositoryService.java
@@ -20,10 +20,7 @@ public static Condition findCondition(AbstractJdbcRepository<Execution> jdbcRepo
 
         if (labels != null) {
             labels.forEach((key, value) -> {
-                Field<String> keyField = DSL.field("JQ_STRING(\"value\", '.labels[]?.key')", String.class);
-                conditions.add(keyField.eq(key));
-
-                Field<String> valueField = DSL.field("JQ_STRING(\"value\", '.labels[]?.value')", String.class);
+                Field<String> valueField = DSL.field("JQ_STRING(\"value\", '.labels[]? | select(.key == \"" + key + "\") | .value')", String.class);
                 if (value == null) {
                     conditions.add(valueField.isNull());
                 } else {

diff --git a/jdbc-h2/src/main/java/io/kestra/repository/h2/H2FlowRepositoryService.java b/jdbc-h2/src/main/java/io/kestra/repository/h2/H2FlowRepositoryService.java
@@ -21,10 +21,7 @@ public static Condition findCondition(AbstractJdbcRepository<Flow> jdbcRepositor
 
         if (labels != null) {
             labels.forEach((key, value) -> {
-                Field<String> keyField = DSL.field("JQ_STRING(\"value\", '.labels[]?.key')", String.class);
-                conditions.add(keyField.eq(key));
-
-                Field<String> valueField = DSL.field("JQ_STRING(\"value\", '.labels[]?.value')", String.class);
+                Field<String> valueField = DSL.field("JQ_STRING(\"value\", '.labels[]? | select(.key == \"" + key + "\") | .value')", String.class);
                 if (value == null) {
                     conditions.add(valueField.isNull());
                 } else {

diff --git a/jdbc-h2/src/main/java/io/kestra/runner/h2/H2Functions.java b/jdbc-h2/src/main/java/io/kestra/runner/h2/H2Functions.java
@@ -11,20 +11,16 @@
 import net.thisptr.jackson.jq.Versions;
 
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 import java.util.function.Function;
 import java.util.stream.Collectors;
 import java.util.stream.StreamSupport;
 
 public class H2Functions {
-    private static final Scope rootScope;
-    private static final Scope scope;
+    private static final Scope scope = Scope.newEmptyScope();
 
     static {
-        rootScope = Scope.newEmptyScope();
-        BuiltinFunctionLoader.getInstance().loadFunctions(Versions.JQ_1_6, rootScope);
-        scope = Scope.newEmptyScope();
+        BuiltinFunctionLoader.getInstance().loadFunctions(Versions.JQ_1_6, scope);
     }
 
     public static Boolean jqBoolean(String value, String expression) {

diff --git a/.../repository/h2/H2lFlowRepositoryTest.java → ...a/repository/h2/H2FlowRepositoryTest.java b/.../repository/h2/H2lFlowRepositoryTest.java → ...a/repository/h2/H2FlowRepositoryTest.java
@@ -6,7 +6,7 @@
 import java.io.IOException;
 import java.net.URISyntaxException;
 
-public class H2lFlowRepositoryTest extends AbstractJdbcFlowRepositoryTest {
+public class H2FlowRepositoryTest extends AbstractJdbcFlowRepositoryTest {
 
     // On H2 we must reset the database and init the flow repository on the same method.
     // That's why the setup is overridden to do noting and the init will do the setup.

diff --git a/jdbc-h2/src/test/java/io/kestra/runner/h2/H2FunctionsTest.java b/jdbc-h2/src/test/java/io/kestra/runner/h2/H2FunctionsTest.java
@@ -18,6 +18,10 @@ public void jqNull() {
     public void jqString() {
         String jqString = H2Functions.jqString("{\"a\": \"b\"}", ".a");
         assertThat(jqString, is("b"));
+
+        // on arrays, it will use the first element
+        jqString = H2Functions.jqString("{\"labels\":[{\"key\":\"a\",\"value\":\"aValue\"},{\"key\":\"b\",\"value\":\"bValue\"}]}", ".labels[].value");
+        assertThat(jqString, is("aValue"));
     }
 
     @Test

diff --git a/jdbc-mysql/src/main/java/io/kestra/repository/mysql/MysqlExecutionRepositoryService.java b/jdbc-mysql/src/main/java/io/kestra/repository/mysql/MysqlExecutionRepositoryService.java
@@ -21,11 +21,8 @@ public static Condition findCondition(AbstractJdbcRepository<Execution> jdbcRepo
 
         if (labels != null) {
             labels.forEach((key, value) -> {
-                Field<String> keyField = DSL.field("JSON_SEARCH(value, 'one', '" + key + "', NULL, '$.labels[*].key')", String.class);
-                conditions.add(keyField.isNotNull());
-
-                Field<String> valueField = DSL.field("JSON_SEARCH(value, 'one', '" + value + "', NULL, '$.labels[*].value')", String.class);
-                conditions.add(valueField.isNotNull());
+                Field<Boolean> valueField = DSL.field("JSON_CONTAINS(value, JSON_ARRAY(JSON_OBJECT('key', '" + key + "', 'value', '" + value + "')), '$.labels')", Boolean.class);
+                conditions.add(valueField.eq(value != null));
             });
         }
 

diff --git a/jdbc-mysql/src/main/java/io/kestra/repository/mysql/MysqlFlowRepositoryService.java b/jdbc-mysql/src/main/java/io/kestra/repository/mysql/MysqlFlowRepositoryService.java
@@ -18,11 +18,8 @@ public static Condition findCondition(AbstractJdbcRepository<Flow> jdbcRepositor
 
         if (labels != null) {
             labels.forEach((key, value) -> {
-                Field<String> keyField = DSL.field("JSON_SEARCH(value, 'one', '" + key + "', NULL, '$.labels[*].key')", String.class);
-                conditions.add(keyField.isNotNull());
-
-                Field<String> valueField = DSL.field("JSON_SEARCH(value, 'one', '" + value + "', NULL, '$.labels[*].value')", String.class);
-                conditions.add(valueField.isNotNull());
+                Field<Boolean> valueField = DSL.field("JSON_CONTAINS(value, JSON_ARRAY(JSON_OBJECT('key', '" + key + "', 'value', '" + value + "')), '$.labels')", Boolean.class);
+                conditions.add(valueField.eq(value != null));
             });
         }
 

diff --git a/jdbc/src/test/java/io/kestra/jdbc/repository/AbstractJdbcFlowRepositoryTest.java b/jdbc/src/test/java/io/kestra/jdbc/repository/AbstractJdbcFlowRepositoryTest.java
@@ -35,18 +35,6 @@ public abstract class AbstractJdbcFlowRepositoryTest extends io.kestra.core.repo
     @Inject
     protected JooqDSLContextWrapper dslContextWrapper;
 
-    @Test
-    protected void find() {
-        List<Flow> save = flowRepository.find(Pageable.unpaged(), null, null, null, null);
-        assertThat((long) save.size(), is(Helpers.FLOWS_COUNT));
-
-        save = flowRepository.find(Pageable.from(1, 10, Sort.UNSORTED), "trigger-multiplecondition", null, null, null);
-        assertThat((long) save.size(), is(6L));
-
-        save = flowRepository.find(Pageable.from(1, 100, Sort.UNSORTED), null, null, null, Map.of("country", "FR"));
-        assertThat(save.size(), is(1));
-    }
-
     @Test
     void findSourceCode() {
         List<SearchResult<Flow>> search = flowRepository.findSourceCode(Pageable.from(1, 10, Sort.UNSORTED), "io.kestra.core.models.conditions.types.MultipleCondition", null, null);

diff --git a/webserver/src/main/java/io/kestra/webserver/utils/RequestUtils.java b/webserver/src/main/java/io/kestra/webserver/utils/RequestUtils.java
@@ -21,7 +21,7 @@ public static Map<String, String> toMap(List<String> queryString) {
 
                 return new AbstractMap.SimpleEntry<>(
                     split[0],
-                    s.substring(s.indexOf(":") + 1)
+                    s.substring(s.indexOf(":") + 1).trim()
                 );
             })
             .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));

diff --git a/webserver/src/test/java/io/kestra/webserver/controllers/ExecutionControllerTest.java b/webserver/src/test/java/io/kestra/webserver/controllers/ExecutionControllerTest.java
@@ -673,15 +673,14 @@ void find() {
 
         assertThat(executions.getTotal(), is(0L));
 
-        // TODO reactivate when H2 label filtering will be fixed
-        /*triggerInputsFlowExecution(false);
+        triggerInputsFlowExecution(false);
 
         // + is there to simulate that a space was added (this can be the case from UI autocompletion for eg.)
         executions = client.toBlocking().retrieve(
             HttpRequest.GET("/api/v1/executions/search?page=1&size=25&labels=url:+"+ENCODED_URL_LABEL_VALUE), PagedResults.class
         );
 
-        assertThat(executions.getTotal(), is(1L));*/
+        assertThat(executions.getTotal(), is(1L));
     }
 
     @Test