fix: the Criterion Transformer correctly handles the rightOperand (#3320

) * fix: the Criterion Transformer correctly handles the rightOperand * DEPENDENCIES * checkstyle * fix tests * pr remarks, improve in-mem predicate generation
eclipse-edc · Jul 21, 2023 · e8ddc6f · e8ddc6f
1 parent 5e8aaa0
commit e8ddc6f
Show file tree

Hide file tree

Showing 8 changed files with 132 additions and 34 deletions.
diff --git a/.../java/org/eclipse/edc/core/transform/transformer/to/JsonObjectToCriterionTransformer.java b/.../java/org/eclipse/edc/core/transform/transformer/to/JsonObjectToCriterionTransformer.java
@@ -45,7 +45,7 @@ public JsonObjectToCriterionTransformer() {
         if ("in".equals(operator)) {
             builder.operandRight(operandRight.asJsonArray().stream().map(this::nodeValue).collect(toList()));
         } else {
-            builder.operandRight(transformString(operandRight, context));
+            builder.operandRight(transformGenericProperty(operandRight, context));
         }
 
         return builder.build();

diff --git a/...a/org/eclipse/edc/core/transform/transformer/to/JsonObjectToCriterionTransformerTest.java b/...a/org/eclipse/edc/core/transform/transformer/to/JsonObjectToCriterionTransformerTest.java
@@ -77,4 +77,26 @@ void transform_shouldConsiderRightAsList_whenOperatorIsIn() {
         assertThat(result.getOperator()).isEqualTo("in");
         assertThat(result.getOperandRight()).isInstanceOf(List.class).asList().containsExactly("bar", "baz");
     }
+
+    @Test
+    void transform_rightOperandIsNumber() {
+        var context = mock(TransformerContext.class);
+        when(context.transform(any(JsonValue.class), eq(Object.class))).thenAnswer(a -> genericTypeTransformer.transform(a.getArgument(0), context));
+        when(context.problem()).thenReturn(mock());
+        var json = Json.createObjectBuilder()
+                .add(JsonLdKeywords.TYPE, CRITERION_TYPE)
+                .add(CRITERION_OPERAND_LEFT, "foo")
+                .add(CRITERION_OPERATOR, "=")
+                .add(CRITERION_OPERAND_RIGHT, 42)
+                .build();
+
+        var result = transformer.transform(getExpanded(json), context);
+
+        assertThat(result).isNotNull();
+        assertThat(result.getOperandRight()).satisfies(obj -> {
+            assertThat(obj).isInstanceOf(Double.class);
+            // must cast to engage the AbstractDoubleAssert
+            assertThat((Double) obj).isEqualTo(42);
+        });
+    }
 }
