[DROOLS-7493] [DROOLS-7497] executable model wrongly rewrites modify in if-block

[tkobayas]

• Additional tests to cover setter order for properperty reactivity [DROOLS-7497]

Ports
This is a PR for main
for 7.x ? ->

JIRA:
https://issues.redhat.com/browse/DROOLS-7493
https://issues.redhat.com/browse/DROOLS-7497

How to replicate CI configuration locally?

Build Chain tool does "simple" maven build(s), the builds are just Maven commands, but because the repositories relates and depends on each other and any change in API or class method could affect several of those repositories there is a need to use build-chain tool to handle cross repository builds and be sure that we always use latest version of the code for each repository.

build-chain tool is a build tool which can be used on command line locally or in Github Actions workflow(s), in case you need to change multiple repositories and send multiple dependent pull requests related with a change you can easily reproduce the same build by executing it on Github hosted environment or locally in your development environment. See local execution details to get more information about it.

How to retest this PR or trigger a specific build:

• for pull request checks
  Please add comment: Jenkins retest this

• for a specific pull request check
  Please add comment: Jenkins (re)run [drools|kogito-runtimes|kogito-apps|kogito-examples] tests

• for a full downstream build

  • for jenkins job: please add comment: Jenkins run fdb
  • for github actions job: add the label run_fdb

• a compile downstream build please add comment: Jenkins run cdb

• a full production downstream build please add comment: Jenkins execute product fdb

• an upstream build please add comment: Jenkins run upstream

• for quarkus branch checks
  Run checks against Quarkus current used branch
  Please add comment: Jenkins run quarkus-branch

• for a quarkus branch specific check
  Run checks against Quarkus current used branch
  Please add comment: Jenkins (re)run [drools|kogito-runtimes|kogito-apps|kogito-examples] quarkus-branch

• for quarkus main checks
  Run checks against Quarkus main branch
  Please add comment: Jenkins run quarkus-main

• for a specific quarkus main check
  Run checks against Quarkus main branch
  Please add comment: Jenkins (re)run [drools|kogito-runtimes|kogito-apps|kogito-examples] quarkus-main

• for quarkus lts checks
  Run checks against Quarkus lts branch
  Please add comment: Jenkins run quarkus-lts

• for a specific quarkus lts check
  Run checks against Quarkus lts branch
  Please add comment: Jenkins (re)run [drools|kogito-runtimes|kogito-apps|kogito-examples] quarkus-lts

• for native checks
  Run native checks
  Please add comment: Jenkins run native

• for a specific native check
  Run native checks
  Please add comment: Jenkins (re)run [drools|kogito-runtimes|kogito-apps|kogito-examples] native

• for native lts checks
  Run native checks against quarkus lts branch
  Please add comment: Jenkins run native-lts

• for a specific native lts check
  Run native checks against quarkus lts branch
  Please add comment: Jenkins (re)run [drools|kogito-runtimes|kogito-apps|kogito-examples] native-lts

How to backport a pull request to a different branch?

In order to automatically create a backporting pull request please add one or more labels having the following format backport-<branch-name>, where <branch-name> is the name of the branch where the pull request must be backported to (e.g., backport-7.67.x to backport the original PR to the 7.67.x branch).

NOTE: backporting is an action aiming to move a change (usually a commit) from a branch (usually the main one) to another one, which is generally referring to a still maintained release branch. Keeping it simple: it is about to move a specific change or a set of them from one branch to another.

Once the original pull request is successfully merged, the automated action will create one backporting pull request per each label (with the previous format) that has been added.

If something goes wrong, the author will be notified and at this point a manual backporting is needed.

NOTE: this automated backporting is triggered whenever a pull request on main branch is labeled or closed, but both conditions must be satisfied to get the new PR created.

[tkobayas]

Hi @mariofusco @lucamolteni

This PR is still draft and has several test failures.

Firstly, this PR solves the issue in https://issues.redhat.com/browse/DROOLS-7493

