This is an automated email from the ASF dual-hosted git repository. jsinovassin pushed a commit to branch UNOMI-982-optional-condition-parameter in repository https://gitbox.apache.org/repos/asf/unomi.git
commit dbc7ede33f98561be402d071049ab17410839723 Author: jsinovassin <[email protected]> AuthorDate: Wed Sep 16 15:05:27 2026 +0200 UNOMI-982: Drop a condition whose optional parameter the context does not supply A condition type can declare an optional parameter, and its parent condition then compares a field to "parameter::<name>". When the caller supplies no value for that name, the comparison states no constraint, so the evaluator has to ignore the comparison. UNOMI-883 changed that behaviour in three places, at https://github.com/apache/unomi/commit/901b5fcdc62a7732097f7db9305dd9404356f8a1 and https://github.com/apache/unomi/pull/773. ConditionContextHelper kept the unresolved value in the parameter map, so the comparison became "field equals null". ConditionEvaluatorDispatcherImpl answered false for a null contextual condition, and ConditionESQueryBuilderDispatcher answered a match-none query. A boolean AND that held one optional sub-condition therefore never matched. getContextualCondition now gives three answers instead of two: - the resolved condition, - null, when the context does not supply a parameter reference the condition carries, - UNRESOLVABLE, when resolution itself failed. A cyclic reference, a depth overrun and a script that did not run all produce UNRESOLVABLE. The callers ignore a null condition and refuse an UNRESOLVABLE one, so UNOMI-883 keeps its guard on the broken cases. Measured on Apache Unomi 3.1.0-SNAPSHOT with the jExperience module installed. The pageViewEventCondition condition type declares pagePath and language as optional parameters. A view event carrying URL parameters fired neither the copyURLParams rule nor the incrementPageViewCount rule. Both rules fire again with this change, and the profile receives utm_content and pageViewCount. Eleven tests in ConditionContextHelperTest asserted the behaviour this change reverses, and they now assert the two answers apart. ConditionEvaluatorDispatcherImplTest gains two tests: an unset parameter drops the condition, and a cyclic reference refuses the condition. --- .../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"); + } }
