From 191aba391713d720f18ca7e3f646cf3c7ac1bf1d Mon Sep 17 00:00:00 2001 From: ThuF Date: Thu, 10 Sep 2026 17:15:41 +0300 Subject: [PATCH] intent: a decision whose then:/else: targets itself is refused like next: (#7292) #7274 (#7226) refused `next: ` on a process step at parse - a self-targeting sequence flow the engine spins on. The same spin one key over was still accepted: `checkDecisionTarget` only checked that the target EXISTS, so `- { name: rated, kind: decision, if: ..., then: rated, else: done }` emitted a sequence flow from the exclusive gateway to itself. A gateway has no wait state, so Flowable spins on it exactly as it did on the `next` self-loop - the spin that made the ResilienceBpmnTest fixture bug worth a parse rule in the first place. `then`/`else` naming the decision itself is now refused with the same sentence as `next`, located on the decision. Deliberately untouched, as the issue asks: `onError: ` (an unbounded retry, arguable semantics - `retry:` exists for it), a timer boundary's `then: ` on a userTask (re-open the task - a legitimate pattern that must stay legal), and multi-step cycles through a wait state. The two new negative tests sit next to `aNextTargetingItselfIsRejected` so the pair stays legible, with a third pinning that a decision branching back through a wait state still parses. Verified: red-first - both new refusal tests fail against the unpatched parser ("Expected IntentValidationException to be thrown, but nothing was thrown") and pass with it; engine-intent parser + generator suites green (642 tests, 0 failures, incl. StepResilienceIntentTest 23/23). Fixes #7292 Co-Authored-By: Claude Opus 5 --- components/engine/engine-intent/CLAUDE.md | 2 +- .../intent/parser/IntentParser.java | 17 ++++++-- .../parser/StepResilienceIntentTest.java | 41 ++++++++++++++++++- 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/components/engine/engine-intent/CLAUDE.md b/components/engine/engine-intent/CLAUDE.md index 84066c2a4c7..dbcb03afbf6 100644 --- a/components/engine/engine-intent/CLAUDE.md +++ b/components/engine/engine-intent/CLAUDE.md @@ -404,7 +404,7 @@ Semantics worth knowing: - **Calculated-field actions + entity `imports:` — server-side call-out for logic too custom to model.** Besides the neutral arithmetic `calculatedOnCreate`/`calculatedOnUpdate` expression (run by the SDK `Calc` evaluator, previewed live in the UI), a field may declare `calculatedActionOnCreate`/`calculatedActionOnUpdate` naming a Java class — a `@Component implements org.eclipse.dirigible.sdk.db.CalculatedField` (`T calculate(E entity)`). `EdmIntentGenerator.propertyMap` emits these as `calculatedActionOnCreate`/`OnUpdate` on the property (and `isCalculated()` now counts an action, so the property is marked calculated even with no expression); the **Java DAO template** (`template-application-dao-java/data/Repository.java.template`) gives the action **precedence** over the expression per slot and emits `entity. = Beans.get(.class).calculate(entity);`, importing `Beans` only when an action is present and `Calc` only when an expression is. An action runs **server-side only** (no client mirror). To reference the action by simple name, the entity declares `imports:` (a multi-line string of Java `import ...;` lines); `EdmIntentGenerator` Base64-encodes it into the `.model` entity's `importsCode` (matching the EDM editor's serialization), which the DAO template's `ModelParameterProcessor` decodes and emits into the repository's import block. The implementation is **hand-written under the project's `custom/` folder** (never `gen/`) — the intent layer emits no Java. **A to-one RELATION may declare the same two keys (since 2026-08-12)** — `RelationIntent.calculatedActionOnCreate`/`OnUpdate`, emitted by the shared `EdmIntentGenerator.putCalculatedAction` from BOTH relation builders (`relationProperty` and `crossModelRelationProperty`), so the FK property carries `isCalculatedProperty` + the action names. **The DAO template needed no change**: a relation IS an ordinary property in the `.model` (the FK column, typed to the target's key), so the shared property loop already emits `entity. = Beans.get(.class).calculate(entity);` against the `Integer` FK. This closes the only default a to-one relation could not express: `init:` is a literal seed id, `dependsOn` is a UI-only cascade that never fires on a server-side create, and `setRelationField` takes a literal id — none can read a value off ANOTHER record (a document's currency from its company's base currency). Before this the key was accepted and **silently dropped** — the typed mapping is Gson, which ignores unknown properties — the #6541 family. Parser rejects it on a collection relation, a composition parent and an EntityStatus badge. Covered by `EdmIntentGeneratorTest.relationCalculatedActionEmitsTheServerSideCallOutOnBothRelationBuilders` + two `IntentParserTest` cases. **Covered by `IntentEmissionCoverageIT`** — the fixture's `Quote.Tariff` relation declares `calculatedActionOnCreate: QuoteTariffAction`, with the required hand-written `custom/QuoteTariffAction.java` written into the emission-test project by the IT itself (a generated repository referencing a missing class fails the whole client-Java batch and every REST assertion in that gate, so the class is part of the fixture rather than assumed). All three layers are asserted: the `.model` property attribute, the repository's `entity.Tariff = Beans.get(QuoteTariffAction.class).calculate(entity);` assignment plus the decoded `imports:` line, and the runtime — a create that OMITS the FK comes back carrying the `base`-flagged tariff (seeded as row 2, deliberately not the first row, so no accidental default can satisfy it), while a create that SUPPLIES one keeps it, which is the "an explicit pick always wins" half of the contract. The model-editor equivalents are the entity **Imports** tab and the property **Calculation** tab's *Action class* inputs (`editor-entity`). Worked example: `dirigiblelabs/sample-intent-multi-model` `sales-invoices` — `SalesInvoice.number` calls `custom/sales_invoices/SalesInvoiceNumberAction.java` (replacing the old inline `UUID.randomUUID()` expression). The SDK interface ships in `api-modules-java` (`org.eclipse.dirigible.sdk.db.CalculatedField`). - **Resolver-path user-task assignment (`assignee: { path: …, fallback: … }`).** A `userTask`'s `assignee` is normally a role/candidate-group literal, or the literal `personal` (the record owner, seeded at process START into `__personalUser` by the trigger listener). The map form routes the task to a person the RECORD names — `assignee: { path: employee.manager, fallback: manager }` — by walking to-one relations off the trigger entity. `ProcessAssigneeSupport` owns the whole feature: the walk (each segment a `manyToOne`/`oneToOne`, the first of the trigger entity and each further one of the previous target, ending at an entity that declares `identity:` — the same mapping the personal surfaces use), the `assignees` glue collection, and the handler/variable naming (`ResolveAssignee` — the `Resolve*` delegate family, NOT `Assign*`, which the effective-dated `resolves` lookup already owns — publishing `__assignee_`). **A cross-model relation may only be the LAST hop:** a projection carries the target's own properties (so its `identityProperty` is resolvable through `CrossModelSupport`, checked at generation time exactly like a cross-model `personal` owner) but not its relations, so there is nothing to walk on from there — the parser says so by name. **`fallback` is required**, which is what makes the unresolvable case total: the generated delegate publishes the variable as `null` FIRST and on every early return (an ABSENT process variable makes Flowable's `${…}` assignee expression throw at task creation; a null one just leaves the task unassigned), so a null hop / missing record / blank identity yields a task the fallback candidate group can still claim, never one nobody can see. Resolution is at **task entry**, not process start — the delegate is inserted as a service task right before the task, so a relation an earlier step of the same process set is visible; that is the whole reason this is not folded into the `__personalUser` seeding. `Assignee.java.template` (`template-application-events-java`) walks `hop0 … hopN` through `findById(fk)` — the FK read off a loaded entity is already the target PK's exact type, so no `Number` accessor dance is needed past the owner (whose id comes from the id-only process context and does need one). Parse-time coverage: `AssigneePathIntentTest`; emission + BPMN: `GlueAssigneesTest`; end-to-end through the real template: `IntentEngineIT` (the fixture's `cfoReview` walks `salesRep.manager`). - **A flow into a step lands on its FIRST node, not on the step** (`BpmnIntentGenerator.TargetResolver.entry`). Several features insert `JavaDelegate` service tasks *before* a step — a `relation.field` resolver, an own-field loader, an `expire:` re-read, an assignee walk — and those delegates are what make the step evaluable. `entry` used to return the raw step id outside a parallel region, on the reasoning that the linear chain connects the delegates by adjacency; that holds for fall-through but NOT for a **jump** — a decision's `then`/`else`, another step's `next` — which sailed straight past them. Harmless-looking for a resolver (a stale variable), fatal for an assignee walk (an unresolved `${…}` expression fails task creation), so `entry` is now unconditional and the two linear-chain jump sites resolve through it. -- **Decision steps**: `if` + `then` are mandatory; `else` is optional and receives the gateway-default flow (so the conditioned branch can actually be skipped - without `else` the default falls through to the next step in the chain). `then`/`else` must name a declared step or the literal `end`; the parser validates this so a typo fails at parse time instead of producing BPMN Flowable rejects. +- **Decision steps**: `if` + `then` are mandatory; `else` is optional and receives the gateway-default flow (so the conditioned branch can actually be skipped - without `else` the default falls through to the next step in the chain). `then`/`else` must name a declared step or the literal `end`, and **neither may name the decision itself** - an exclusive gateway has no wait state, so a branch back to the gateway emits a self-targeting sequence flow Flowable spins on, exactly as a step's `next: ` does (#7226 / #7292); it is refused with the same sentence. A multi-step cycle THROUGH a wait state is legitimate and untouched, as are `onError: ` (an unbounded retry) and a timer boundary's `then: ` (re-open the task). The parser validates all of this so a typo - or a spin - fails at parse time instead of producing BPMN Flowable rejects or spins on. - **`setField` service task + `next` step routing (declarative field-set glue).** A `serviceTask` with `args: { setField: , value: }` sets a `string`/`text` field of the process's **trigger entity** to a literal value, generated as a `gen/events//.java` `JavaDelegate` (`SetFieldSupport` → the `setters` glue collection → `SetField.java.template`) instead of scaffolding a hand-written `custom.` stub - it persists the set column via the targeted single-column `updateProperty(id, "", value)` (a workflow write, not a user edit, so it must not re-fire `onUpdate` reactions; only the set column is in the UPDATE statement, so a concurrent write to any other column cannot be reverted). The canonical use is an approve/reject outcome: the form completes the task with the chosen `action` as a process variable, a `decision` branches on `action == 'approve'`, and the two branches are `setField` tasks (`status=ACTIVE` / `status=REJECTED`). **`args: { next: }`** on any step overrides its linear successor - needed because the BPMN generator builds a **linear** chain, so without it the first branch (`activate`) would fall through into the second (`reject`); `next: done` makes the branches converge. The `then`/`else` fall-through is deliberately NOT auto-converted to a diamond (LoanApproval's `curatorReview` relies on falling through to `notifyMember`), so convergence is explicit via `next`. Scope: literal string values only (the parser validates `setField` is a string/text field of the trigger entity and that `value` is present; `next` must name a declared step or `end`). Non-string fields and expression values are future work. - **`setRelationField` (set a status modelled as a to-one relation).** The generic counterpart to `setField` for a status that is a **FK to a settings/nomenclature entity** (e.g. `Status`) rather than a string column: `args: { setRelationField: , value: }` sets the relation's FK property to the integer **seed id** (unquoted — `entity. = ;`), via the **same** `SetFieldSupport` → `setters` glue → `SetField.java.template` path (the `Setter` carries a `relation` flag; the template branches `#if($relation == "true")` to emit the unquoted assignment). Unlike `setField` (serviceTask only), `setRelationField` is allowed on a **serviceTask** (bound directly by `appendServiceTask`, like `setField`) **and** on a **userTask** (the BPMN inserts the setter `JavaDelegate` right after the task — exactly like the Writer — so e.g. the Approve user task sets `Status=APPROVED` the moment it completes; `BpmnIntentGenerator` builds a `setterByProcessTask` map of user-task setters and `augmentWithResolvers` appends them in a `[writer, setter]` after-task chain, carrying `next` onto the last delegate). The parser validates the relation is a `manyToOne`/`oneToOne` of the trigger entity and that `value` is an integer id. This replaced an unimplemented `setStatus` idea — there is no `setStatus` keyword. **The `-transitioned` topic:** both setter shapes persist via the targeted `updateProperty` (no `-updated` re-fire, deliberately) but DO publish the fresh entity JSON (re-loaded after the write) on `---transitioned` — the dedicated status-reached channel. **Publication is deferred to the end of the synchronous BPMN chain** (`Process.executeAfterCommit` — a Flowable COMMITTED transaction listener): service tasks that follow the setter in the same chain (a number-generation delegate) complete before any consumer can react, so a consumer that re-loads the source by id observes their writes instead of racing them (an auto-posted journal entry used to catch the create-time UUID placeholder as `documentNumber`). **On a mid-chain FAILURE this is correct by design, not a lost event:** the status write commits in its own session (client-Java writes are per-operation — cloud-native, no ambient cross-step transaction), while the deferred publish only fires on the Flowable COMMITTED event; if a later task rolls the chain back, the publish deliberately does NOT fire, and the transition is unwound by the flow's compensation / error path (a compensating status set), never by a DB rollback. Checks + Saga-style compensation, NOT distributed transactions, are the consistency model here — do not "fix" this by enrolling the entity write in the BPMN transaction. Reactions/notifications keep binding `-updated`/create topics and never see it (no loops); a posting-glue or integration consumer binds `-transitioned` to observe workflow transitions that are otherwise event-silent (the Wave-0 accounting spike's core finding: an Issue step was invisible to every entity event). Emitted by `SetField.java.template`; covered by the `set_field_glue_...` IT assertion. **Placement rule (applies to `setField` too, documented for the editor AI in `intent-assistant-guide.md`):** when a task is **followed by a decision** (Approve/Reject), put the status set on a **`serviceTask` on the chosen branch**, NOT on the user task — setting it on the task makes a Reject transition `DRAFT → APPROVED → CANCELLED` (an artificial APPROVED hop) before the cancel branch overrides it. Set on the task **only** for a **single-action** task with no following decision (no branch ⇒ no transient state). The `sales-invoices` showcase follows this (Approve task has no set; an `activate` serviceTask sets APPROVED on the approve branch; single-action `issue`/`send` set on the task). - **`delegate` service task (call a reusable, author-named client `JavaDelegate`).** `args: { delegate: , fields: { : , ... } }` binds a serviceTask to a hand-written client delegate via **`flowable:class`** (NOT the `${JavaTask}` dispatcher). This is the *fourth* service-task shape alongside `setField`/`setRelationField` (→ generated `gen.events..` via `${JavaTask}`+`handler`), `call` (→ `${JSTask}` TS handler), and the bare fallback (→ `custom.` + a scaffolded stub). Why `flowable:class` and not `${JavaTask}`: `DirigibleJavaCallDelegate` (`${JavaTask}`) reads only its `handler` field, so it can't pass parameters; `flowable:class` (resolved through `BpmFlowableConfig`'s `ClientAwareClassLoader`, which consults the client class loader) lets Flowable **inject** the declared `fields` as delegate fields — so one *general* delegate serves many steps. (Since #7058 the delegate's own **collaborators** are wired by the client bean container on BOTH paths — a constructor or `@Inject` field over the project's `@Component`s — so `fields:` is about per-step *parameters*, not about reaching services; a delegate must still never be a `@Component` itself.) `BpmnIntentGenerator.appendServiceTask` branches to `appendDelegateServiceTask` (emitting `flowable:class` + one `` per `fields` entry, in declaration order); `ServiceTaskHandlerGenerator` **skips** delegate steps (no `custom/` stub — the developer owns the class, which may live in *another* published project since client Java compiles in one cross-project batch). Parser: `delegate` is serviceTask-only, mutually exclusive with `setField`/`setRelationField`/`call`, and `fields` values must be scalars. **Worked example:** `sample-intent-multi-model` — `sales-invoices`' `generateNumber` step (after `issue`) binds `custom.sales_invoices.DocumentNumberGeneratorDelegate` with `fields: { type: "Sales Invoice" }`. **The delegate lives in the document's OWN project** (sales-invoices), because it must load/save the invoice through the generated `SalesInvoiceRepository` (validations, events, i18n — NEVER the generic `Store`; see the engine-java guide's repository-only rule): it reads the record id from the `Id` process variable, `findById`s it, asks the reusable `custom.numbers.DocumentNumberGenerator` (in the `numbers` project — a codbex-number-generator port over its own `NumberRepository`, entity-agnostic) for the next formatted number of the injected `type`, sets `entity.Number`, and persists via `updateWithoutEvent` (workflow write). Only the entity-agnostic generator is shared; the entity-touching delegate is per-project. Covered by `IntentEngineIT.delegate_service_task_binds_a_client_java_delegate_via_flowable_class_with_injected_fields`. diff --git a/components/engine/engine-intent/src/main/java/org/eclipse/dirigible/components/intent/parser/IntentParser.java b/components/engine/engine-intent/src/main/java/org/eclipse/dirigible/components/intent/parser/IntentParser.java index 34bc1f74208..ebd0d0424ca 100644 --- a/components/engine/engine-intent/src/main/java/org/eclipse/dirigible/components/intent/parser/IntentParser.java +++ b/components/engine/engine-intent/src/main/java/org/eclipse/dirigible/components/intent/parser/IntentParser.java @@ -6852,9 +6852,9 @@ private static RelationIntent toOneRelationByName(EntityIntent entity, String na /** * Decision steps must declare {@code if} and {@code then}; {@code then} and the optional - * {@code else} must reference a declared step of the same process (or the literal {@code end}). - * Without this check a typo silently produces BPMN that Flowable rejects on the next - * synchronization cycle. + * {@code else} must reference a declared step of the same process (or the literal {@code end}), and + * neither may name the decision itself. Without this check a typo silently produces BPMN that + * Flowable rejects on the next synchronization cycle. */ private static void validateDecisionTargets(ProcessIntent process, List issues) { Set stepNames = new HashSet<>(); @@ -6883,7 +6883,16 @@ private static void validateDecisionTargets(ProcessIntent process, List private static void checkDecisionTarget(ProcessIntent process, StepIntent step, String arg, String target, Set stepNames, List issues) { - if (!isRoutingLiteral(target) && !stepNames.contains(target)) { + if (isRoutingLiteral(target)) { + return; + } + if (target.equals(step.getName())) { + // An exclusive gateway has no wait state, so a branch back to the gateway emits a + // self-targeting sequence flow the engine spins on - the `next: ` spin one key over + // (dirigible #7226 / #7292). A cycle THROUGH a wait state stays legal. + issues.add("process [" + process.getName() + "] decision [" + step.getName() + "] `" + arg + + "` targets itself - a self-loop that never advances"); + } else if (!stepNames.contains(target)) { issues.add("process [" + process.getName() + "] decision [" + step.getName() + "] `" + arg + "` references unknown step [" + target + "]"); } diff --git a/components/engine/engine-intent/src/test/java/org/eclipse/dirigible/components/intent/parser/StepResilienceIntentTest.java b/components/engine/engine-intent/src/test/java/org/eclipse/dirigible/components/intent/parser/StepResilienceIntentTest.java index c1b163a2185..aa89c159fc1 100644 --- a/components/engine/engine-intent/src/test/java/org/eclipse/dirigible/components/intent/parser/StepResilienceIntentTest.java +++ b/components/engine/engine-intent/src/test/java/org/eclipse/dirigible/components/intent/parser/StepResilienceIntentTest.java @@ -22,7 +22,9 @@ * {@code {error}} placeholder a {@code setField} on the error route reads. The parser must reject a * malformed retry cycle, a dangling error route, resilience on a service-task shape it does not * apply to, resilience on a fan-out send (which never fails, so neither key could fire), and an - * {@code {error}} nothing would ever populate. + * {@code {error}} nothing would ever populate. Also the routing self-loops that spin a + * wait-state-free flow: a step's {@code next} and a decision's {@code then}/{@code else} naming + * their own step. */ class StepResilienceIntentTest { @@ -114,6 +116,43 @@ void aNextTargetingItselfIsRejected() { assertTrue(issue.contains("step [provisionApp]"), "the message must locate the step: " + issue); } + /** + * The same spin one key over: an exclusive gateway has no wait state, so a decision branching back + * to itself emits a self-targeting sequence flow Flowable spins on. It names an existing step, so + * only a dedicated check catches it (dirigible #7292). + */ + @Test + void aDecisionThenTargetingItselfIsRejected() { + String issue = assertIssue(withDecision("rated", "settle"), "`then` targets itself - a self-loop that never advances"); + assertTrue(issue.contains("decision [rated]"), "the message must locate the decision: " + issue); + } + + @Test + void aDecisionElseTargetingItselfIsRejected() { + String issue = assertIssue(withDecision("settle", "rated"), "`else` targets itself - a self-loop that never advances"); + assertTrue(issue.contains("decision [rated]"), "the message must locate the decision: " + issue); + } + + /** + * A branch back through a wait state is a legitimate cycle - only the direct self-edge is refused. + */ + @Test + void aDecisionBranchingBackThroughAWaitStateIsAccepted() { + assertDoesNotThrow(() -> IntentParser.parse(withDecision("reconsider", "settle"))); + } + + /** The showcase plus a decision whose two branches take the given targets. */ + private static String withDecision(String thenTarget, String elseTarget) { + return YAML + """ + - name: Review + trigger: { onCreate: TenantApplication } + steps: + - { name: reconsider, kind: userTask, args: { assignee: owner, next: rated } } + - { name: rated, kind: decision, args: { if: "id > 0", then: %s, else: %s } } + - { name: settle, kind: serviceTask, args: { setField: failureMessage, value: settled, next: end } } + """.formatted(thenTarget, elseTarget); + } + /** The literal `end` routes the failure to the process end, like a decision branch. */ @Test void onErrorMayRouteToEnd() {