From 248a28fb1180d96356e9cbeb7349062cdb430eec Mon Sep 17 00:00:00 2001 From: delchev Date: Wed, 9 Sep 2026 15:38:05 +0300 Subject: [PATCH] intent: a row outside every key-tuple is not guarded, and a rule match needs a literal (#7180) #7166 made `Criteria.eq` / `ne` null-safe, which is right for the idempotency guard it was written for and changed two generated call sites that pass a possibly-null value without saying what they mean by it. The aggregate guard is the one that matters. `aggregates:` is explicit that a source row with any grouping key unset belongs to no tuple and is ignored - the generated aggregate handler says so and materialises no target row for such a row - but the guard that recomputes the same total said nothing about it, and relied on SQL three-valued logic for the answer. So a movement authored without its warehouse used to be weighed against zero and rejected for breaching a pool that does not exist; since #7166 it is weighed against the sum of every other tuple-less row, a pool no aggregate row materialises. Both readings are wrong in the same way: a record that contributes to no total can breach no minimum, so the guard does not apply to it at all - no throw, no marker flip, no forced status - and the generated code now says that in a null test of its own instead of leaving it to how a comparison with no value happens to render. The sibling roll-up capacity guard has always skipped a null FK this way. The skip is logged at debug, naming the aggregate, so a record that is never guarded can be traced to the key it lacks. The posting's rule lookup turns out not to be exposed to the change at all: the value bound there is the authored literal of `rule.match`, never a value read off the source, so it is never null and the null-safe `eq` cannot reach it. What it was exposed to is an authoring accident the parser let through - a blank literal, which emits a lookup for the empty string, matches no rule row, and sends every source document to the unposted worklist with the intent, the generation and the publish all green. That is now refused where it is authored, and the template says why no runtime guard belongs there (a value omitted outright was already refused as an empty selector: the typed mapping drops the null entry). Verified through IntentEmissionCoverageIT, which renders both templates and compiles the generated client Java (327 units, no failures). Fixes #7180 Co-Authored-By: Claude Opus 5 --- .../generator/edm/EdmIntentGenerator.java | 3 ++ .../intent/parser/IntentParser.java | 29 ++++++++++++ .../main/resources/intent-assistant-guide.md | 5 ++- .../generator/edm/EdmIntentGeneratorTest.java | 3 ++ .../intent/parser/IntentParserTest.java | 29 ++++++++++++ .../data/Repository.java.template | 45 +++++++++++++------ .../events/Posting.java.template | 3 ++ .../tests/api/IntentEmissionCoverageIT.java | 7 +++ 8 files changed, 110 insertions(+), 14 deletions(-) diff --git a/components/engine/engine-intent/src/main/java/org/eclipse/dirigible/components/intent/generator/edm/EdmIntentGenerator.java b/components/engine/engine-intent/src/main/java/org/eclipse/dirigible/components/intent/generator/edm/EdmIntentGenerator.java index 70dc1572bd0..1f0ff64dd85 100644 --- a/components/engine/engine-intent/src/main/java/org/eclipse/dirigible/components/intent/generator/edm/EdmIntentGenerator.java +++ b/components/engine/engine-intent/src/main/java/org/eclipse/dirigible/components/intent/generator/edm/EdmIntentGenerator.java @@ -2149,6 +2149,9 @@ private static List> buildChecks(EntityIntent entity, List + * The selector is rendered into the generated posting handler AS a Java literal, so a blank one + * emits {@code .eq("", "")} - a lookup that matches no rule row and therefore leaves every + * source document silently on the unposted worklist, with the intent, the generation and the + * publish all green. Refused here so the accident is named where it is authored (#7180). A value + * omitted outright ({@code documentType:} with nothing after it) never reaches this method: the + * typed mapping drops the null entry, so the empty selector is caught by the single-selector rule + * above. + * + * @param subject the message prefix naming the posting + * @param match the single-entry match selector + * @param issues collected validation issues + */ + private static void validateRuleMatchHasALiteral(String subject, java.util.Map match, List issues) { + Map.Entry selector = match.entrySet() + .iterator() + .next(); + Object value = selector.getValue(); + if (value == null || String.valueOf(value) + .isBlank()) { + issues.add(subject + " rule.match [" + selector.getKey() + + "] has no value - a determination rule selects on a literal, and an empty one matches no rule row"); + } + } + /** * The determination rule's {@code match} column must not be a translated one. The selector is a * literal authored in the model and compared against the rule row's own column, so the moment that @@ -7039,6 +7067,7 @@ private static void validatePostings(IntentModel model, Set usesAliases, issues.add(subject + " rule.match must be a single `column: literal` selector"); } else { validateRuleMatchIsNotTranslated(subject, ruleEntity, (java.util.Map) match, issues); + validateRuleMatchHasALiteral(subject, (java.util.Map) match, issues); } } // items diff --git a/components/engine/engine-intent/src/main/resources/intent-assistant-guide.md b/components/engine/engine-intent/src/main/resources/intent-assistant-guide.md index 848c261d494..dd44ffd53fb 100644 --- a/components/engine/engine-intent/src/main/resources/intent-assistant-guide.md +++ b/components/engine/engine-intent/src/main/resources/intent-assistant-guide.md @@ -593,7 +593,10 @@ field may declare: references, arithmetic over the SOURCE's fields, or - for a to-one relation cell - a bare SOURCE relation name whose FK is copied onto the line; a row `when` is ` ==|!= `. A missing rule row or null referenced column SKIPS the posting (the unposted worklist = final-status - documents with no back-referencing target), never throws. + documents with no back-referencing target), never throws. `rule.match` is a single + `column: literal` selector and the literal must be there - an empty one is refused at parse, because + it is rendered into the handler as the authored literal and would select no rule row at all, leaving + every source document on the worklist with nothing failing anywhere. **Conditional rule column** - when the account must be chosen by a source value (a payment posts to the bank account for a transfer, the cash account for cash), a single row selects the rule column by a classifier instead of duplicating the row per case (the `by`/`cases`/`default` shape the diff --git a/components/engine/engine-intent/src/test/java/org/eclipse/dirigible/components/intent/generator/edm/EdmIntentGeneratorTest.java b/components/engine/engine-intent/src/test/java/org/eclipse/dirigible/components/intent/generator/edm/EdmIntentGeneratorTest.java index b81d2f5e1e4..ea91faf7576 100644 --- a/components/engine/engine-intent/src/test/java/org/eclipse/dirigible/components/intent/generator/edm/EdmIntentGeneratorTest.java +++ b/components/engine/engine-intent/src/test/java/org/eclipse/dirigible/components/intent/generator/edm/EdmIntentGeneratorTest.java @@ -874,6 +874,9 @@ void guardCheckEmitsKeyedAggregateGuard() { assertEquals("0", guard.get("minimum")); assertEquals("Insufficient stock", guard.get("message")); assertEquals("INVENTORY_BLOCK_NEGATIVE_STOCK", guard.get("enabledBy")); + // The guard names the aggregate it protects, so the skip it logs for a row that belongs to no + // key-tuple can be traced back to a declaration (#7180). + assertEquals("onHand", guard.get("aggregate")); List> keys = (List>) guard.get("keys"); assertEquals(2, keys.size()); assertEquals("Product", keys.get(0) diff --git a/components/engine/engine-intent/src/test/java/org/eclipse/dirigible/components/intent/parser/IntentParserTest.java b/components/engine/engine-intent/src/test/java/org/eclipse/dirigible/components/intent/parser/IntentParserTest.java index 67d86509886..712b5951de3 100644 --- a/components/engine/engine-intent/src/test/java/org/eclipse/dirigible/components/intent/parser/IntentParserTest.java +++ b/components/engine/engine-intent/src/test/java/org/eclipse/dirigible/components/intent/parser/IntentParserTest.java @@ -1227,6 +1227,35 @@ void conditionalRuleUnknownClassifierIsRejected() { "expected an unknown-classifier issue, got: " + ex.getIssues()); } + /** + * A determination rule's {@code match} needs a literal that says something (#7180). + * + *

+ * The value is rendered into the generated posting handler AS an authored Java literal, so a blank + * one emits a lookup on the empty string - it matches no rule row, and every source document is + * left silently on the unposted worklist with the parse, the generation and the publish all green. + * A value omitted outright is already refused as an empty selector (the typed mapping drops a null + * entry, so the selector is not there at all); both readings now fail where they are authored. + */ + @Test + void postingRuleMatchWithNoLiteralIsRejected() { + String item = "{ Account: rule(BankAccount), debit: \"Amount\" }"; + String blank = conditionalRulePosting(item).replace("match: { documentType: \"Payment\" }", "match: { documentType: \"\" }"); + IntentValidationException blankEx = assertThrows(IntentValidationException.class, () -> IntentParser.parse(blank)); + assertTrue(blankEx.getIssues() + .stream() + .anyMatch(i -> i.contains("rule.match [documentType] has no value")), + "expected a blank rule.match issue, got: " + blankEx.getIssues()); + String omitted = conditionalRulePosting(item).replace("match: { documentType: \"Payment\" }", "match: { documentType: }"); + IntentValidationException omittedEx = assertThrows(IntentValidationException.class, () -> IntentParser.parse(omitted)); + assertTrue(omittedEx.getIssues() + .stream() + .anyMatch(i -> i.contains("rule.match must be a single `column: literal` selector")), + "expected an empty-selector issue, got: " + omittedEx.getIssues()); + // ...and the authored literal still parses, so nothing written before this changes. + IntentParser.parse(conditionalRulePosting(item)); + } + /** The event declares exactly one trigger - onTransition XOR onCreate. */ @Test void postingEventDeclaresExactlyOneTrigger() { diff --git a/components/template/template-application-dao-java/src/main/resources/META-INF/dirigible/template-application-dao-java/data/Repository.java.template b/components/template/template-application-dao-java/src/main/resources/META-INF/dirigible/template-application-dao-java/data/Repository.java.template index 9bee867b124..511ae0f4e1f 100644 --- a/components/template/template-application-dao-java/src/main/resources/META-INF/dirigible/template-application-dao-java/data/Repository.java.template +++ b/components/template/template-application-dao-java/src/main/resources/META-INF/dirigible/template-application-dao-java/data/Repository.java.template @@ -178,7 +178,8 @@ package gen.${javaGenFolderName}.data.${javaPerspectiveName}; ## Aggregate guard (intent `checks: kind: guard`): reject a create/update whose post-state would drop ## a keyed aggregate sum below its minimum (negative stock, credit limit). Recomputed synchronously from ## THIS entity's store for the incoming row's key-tuple (race-free, not the async-maintained target), -## excluding this row on update, then adding the incoming value. Optionally gated by a config key. +## excluding this row on update, then adding the incoming value. Optionally gated by a config key. A row +## with any grouping key null belongs to no key-tuple and is not guarded at all (#7180). #macro(aggregateGuardCheck) #if($guardChecks && $guardChecks.size() > 0) #foreach($guard in $guardChecks) @@ -186,17 +187,32 @@ package gen.${javaGenFolderName}.data.${javaPerspectiveName}; if ("true".equalsIgnoreCase(org.eclipse.dirigible.sdk.core.Configurations.get("${guard.enabledBy}", ""))) { #end { - java.math.BigDecimal guardSum = java.math.BigDecimal.ZERO; - for (${name}Entity aggRow : findAll(Criteria.create()#foreach($k in $guard.keys).eq("${k.key}", entity.${k.key})#end)) { - if (entity.${guard.pk} != null && entity.${guard.pk}.equals(aggRow.${guard.pk})) { - continue; - } - if (aggRow.${guard.sumField} != null) { - guardSum = guardSum.add(aggRow.${guard.sumField}); + // A row with any grouping key null belongs to NO key-tuple, and the aggregate this guards + // ignores it for exactly that reason (the generated aggregate handler's own contract), so + // it contributes to no sum and can breach no minimum: there is nothing to guard and the + // row passes. Said explicitly, because the lookup below no longer says it by accident - + // Criteria.eq is NULL-SAFE (#7134), so a null key that used to match no row now matches + // the null group, weighing the write against a pool of tuple-less rows that no aggregate + // row materialises (#7180). The sibling roll-up capacity guard skips on a null FK the + // same way. + boolean guardKeyed = #foreach($k in $guard.keys)#if($foreach.first)entity.${k.key} != null#else && entity.${k.key} != null#end#end; + boolean guardWithin = true; + if (guardKeyed) { + java.math.BigDecimal guardSum = java.math.BigDecimal.ZERO; + for (${name}Entity aggRow : findAll(Criteria.create()#foreach($k in $guard.keys).eq("${k.key}", entity.${k.key})#end)) { + if (entity.${guard.pk} != null && entity.${guard.pk}.equals(aggRow.${guard.pk})) { + continue; + } + if (aggRow.${guard.sumField} != null) { + guardSum = guardSum.add(aggRow.${guard.sumField}); + } } + java.math.BigDecimal guardIncoming = entity.${guard.sumField} == null ? java.math.BigDecimal.ZERO : entity.${guard.sumField}; + guardWithin = guardSum.add(guardIncoming).compareTo(new java.math.BigDecimal("${guard.minimum}")) >= 0; + } else { + LOG.debug("Aggregate guard [${guard.aggregate}] skipped for a ${name} with a null grouping key" + + " - the row belongs to no key-tuple of the aggregate and contributes to no sum"); } - java.math.BigDecimal guardIncoming = entity.${guard.sumField} == null ? java.math.BigDecimal.ZERO : entity.${guard.sumField}; - boolean guardWithin = guardSum.add(guardIncoming).compareTo(new java.math.BigDecimal("${guard.minimum}")) >= 0; #if($guard.outcome == "task") // outcome: task - do NOT fail the write. Stamp the marker so this entity's process decision // can route the record to a hold/review step (the order is accepted, then parked). @@ -258,7 +274,7 @@ import org.eclipse.dirigible.sdk.utils.Calc; #if($haveCalculatedPropertyAction) import org.eclipse.dirigible.sdk.component.Beans; #end -#if($reportedOnUpdate.size() > 0) +#if($reportedOnUpdate.size() > 0 || ($guardChecks && $guardChecks.size() > 0)) import org.eclipse.dirigible.sdk.log.Logger; import org.eclipse.dirigible.sdk.log.Logging; #end @@ -305,8 +321,11 @@ ${importsCode} @Component("${javaGenFolderName}_${name}Repository") public class ${name}Repository extends JavaRepository<${name}Entity> { -#if($reportedOnUpdate.size() > 0) - /** Reports a system-owned column a full-row update() carried a different value for - see update(). */ +#if($reportedOnUpdate.size() > 0 || ($guardChecks && $guardChecks.size() > 0)) + /** + * Reports a system-owned column a full-row update() carried a different value for - see update() - + * and an aggregate guard skipped for a row that belongs to no key-tuple - see save(). + */ private static final Logger LOG = Logging.getLogger("gen.${javaGenFolderName}.data.${javaPerspectiveName}.${name}Repository"); #end diff --git a/components/template/template-application-events-java/src/main/resources/META-INF/dirigible/template-application-events-java/events/Posting.java.template b/components/template/template-application-events-java/src/main/resources/META-INF/dirigible/template-application-events-java/events/Posting.java.template index 9ecae8b7c18..23695ded056 100644 --- a/components/template/template-application-events-java/src/main/resources/META-INF/dirigible/template-application-events-java/events/Posting.java.template +++ b/components/template/template-application-events-java/src/main/resources/META-INF/dirigible/template-application-events-java/events/Posting.java.template @@ -76,6 +76,9 @@ public class ${className}Posting implements MessageHandler { #if($hasRule) // Determination rule first (the item derivation depends on it): a missing rule or a null // referenced column SKIPS - the document stays on the unposted worklist, never half-posted. + // The match value is the AUTHORED literal of rule.match, not a value read off the source, so it + // is never null here and the NULL-SAFE Criteria.eq (#7134) cannot turn this lookup into a match + // on the rule table's null group; an empty authored literal is refused at parse (#7180). java.util.List ruleRows = new gen.${javaGenFolderName}.data.${ruleJavaPerspective}.${ruleEntity}Repository().findAll( Criteria.create().eq("${ruleMatchProperty}", ${ruleMatchValueJava})); diff --git a/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/IntentEmissionCoverageIT.java b/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/IntentEmissionCoverageIT.java index 13a0f2faf07..9043da2aac6 100644 --- a/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/IntentEmissionCoverageIT.java +++ b/tests/tests-integrations/src/main/java/org/eclipse/dirigible/integration/tests/api/IntentEmissionCoverageIT.java @@ -2320,6 +2320,13 @@ private void assertEmission() { // both PERSIST the row and mark it instead of throwing. assertTrue(ledgerRepository.contains("Criteria.create().eq(\"Person\", entity.Person).eq(\"Unit\", entity.Unit)"), "a guard must recompute its aggregate over the incoming row's full key-tuple"); + // ...and only for a row that HAS a full key-tuple. Criteria.eq is null-safe (#7134), so a null + // key no longer matches nothing - it matches the null group, a pool of tuple-less rows that the + // aggregate handler ignores by contract and materialises no target row for (#7180). + assertTrue(ledgerRepository.contains("boolean guardKeyed = entity.Person != null && entity.Unit != null;"), + "a guard must test every grouping key for null before it recomputes: " + ledgerRepository); + assertTrue(ledgerRepository.contains("boolean guardWithin = true;") && ledgerRepository.contains("if (guardKeyed) {"), + "a row belonging to no key-tuple must pass the guard untouched - no throw, no marker, no forced status"); assertTrue(ledgerRepository.contains("throw new ValidationException(\"Insufficient balance\")"), "outcome block must fail the write with the authored message"); assertTrue(ledgerRepository.contains("Configurations.get(\"EMISSION_BLOCK_NEGATIVE_LEDGER\""),