UNOMI-419 add IT to cover condition validation in segment (#254)
* UNOMI-419 fix GraphQLListIT.java
* UNOMI-419 add IT to cover condition validation
* UNOMI-419 Add IT for bad parameter type (string instead integer)
Co-authored-by: Kevan <kevan@jahia.com>
diff --git a/api/src/main/java/org/apache/unomi/api/exceptions/BadSegmentConditionException.java b/api/src/main/java/org/apache/unomi/api/exceptions/BadSegmentConditionException.java
new file mode 100644
index 0000000..bed7131
--- /dev/null
+++ b/api/src/main/java/org/apache/unomi/api/exceptions/BadSegmentConditionException.java
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.unomi.api.exceptions;
+
+public class BadSegmentConditionException extends RuntimeException {
+ public BadSegmentConditionException() {
+ super();
+ }
+}
diff --git a/itests/src/test/java/org/apache/unomi/itests/SegmentIT.java b/itests/src/test/java/org/apache/unomi/itests/SegmentIT.java
index cd4e917..fceb70c 100644
--- a/itests/src/test/java/org/apache/unomi/itests/SegmentIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/SegmentIT.java
@@ -18,8 +18,10 @@
package org.apache.unomi.itests;
import org.apache.unomi.api.Metadata;
+import org.apache.unomi.api.conditions.Condition;
import org.apache.unomi.api.segments.Segment;
import org.apache.unomi.api.services.SegmentService;
+import org.apache.unomi.api.exceptions.BadSegmentConditionException;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
@@ -38,8 +40,10 @@
@ExamReactorStrategy(PerSuite.class)
public class SegmentIT extends BaseIT {
private final static Logger LOGGER = LoggerFactory.getLogger(SegmentIT.class);
+ private final static String SEGMENT_ID = "test-segment-id-2";
- @Inject @Filter(timeout = 600000)
+ @Inject
+ @Filter(timeout = 600000)
protected SegmentService segmentService;
@Before
@@ -54,4 +58,57 @@
Assert.assertEquals("Segment metadata list should be empty", 0, segmentMetadatas.size());
LOGGER.info("Retrieved " + segmentMetadatas.size() + " segment metadata entries");
}
+
+ @Test(expected = BadSegmentConditionException.class)
+ public void testSegmentWithNullCondition() {
+ Metadata segmentMetadata = new Metadata(SEGMENT_ID);
+ Segment segment = new Segment();
+ segment.setMetadata(segmentMetadata);
+ segment.setCondition(null);
+
+ segmentService.setSegmentDefinition(segment);
+ }
+
+ @Test(expected = BadSegmentConditionException.class)
+ public void testSegmentWithInValidCondition() {
+ Metadata segmentMetadata = new Metadata(SEGMENT_ID);
+ Segment segment = new Segment();
+ segment.setMetadata(segmentMetadata);
+ Condition condition = new Condition();
+ condition.setParameter("param", "param value");
+ condition.setConditionTypeId("fakeConditionId");
+ segment.setCondition(condition);
+
+ segmentService.setSegmentDefinition(segment);
+ }
+
+ @Test(expected = BadSegmentConditionException.class)
+ public void testSegmentWithInvalidConditionParameterTypes() {
+ Metadata segmentMetadata = new Metadata(SEGMENT_ID);
+ Segment segment = new Segment(segmentMetadata);
+ Condition segmentCondition = new Condition(definitionsService.getConditionType("pastEventCondition"));
+ segmentCondition.setParameter("minimumEventCount", "2");
+ segmentCondition.setParameter("numberOfDays", "10");
+ Condition pastEventEventCondition = new Condition(definitionsService.getConditionType("eventTypeCondition"));
+ pastEventEventCondition.setParameter("eventTypeId", "test-event-type");
+ segmentCondition.setParameter("eventCondition", pastEventEventCondition);
+ segment.setCondition(segmentCondition);
+ segmentService.setSegmentDefinition(segment);
+ }
+
+ @Test
+ public void testSegmentWithValidCondition() {
+ Metadata segmentMetadata = new Metadata(SEGMENT_ID);
+ Segment segment = new Segment(segmentMetadata);
+ Condition segmentCondition = new Condition(definitionsService.getConditionType("pastEventCondition"));
+ segmentCondition.setParameter("minimumEventCount", 2);
+ segmentCondition.setParameter("numberOfDays", 10);
+ Condition pastEventEventCondition = new Condition(definitionsService.getConditionType("eventTypeCondition"));
+ pastEventEventCondition.setParameter("eventTypeId", "test-event-type");
+ segmentCondition.setParameter("eventCondition", pastEventEventCondition);
+ segment.setCondition(segmentCondition);
+ segmentService.setSegmentDefinition(segment);
+
+ segmentService.removeSegmentDefinition(SEGMENT_ID, false);
+ }
}
diff --git a/itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLListIT.java b/itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLListIT.java
index bfde614..96fbdb7 100644
--- a/itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLListIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLListIT.java
@@ -121,7 +121,7 @@
Assert.assertEquals(userList.getMetadata().getScope(), context.getValue("data.cdp.addProfileToList.view.name"));
}
- Thread.sleep(5000);
+ refreshPersistence();
try (CloseableHttpResponse response = post("graphql/list/find-lists.json")) {
final ResponseContext context = ResponseContext.parse(response.getEntity());
diff --git a/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ElasticSearchPersistenceServiceImpl.java b/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ElasticSearchPersistenceServiceImpl.java
index ae73987..c263ac9 100644
--- a/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ElasticSearchPersistenceServiceImpl.java
+++ b/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ElasticSearchPersistenceServiceImpl.java
@@ -1582,14 +1582,14 @@
}
@Override
- public boolean isValidCondition(Condition query, Item item) {
+ public boolean isValidCondition(Condition condition, Item item) {
try {
- conditionEvaluatorDispatcher.eval(query, item);
- QueryBuilder builder = QueryBuilders.boolQuery()
+ conditionEvaluatorDispatcher.eval(condition, item);
+ QueryBuilders.boolQuery()
.must(QueryBuilders.idsQuery().addIds(item.getItemId()))
- .must(conditionESQueryBuilderDispatcher.buildFilter(query));
+ .must(conditionESQueryBuilderDispatcher.buildFilter(condition));
} catch (Exception e) {
- logger.error("failed at setSegmentDefinition, condition={}", query.toString(), e);
+ logger.error("Failed to validate condition, condition={}", condition, e);
return false;
}
return true;
diff --git a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java
index bd66ce7..31c0239 100644
--- a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java
+++ b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PersistenceService.java
@@ -320,11 +320,11 @@
/**
* validates if a condition throws exception at query build.
*
- * @param query the condition we're testing the specified item against
+ * @param condition the condition we're testing the specified item against
* @param item the item we're checking against the specified condition
* @return {@code true} if the item satisfies the condition, {@code false} otherwise
*/
- boolean isValidCondition(Condition query, Item item);
+ boolean isValidCondition(Condition condition, Item item);
/**
* Same as {@code query(fieldName, fieldValue, sortBy, clazz, 0, -1).getList()}
*
diff --git a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
index 363395d..7d8c2ce 100644
--- a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
+++ b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
@@ -39,6 +39,7 @@
import org.apache.unomi.persistence.spi.aggregate.TermsAggregate;
import org.apache.unomi.services.impl.AbstractServiceImpl;
import org.apache.unomi.services.impl.ParserHelper;
+import org.apache.unomi.api.exceptions.BadSegmentConditionException;
import org.osgi.framework.Bundle;
import org.osgi.framework.BundleContext;
import org.osgi.framework.BundleEvent;
@@ -257,8 +258,9 @@
public void setSegmentDefinition(Segment segment) {
ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
- if (persistenceService.isValidCondition(segment.getCondition(), new Profile()) == false)
+ if (!persistenceService.isValidCondition(segment.getCondition(), new Profile())) {
throw new BadSegmentConditionException();
+ }
if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
@@ -1142,7 +1144,4 @@
public void setTaskExecutionPeriod(long taskExecutionPeriod) {
this.taskExecutionPeriod = taskExecutionPeriod;
}
-
- private class BadSegmentConditionException extends RuntimeException {
- }
}