A) In exec-model, modify is always rendered as drools.update at the end of consequence, so for example, it is moved out from if-block.
B) In exec-model, if modify block doesn't have setters, drools.update is not rendered even if setters exist before the modify block (it works with non-exec-model).

So I explicitly add drools.update in the same block where modify existed.

Then, another concern arises. Do we want to enlist properties for property reactivity which are set after modify/update?

e.g. In this case, value2 should be enlisted as property reactivity? (https://issues.redhat.com/browse/DROOLS-7497)

modify ($fact) {
  setValue1(2)
}
$fact.setValue2(2);

C) In non-exec-model,
C.1) With java dialect, RHS is analyzed only before modify/update. So value2 is not property reactive
C.2) With mvel dialect, the whole RHS is analyzed. So value2 is property reactive
D) In exec-model, with both dialect, RHS is analyzed only before modify/update. So value2 is not property reactive. However, as described above, the current implementation (before this PR) renders modify as drools.update at the end of consequence. So modify enlists all setters in RHS.

So, more or less, there is inconsistency in the behavior.

Personaly, I like C.2) behavior (= analyze whole RHS and make them all property reactive), because PropagationEntry.Update is evaluated after whole RHS execution regardless of the modify block position in the RHS, so anyway, all changes in RHS will affects in the evaluation.

Could you share your thoughts on this? Thanks!

(Sorry, in my PropertyReactivityMatrixTest, value1 and value2 order is inverted, in order to meet with existing rules)

[kie-ci4]

(tests) - drools job #1529 was: UNSTABLE
Possible explanation: This should be test failures

Reproducer

export BUILD_MVN_OPTS_CURRENT=-Dfull -Prun-code-coverage
build-chain build cross_pr -f 'https://raw.githubusercontent.com/kiegroup/drools/main/.ci/buildchain-config.yaml' -o 'bc' -p kiegroup/drools -u https://github.com/kiegroup/drools/pull/5380 --skipParallelCheckout

NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution

Please look here: https://eng-jenkins-csb-business-automation.apps.ocp-c1.prod.psi.redhat.com/job/KIE/job/drools/job/main/job/pullrequest/job/drools.tests.drools/1529/display/redirect

Test results:

• PASSED: 21637
• FAILED: 8

Those are the test failures:

org.drools.model.codegen.execmodel.PropertyReactivityMatrixTest.assignmentBeforeAndAfterModify_shouldTriggerReactivity[STANDARD_FROM_DRL JAVA]

[Should trigger property reactivity on value1]
Expecting actual:
[]
to contain exactly (and in same order):
["R2 fired"]
but could not find the following elements:
["R2 fired"]

org.drools.model.codegen.execmodel.PropertyReactivityMatrixTest.assignmentBeforeAndAfterUpdate_shouldTriggerReactivity[STANDARD_FROM_DRL JAVA]

[Should trigger property reactivity on value1]
Expecting actual:
[]
to contain exactly (and in same order):
["R2 fired"]
but could not find the following elements:
["R2 fired"]

org.drools.model.codegen.execmodel.PropertyReactivityMatrixTest.assignmentAfterIfBlockModify_shouldTriggerReactivity[STANDARD_FROM_DRL JAVA]

[Should trigger property reactivity on value1]
Expecting actual:
[]
to contain exactly (and in same order):
["R2 fired"]
but could not find the following elements:
["R2 fired"]

org.drools.model.codegen.execmodel.PropertyReactivityMatrixTest.assignmentAfterModify_shouldTriggerReactivity[STANDARD_FROM_DRL JAVA]

[Should trigger property reactivity on value1]
Expecting actual:
[]
to contain exactly (and in same order):
["R2 fired"]
but could not find the following elements:
["R2 fired"]

org.drools.model.codegen.execmodel.PropertyReactivityMatrixTest.assignmentBeforeAndAfterUpdate_shouldTriggerReactivity[PATTERN_DSL JAVA]

