This is an automated email from the ASF dual-hosted git repository.
jsinovassin 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 ab028177a UNOMI-982: Drop a condition whose optional parameter the
context does not supply (#864)
ab028177a is described below
commit ab028177a0503a83331fe4afc97279720812c49d
Author: Jonathan SINOVASSIN-NAIK <[email protected]>
AuthorDate: Mon Sep 21 16:38:50 2026 +0200
UNOMI-982: Drop a condition whose optional parameter the context does not
supply (#864)
---
.../ConditionESQueryBuilderDispatcher.java | 18 +++++---
.../ConditionOSQueryBuilderDispatcher.java | 18 +++++---
.../spi/conditions/ConditionContextHelper.java | 28 ++++++++++++-
.../impl/ConditionEvaluatorDispatcherImpl.java | 15 ++++++-
.../spi/conditions/ConditionContextHelperTest.java | 33 ++++++++-------
.../impl/ConditionEvaluatorDispatcherImplTest.java | 48 ++++++++++++++++++++++
6 files changed, 129 insertions(+), 31 deletions(-)
diff --git
a/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ConditionESQueryBuilderDispatcher.java
b/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ConditionESQueryBuilderDispatcher.java
index 239ae260b..f742e5cdd 100644
---
a/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ConditionESQueryBuilderDispatcher.java
+++
b/persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ConditionESQueryBuilderDispatcher.java
@@ -188,11 +188,19 @@ public class ConditionESQueryBuilderDispatcher extends
ConditionQueryBuilderDisp
if (finalQueryBuilderKey != null) {
ConditionESQueryBuilder queryBuilder =
queryBuilders.get(finalQueryBuilderKey);
Condition contextualCondition =
ConditionContextHelper.getContextualCondition(effectiveCondition, context,
scriptExecutor);
- if (contextualCondition != null) {
- return queryBuilder.buildQuery(contextualCondition, context,
this);
+ if (ConditionContextHelper.UNRESOLVABLE == contextualCondition) {
+ // A cycle, a depth overrun or a script that did not run.
Nothing is known about what
+ // this condition would have constrained, so it selects
nothing.
+ LOGGER.warn("Could not resolve the condition of type {},
returning a match-none query",
+ effectiveCondition.getConditionTypeId());
+ return Query.of(q -> q.matchNone(m -> m));
}
- LOGGER.warn("getContextualCondition returned null for
conditionTypeId={}, returning match-none query",
- effectiveCondition.getConditionTypeId());
+ if (contextualCondition == null) {
+ // The condition carries a parameter reference the context
does not supply, which
+ // states no constraint, so it selects everything and the
enclosing query decides.
+ return Query.of(q -> q.matchAll(m -> m));
+ }
+ return queryBuilder.buildQuery(contextualCondition, context, this);
} else {
LOGGER.warn("No matching query builder for conditionTypeId={}
(queryBuilderKey={})",
effectiveCondition.getConditionTypeId(), queryBuilderKey);
@@ -268,7 +276,7 @@ public class ConditionESQueryBuilderDispatcher extends
ConditionQueryBuilderDisp
if (finalQueryBuilderKey != null) {
ConditionESQueryBuilder queryBuilder =
queryBuilders.get(finalQueryBuilderKey);
Condition contextualCondition =
ConditionContextHelper.getContextualCondition(effectiveCondition, context,
scriptExecutor);
- if (contextualCondition != null) {
+ if (contextualCondition != null &&
ConditionContextHelper.UNRESOLVABLE != contextualCondition) {
return queryBuilder.count(contextualCondition, context, this);
}
}
diff --git
a/persistence-opensearch/core/src/main/java/org/apache/unomi/persistence/opensearch/ConditionOSQueryBuilderDispatcher.java
b/persistence-opensearch/core/src/main/java/org/apache/unomi/persistence/opensearch/ConditionOSQueryBuilderDispatcher.java
index b11cf3d20..e577ed821 100644
---
a/persistence-opensearch/core/src/main/java/org/apache/unomi/persistence/opensearch/ConditionOSQueryBuilderDispatcher.java
+++
b/persistence-opensearch/core/src/main/java/org/apache/unomi/persistence/opensearch/ConditionOSQueryBuilderDispatcher.java
@@ -187,11 +187,19 @@ public class ConditionOSQueryBuilderDispatcher extends
ConditionQueryBuilderDisp
if (finalQueryBuilderKey != null) {
ConditionOSQueryBuilder queryBuilder =
queryBuilders.get(finalQueryBuilderKey);
Condition contextualCondition =
ConditionContextHelper.getContextualCondition(effectiveCondition, context,
scriptExecutor);
- if (contextualCondition != null) {
- return queryBuilder.buildQuery(contextualCondition, context,
this);
+ if (ConditionContextHelper.UNRESOLVABLE == contextualCondition) {
+ // A cycle, a depth overrun or a script that did not run.
Nothing is known about what
+ // this condition would have constrained, so it selects
nothing.
+ LOGGER.warn("Could not resolve the condition of type {},
returning a match-none query",
+ effectiveCondition.getConditionTypeId());
+ return Query.of(q -> q.matchNone(m -> m));
}
- LOGGER.warn("getContextualCondition returned null for
conditionTypeId={}, returning match-none query",
- effectiveCondition.getConditionTypeId());
+ if (contextualCondition == null) {
+ // The condition carries a parameter reference the context
does not supply, which
+ // states no constraint, so it selects everything and the
enclosing query decides.
+ return Query.of(q -> q.matchAll(m -> m));
+ }
+ return queryBuilder.buildQuery(contextualCondition, context, this);
} else {
LOGGER.warn("No matching query builder for conditionTypeId={}
(queryBuilderKey={})",
effectiveCondition.getConditionTypeId(), queryBuilderKey);
@@ -267,7 +275,7 @@ public class ConditionOSQueryBuilderDispatcher extends
ConditionQueryBuilderDisp
if (finalQueryBuilderKey != null) {
ConditionOSQueryBuilder queryBuilder =
queryBuilders.get(finalQueryBuilderKey);
Condition contextualCondition =
ConditionContextHelper.getContextualCondition(effectiveCondition, context,
scriptExecutor);
- if (contextualCondition != null) {
+ if (contextualCondition != null &&
ConditionContextHelper.UNRESOLVABLE != contextualCondition) {
return queryBuilder.count(contextualCondition, context, this);
}
}
diff --git
a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java
b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java
index bb37c1985..b98500606 100644
---
a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java
+++
b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java
@@ -70,6 +70,18 @@ public class ConditionContextHelper {
*/
private static final Object RESOLUTION_ERROR = new Object();
+ /**
+ * Returned by {@link #getContextualCondition} when the condition cannot
be resolved at all,
+ * because a parameter reference forms a cycle, exceeds {@link
#MAX_RESOLUTION_DEPTH}, or a
+ * script expression could not run.
+ * <p>
+ * This is a different answer from {@code null}, which says the condition
carries a parameter
+ * reference the context does not supply. A caller drops a {@code null}
condition, because an
+ * unset parameter states no constraint. A caller must not drop this one:
nothing is known
+ * about what it would have constrained, so the caller refuses the match
instead.
+ */
+ public static final Condition UNRESOLVABLE = new Condition();
+
/**
* Expected-type names that {@link #isTypeCompatible(String, String)} can
actually reason
* about. A mismatch against one of these is a genuine, actionable signal.
Anything else is
@@ -242,7 +254,10 @@ public class ConditionContextHelper {
context, condition.getParameterValues(), scriptExecutor,
parameterDefs, tracerService, condition.getConditionTypeId(),
effectiveValidators);
- if (rawValues == null || rawValues == RESOLUTION_ERROR) {
+ if (rawValues == RESOLUTION_ERROR) {
+ return UNRESOLVABLE;
+ }
+ if (rawValues == null) {
return null;
}
@SuppressWarnings("unchecked")
@@ -396,8 +411,17 @@ public class ConditionContextHelper {
context, paramValue, scriptExecutor, parameterDefs,
tracerService, conditionTypeId, valueTypeValidators,
resolutionChain, depth);
- // If resolution returned an error marker, return null for
entire map
+ // Propagate the error marker so the caller can tell a broken
reference from an
+ // unset one.
if (parameter == RESOLUTION_ERROR) {
+ return RESOLUTION_ERROR;
+ }
+
+ // A parameter reference the context does not supply voids the
whole condition, so
+ // that the caller drops it. This is how an optional condition
parameter works: a
+ // pageViewEventCondition with no pagePath must not become
"pagePath equals null",
+ // which matches no page at all.
+ if (parameter == null && isParameterReference(paramValue)) {
return null;
}
diff --git
a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/evaluator/impl/ConditionEvaluatorDispatcherImpl.java
b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/evaluator/impl/ConditionEvaluatorDispatcherImpl.java
index 4fefdff52..b6492bf2d 100644
---
a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/evaluator/impl/ConditionEvaluatorDispatcherImpl.java
+++
b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/evaluator/impl/ConditionEvaluatorDispatcherImpl.java
@@ -228,12 +228,23 @@ public class ConditionEvaluatorDispatcherImpl
// Use effective condition for evaluation
Condition contextualCondition =
ConditionContextHelper.getContextualCondition(
effectiveCondition, context, scriptExecutor, true,
tracerService);
- if (contextualCondition == null) {
+ if (ConditionContextHelper.UNRESOLVABLE ==
contextualCondition) {
+ // A cycle, a depth overrun or a script that did not
run. Nothing is known
+ // about what this condition would have constrained,
so it does not match.
if (tracer != null) {
- tracer.endOperation(false, "Contextual condition
is null");
+ tracer.endOperation(false, "Contextual condition
could not be resolved");
}
return false;
}
+ if (contextualCondition == null) {
+ // The condition carries a parameter reference the
context does not supply,
+ // which states no constraint. Drop it rather than
fail it: an AND that holds
+ // an optional sub-condition must still match when the
parameter is unset.
+ if (tracer != null) {
+ tracer.endOperation(true, "Contextual condition
carries an unset parameter, ignoring it");
+ }
+ return true;
+ }
final Condition finalContextualCondition =
contextualCondition;
boolean result = new
MetricAdapter<Boolean>(metricsService, this.getClass().getName() +
".conditions." + conditionEvaluatorKey) {
@Override
diff --git
a/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelperTest.java
b/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelperTest.java
index 3ee6e6f11..2e9a870d0 100644
---
a/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelperTest.java
+++
b/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelperTest.java
@@ -83,8 +83,8 @@ public class ConditionContextHelperTest {
Condition resolved = ConditionContextHelper.getContextualCondition(
condition, context, scriptExecutor);
- assertNotNull("Resolved condition should not be null", resolved);
- assertNull("Missing parameter reference should resolve to null",
resolved.getParameterValues().get("testParam"));
+ assertNull("A parameter reference the context does not supply states
no constraint, "
+ + "so the whole condition is voided and the caller drops it",
resolved);
}
@Test
@@ -190,7 +190,7 @@ public class ConditionContextHelperTest {
Condition resolved = ConditionContextHelper.getContextualCondition(
condition, context, scriptExecutor);
- assertNull("Condition with cyclic reference should return null",
resolved);
+ assertSame("Condition with cyclic reference cannot be resolved, so it
answers UNRESOLVABLE", ConditionContextHelper.UNRESOLVABLE, resolved);
}
@Test
@@ -203,7 +203,7 @@ public class ConditionContextHelperTest {
Condition resolved = ConditionContextHelper.getContextualCondition(
condition, context, scriptExecutor);
- assertNull("Condition with two-level cyclic reference should return
null", resolved);
+ assertSame("Condition with two-level cyclic reference cannot be
resolved, so it answers UNRESOLVABLE", ConditionContextHelper.UNRESOLVABLE,
resolved);
}
@Test
@@ -217,7 +217,7 @@ public class ConditionContextHelperTest {
Condition resolved = ConditionContextHelper.getContextualCondition(
condition, context, scriptExecutor);
- assertNull("Condition with three-level cyclic reference should return
null", resolved);
+ assertSame("Condition with three-level cyclic reference cannot be
resolved, so it answers UNRESOLVABLE", ConditionContextHelper.UNRESOLVABLE,
resolved);
}
@Test
@@ -231,7 +231,7 @@ public class ConditionContextHelperTest {
Condition resolved = ConditionContextHelper.getContextualCondition(
condition, context, scriptExecutor);
- assertNull("Condition with script-based cyclic reference should return
null", resolved);
+ assertSame("Condition with script-based cyclic reference cannot be
resolved, so it answers UNRESOLVABLE", ConditionContextHelper.UNRESOLVABLE,
resolved);
}
// ========== Maximum Depth Tests ==========
@@ -252,7 +252,7 @@ public class ConditionContextHelperTest {
Condition resolved = ConditionContextHelper.getContextualCondition(
condition, context, scriptExecutor);
- assertNull("Condition exceeding maximum depth should return null",
resolved);
+ assertSame("Condition exceeding maximum depth cannot be resolved, so
it answers UNRESOLVABLE", ConditionContextHelper.UNRESOLVABLE, resolved);
}
// ========== Nested Structure Tests ==========
@@ -438,8 +438,7 @@ public class ConditionContextHelperTest {
Condition resolved = ConditionContextHelper.getContextualCondition(
condition, new HashMap<>(), scriptExecutor);
- assertNotNull("Resolved condition should not be null", resolved);
- assertNull("Parameter reference in empty context should resolve to
null", resolved.getParameterValues().get("testParam"));
+ assertNull("A parameter reference an empty context cannot supply voids
the whole condition", resolved);
}
@Test
@@ -470,8 +469,8 @@ public class ConditionContextHelperTest {
Condition resolved = ConditionContextHelper.getContextualCondition(
condition, context, scriptExecutor);
- assertNotNull("Resolved condition should not be null", resolved);
- assertNull("Script returning null should resolve to null",
resolved.getParameterValues().get("testParam"));
+ assertNull("A script:: value that returns null supplies no value, so
it voids the whole condition",
+ resolved);
}
@Test
@@ -529,7 +528,8 @@ public class ConditionContextHelperTest {
// Pass null as scriptExecutor — simulates OSGi service not yet wired
Condition resolved =
ConditionContextHelper.getContextualCondition(condition, context, null);
- assertNull("getContextualCondition must return null when
scriptExecutor is null and a script:: value is present", resolved);
+ assertSame("A script:: value with no script executor cannot be
resolved, so it answers UNRESOLVABLE",
+ ConditionContextHelper.UNRESOLVABLE, resolved);
}
@Test
@@ -863,9 +863,8 @@ public class ConditionContextHelperTest {
Condition resolved = ConditionContextHelper.getContextualCondition(
condition, context, scriptExecutor, false);
- assertNotNull(resolved);
- assertEquals("equals", resolved.getParameter("comparisonOperator"));
- assertNull(resolved.getParameter("other"));
+ assertNull("One unset parameter reference voids the condition,
whatever the other parameters hold",
+ resolved);
}
// ========== Multivalued parameter validation ==========
@@ -1152,8 +1151,8 @@ public class ConditionContextHelperTest {
Condition resolved = ConditionContextHelper.getContextualCondition(
condition, context, scriptExecutor, true);
- assertNotNull(resolved);
- assertNull(resolved.getParameter("testParam"));
+ assertNull("An unset parameter reference voids the condition, so there
is nothing left to validate",
+ resolved);
}
// ========== Helper Methods ==========
diff --git
a/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/conditions/evaluator/impl/ConditionEvaluatorDispatcherImplTest.java
b/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/conditions/evaluator/impl/ConditionEvaluatorDispatcherImplTest.java
index 3994becae..8fe6a5746 100644
---
a/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/conditions/evaluator/impl/ConditionEvaluatorDispatcherImplTest.java
+++
b/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/conditions/evaluator/impl/ConditionEvaluatorDispatcherImplTest.java
@@ -25,6 +25,9 @@ import org.apache.unomi.api.services.TypeResolutionService;
import org.apache.unomi.metrics.MetricsService;
import
org.apache.unomi.persistence.spi.conditions.evaluator.ConditionEvaluator;
import org.apache.unomi.scripting.ScriptExecutor;
+
+import java.util.HashMap;
+import java.util.Map;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -208,4 +211,49 @@ public class ConditionEvaluatorDispatcherImplTest {
assertFalse("Cycle in ConditionType parent chain must return false,
not cause infinite recursion or NPE", result);
}
+
+ // A condition parameter that reads "parameter::x" where the context holds
no x states no
+ // constraint, so the dispatcher drops the condition instead of failing
it. This is what makes a
+ // condition type parameter optional: pageViewEventCondition compares the
page path to
+ // "parameter::pagePath", and a rule that sets no pagePath must still
match every page view.
+ @Test
+ public void eval_unsetParameterReference_dropsConditionAndReturnsTrue() {
+ dispatcher.addEvaluator("neverMatches", (condition, item, ctx, d) ->
false);
+
+ ConditionType type = new ConditionType(new Metadata());
+ type.setItemId("optionalParameterType");
+ type.setConditionEvaluator("neverMatches");
+
+ Condition condition = new Condition(type);
+ condition.setParameter("propertyValue", "parameter::pagePath");
+
+ boolean result = dispatcher.eval(condition, dummyProfile, new
HashMap<>());
+
+ assertTrue("An unset parameter reference must drop the condition, not
fail it", result);
+
+ dispatcher.removeEvaluator("neverMatches");
+ }
+
+ // A parameter reference that cannot be resolved at all is a different
answer from an unset one.
+ // Nothing is known about what the condition would have constrained, so it
must not match.
+ @Test
+ public void eval_cyclicParameterReference_returnsFalse() {
+ dispatcher.addEvaluator("alwaysMatches", (condition, item, ctx, d) ->
true);
+
+ ConditionType type = new ConditionType(new Metadata());
+ type.setItemId("cyclicParameterType");
+ type.setConditionEvaluator("alwaysMatches");
+
+ Condition condition = new Condition(type);
+ condition.setParameter("propertyValue", "parameter::loop");
+
+ Map<String, Object> context = new HashMap<>();
+ context.put("loop", "parameter::loop");
+
+ boolean result = dispatcher.eval(condition, dummyProfile, context);
+
+ assertFalse("A cyclic parameter reference must not match", result);
+
+ dispatcher.removeEvaluator("alwaysMatches");
+ }
}