diff --git a/...or/api/management/transferprocess/transform/JsonObjectFromTransferProcessTransformer.java b/...or/api/management/transferprocess/transform/JsonObjectFromTransferProcessTransformer.java
@@ -66,11 +66,11 @@ public JsonObjectFromTransferProcessTransformer(JsonBuilderFactory builderFactor
                 .add(TRANSFER_PROCESS_STATE_TIMESTAMP, input.getStateTimestamp())
                 .add(TRANSFER_PROCESS_TYPE_TYPE, input.getType().name())
                 .add(TRANSFER_PROCESS_ASSET_ID, input.getAssetId())
-                .add(TRANSFER_PROCESS_CONNECTOR_ID, input.getConnectorId())
                 .add(TRANSFER_PROCESS_CONTRACT_ID, input.getContractId())
                 .add(TRANSFER_PROCESS_CALLBACK_ADDRESSES, callbackAddresses)
                 .add(TRANSFER_PROCESS_DATA_DESTINATION, dataDestination);
 
+        Optional.ofNullable(input.getConnectorId()).ifPresent(it -> builder.add(TRANSFER_PROCESS_CONNECTOR_ID, it));
         Optional.ofNullable(input.getErrorDetail()).ifPresent(it -> builder.add(TRANSFER_PROCESS_ERROR_DETAIL, it));
 
         return builder.build();

diff --git a/...org/eclipse/edc/connector/store/sql/transferprocess/PostgresTransferProcessStoreTest.java b/...org/eclipse/edc/connector/store/sql/transferprocess/PostgresTransferProcessStoreTest.java
@@ -171,6 +171,7 @@ void find_queryByLease() {
 
     }
 
+
     @Test
     void create_withoutDataRequest_throwsException() {
         var t1 = TestFunctions.createTransferProcessBuilder("id1")

diff --git a/...n/core-spi/src/main/java/org/eclipse/edc/spi/query/BaseCriterionToPredicateConverter.java b/...n/core-spi/src/main/java/org/eclipse/edc/spi/query/BaseCriterionToPredicateConverter.java
@@ -45,7 +45,7 @@ public Predicate<T> convert(Criterion criterion) {
     /**
      * Method to extract an object's field's value
      *
-     * @param key Then name of the field
+     * @param key    Then name of the field
      * @param object The target object
      */
     protected abstract Object property(String key, Object object);
@@ -63,6 +63,11 @@ private Predicate<T> equalPredicate(Criterion criterion) {
                 return Objects.equals(enumProperty.name(), criterion.getOperandRight());
             }
 
+            if (property instanceof Number c1 && criterion.getOperandRight() instanceof Number c2) {
+                // interpret as double to not lose any precision
+                return Double.compare(c1.doubleValue(), c2.doubleValue()) == 0;
+            }
+
             return Objects.equals(property, criterion.getOperandRight());
         };
     }
@@ -74,8 +79,7 @@ private Predicate<T> inPredicate(Criterion criterion) {
 
             var rightOp = criterion.getOperandRight();
 
-            if (rightOp instanceof Iterable) {
-                var iterable = (Iterable<?>) rightOp;
+            if (rightOp instanceof Iterable<?> iterable) {
                 for (var value : iterable) {
                     if (value.equals(property)) {
                         return true;

diff --git a/...re-spi/src/test/java/org/eclipse/edc/spi/query/BaseCriterionToPredicateConverterTest.java b/...re-spi/src/test/java/org/eclipse/edc/spi/query/BaseCriterionToPredicateConverterTest.java
@@ -63,6 +63,19 @@ void convertEqual_enumShouldCheckEntryName() {
                 .rejects(new TestObject("any", ENTRY1), new TestObject("any", null));
     }
 
+    @Test
+    void convertEqual_integerAndDouble() {
+        var predicate = converter.convert(new Criterion("intValue", "=", 42));
+
+        assertThat(predicate)
+                .rejects(new TestObject("any", ENTRY2), new TestObject(-1))
+                .accepts(new TestObject(42));
+    }
+
+    public enum TestEnum {
+        ENTRY1, ENTRY2
+    }
+
     private static class TestCriterionToPredicateConverter extends BaseCriterionToPredicateConverter<TestObject> {
 
         @Override
@@ -72,27 +85,22 @@ protected Object property(String key, Object object) {
     }
 
     private static class TestObject {
-        private final String value;
-        private final TestEnum enumValue;
+        private String value;
+        private TestEnum enumValue;
+        private Integer intValue;
 
         private TestObject(String value) {
-            this(value, TestEnum.ENTRY1);
+            this(value, null);
+        }
+
+        private TestObject(int value) {
+            this.intValue = value;
         }
 
         private TestObject(String value, TestEnum enumValue) {
             this.value = value;
             this.enumValue = enumValue;
-        }
 
-        @Override
-        public String toString() {
-            return "TestObject{" +
-                    "value='" + value + '\'' +
-                    '}';
         }
     }
-
-    public enum TestEnum {
-        ENTRY1, ENTRY2;
-    }
 }
diff --git a/...g/eclipse/edc/connector/transfer/spi/testfixtures/store/TransferProcessStoreTestBase.java b/...g/eclipse/edc/connector/transfer/spi/testfixtures/store/TransferProcessStoreTestBase.java
@@ -68,6 +68,20 @@ public TransferProcessStoreTestBase() {
         System.setProperty("transferprocessstore.supports.collectionQuery", String.valueOf(supportsCollectionQuery()));
     }
 
+    @Test
+    void find_queryByState() {
+        var tp = createTransferProcessBuilder("testprocess1").state(800).build();
+        getTransferProcessStore().save(tp);
+
+        var query = QuerySpec.Builder.newInstance()
+                .filter(List.of(new Criterion("state", "=", 800)))
+                .build();
+
+        var result = getTransferProcessStore().findAll(query).toList();
+        assertThat(result).hasSize(1).usingRecursiveFieldByFieldElementComparator().containsExactly(tp);
+
+    }
+
     @Test
     void create() {
         var transferProcess = createTransferProcessBuilder("test-id")
@@ -907,15 +921,6 @@ void findAll_verifySorting() {
         assertThat(getTransferProcessStore().findAll(QuerySpec.Builder.newInstance().sortField("id").sortOrder(SortOrder.DESC).build())).hasSize(10).isSortedAccordingTo((c1, c2) -> c2.getId().compareTo(c1.getId()));
     }
 
-    @Test
-    protected void findAll_verifySorting_invalidProperty() {
-        range(0, 10).forEach(i -> getTransferProcessStore().updateOrCreate(createTransferProcess("test-neg-" + i)));
-
-        var query = QuerySpec.Builder.newInstance().sortField("notexist").sortOrder(SortOrder.DESC).build();
-
-        assertThat(getTransferProcessStore().findAll(query).collect(Collectors.toList())).isEmpty();
-    }
-
     @Test
     void findByIdAndLease_shouldReturnTheEntityAndLeaseIt() {
         var id = UUID.randomUUID().toString();
@@ -978,13 +983,13 @@ void findByCorrelationIdAndLease_shouldReturnAlreadyLeased_whenEntityIsAlreadyLe
         assertThat(result).isFailed().extracting(StoreFailure::getReason).isEqualTo(ALREADY_LEASED);
     }
 
-    private void delayByTenMillis(TransferProcess t) {
-        try {
-            Thread.sleep(10);
-        } catch (InterruptedException ignored) {
-            // noop
-        }
-        t.updateStateTimestamp();
+    @Test
+    protected void findAll_verifySorting_invalidProperty() {
+        range(0, 10).forEach(i -> getTransferProcessStore().updateOrCreate(createTransferProcess("test-neg-" + i)));
+
+        var query = QuerySpec.Builder.newInstance().sortField("notexist").sortOrder(SortOrder.DESC).build();
+
+        assertThat(getTransferProcessStore().findAll(query).collect(Collectors.toList())).isEmpty();
     }
 
     protected abstract boolean supportsCollectionQuery();
@@ -1001,4 +1006,13 @@ protected void leaseEntity(String negotiationId, String owner) {
 
     protected abstract boolean isLeasedBy(String negotiationId, String owner);
 
+    private void delayByTenMillis(TransferProcess t) {
+        try {
+            Thread.sleep(10);
+        } catch (InterruptedException ignored) {
+            // noop
+        }
+        t.updateStateTimestamp();
+    }
+
 }
diff --git a/.../src/test/java/org/eclipse/edc/test/e2e/managementapi/TransferProcessApiEndToEndTest.java b/.../src/test/java/org/eclipse/edc/test/e2e/managementapi/TransferProcessApiEndToEndTest.java
@@ -14,11 +14,15 @@
 
 package org.eclipse.edc.test.e2e.managementapi;
 
+import com.fasterxml.jackson.core.JsonProcessingException;
 import jakarta.json.Json;
+import jakarta.json.JsonArray;
 import jakarta.json.JsonArrayBuilder;
+import jakarta.json.JsonObject;
 import org.eclipse.edc.connector.transfer.spi.store.TransferProcessStore;
 import org.eclipse.edc.connector.transfer.spi.types.DataRequest;
 import org.eclipse.edc.connector.transfer.spi.types.TransferProcess;
+import org.eclipse.edc.jsonld.util.JacksonJsonLd;
 import org.eclipse.edc.junit.annotations.EndToEndTest;
 import org.eclipse.edc.spi.types.domain.DataAddress;
 import org.eclipse.edc.spi.types.domain.callback.CallbackAddress;
@@ -29,9 +33,11 @@
 
 import static io.restassured.http.ContentType.JSON;
 import static jakarta.json.Json.createObjectBuilder;
+import static java.lang.String.format;
 import static java.util.Collections.emptySet;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.eclipse.edc.connector.transfer.spi.types.TransferProcessStates.COMPLETED;
+import static org.eclipse.edc.connector.transfer.spi.types.TransferProcessStates.DEPROVISIONED;
 import static org.eclipse.edc.connector.transfer.spi.types.TransferProcessStates.REQUESTED;
 import static org.eclipse.edc.jsonld.spi.JsonLdKeywords.CONTEXT;
 import static org.eclipse.edc.jsonld.spi.JsonLdKeywords.ID;
@@ -152,6 +158,49 @@ void terminate() {
                 .statusCode(204);
     }
 
+    @Test
+    void query_byState() throws JsonProcessingException {
+
+        var state = DEPROVISIONED;
+        var tp = createTransferProcessBuilder("test-tp")
+                .state(state.code())
+                .build();
+        getStore().save(tp);
+
+
+        var content = """
+                {
+                    "@context": {
+                        "edc": "https://w3id.org/edc/v0.0.1/ns/"
+                    },
+                    "@type": "QuerySpec",
+                    "filterExpression": [
+                        {
+                            "operandLeft": "state",
+                            "operandRight": %d,
+                            "operator": "="
+                        }
+                    ],
+                    "limit": 100,
+                    "offset": 0
+                }
+                """;
+        content = format(content, state.code());
+        JsonObject query = JacksonJsonLd.createObjectMapper()
+                .readValue(content, JsonObject.class);
+
+        var result = baseRequest()
+                .contentType(JSON)
+                .body(query)
+                .post("/v2/transferprocesses/request")
+                .then()
+                .statusCode(200)
+                .extract().body().as(JsonArray.class);
+
+        assertThat(result).isNotEmpty();
+        assertThat(result).anySatisfy(it -> assertThat(it.asJsonObject().getString("edc:state")).isEqualTo(state.toString()));
+    }
+
     private TransferProcessStore getStore() {
         return controlPlane.getContext().getService(TransferProcessStore.class);
     }