[Should trigger property reactivity on value1]
Expecting actual:
[]
to contain exactly (and in same order):
["R2 fired"]
but could not find the following elements:
["R2 fired"]

org.drools.model.codegen.execmodel.PropertyReactivityMatrixTest.assignmentAfterIfBlockModify_shouldTriggerReactivity[PATTERN_DSL JAVA]

[Should trigger property reactivity on value1]
Expecting actual:
[]
to contain exactly (and in same order):
["R2 fired"]
but could not find the following elements:
["R2 fired"]

org.drools.model.codegen.execmodel.PropertyReactivityMatrixTest.assignmentBeforeAndAfterUpdate_shouldTriggerReactivity[PATTERN_DSL MVEL]

[Should trigger property reactivity on value1]
Expecting actual:
[]
to contain exactly (and in same order):
["R2 fired"]
but could not find the following elements:
["R2 fired"]

org.drools.model.codegen.execmodel.PropertyReactivityMatrixTest.assignmentAfterIfBlockModify_shouldTriggerReactivity[PATTERN_DSL MVEL]

[Should trigger property reactivity on value1]
Expecting actual:
[]
to contain exactly (and in same order):
["R2 fired"]
but could not find the following elements:
["R2 fired"]

[lucamolteni]

Personaly, I like C.2) behavior (= analyze whole RHS and make them all property reactive), because PropagationEntry.Update is evaluated after whole RHS execution regardless of the modify block position in the RHS, so anyway, all changes in RHS will affects in the evaluation.

Could you share your thoughts on this? Thanks!

(Sorry, in my PropertyReactivityMatrixTest, value1 and value2 order is inverted, in order to meet with existing rules)

I agree there should be some consistency eventually, and C.2 seems the good choice.

We have considered using the mvel-compiler for MVEL and Java "classic" (non exec model) evaluation as well so once we solve it there it could be possible to have it everywhere, eventually

[tkobayas]

We have considered using the mvel-compiler for MVEL and Java "classic" (non exec model) evaluation as well so once we solve it there it could be possible to have it everywhere, eventually

Great. It should be nice :)

[mariofusco]

This not an obvious choice and deserves a few considerations. First of all, let me do a small digression, starting from user's expectations (that unfortunately cannot be fulfilled and I will explain why). If I write a consequence like:

modify ($fact) {
  setValue1(2)
}
$fact.setValue2(2);

I would expect that it would be syntactic sugar for something like:

$fact.setValue1(2);
update($fact);
$fact.setValue2(2);

meaning that I'd expect the instruction to be executed in that order. First I set value1 and propagate the update of the fact into the engine with only that change and only after I also change value2, but without notifying the engine of this second modification.

As anticipated however, given the current implementation, this expectation cannot be satisfied because what the update does is only scheduling an action on the propagation list, and when that update action is dequeued and actually executed the fact to be updated also has value2 already changed.

Long story short, ideally I'd prefer C.1, but since it doesn't reflect how the fact update is propagated into the engine we are practically obliged to go with C.2. More importantly we need to make sure that the C.2 behavior is enforced consistently with and without exec model and regardless of the dialect.

[tkobayas]

Long story short, ideally I'd prefer C.1, but since it doesn't reflect how the fact update is propagated into the engine we are practically obliged to go with C.2. More importantly we need to make sure that the C.2 behavior is enforced consistently with and without exec model and regardless of the dialect.

Thanks, I completely agree with you. I will also add docs to explain the behavior.

[kie-ci4]

(tests) - drools job #1532 was: FAILURE
Possible explanation: Pipeline failure or project build failure

Reproducer

export BUILD_MVN_OPTS_CURRENT=-Dfull -Prun-code-coverage
build-chain build cross_pr -f 'https://raw.githubusercontent.com/kiegroup/drools/main/.ci/buildchain-config.yaml' -o 'bc' -p kiegroup/drools -u https://github.com/kiegroup/drools/pull/5380 --skipParallelCheckout

NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution

Please look here: https://eng-jenkins-csb-business-automation.apps.ocp-c1.prod.psi.redhat.com/job/KIE/job/drools/job/main/job/pullrequest/job/drools.tests.drools/1532/display/redirect

Test results:

• PASSED: 21655
• FAILED: 0

Those are the test failures: none

See console log:

Console Logs

[2023-07-11T09:17:30.832Z] at org.apache.maven.cli.MavenCli.execute (MavenCli.java:960)
[2023-07-11T09:17:30.832Z] at org.apache.maven.cli.MavenCli.doMain (MavenCli.java:293)
[2023-07-11T09:17:30.832Z] at org.apache.maven.cli.MavenCli.main (MavenCli.java:196)
[2023-07-11T09:17:30.832Z] at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0 (Native Method)
[2023-07-11T09:17:30.832Z] at jdk.internal.reflect.NativeMethodAccessorImpl.invoke (NativeMethodAccessorImpl.java:62)
[2023-07-11T09:17:30.832Z] at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke (DelegatingMethodAccessorImpl.java:43)
[2023-07-11T09:17:30.832Z] at java.lang.reflect.Method.invoke (Method.java:566)
[2023-07-11T09:17:30.832Z] at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:282)
[2023-07-11T09:17:30.832Z] at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:225)
[2023-07-11T09:17:30.832Z] at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:406)
[2023-07-11T09:17:30.832Z] at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:347)
[2023-07-11T09:17:30.832Z] [ERROR]
[2023-07-11T09:17:30.832Z] [ERROR] Re-run Maven using the -X switch to enable full debug logging.
[2023-07-11T09:17:30.832Z] [ERROR]
[2023-07-11T09:17:30.832Z] [ERROR] For more information about the errors and possible solutions, please read the following articles:
[2023-07-11T09:17:30.832Z] [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
[2023-07-11T09:17:30.832Z] [ERROR]
[2023-07-11T09:17:30.832Z] [ERROR] After correcting the problems, you can resume the build with the command
[2023-07-11T09:17:30.832Z] [ERROR] mvn -rf :drools-drlonyaml-cli-tests
[2023-07-11T09:17:31.086Z] + test 1 -eq 0
[Pipeline] }
[Pipeline] // withCredentials
[Pipeline] }
[Pipeline] // dir
[Pipeline] }
[Pipeline] // script
[Pipeline] }
[Pipeline] // withEnv
[Pipeline] }
[Pipeline] // stage
[Pipeline] stage
[Pipeline] { (Upload artifacts to repository)
Stage "Upload artifacts to repository" skipped due to earlier failure(s)
[Pipeline] }
[Pipeline] // stage
[Pipeline] stage
[Pipeline] { (check space after build)
Stage "check space after build" skipped due to earlier failure(s)
[Pipeline] }
[Pipeline] // stage
[Pipeline] stage
[Pipeline] { (Declarative: Post Actions)
[Pipeline] script
[Pipeline] {
[Pipeline] sh
[2023-07-11T09:17:32.028Z] + wget --no-check-certificate -qO - https://eng-jenkins-csb-business-automation.apps.ocp-c1.prod.psi.redhat.com/job/KIE/job/drools/job/main/job/pullrequest/job/drools.tests.drools/1532/api/json
[Pipeline] readJSON
[Pipeline] sh
[2023-07-11T09:17:32.349Z] + wget --no-check-certificate -qO - https://eng-jenkins-csb-business-automation.apps.ocp-c1.prod.psi.redhat.com/job/KIE/job/drools/job/main/job/pullrequest/job/drools.tests.drools/1532/consoleText
[2023-07-11T09:17:32.349Z] + tail -n 50

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

96.4% Coverage
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[pibizza]

Looks good to me.

[tkobayas]

@mariofusco please merge this PR, thanks!