tkobayas commented on PR #7160:
URL: https://github.com/apache/incubator-kie/pull/7160#issuecomment-6516366961
Thank you @Rikkola for the update. Here are the AI review results for the
latest changes. You might want to address the issues in separate PRs, but I'll
leave that up to you.
---
- **Fixed:** the activation-time veto. `validateActivationMatching()` now
rejects a `oneOf` with two or more children that already match at activation,
including when it is wrapped in `and(...)` or the matching child is an
`or(...)`. A `oneOf` with exactly one such child is still accepted and works.
- **Fixed:** both Copilot findings. A stale `other` no longer completes
`sequence(s, or(noneOrAll(a, b), other), ack)`, and `and(oneOf(a, b))` now
vetoes on `a, b`.
- **OK:** moving the `AlphaAdapter` filter-skip bug to a separate PR. Could
you link the issue or PR here once it exists?
- **OK:** the `oneOf`-under-`or` behaviour is now documented and pinned by
tests. See item 1, though. It only holds when the next trigger sits inside the
same `and(...)`.
### 1. [Major] A guard-only `or(...)` step is not folded into the following
trigger
`hasPositiveTrigger()` treats an `or` as positive only if every branch is
positive, so `or(x, noneOrAll(a, b))` and `or(oneOf(a, b), c)` count as
non-positive. But `rewriteRevertingGateSteps()` folds only bare `oneOf` /
`noneOrAll`, and `flattenNonPositiveAndSteps()` flattens only `and`. Such an
`or` therefore stays a standalone step, and the step completes as soon as any
one branch matches:
- A `noneOrAll` branch matches at activation, so the whole step is skipped
and the guard is silently ignored.
- A `oneOf` branch completes the step on its first match, so a later
over-match is never seen. That contradicts the new `oneOf()` javadoc, which
says the veto propagates past `or()`.
So the same guard behaves differently depending on whether the trigger is
written inside `and(..., d)` or as the next step. (`ev(name)` is
`pattern(declarationOf(Event.class)).expr("is-" + name, e ->
e.getName().equals(name))`.)
```text
sequence(s, oneOf(a, b), d) s,a,b,d -> 0 firings
sequence(s, or(oneOf(a, b), oneOf(c, e)), d) s,a,b,d -> 1 firing
(expected 0)
sequence(s, and(or(oneOf(a, b), c), d)) s,a,c,b,d -> 0 firings
(as documented)
sequence(s, or(oneOf(a, b), c), d) s,a,c,b,d -> 1 firing
(expected 0)
sequence(s, and(or(x, noneOrAll(a, b)), d)) s,a,d -> 0 firings
sequence(s, or(x, noneOrAll(a, b)), d) s,a,d -> 1 firing
(expected 0)
```
This is the same class of problem as Copilot's `and(oneOf(a, b))` finding,
but only `and` was fixed. One option is to generalize the bare-`oneOf` rule:
fold every step that is neither positive nor `absent()` into the following
positive step. That would probably make `flattenNonPositiveAndSteps()`
unnecessary too. The other option is to reject these shapes at intermediate
positions.
### 2. [Minor] Existing tests from `main` are deleted
The PR deletes `SequenceReviewProbeTest.java` (added in #6909) and
`PatternDSLSequenceConsequenceTest.twoSequencesInOneRuleMultipleAnchorsBothFire`.
Three of the probe tests were moved to `PatternDSLSequenceEdgeCaseTest`. These
four were not moved anywhere: `sameTypeInTwoSteps`,
`stepWithTwoConstraintsIsUnsatisfiable`,
`stepWithTwoConstraintsBothSatisfiedFires`, and
`joinAfterSequenceHasCorrectTupleWidth`. I restored the deleted tests
temporarily and ran them against this PR. All of them pass, except
`stepWithNoConstraintThrowsDescriptiveError`, whose behaviour this PR changes
on purpose. Could you keep them, e.g. by moving them into
`PatternDSLSequenceEdgeCaseTest`?
### 3. Small notes
- **Unconstrained steps:** a step without `expr()` now matches any fact of
its type through `TrueConstraint`, instead of failing with the error added in
#6909. I checked that facts of other types still don't satisfy it. Since this
changes v1 behaviour independently of the new API, it may be worth mentioning
in the PR description.
- **Same-fact veto (open from last round):** in `sequence(s, absent(s), d)`
with arrivals s, s, d, should the vetoing `s` also start a new attempt? It
still fires 0 times. Today the result depends on the filter-skip bug and the
filter order, so the fix in the separate PR may change it. It would be good to
decide the intended behaviour and pin it with a test.
- **Nested `absent()`:** `and(absent(x), d)` and `oneOf(absent(x), a)` still
throw `UnsupportedOperationException` ("does not yet support condition type NOT
as a composite step gate") when the KieBase is built, while the other shape
errors are `IllegalArgumentException`s from `rule.build()`. The message is
clear, so this is optional.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
