jsinovassin opened a new pull request, #864: URL: https://github.com/apache/unomi/pull/864
Fixes UNOMI-982, at https://issues.apache.org/jira/browse/UNOMI-982 ### What this changes 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. Apache Unomi 3.1 evaluates the comparison instead, so it becomes `field equals null` and matches nothing. A `booleanCondition` with the `and` operator holds that comparison, so the whole condition never matches, and every rule built on the condition type stops firing. The failure is silent. Unomi logs no warning and no error, the rule stays enabled and valid in persistence, and the REST API returns it unchanged. ### Where the behaviour changed UNOMI-883 changed three places, at https://github.com/apache/unomi/commit/901b5fcdc62a7732097f7db9305dd9404356f8a1 and https://github.com/apache/unomi/pull/773. `ConditionContextHelper.parseParameterWithValidationRecursive` keeps the unresolved value in the parameter map. Before that commit, an unresolved parameter reference voided the whole condition. `ConditionEvaluatorDispatcherImpl.eval` answers `false` for a null contextual condition. Before that commit it answered `true`, which means the caller drops the condition. `ConditionESQueryBuilderDispatcher.buildFilter` answers a match-none query for a null contextual condition. Before that commit it answered a match-all query. `ConditionOSQueryBuilderDispatcher.buildFilter` has the same shape. ### Mechanism `getContextualCondition` now gives three answers instead of two: - the resolved condition, - `null`, when the context does not supply a parameter reference the condition carries, - a new `ConditionContextHelper.UNRESOLVABLE` sentinel, 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. ### Effect Measured on Apache Unomi 3.1.0-SNAPSHOT, with the Jahia jExperience module installed. That module declares `pageViewEventCondition`, whose `pagePath` and `language` parameters are optional. A `view` event carrying URL parameters fired neither the `copyURLParams` rule nor the `incrementPageViewCount` rule: ``` {"nbOfVisits":1,"lastVisit":"...","firstVisit":"...","totalNbOfVisits":1} ``` Both rules fire again with this change: ``` {"nbOfVisits":1,"pageViewCount":{"copyURLParams":1},"lastVisit":"...","firstVisit":"...","totalNbOfVisits":1,"utm_content":["testParamValue"]} ``` The Cypress test `live/copyURLParamsToProfile` of the jExperience module went from 0 of 2 to 2 of 2. ### Tests `mvn test` on `persistence-spi` reports 134 tests and no failure. Eleven tests in `ConditionContextHelperTest` asserted the behaviour this pull request 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. ### Notes for the reviewer The two past event query builders, `PastEventConditionESQueryBuilder` and `PastEventConditionOSQueryBuilder`, still throw on a null contextual condition. They call `getContextualCondition` on the top level event condition, whose own parameter values hold no reference, so this change does not alter their behaviour. The `List` branch of `parseParameterWithValidationRecursive` still keeps a `null` element for an unresolved reference. Apache Unomi 2.x skipped that element. No test covers the difference, so this pull request leaves the branch alone. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
