From 6d50fe0b9a45979c9142fc044cdd7744f832667d Mon Sep 17 00:00:00 2001 From: Enrico Date: Mon, 13 Jul 2026 14:15:28 +0200 Subject: [PATCH 1/3] feat: Emit effects for assignments to program rule variables backed by a field [DHIS2-21730] ASSIGN actions targeting a program rule variable now emit a RuleEffect when the variable is backed by a data element or tracked entity attribute, with the backing field and attributeType injected into the effect action so consumers can apply it like a direct field assignment. Emission is matched to the evaluation context (data elements in the event pass, attributes in the enrollment pass) so evaluateAll emits each assignment exactly once. Calculated values keep their silent value-map-only behavior, and rule chaining is unchanged. The two ASSIGN modes are now dispatched through the new sealed AssignTarget type (RuleAction.assignTarget()), which accepts both #{variableName} and A{variableName} content notations and reports malformed actions with actionable error messages instead of "unexpected exception: null". Co-Authored-By: Claude Fable 5 --- api/rule-engine.api | 32 +++++ build.gradle.kts | 2 +- .../dhis/rules/engine/DefaultRuleEngine.kt | 5 +- .../rules/engine/RuleConditionEvaluator.kt | 71 +++++++--- .../engine/RuleEngineMultipleExecution.kt | 3 +- .../dhis/rules/engine/RuleEvaluationResult.kt | 2 +- .../hisp/dhis/rules/models/AssignTarget.kt | 34 +++++ .../org/hisp/dhis/rules/models/RuleAction.kt | 35 +++++ .../kotlin/org/hisp/dhis/rules/utils/Utils.kt | 4 +- .../dhis/rules/RuleEngineEffectTypesTest.kt | 125 ++++++++++++++++++ .../models/RuleActionAssignTargetTest.kt | 66 +++++++++ 11 files changed, 358 insertions(+), 21 deletions(-) create mode 100644 src/commonMain/kotlin/org/hisp/dhis/rules/models/AssignTarget.kt create mode 100644 src/commonTest/kotlin/org/hisp/dhis/rules/models/RuleActionAssignTargetTest.kt diff --git a/api/rule-engine.api b/api/rule-engine.api index a4d783e6..94d9f354 100644 --- a/api/rule-engine.api +++ b/api/rule-engine.api @@ -156,6 +156,31 @@ public final class org/hisp/dhis/rules/engine/RuleVariableValue { public final fun toVariableValue ()Lorg/hisp/dhis/lib/expression/spi/VariableValue; } +public abstract class org/hisp/dhis/rules/models/AssignTarget { +} + +public final class org/hisp/dhis/rules/models/AssignTarget$Field : org/hisp/dhis/rules/models/AssignTarget { + public fun (Ljava/lang/String;)V + public final fun component1 ()Ljava/lang/String; + public final fun copy (Ljava/lang/String;)Lorg/hisp/dhis/rules/models/AssignTarget$Field; + public static synthetic fun copy$default (Lorg/hisp/dhis/rules/models/AssignTarget$Field;Ljava/lang/String;ILjava/lang/Object;)Lorg/hisp/dhis/rules/models/AssignTarget$Field; + public fun equals (Ljava/lang/Object;)Z + public final fun getField ()Ljava/lang/String; + public fun hashCode ()I + public fun toString ()Ljava/lang/String; +} + +public final class org/hisp/dhis/rules/models/AssignTarget$Variable : org/hisp/dhis/rules/models/AssignTarget { + public fun (Ljava/lang/String;)V + public final fun component1 ()Ljava/lang/String; + public final fun copy (Ljava/lang/String;)Lorg/hisp/dhis/rules/models/AssignTarget$Variable; + public static synthetic fun copy$default (Lorg/hisp/dhis/rules/models/AssignTarget$Variable;Ljava/lang/String;ILjava/lang/Object;)Lorg/hisp/dhis/rules/models/AssignTarget$Variable; + public fun equals (Ljava/lang/Object;)Z + public final fun getName ()Ljava/lang/String; + public fun hashCode ()I + public fun toString ()Ljava/lang/String; +} + public final class org/hisp/dhis/rules/models/AttributeType : java/lang/Enum { public static final field DATA_ELEMENT Lorg/hisp/dhis/rules/models/AttributeType; public static final field TRACKED_ENTITY_ATTRIBUTE Lorg/hisp/dhis/rules/models/AttributeType; @@ -203,8 +228,12 @@ public final class org/hisp/dhis/rules/models/Rule : java/lang/Comparable { } public final class org/hisp/dhis/rules/models/RuleAction : java/lang/Comparable { + public static final field ASSIGN Ljava/lang/String; + public static final field Companion Lorg/hisp/dhis/rules/models/RuleAction$Companion; + public static final field ERROR Ljava/lang/String; public fun (Ljava/lang/String;Ljava/lang/String;Ljava/util/Map;Ljava/lang/Integer;)V public synthetic fun (Ljava/lang/String;Ljava/lang/String;Ljava/util/Map;Ljava/lang/Integer;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public final fun assignTarget ()Lorg/hisp/dhis/rules/models/AssignTarget; public final fun attributeType ()Ljava/lang/String; public synthetic fun compareTo (Ljava/lang/Object;)I public fun compareTo (Lorg/hisp/dhis/rules/models/RuleAction;)I @@ -225,6 +254,9 @@ public final class org/hisp/dhis/rules/models/RuleAction : java/lang/Comparable public fun toString ()Ljava/lang/String; } +public final class org/hisp/dhis/rules/models/RuleAction$Companion { +} + public final class org/hisp/dhis/rules/models/RuleAttributeValue { public fun (Ljava/lang/String;Ljava/lang/String;)V public final fun component1 ()Ljava/lang/String; diff --git a/build.gradle.kts b/build.gradle.kts index 7490eb39..7a5bee98 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -12,7 +12,7 @@ repositories { maven { url = uri("https://central.sonatype.com/repository/maven-snapshots/") } } -version = "3.8.1-SNAPSHOT" +version = "3.8.2-SNAPSHOT" group = "org.hisp.dhis.rules" if (project.hasProperty("removeSnapshotSuffix")) { diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/DefaultRuleEngine.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/DefaultRuleEngine.kt index ce11d0fc..aa9d8007 100644 --- a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/DefaultRuleEngine.kt +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/DefaultRuleEngine.kt @@ -28,7 +28,7 @@ internal class DefaultRuleEngine : RuleEngine { ruleEnrollment, target, ) - return RuleConditionEvaluator().getRuleEffects( + return RuleConditionEvaluator(executionContext.ruleVariables).getRuleEffects( TrackerObjectType.EVENT, target.event, valueMap, @@ -50,7 +50,7 @@ internal class DefaultRuleEngine : RuleEngine { ruleEvents.toSet(), target, ) - return RuleConditionEvaluator().getRuleEffects( + return RuleConditionEvaluator(executionContext.ruleVariables).getRuleEffects( TrackerObjectType.ENROLLMENT, target.enrollment, valueMap, @@ -70,6 +70,7 @@ internal class DefaultRuleEngine : RuleEngine { .multipleBuild(executionContext.constantsValues, executionContext.ruleVariables, eventsTarget.toSet(), enrollmentTarget) return RuleEngineMultipleExecution().execute( executionContext.rules, + executionContext.ruleVariables, valueMap, executionContext.ruleSupplementaryData, ) diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConditionEvaluator.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConditionEvaluator.kt index 52ce8694..e7ba23b0 100644 --- a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConditionEvaluator.kt +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConditionEvaluator.kt @@ -11,9 +11,12 @@ import org.hisp.dhis.rules.engine.RuleEvaluationResult.Companion.errorRule import org.hisp.dhis.rules.engine.RuleEvaluationResult.Companion.evaluatedResult import org.hisp.dhis.rules.engine.RuleEvaluationResult.Companion.notEvaluatedResult import org.hisp.dhis.rules.models.* -import org.hisp.dhis.rules.utils.unwrapVariableName -internal class RuleConditionEvaluator { +internal class RuleConditionEvaluator( + ruleVariables: List = emptyList(), +) { + private val ruleVariablesByName: Map = ruleVariables.associateBy { it.name } + fun getEvaluatedAndErrorRuleEffects( targetType: TrackerObjectType, targetUid: String, @@ -70,18 +73,20 @@ internal class RuleConditionEvaluator { ) { for (action in if (attributeType == AttributeType.DATA_ELEMENT) rule.actionsForDataElement else rule.actionsForEnrollment) { try { - // Check if action is assigning value to calculated variable - if (isAssignToCalculatedValue(action)) { + // Assignments to program rule variables mutate the value map for + // subsequent rules; when the variable is backed by a data element or + // attribute matching the evaluation context, an effect targeting the + // backing field is emitted as well + val target = action.assignTarget() + if (target is AssignTarget.Variable) { + val data = processRuleAction(rule, action, valueMap, supplementaryMap) updateValueMap( - unwrapVariableName(action.content()!!), - VariableValue( - ValueType.STRING, - process(action.dataExpression, valueMap, supplementaryMap), - listOf(), - null, - ), + target.name, + VariableValue(ValueType.STRING, data, listOf(), null), valueMap, ) + createVariableAssignEffect(rule, action, target, data, attributeType) + ?.let { ruleEffects.add(it) } } else { ruleEffects.add(create(rule, action, valueMap, supplementaryMap)) } @@ -116,9 +121,12 @@ internal class RuleConditionEvaluator { targetUid: String, ruleEvaluationResults: MutableList, ) { + // IllegalArgumentException signals a rule-configuration problem (e.g. a malformed + // ASSIGN target), reported like an expression error rather than an unexpected one + val isConfigurationError = e is IllegalExpressionException || e is IllegalArgumentException val errorMessage: String errorMessage = - if (ruleAction != null && e is IllegalExpressionException) { + if (ruleAction != null && isConfigurationError) { "Action " + ruleAction::class.simpleName + " from rule " + rule.name + " with id " + rule.uid + " executed for " + targetType.name + "(" + targetUid + ")" + @@ -130,7 +138,7 @@ internal class RuleConditionEvaluator { " executed for " + targetType.name + "(" + targetUid + ")" + " with condition (" + rule.condition + ")" + " raised an unexpected exception: " + e.message - } else if (e is IllegalExpressionException) { + } else if (isConfigurationError) { "Rule " + rule.name + " with id " + rule.uid + " executed for " + targetType.name + "(" + targetUid + ")" + " with condition (" + rule.condition + ")" + @@ -175,7 +183,40 @@ internal class RuleConditionEvaluator { result } - private fun isAssignToCalculatedValue(ruleAction: RuleAction): Boolean = ruleAction.type == "ASSIGN" && ruleAction.field().isNullOrEmpty() + /** + * Builds the effect for an assignment to a program rule variable, so the caller can apply + * the value to the data element or attribute the variable is backed by. The backing field + * and its attribute type are injected into a copy of the action, making the effect + * self-contained for consumers that read `field`. + * + * Returns null when there is nothing to apply: calculated values are not backed by a field + * (their `field` is the variable's own uid), and variables whose backing does not match the + * evaluation context are left to the pass that owns them (data elements to the event pass, + * tracked entity attributes to the enrollment pass) so evaluateAll emits each assignment + * exactly once. + */ + private fun createVariableAssignEffect( + rule: Rule, + ruleAction: RuleAction, + target: AssignTarget.Variable, + data: String?, + attributeType: AttributeType, + ): RuleEffect? { + val variable = ruleVariablesByName[target.name] ?: return null + if (variable is RuleVariableCalculatedValue || variable.field.isEmpty()) return null + val backingType = + if (variable is RuleVariableAttribute) { + AttributeType.TRACKED_ENTITY_ATTRIBUTE + } else { + AttributeType.DATA_ELEMENT + } + if (backingType != attributeType) return null + val effectAction = + ruleAction.copy( + values = ruleAction.values + mapOf("field" to variable.field, "attributeType" to backingType.name), + ) + return RuleEffect(rule.uid, effectAction, if (data.isNullOrEmpty()) null else data) + } private fun updateValueMap( variable: String, @@ -191,7 +232,7 @@ internal class RuleConditionEvaluator { valueMap: MutableMap, supplementaryMap: Map>, ): RuleEffect { - if (ruleAction.type == "ASSIGN") { + if (ruleAction.type == RuleAction.ASSIGN) { val data = processRuleAction(rule, ruleAction, valueMap, supplementaryMap) updateValueMap(ruleAction.field()!!, VariableValue(ValueType.STRING, data, listOf(), null), valueMap) return if (data.isNullOrEmpty()) { diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleEngineMultipleExecution.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleEngineMultipleExecution.kt index ca23a2a9..421dab0c 100644 --- a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleEngineMultipleExecution.kt +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleEngineMultipleExecution.kt @@ -7,11 +7,12 @@ import org.hisp.dhis.rules.utils.filterRules internal class RuleEngineMultipleExecution { fun execute( rules: List, + ruleVariables: List, ruleVariableValueMap: RuleVariableValueMap, ruleSupplementaryData: RuleSupplementaryData, ): List { val supplementaryMap = RuleConditionEvaluator.convertSupplementaryData(ruleSupplementaryData) - val evaluator = RuleConditionEvaluator() + val evaluator = RuleConditionEvaluator(ruleVariables) val ruleEffects: MutableList = ArrayList() val enrollmentRules: List = filterRules(rules).sorted() val rulesByStage: Map> = rules diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleEvaluationResult.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleEvaluationResult.kt index 1e5a91ac..11934916 100644 --- a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleEvaluationResult.kt +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleEvaluationResult.kt @@ -26,7 +26,7 @@ internal data class RuleEvaluationResult( listOf( RuleEffect( rule.uid, - RuleAction(errorMessage, "ERROR"), + RuleAction(errorMessage, RuleAction.ERROR), errorMessage, ), ) diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/models/AssignTarget.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/models/AssignTarget.kt new file mode 100644 index 00000000..504daf91 --- /dev/null +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/models/AssignTarget.kt @@ -0,0 +1,34 @@ +package org.hisp.dhis.rules.models + +/** + * The target of an ASSIGN rule action, resolved by [RuleAction.assignTarget]. + * + * The two targets differ in what they update during evaluation, not only in where the value + * lands: a [Field] assignment produces a [RuleEffect] for the caller to apply, while a + * [Variable] assignment updates the program rule variable so that rules evaluated afterwards + * see the new value, and additionally produces a [RuleEffect] when the variable is backed by + * a data element or tracked entity attribute. + */ +sealed class AssignTarget { + /** + * Assign to a program rule variable, referenced by name in the action's `content`. + * + * The engine updates the variable in the evaluation state, making the value visible to + * the conditions and expressions of rules evaluated afterwards. When the variable is + * backed by a data element or tracked entity attribute, a [RuleEffect] targeting the + * backing field is also emitted — during event evaluation for data elements, during + * enrollment evaluation for attributes — with the backing field and its attribute type + * injected into a copy of the action. Calculated values are not backed by a field and + * emit no effect. + */ + data class Variable(val name: String) : AssignTarget() + + /** + * Assign to the data element or tracked entity attribute named by the action's `field`. + * + * The engine emits a [RuleEffect] carrying the evaluated value; applying it is the + * caller's responsibility. Program rule variables sourced from the assigned field are + * not updated, so rules evaluated afterwards do not see the new value through them. + */ + data class Field(val field: String) : AssignTarget() +} diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/models/RuleAction.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/models/RuleAction.kt index 010f92d1..c676dbd7 100644 --- a/src/commonMain/kotlin/org/hisp/dhis/rules/models/RuleAction.kt +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/models/RuleAction.kt @@ -19,6 +19,33 @@ data class RuleAction( fun attributeType(): String? = values["attributeType"] + /** + * Resolves the target of this action when it is an ASSIGN action. + * + * Returns null for any other action type. For ASSIGN actions, returns + * [AssignTarget.Field] when a non-empty `field` value is present, otherwise + * [AssignTarget.Variable] with the program rule variable name taken from `content`, + * accepting both the `#{variableName}` and `A{variableName}` notations. + * + * @throws IllegalArgumentException if the ASSIGN action defines neither a `field` nor a + * `content` value, or if `content` is not a program rule variable reference like + * `#{variableName}`. + */ + fun assignTarget(): AssignTarget? { + if (type != ASSIGN) return null + val field = field() + if (!field.isNullOrEmpty()) return AssignTarget.Field(field) + val content = content() + require(!content.isNullOrEmpty()) { + "ASSIGN action must define either 'field' (the data element or attribute to assign to) " + + "or 'content' (the program rule variable to assign to)" + } + require((content.startsWith("#{") || content.startsWith("A{")) && content.endsWith("}")) { + "ASSIGN action content must be a program rule variable reference like #{variableName}, but was: $content" + } + return AssignTarget.Variable(content.substring(2, content.length - 1)) + } + override fun compareTo(other: RuleAction): Int = when { this.priority != null && other.priority != null -> { @@ -34,4 +61,12 @@ data class RuleAction( 0 } } + + companion object { + /** Action type interpreted by the engine: assigns a value to a field or variable. */ + const val ASSIGN = "ASSIGN" + + /** Action type of the synthetic effect emitted when a rule or action fails to evaluate. */ + const val ERROR = "ERROR" + } } diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/utils/Utils.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/utils/Utils.kt index 7a55f9c3..818e73e2 100644 --- a/src/commonMain/kotlin/org/hisp/dhis/rules/utils/Utils.kt +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/utils/Utils.kt @@ -9,7 +9,9 @@ fun unwrapVariableName(variable: String): String { if (variable.startsWith("#{") && variable.endsWith("}")) { return variable.substring(2, variable.length - 1) } - throw IllegalArgumentException("Malformed variable: $variable") + throw IllegalArgumentException( + "Malformed variable: expected a program rule variable reference like #{variableName}, but was: $variable", + ) } fun currentDate(): LocalDate = diff --git a/src/commonTest/kotlin/org/hisp/dhis/rules/RuleEngineEffectTypesTest.kt b/src/commonTest/kotlin/org/hisp/dhis/rules/RuleEngineEffectTypesTest.kt index 8e3823c1..f0cfcaf6 100644 --- a/src/commonTest/kotlin/org/hisp/dhis/rules/RuleEngineEffectTypesTest.kt +++ b/src/commonTest/kotlin/org/hisp/dhis/rules/RuleEngineEffectTypesTest.kt @@ -9,11 +9,24 @@ import org.hisp.dhis.rules.models.* import org.hisp.dhis.rules.utils.currentDate import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertTrue // ToDo: function tests (check that function invocations are producing expected values; check nested function invocation) // ToDo: various source type tests (referencing variables from different events) class RuleEngineEffectTypesTest { + private fun getTestRuleEnrollment(): RuleEnrollment = + RuleEnrollment( + enrollment = "test_enrollment", + programName = "test_program", + incidentDate = RuleLocalDate.currentDate(), + enrollmentDate = RuleLocalDate.currentDate(), + status = RuleEnrollmentStatus.ACTIVE, + organisationUnit = "test_ou", + organisationUnitCode = "test_ou_code", + attributeValues = listOf(RuleAttributeValue("test_attribute_one", "test_value")), + ) + private fun getTestRuleEvent(status: RuleEventStatus): RuleEvent = RuleEvent( event = "test_event", @@ -52,6 +65,99 @@ class RuleEngineEffectTypesTest { assertEquals(ruleAction, ruleEffects[0].ruleAction) } + @Test + fun shouldAssignCorrectlyToAnAttribute() { + val ruleAction = RuleAction("'test_string'", "ASSIGN", + mapOf(Pair("content", "A{attribute}"))) + val rule = Rule("true", listOf(ruleAction)) + val ruleVariable = RuleVariableAttribute( + "attribute", + true, + ArrayList(), + "test_attribute_one", + RuleValueType.TEXT, + ) + val ruleEffects = + RuleEngine.getInstance().evaluate( + getTestRuleEnrollment(), + emptyList(), + RuleEngineContext(listOf(rule), listOf(ruleVariable)), + ) + assertEquals(1, ruleEffects.size) + assertEquals("test_string", ruleEffects[0].data) + assertEquals("test_attribute_one", ruleEffects[0].ruleAction.field()) + assertEquals(AttributeType.TRACKED_ENTITY_ATTRIBUTE.name, ruleEffects[0].ruleAction.attributeType()) + assertEquals("A{attribute}", ruleEffects[0].ruleAction.content()) + } + + @Test + fun assignToAttributeVariableMustNotProduceEventEffect() { + val ruleAction = RuleAction("'test_string'", "ASSIGN", mapOf(Pair("content", "A{attribute}"))) + val rule = Rule("true", listOf(ruleAction)) + val ruleVariable = RuleVariableAttribute( + "attribute", + true, + ArrayList(), + "test_attribute_one", + RuleValueType.TEXT, + ) + val ruleEffects = + RuleEngine.getInstance().evaluate( + getTestRuleEvent(RuleEventStatus.ACTIVE), + null, + emptyList(), + RuleEngineContext(listOf(rule), listOf(ruleVariable)), + ) + assertEquals(0, ruleEffects.size) + } + + @Test + fun shouldAssignCorrectlyToADataElementBackedVariable() { + val ruleAction = RuleAction("'test_string'", "ASSIGN", mapOf(Pair("content", "#{variable}"))) + val rule = Rule("true", listOf(ruleAction)) + val ruleVariable = RuleVariableCurrentEvent( + "variable", + true, + ArrayList(), + "test_data_element", + RuleValueType.TEXT, + ) + val ruleEffects = + RuleEngine.getInstance().evaluate( + getTestRuleEvent(RuleEventStatus.ACTIVE), + null, + emptyList(), + RuleEngineContext(listOf(rule), listOf(ruleVariable)), + ) + assertEquals(1, ruleEffects.size) + assertEquals("test_string", ruleEffects[0].data) + assertEquals("test_data_element", ruleEffects[0].ruleAction.field()) + assertEquals(AttributeType.DATA_ELEMENT.name, ruleEffects[0].ruleAction.attributeType()) + } + + @Test + fun assignToAttributeVariableMustProduceSingleEffectInMultipleExecution() { + val ruleAction = RuleAction("'test_string'", "ASSIGN", mapOf(Pair("content", "A{attribute}"))) + val rule = Rule("true", listOf(ruleAction)) + val ruleVariable = RuleVariableAttribute( + "attribute", + true, + ArrayList(), + "test_attribute_one", + RuleValueType.TEXT, + ) + val ruleEffects = + RuleEngine.getInstance().evaluateAll( + getTestRuleEnrollment(), + listOf(getTestRuleEvent(RuleEventStatus.ACTIVE)), + RuleEngineContext(listOf(rule), listOf(ruleVariable)), + ) + val allEffects = ruleEffects.flatMap { it.ruleEffects } + assertEquals(1, allEffects.size) + assertEquals("test_attribute_one", allEffects[0].ruleAction.field()) + assertEquals(1, ruleEffects.single { it.isEnrollment }.ruleEffects.size) + } + @Test fun simpleConditionMustResultInAssignEffectMultipleExecution() { val ruleAction = RuleAction("'test_string'", "ASSIGN", mapOf(Pair("field", "test_data_element"))) @@ -110,6 +216,25 @@ class RuleEngineEffectTypesTest { assertEquals(eventEffects.map { it.data }, eventEffectsAll.map { it.data }) } + @Test + fun malformedAssignProducesClearErrorEffectDuringEvaluation() { + val action = RuleAction("2+2", RuleAction.ASSIGN) + val rule = Rule("true", listOf(action), "rule_uid") + val ruleEffects = + RuleEngine.getInstance().evaluateAll( + null, + listOf(getTestRuleEvent(RuleEventStatus.ACTIVE)), + getRuleEngineContext(listOf(rule)), + ) + assertEquals(1, ruleEffects.size) + assertEquals(1, ruleEffects[0].ruleEffects.size) + val errorEffect = ruleEffects[0].ruleEffects[0] + assertEquals(RuleAction.ERROR, errorEffect.ruleAction.type) + assertTrue(errorEffect.data!!.contains("raised an error")) + assertTrue(errorEffect.data!!.contains("'field'")) + assertTrue(errorEffect.data!!.contains("'content'")) + } + @Test fun testEnvironmentVariableExpression() { val ruleAction = RuleAction("", "HIDEFIELD", mapOf(Pair("content", "test_action_content"), Pair("field", "test_data_element"))) diff --git a/src/commonTest/kotlin/org/hisp/dhis/rules/models/RuleActionAssignTargetTest.kt b/src/commonTest/kotlin/org/hisp/dhis/rules/models/RuleActionAssignTargetTest.kt new file mode 100644 index 00000000..f0bbf9c8 --- /dev/null +++ b/src/commonTest/kotlin/org/hisp/dhis/rules/models/RuleActionAssignTargetTest.kt @@ -0,0 +1,66 @@ +package org.hisp.dhis.rules.models + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertNull +import kotlin.test.assertTrue + +class RuleActionAssignTargetTest { + @Test + fun assignTargetIsNullForNonAssignActions() { + val action = RuleAction("'text'", "DISPLAYTEXT", mapOf("content" to "label")) + assertNull(action.assignTarget()) + } + + @Test + fun assignTargetIsFieldWhenFieldIsSet() { + val action = RuleAction("'value'", RuleAction.ASSIGN, mapOf("field" to "test_data_element")) + assertEquals(AssignTarget.Field("test_data_element"), action.assignTarget()) + } + + @Test + fun assignTargetPrefersFieldOverContent() { + val action = + RuleAction( + "'value'", + RuleAction.ASSIGN, + mapOf("field" to "test_data_element", "content" to "#{test_variable}"), + ) + assertEquals(AssignTarget.Field("test_data_element"), action.assignTarget()) + } + + @Test + fun assignTargetIsVariableWhenOnlyContentIsSet() { + val action = RuleAction("2+2", RuleAction.ASSIGN, mapOf("content" to "#{test_variable}")) + assertEquals(AssignTarget.Variable("test_variable"), action.assignTarget()) + } + + @Test + fun assignTargetAcceptsAttributeNotationInContent() { + val action = RuleAction("2+2", RuleAction.ASSIGN, mapOf("content" to "A{test_variable}")) + assertEquals(AssignTarget.Variable("test_variable"), action.assignTarget()) + } + + @Test + fun assignTargetIsVariableWhenFieldIsEmpty() { + val action = RuleAction("2+2", RuleAction.ASSIGN, mapOf("field" to "", "content" to "#{test_variable}")) + assertEquals(AssignTarget.Variable("test_variable"), action.assignTarget()) + } + + @Test + fun assignTargetFailsWhenNeitherFieldNorContentIsSet() { + val action = RuleAction("2+2", RuleAction.ASSIGN) + val exception = assertFailsWith { action.assignTarget() } + assertTrue(exception.message!!.contains("'field'")) + assertTrue(exception.message!!.contains("'content'")) + } + + @Test + fun assignTargetFailsWhenContentIsNotAVariableReference() { + val action = RuleAction("2+2", RuleAction.ASSIGN, mapOf("content" to "test_variable")) + val exception = assertFailsWith { action.assignTarget() } + assertTrue(exception.message!!.contains("#{variableName}")) + assertTrue(exception.message!!.contains("test_variable")) + } +} From 06272691324124ea1f157ea5690bac30a9488a15 Mon Sep 17 00:00:00 2001 From: Enrico Date: Mon, 13 Jul 2026 14:37:20 +0200 Subject: [PATCH 2/3] Fix sonar issue --- .../rules/engine/RuleConditionEvaluator.kt | 52 ++++++++++--------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConditionEvaluator.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConditionEvaluator.kt index e7ba23b0..f41c827d 100644 --- a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConditionEvaluator.kt +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConditionEvaluator.kt @@ -124,30 +124,34 @@ internal class RuleConditionEvaluator( // IllegalArgumentException signals a rule-configuration problem (e.g. a malformed // ASSIGN target), reported like an expression error rather than an unexpected one val isConfigurationError = e is IllegalExpressionException || e is IllegalArgumentException - val errorMessage: String - errorMessage = - if (ruleAction != null && isConfigurationError) { - "Action " + ruleAction::class.simpleName + - " from rule " + rule.name + " with id " + rule.uid + - " executed for " + targetType.name + "(" + targetUid + ")" + - " with condition (" + rule.condition + ")" + - " raised an error: " + e.message - } else if (ruleAction != null) { - "Action " + ruleAction::class.simpleName + - " from rule " + rule.name + " with id " + rule.uid + - " executed for " + targetType.name + "(" + targetUid + ")" + - " with condition (" + rule.condition + ")" + - " raised an unexpected exception: " + e.message - } else if (isConfigurationError) { - "Rule " + rule.name + " with id " + rule.uid + - " executed for " + targetType.name + "(" + targetUid + ")" + - " with condition (" + rule.condition + ")" + - " raised an error: " + e.message - } else { - "Rule " + rule.name + " with id " + rule.uid + - " executed for " + targetType.name + "(" + targetUid + ")" + - " with condition (" + rule.condition + ")" + - " raised an unexpected exception: " + e.message + val errorMessage = + when { + ruleAction != null && isConfigurationError -> { + "Action " + ruleAction::class.simpleName + + " from rule " + rule.name + " with id " + rule.uid + + " executed for " + targetType.name + "(" + targetUid + ")" + + " with condition (" + rule.condition + ")" + + " raised an error: " + e.message + } + ruleAction != null -> { + "Action " + ruleAction::class.simpleName + + " from rule " + rule.name + " with id " + rule.uid + + " executed for " + targetType.name + "(" + targetUid + ")" + + " with condition (" + rule.condition + ")" + + " raised an unexpected exception: " + e.message + } + isConfigurationError -> { + "Rule " + rule.name + " with id " + rule.uid + + " executed for " + targetType.name + "(" + targetUid + ")" + + " with condition (" + rule.condition + ")" + + " raised an error: " + e.message + } + else -> { + "Rule " + rule.name + " with id " + rule.uid + + " executed for " + targetType.name + "(" + targetUid + ")" + + " with condition (" + rule.condition + ")" + + " raised an unexpected exception: " + e.message + } } log.severe(errorMessage) ruleEvaluationResults.add(errorRule(rule, errorMessage)) From 76597f6fe927d8de1bebed1fa7492a11ba6ec978 Mon Sep 17 00:00:00 2001 From: Enrico Date: Tue, 14 Jul 2026 10:24:16 +0200 Subject: [PATCH 3/3] fix: Validate ASSIGN targets and always emit attribute-backed assign effects [DHIS2-21730] --- api/rule-engine.api | 24 ++-- .../kotlin/org/hisp/dhis/rules/Logger.kt | 1 + .../rules/engine/RuleConditionEvaluator.kt | 131 ++++++++++-------- .../engine/RuleConfigurationException.kt | 10 ++ .../engine/RuleEngineMultipleExecution.kt | 2 + .../hisp/dhis/rules/models/AssignTarget.kt | 29 +++- .../org/hisp/dhis/rules/models/RuleAction.kt | 49 +++++-- .../kotlin/org/hisp/dhis/rules/utils/Utils.kt | 11 +- .../dhis/rules/RuleEngineEffectTypesTest.kt | 81 ++++++++++- .../models/RuleActionAssignTargetTest.kt | 28 ++-- .../kotlin/org/hisp/dhis/rules/Logger.kt | 1 + .../kotlin/org/hisp/dhis/rules/Logger.kt | 8 +- .../kotlin/org/hisp/dhis/rules/Logger.kt | 6 +- 13 files changed, 267 insertions(+), 114 deletions(-) create mode 100644 src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConfigurationException.kt diff --git a/api/rule-engine.api b/api/rule-engine.api index 94d9f354..6dfaf7e7 100644 --- a/api/rule-engine.api +++ b/api/rule-engine.api @@ -17,14 +17,16 @@ public final class org/hisp/dhis/rules/DateUtils$Companion { } public final class org/hisp/dhis/rules/Logger { - public fun (Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;)V + public fun (Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;)V public final fun component1 ()Lkotlin/jvm/functions/Function1; public final fun component2 ()Lkotlin/jvm/functions/Function1; - public final fun copy (Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;)Lorg/hisp/dhis/rules/Logger; - public static synthetic fun copy$default (Lorg/hisp/dhis/rules/Logger;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)Lorg/hisp/dhis/rules/Logger; + public final fun component3 ()Lkotlin/jvm/functions/Function1; + public final fun copy (Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;)Lorg/hisp/dhis/rules/Logger; + public static synthetic fun copy$default (Lorg/hisp/dhis/rules/Logger;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)Lorg/hisp/dhis/rules/Logger; public fun equals (Ljava/lang/Object;)Z public final fun getFine ()Lkotlin/jvm/functions/Function1; public final fun getSevere ()Lkotlin/jvm/functions/Function1; + public final fun getWarning ()Lkotlin/jvm/functions/Function1; public fun hashCode ()I public fun toString ()Ljava/lang/String; } @@ -170,6 +172,17 @@ public final class org/hisp/dhis/rules/models/AssignTarget$Field : org/hisp/dhis public fun toString ()Ljava/lang/String; } +public final class org/hisp/dhis/rules/models/AssignTarget$Invalid : org/hisp/dhis/rules/models/AssignTarget { + public fun (Ljava/lang/String;)V + public final fun component1 ()Ljava/lang/String; + public final fun copy (Ljava/lang/String;)Lorg/hisp/dhis/rules/models/AssignTarget$Invalid; + public static synthetic fun copy$default (Lorg/hisp/dhis/rules/models/AssignTarget$Invalid;Ljava/lang/String;ILjava/lang/Object;)Lorg/hisp/dhis/rules/models/AssignTarget$Invalid; + public fun equals (Ljava/lang/Object;)Z + public final fun getReason ()Ljava/lang/String; + public fun hashCode ()I + public fun toString ()Ljava/lang/String; +} + public final class org/hisp/dhis/rules/models/AssignTarget$Variable : org/hisp/dhis/rules/models/AssignTarget { public fun (Ljava/lang/String;)V public final fun component1 ()Ljava/lang/String; @@ -607,8 +620,3 @@ public final class org/hisp/dhis/rules/models/TriggerEnvironment : java/lang/Enu public static fun values ()[Lorg/hisp/dhis/rules/models/TriggerEnvironment; } -public final class org/hisp/dhis/rules/utils/UtilsKt { - public static final fun currentDate ()Lkotlinx/datetime/LocalDate; - public static final fun unwrapVariableName (Ljava/lang/String;)Ljava/lang/String; -} - diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/Logger.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/Logger.kt index c4fff9bf..e890d51c 100644 --- a/src/commonMain/kotlin/org/hisp/dhis/rules/Logger.kt +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/Logger.kt @@ -2,6 +2,7 @@ package org.hisp.dhis.rules data class Logger( val severe: (String) -> Unit, + val warning: (String) -> Unit, val fine: (String) -> Unit, ) diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConditionEvaluator.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConditionEvaluator.kt index f41c827d..426dea4b 100644 --- a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConditionEvaluator.kt +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConditionEvaluator.kt @@ -24,8 +24,10 @@ internal class RuleConditionEvaluator( supplementaryMap: Map>, rules: List, attributeType: AttributeType, + hasEnrollmentPass: Boolean = false, ): List { - val ruleEvaluationResults = getRuleEvaluationResults(targetType, targetUid, valueMap, supplementaryMap, rules, attributeType) + val ruleEvaluationResults = + getRuleEvaluationResults(targetType, targetUid, valueMap, supplementaryMap, rules, attributeType, hasEnrollmentPass) return ruleEvaluationResults .flatMap { result -> result.ruleEffects } } @@ -52,6 +54,12 @@ internal class RuleConditionEvaluator( .flatMap { result -> result.ruleEffects } } + /** + * [hasEnrollmentPass] tells this pass whether the surrounding execution also evaluates + * the enrollment, which owns the effects for assignments to attribute-backed variables; + * when it does not (single-target evaluation, executions without an enrollment), the + * event pass emits them instead. + */ fun getRuleEvaluationResults( targetType: TrackerObjectType, targetUid: String, @@ -59,6 +67,7 @@ internal class RuleConditionEvaluator( supplementaryMap: Map>, rules: List, attributeType: AttributeType, + hasEnrollmentPass: Boolean = false, ): List { val ruleEvaluationResults: MutableList = ArrayList() for (rule in rules) { @@ -75,20 +84,33 @@ internal class RuleConditionEvaluator( try { // Assignments to program rule variables mutate the value map for // subsequent rules; when the variable is backed by a data element or - // attribute matching the evaluation context, an effect targeting the - // backing field is emitted as well - val target = action.assignTarget() - if (target is AssignTarget.Variable) { - val data = processRuleAction(rule, action, valueMap, supplementaryMap) - updateValueMap( - target.name, - VariableValue(ValueType.STRING, data, listOf(), null), - valueMap, - ) - createVariableAssignEffect(rule, action, target, data, attributeType) - ?.let { ruleEffects.add(it) } - } else { - ruleEffects.add(create(rule, action, valueMap, supplementaryMap)) + // attribute, an effect targeting the backing field is emitted as well + when (val target = action.assignTarget()) { + is AssignTarget.Variable -> { + // Assigning to a name without a program rule variable is + // allowed: the value is visible to subsequent rules through + // the value map, but there is no backing field to emit an + // effect for, which is worth a warning as it may be a typo + val variable = ruleVariablesByName[target.name] + if (variable == null) { + log.warning( + "ASSIGN action in rule " + rule.uid + " assigns to '" + target.name + + "' which is not a defined program rule variable; the value is" + + " visible to subsequent rules but no effect is emitted", + ) + } + val data = processRuleAction(rule, action, valueMap, supplementaryMap) + updateValueMap( + target.name, + VariableValue(ValueType.STRING, data, listOf(), null), + valueMap, + ) + variable + ?.let { createVariableAssignEffect(rule, action, it, data, attributeType, hasEnrollmentPass) } + ?.let { ruleEffects.add(it) } + } + is AssignTarget.Invalid -> throw RuleConfigurationException(target.reason) + else -> ruleEffects.add(create(rule, action, valueMap, supplementaryMap)) } } catch (e: Exception) { addRuleErrorResult(rule, action, e, targetType, targetUid, ruleEvaluationResults) @@ -121,38 +143,21 @@ internal class RuleConditionEvaluator( targetUid: String, ruleEvaluationResults: MutableList, ) { - // IllegalArgumentException signals a rule-configuration problem (e.g. a malformed - // ASSIGN target), reported like an expression error rather than an unexpected one - val isConfigurationError = e is IllegalExpressionException || e is IllegalArgumentException - val errorMessage = - when { - ruleAction != null && isConfigurationError -> { - "Action " + ruleAction::class.simpleName + - " from rule " + rule.name + " with id " + rule.uid + - " executed for " + targetType.name + "(" + targetUid + ")" + - " with condition (" + rule.condition + ")" + - " raised an error: " + e.message - } - ruleAction != null -> { - "Action " + ruleAction::class.simpleName + - " from rule " + rule.name + " with id " + rule.uid + - " executed for " + targetType.name + "(" + targetUid + ")" + - " with condition (" + rule.condition + ")" + - " raised an unexpected exception: " + e.message - } - isConfigurationError -> { - "Rule " + rule.name + " with id " + rule.uid + - " executed for " + targetType.name + "(" + targetUid + ")" + - " with condition (" + rule.condition + ")" + - " raised an error: " + e.message - } - else -> { - "Rule " + rule.name + " with id " + rule.uid + - " executed for " + targetType.name + "(" + targetUid + ")" + - " with condition (" + rule.condition + ")" + - " raised an unexpected exception: " + e.message - } + // Expression and rule-configuration errors are reported as errors; anything else as + // an unexpected exception + val isConfigurationError = e is IllegalExpressionException || e is RuleConfigurationException + val subject = + if (ruleAction != null) { + "Action " + ruleAction::class.simpleName + " from rule " + } else { + "Rule " } + val failure = if (isConfigurationError) "an error" else "an unexpected exception" + val errorMessage = + subject + rule.name + " with id " + rule.uid + + " executed for " + targetType.name + "(" + targetUid + ")" + + " with condition (" + rule.condition + ")" + + " raised " + failure + ": " + e.message log.severe(errorMessage) ruleEvaluationResults.add(errorRule(rule, errorMessage)) } @@ -191,22 +196,25 @@ internal class RuleConditionEvaluator( * Builds the effect for an assignment to a program rule variable, so the caller can apply * the value to the data element or attribute the variable is backed by. The backing field * and its attribute type are injected into a copy of the action, making the effect - * self-contained for consumers that read `field`. + * self-contained for consumers that read `field`; the copy resolves to + * [AssignTarget.Field] and compares unequal to the configured action. * - * Returns null when there is nothing to apply: calculated values are not backed by a field - * (their `field` is the variable's own uid), and variables whose backing does not match the - * evaluation context are left to the pass that owns them (data elements to the event pass, - * tracked entity attributes to the enrollment pass) so evaluateAll emits each assignment - * exactly once. + * Returns null when this pass does not emit the effect: calculated values are not backed + * by a field (their `field` is the variable's own uid), data-element backed effects are + * only emitted during event evaluation, and attribute backed effects are emitted during + * enrollment evaluation when an enrollment pass evaluates the rule — otherwise + * (single-event evaluation, executions without an enrollment, rules scoped to a program + * stage and thus excluded from the enrollment pass) the event pass emits them, so the + * assignment is never silently dropped. */ private fun createVariableAssignEffect( rule: Rule, ruleAction: RuleAction, - target: AssignTarget.Variable, + variable: RuleVariable, data: String?, attributeType: AttributeType, + hasEnrollmentPass: Boolean, ): RuleEffect? { - val variable = ruleVariablesByName[target.name] ?: return null if (variable is RuleVariableCalculatedValue || variable.field.isEmpty()) return null val backingType = if (variable is RuleVariableAttribute) { @@ -214,11 +222,16 @@ internal class RuleConditionEvaluator( } else { AttributeType.DATA_ELEMENT } - if (backingType != attributeType) return null - val effectAction = - ruleAction.copy( - values = ruleAction.values + mapOf("field" to variable.field, "attributeType" to backingType.name), - ) + val emitted = + when (backingType) { + attributeType -> true + AttributeType.TRACKED_ENTITY_ATTRIBUTE -> + !(hasEnrollmentPass && rule.programStage.isNullOrEmpty()) + + else -> false + } + if (!emitted) return null + val effectAction = ruleAction.withAssignBacking(variable.field, backingType.name) return RuleEffect(rule.uid, effectAction, if (data.isNullOrEmpty()) null else data) } diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConfigurationException.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConfigurationException.kt new file mode 100644 index 00000000..9097e77b --- /dev/null +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleConfigurationException.kt @@ -0,0 +1,10 @@ +package org.hisp.dhis.rules.engine + +/** + * Signals a rule-configuration problem detected while executing an action, such as a + * malformed ASSIGN target. Reported as an error effect like an expression error, rather + * than as an unexpected exception. + */ +internal class RuleConfigurationException( + message: String, +) : IllegalArgumentException(message) diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleEngineMultipleExecution.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleEngineMultipleExecution.kt index 421dab0c..68a53115 100644 --- a/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleEngineMultipleExecution.kt +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/engine/RuleEngineMultipleExecution.kt @@ -13,6 +13,7 @@ internal class RuleEngineMultipleExecution { ): List { val supplementaryMap = RuleConditionEvaluator.convertSupplementaryData(ruleSupplementaryData) val evaluator = RuleConditionEvaluator(ruleVariables) + val hasEnrollmentPass = ruleVariableValueMap.enrollmentMap.isNotEmpty() val ruleEffects: MutableList = ArrayList() val enrollmentRules: List = filterRules(rules).sorted() val rulesByStage: Map> = rules @@ -50,6 +51,7 @@ internal class RuleEngineMultipleExecution { supplementaryMap, eventRules, AttributeType.DATA_ELEMENT, + hasEnrollmentPass, ), ), ) diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/models/AssignTarget.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/models/AssignTarget.kt index 504daf91..ab1f308c 100644 --- a/src/commonMain/kotlin/org/hisp/dhis/rules/models/AssignTarget.kt +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/models/AssignTarget.kt @@ -3,8 +3,8 @@ package org.hisp.dhis.rules.models /** * The target of an ASSIGN rule action, resolved by [RuleAction.assignTarget]. * - * The two targets differ in what they update during evaluation, not only in where the value - * lands: a [Field] assignment produces a [RuleEffect] for the caller to apply, while a + * The two valid targets differ in what they update during evaluation, not only in where the + * value lands: a [Field] assignment produces a [RuleEffect] for the caller to apply, while a * [Variable] assignment updates the program rule variable so that rules evaluated afterwards * see the new value, and additionally produces a [RuleEffect] when the variable is backed by * a data element or tracked entity attribute. @@ -16,10 +16,19 @@ sealed class AssignTarget { * The engine updates the variable in the evaluation state, making the value visible to * the conditions and expressions of rules evaluated afterwards. When the variable is * backed by a data element or tracked entity attribute, a [RuleEffect] targeting the - * backing field is also emitted — during event evaluation for data elements, during - * enrollment evaluation for attributes — with the backing field and its attribute type - * injected into a copy of the action. Calculated values are not backed by a field and - * emit no effect. + * backing field is also emitted, carrying a copy of the action with the backing field + * and its attribute type injected. The copy resolves to [Field] through + * [RuleAction.assignTarget] — matching how the effect is meant to be applied — and + * compares unequal to the configured action. + * + * Data-element backed effects are emitted during event evaluation. Attribute backed + * effects are emitted during enrollment evaluation when the execution has an enrollment + * pass evaluating the rule, and otherwise (single-event evaluation, executions without + * an enrollment, rules scoped to a program stage) during event evaluation, so the + * assignment is never silently dropped. Calculated values are not backed by a field and + * emit no effect. Assigning to a name that is not a defined program rule variable still + * updates the evaluation state, but has no backing field, emits no effect and logs a + * warning, as it may be a configuration mistake. */ data class Variable(val name: String) : AssignTarget() @@ -31,4 +40,12 @@ sealed class AssignTarget { * not updated, so rules evaluated afterwards do not see the new value through them. */ data class Field(val field: String) : AssignTarget() + + /** + * The action is an ASSIGN whose target cannot be resolved: it defines neither a `field` + * nor a `content` value, or its `content` is not a program rule variable reference like + * `#{variableName}`. The engine reports the assignment as an error effect; [reason] + * describes what is wrong with the configuration. + */ + data class Invalid(val reason: String) : AssignTarget() } diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/models/RuleAction.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/models/RuleAction.kt index c676dbd7..db4fc242 100644 --- a/src/commonMain/kotlin/org/hisp/dhis/rules/models/RuleAction.kt +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/models/RuleAction.kt @@ -13,11 +13,11 @@ data class RuleAction( if (data.isNullOrEmpty()) Result.success(null) else runCatching { Expression(data!!, ExpressionMode.RULE_ENGINE_ACTION, false) } - fun content(): String? = values["content"] + fun content(): String? = values[CONTENT] - fun field(): String? = values["field"] + fun field(): String? = values[FIELD] - fun attributeType(): String? = values["attributeType"] + fun attributeType(): String? = values[ATTRIBUTE_TYPE] /** * Resolves the target of this action when it is an ASSIGN action. @@ -25,10 +25,9 @@ data class RuleAction( * Returns null for any other action type. For ASSIGN actions, returns * [AssignTarget.Field] when a non-empty `field` value is present, otherwise * [AssignTarget.Variable] with the program rule variable name taken from `content`, - * accepting both the `#{variableName}` and `A{variableName}` notations. - * - * @throws IllegalArgumentException if the ASSIGN action defines neither a `field` nor a - * `content` value, or if `content` is not a program rule variable reference like + * accepting both the `#{variableName}` and `A{variableName}` notations. Returns + * [AssignTarget.Invalid] when the ASSIGN action defines neither a `field` nor a + * `content` value, or when `content` is not a program rule variable reference like * `#{variableName}`. */ fun assignTarget(): AssignTarget? { @@ -36,16 +35,36 @@ data class RuleAction( val field = field() if (!field.isNullOrEmpty()) return AssignTarget.Field(field) val content = content() - require(!content.isNullOrEmpty()) { - "ASSIGN action must define either 'field' (the data element or attribute to assign to) " + - "or 'content' (the program rule variable to assign to)" + if (content.isNullOrEmpty()) { + return AssignTarget.Invalid( + "ASSIGN action must define either 'field' (the data element or attribute to assign to) " + + "or 'content' (the program rule variable to assign to)", + ) } - require((content.startsWith("#{") || content.startsWith("A{")) && content.endsWith("}")) { - "ASSIGN action content must be a program rule variable reference like #{variableName}, but was: $content" + val name = + if ((content.startsWith("#{") || content.startsWith("A{")) && content.endsWith("}")) { + content.substring(2, content.length - 1) + } else { + null + } + if (name.isNullOrEmpty() || name.contains('{') || name.contains('}')) { + return AssignTarget.Invalid( + "ASSIGN action content must be a program rule variable reference like #{variableName}, but was: $content", + ) } - return AssignTarget.Variable(content.substring(2, content.length - 1)) + return AssignTarget.Variable(name) } + /** + * Copy of this action with [field] and [attributeType] injected into [values], used by + * the engine to emit self-contained effects for assignments to program rule variables + * backed by a data element or tracked entity attribute. + */ + internal fun withAssignBacking( + field: String, + attributeType: String, + ): RuleAction = copy(values = values + listOf(FIELD to field, ATTRIBUTE_TYPE to attributeType)) + override fun compareTo(other: RuleAction): Int = when { this.priority != null && other.priority != null -> { @@ -68,5 +87,9 @@ data class RuleAction( /** Action type of the synthetic effect emitted when a rule or action fails to evaluate. */ const val ERROR = "ERROR" + + private const val CONTENT = "content" + private const val FIELD = "field" + private const val ATTRIBUTE_TYPE = "attributeType" } } diff --git a/src/commonMain/kotlin/org/hisp/dhis/rules/utils/Utils.kt b/src/commonMain/kotlin/org/hisp/dhis/rules/utils/Utils.kt index 818e73e2..90f4a512 100644 --- a/src/commonMain/kotlin/org/hisp/dhis/rules/utils/Utils.kt +++ b/src/commonMain/kotlin/org/hisp/dhis/rules/utils/Utils.kt @@ -5,16 +5,7 @@ import kotlinx.datetime.TimeZone import kotlinx.datetime.toLocalDateTime import kotlin.time.Clock -fun unwrapVariableName(variable: String): String { - if (variable.startsWith("#{") && variable.endsWith("}")) { - return variable.substring(2, variable.length - 1) - } - throw IllegalArgumentException( - "Malformed variable: expected a program rule variable reference like #{variableName}, but was: $variable", - ) -} - -fun currentDate(): LocalDate = +internal fun currentDate(): LocalDate = Clock.System .now() .toLocalDateTime(TimeZone.currentSystemDefault()) diff --git a/src/commonTest/kotlin/org/hisp/dhis/rules/RuleEngineEffectTypesTest.kt b/src/commonTest/kotlin/org/hisp/dhis/rules/RuleEngineEffectTypesTest.kt index f0cfcaf6..bbba71d5 100644 --- a/src/commonTest/kotlin/org/hisp/dhis/rules/RuleEngineEffectTypesTest.kt +++ b/src/commonTest/kotlin/org/hisp/dhis/rules/RuleEngineEffectTypesTest.kt @@ -67,7 +67,7 @@ class RuleEngineEffectTypesTest { @Test fun shouldAssignCorrectlyToAnAttribute() { - val ruleAction = RuleAction("'test_string'", "ASSIGN", + val ruleAction = RuleAction("'test_string'", RuleAction.ASSIGN, mapOf(Pair("content", "A{attribute}"))) val rule = Rule("true", listOf(ruleAction)) val ruleVariable = RuleVariableAttribute( @@ -91,8 +91,8 @@ class RuleEngineEffectTypesTest { } @Test - fun assignToAttributeVariableMustNotProduceEventEffect() { - val ruleAction = RuleAction("'test_string'", "ASSIGN", mapOf(Pair("content", "A{attribute}"))) + fun assignToAttributeVariableMustProduceEffectInEventOnlyEvaluation() { + val ruleAction = RuleAction("'test_string'", RuleAction.ASSIGN, mapOf(Pair("content", "A{attribute}"))) val rule = Rule("true", listOf(ruleAction)) val ruleVariable = RuleVariableAttribute( "attribute", @@ -101,6 +101,8 @@ class RuleEngineEffectTypesTest { "test_attribute_one", RuleValueType.TEXT, ) + // single-event evaluation has no enrollment pass to emit the attribute assignment, + // so the event pass emits it val ruleEffects = RuleEngine.getInstance().evaluate( getTestRuleEvent(RuleEventStatus.ACTIVE), @@ -108,12 +110,15 @@ class RuleEngineEffectTypesTest { emptyList(), RuleEngineContext(listOf(rule), listOf(ruleVariable)), ) - assertEquals(0, ruleEffects.size) + assertEquals(1, ruleEffects.size) + assertEquals("test_string", ruleEffects[0].data) + assertEquals("test_attribute_one", ruleEffects[0].ruleAction.field()) + assertEquals(AttributeType.TRACKED_ENTITY_ATTRIBUTE.name, ruleEffects[0].ruleAction.attributeType()) } @Test fun shouldAssignCorrectlyToADataElementBackedVariable() { - val ruleAction = RuleAction("'test_string'", "ASSIGN", mapOf(Pair("content", "#{variable}"))) + val ruleAction = RuleAction("'test_string'", RuleAction.ASSIGN, mapOf(Pair("content", "#{variable}"))) val rule = Rule("true", listOf(ruleAction)) val ruleVariable = RuleVariableCurrentEvent( "variable", @@ -137,7 +142,7 @@ class RuleEngineEffectTypesTest { @Test fun assignToAttributeVariableMustProduceSingleEffectInMultipleExecution() { - val ruleAction = RuleAction("'test_string'", "ASSIGN", mapOf(Pair("content", "A{attribute}"))) + val ruleAction = RuleAction("'test_string'", RuleAction.ASSIGN, mapOf(Pair("content", "A{attribute}"))) val rule = Rule("true", listOf(ruleAction)) val ruleVariable = RuleVariableAttribute( "attribute", @@ -158,6 +163,70 @@ class RuleEngineEffectTypesTest { assertEquals(1, ruleEffects.single { it.isEnrollment }.ruleEffects.size) } + @Test + fun assignToAttributeVariableInStageScopedRuleMustProduceEventEffect() { + val ruleAction = RuleAction("'test_string'", RuleAction.ASSIGN, mapOf(Pair("content", "A{attribute}"))) + // a stage-scoped rule is excluded from the enrollment pass, so the event pass must + // emit the attribute assignment instead + val rule = Rule(condition = "true", actions = listOf(ruleAction), programStage = "test_program_stage") + val ruleVariable = RuleVariableAttribute( + "attribute", + true, + ArrayList(), + "test_attribute_one", + RuleValueType.TEXT, + ) + val ruleEffects = + RuleEngine.getInstance().evaluateAll( + getTestRuleEnrollment(), + listOf(getTestRuleEvent(RuleEventStatus.ACTIVE)), + RuleEngineContext(listOf(rule), listOf(ruleVariable)), + ) + val allEffects = ruleEffects.flatMap { it.ruleEffects } + assertEquals(1, allEffects.size) + assertEquals("test_attribute_one", allEffects[0].ruleAction.field()) + assertEquals(AttributeType.TRACKED_ENTITY_ATTRIBUTE.name, allEffects[0].ruleAction.attributeType()) + assertEquals(0, ruleEffects.single { it.isEnrollment }.ruleEffects.size) + } + + @Test + fun assignToAttributeVariableMustProduceEventEffectWhenThereIsNoEnrollment() { + val ruleAction = RuleAction("'test_string'", RuleAction.ASSIGN, mapOf(Pair("content", "A{attribute}"))) + val rule = Rule("true", listOf(ruleAction)) + val ruleVariable = RuleVariableAttribute( + "attribute", + true, + ArrayList(), + "test_attribute_one", + RuleValueType.TEXT, + ) + val ruleEffects = + RuleEngine.getInstance().evaluateAll( + null, + listOf(getTestRuleEvent(RuleEventStatus.ACTIVE)), + RuleEngineContext(listOf(rule), listOf(ruleVariable)), + ) + val allEffects = ruleEffects.flatMap { it.ruleEffects } + assertEquals(1, allEffects.size) + assertEquals("test_attribute_one", allEffects[0].ruleAction.field()) + } + + @Test + fun assignToUndeclaredVariableMustNotProduceEffect() { + val ruleAction = RuleAction("'test_string'", RuleAction.ASSIGN, mapOf(Pair("content", "#{undeclared_variable}"))) + val rule = Rule("true", listOf(ruleAction), "rule_uid") + // assigning to a name without a program rule variable keeps the value visible to + // subsequent rules but has no backing field to emit an effect for (a warning is logged) + val ruleEffects = + RuleEngine.getInstance().evaluateAll( + null, + listOf(getTestRuleEvent(RuleEventStatus.ACTIVE)), + getRuleEngineContext(listOf(rule)), + ) + assertEquals(1, ruleEffects.size) + assertEquals(0, ruleEffects[0].ruleEffects.size) + } + @Test fun simpleConditionMustResultInAssignEffectMultipleExecution() { val ruleAction = RuleAction("'test_string'", "ASSIGN", mapOf(Pair("field", "test_data_element"))) diff --git a/src/commonTest/kotlin/org/hisp/dhis/rules/models/RuleActionAssignTargetTest.kt b/src/commonTest/kotlin/org/hisp/dhis/rules/models/RuleActionAssignTargetTest.kt index f0bbf9c8..1764e01c 100644 --- a/src/commonTest/kotlin/org/hisp/dhis/rules/models/RuleActionAssignTargetTest.kt +++ b/src/commonTest/kotlin/org/hisp/dhis/rules/models/RuleActionAssignTargetTest.kt @@ -2,7 +2,7 @@ package org.hisp.dhis.rules.models import kotlin.test.Test import kotlin.test.assertEquals -import kotlin.test.assertFailsWith +import kotlin.test.assertIs import kotlin.test.assertNull import kotlin.test.assertTrue @@ -49,18 +49,28 @@ class RuleActionAssignTargetTest { } @Test - fun assignTargetFailsWhenNeitherFieldNorContentIsSet() { + fun assignTargetIsInvalidWhenNeitherFieldNorContentIsSet() { val action = RuleAction("2+2", RuleAction.ASSIGN) - val exception = assertFailsWith { action.assignTarget() } - assertTrue(exception.message!!.contains("'field'")) - assertTrue(exception.message!!.contains("'content'")) + val target = action.assignTarget() + assertIs(target) + assertTrue(target.reason.contains("'field'")) + assertTrue(target.reason.contains("'content'")) } @Test - fun assignTargetFailsWhenContentIsNotAVariableReference() { + fun assignTargetIsInvalidWhenContentIsNotAVariableReference() { val action = RuleAction("2+2", RuleAction.ASSIGN, mapOf("content" to "test_variable")) - val exception = assertFailsWith { action.assignTarget() } - assertTrue(exception.message!!.contains("#{variableName}")) - assertTrue(exception.message!!.contains("test_variable")) + val target = action.assignTarget() + assertIs(target) + assertTrue(target.reason.contains("#{variableName}")) + assertTrue(target.reason.contains("test_variable")) + } + + @Test + fun assignTargetIsInvalidWhenVariableNameIsEmptyOrMalformed() { + for (content in listOf("#{}", "A{}", "#{a}b}", "#{a{b}", "#{", "A{{}")) { + val action = RuleAction("2+2", RuleAction.ASSIGN, mapOf("content" to content)) + assertIs(action.assignTarget(), "content: $content") + } } } diff --git a/src/jsMain/kotlin/org/hisp/dhis/rules/Logger.kt b/src/jsMain/kotlin/org/hisp/dhis/rules/Logger.kt index 6edf51c3..07e48cf6 100644 --- a/src/jsMain/kotlin/org/hisp/dhis/rules/Logger.kt +++ b/src/jsMain/kotlin/org/hisp/dhis/rules/Logger.kt @@ -3,6 +3,7 @@ package org.hisp.dhis.rules actual fun createLogger(className: String): Logger = Logger( severe = { message -> console.error(message) }, + warning = { message -> console.warn(message) }, fine = { message -> if (RuleEngineJs.verbose) console.info(message) }, diff --git a/src/jvmMain/kotlin/org/hisp/dhis/rules/Logger.kt b/src/jvmMain/kotlin/org/hisp/dhis/rules/Logger.kt index 83b1294e..317e5250 100644 --- a/src/jvmMain/kotlin/org/hisp/dhis/rules/Logger.kt +++ b/src/jvmMain/kotlin/org/hisp/dhis/rules/Logger.kt @@ -2,7 +2,11 @@ package org.hisp.dhis.rules import org.slf4j.LoggerFactory -actual fun createLogger(className: String): Logger{ +actual fun createLogger(className: String): Logger { val javaLogger = LoggerFactory.getLogger(className) - return Logger({message -> javaLogger.error(message)}, {message: String -> javaLogger.debug(message)}) + return Logger( + severe = { message -> javaLogger.error(message) }, + warning = { message -> javaLogger.warn(message) }, + fine = { message -> javaLogger.debug(message) }, + ) } \ No newline at end of file diff --git a/src/nativeMain/kotlin/org/hisp/dhis/rules/Logger.kt b/src/nativeMain/kotlin/org/hisp/dhis/rules/Logger.kt index 52af2b37..23e11206 100644 --- a/src/nativeMain/kotlin/org/hisp/dhis/rules/Logger.kt +++ b/src/nativeMain/kotlin/org/hisp/dhis/rules/Logger.kt @@ -1,5 +1,9 @@ package org.hisp.dhis.rules actual fun createLogger(className: String): Logger { - return Logger({ message -> println(message) }, { message: String -> println(message) }) + return Logger( + severe = { message -> println(message) }, + warning = { message -> println(message) }, + fine = { message: String -> println(message) }, + ) } \ No newline at end of file