[unomi] branch master updated: UNOMI-419 - on segment creation, validate segment condition and logging errors (#238)

[ta...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

taybou pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/unomi.git


The following commit(s) were added to refs/heads/master by this push:
     new 1565f21  UNOMI-419 - on segment creation, validate segment condition and logging errors (#238)
1565f21 is described below

commit 1565f21c6c0f8c2a37a401a2c33247e3eb4f0e4c
Author: giladw <gw...@yotpo.com>
AuthorDate: Wed Feb 17 16:55:01 2021 +0200

    UNOMI-419 - on segment creation, validate segment condition and logging errors (#238)
    
    * on segment creation, validate segment condition and logging errors
    
    * use save with alwaysOverwrite to ensure update of segment
    
    * change condition validation to return boolean
---
 .../elasticsearch/ElasticSearchPersistenceServiceImpl.java | 14 ++++++++++++++
 .../apache/unomi/persistence/spi/PersistenceService.java   |  8 ++++++++
 .../unomi/services/impl/segments/SegmentServiceImpl.java   |  8 ++++++--
 3 files changed, 28 insertions(+), 2 deletions(-)

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 5ddeafd..2f5a3bc 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
@@ -1537,6 +1537,20 @@ public class ElasticSearchPersistenceServiceImpl implements PersistenceService,
     }
 
     @Override
+    public boolean isValidCondition(Condition query, Item item) {
+        try {
+            conditionEvaluatorDispatcher.eval(query, item);
+            QueryBuilder builder = QueryBuilders.boolQuery()
+                    .must(QueryBuilders.idsQuery().addIds(item.getItemId()))
+                    .must(conditionESQueryBuilderDispatcher.buildFilter(query));
+        } catch (Exception e) {
+            logger.error("failed at setSegmentDefinition, condition={}", query.toString(), e);
+            return false;
+        }
+        return true;
+    }
+
+    @Override
     public boolean testMatch(Condition query, Item item) {
         long startTime = System.currentTimeMillis();
         try {
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 ef669a0..c054ba0 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
@@ -306,6 +306,14 @@ public interface PersistenceService {
     boolean testMatch(Condition query, Item item);
 
     /**
+     * validates if a condition throws exception at query build.
+     *
+     * @param query 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);
+    /**
      * Same as {@code query(fieldName, fieldValue, sortBy, clazz, 0, -1).getList()}
      *
      * @see #query(Condition, String, Class, int, int)
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 e971432..a0a1a02 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
@@ -234,12 +234,14 @@ public class SegmentServiceImpl extends AbstractServiceImpl implements SegmentSe
 
     public void setSegmentDefinition(Segment segment) {
         ParserHelper.resolveConditionType(definitionsService, segment.getCondition());
+        if (persistenceService.isValidCondition(segment.getCondition(), new Profile()) == false)
+            throw new BadSegmentConditionException();
+
         if (segment.getMetadata().isEnabled() && !segment.getMetadata().isMissingPlugins()) {
             updateAutoGeneratedRules(segment.getMetadata(), segment.getCondition());
         }
         // make sure we update the name and description metadata that might not match, so first we remove the entry from the map
-        persistenceService.save(segment);
-
+        persistenceService.save(segment, null, true);
         updateExistingProfilesForSegment(segment);
     }
 
@@ -1098,4 +1100,6 @@ public class SegmentServiceImpl extends AbstractServiceImpl implements SegmentSe
         this.taskExecutionPeriod = taskExecutionPeriod;
     }
 
+    private class BadSegmentConditionException extends RuntimeException {
+    }
 }