Copilot commented on code in PR #7159:
URL: https://github.com/apache/incubator-kie/pull/7159#discussion_r4166027449
##########
drools-core/src/main/java/org/drools/core/common/TupleSetsImpl.java:
##########
@@ -185,7 +185,9 @@ public void removeInsert(TupleImpl tuple) {
if ( next != null ) {
setPreviousTuple( next, previous );
}
- setNextTuple( previous, next );
+ if ( previous != null ) {
+ setNextTuple( previous, next );
+ }
Review Comment:
This guard prevents the exception but does not make this unlink valid. In
this branch `tuple != insertFirst`; if `previous == null`, the tuple cannot be
a consistently linked non-head node. The method still rewrites `next.previous`,
clears the tuple, and decrements `insertSize`, so the reported divergence can
leave the insert list and its size inconsistent instead of fixing it. Please
either repair the right-staging links before removal or explicitly handle a
tuple that is not a member without mutating/decrementing this list.
This issue also appears in the following locations of the same file:
- line 209
- line 229
##########
drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/SubnetworkTest.java:
##########
@@ -521,4 +521,81 @@ private void
testFromCollectInSubnetwork(KieBaseTestConfiguration kieBaseTestCon
kieSession.dispose();
}
}
+
+ // --- fact types for the not/exists shared-subnetwork NPE test ---
+ public static class InputFact { private final int id; public
InputFact(int id) { this.id = id; } public int getId() { return id; } }
+ public static class SubFact1 { private final int id; public
SubFact1(int id) { this.id = id; } public int getId() { return id; } }
+ public static class SubFact2 { private final int id; public
SubFact2(int id) { this.id = id; } public int getId() { return id; } }
+ public static class OtherFact {
}
+ public static class Anchor {
}
+
+ /**
+ * Regression test for apache/incubator-kie-issues#7121.
+ *
+ * When two rules wrap an identical CE in {@code not(...)} and {@code
exists(...)}
+ * respectively, the engine shares one TupleToObjectNode feeding both beta
nodes.
+ * A retraction that removes an insert staged on the shared subnetwork
while a
+ * second subnetwork tuple is also staged concurrently causes
+ * {@code TupleSetsImpl.removeInsert/removeDelete/removeUpdate} to call
+ * {@code setNextTuple(previous, next)} with {@code previous == null} —
NPE.
+ */
+ @ParameterizedTest(name = "KieBase type={0}")
+ @MethodSource("parameters")
+ @Timeout(10000)
+ public void
sharedSubnetworkNotAndExistsDoesNotNpeOnConcurrentStagedDelete(KieBaseTestConfiguration
kieBaseTestConfiguration) {
+ // apache/incubator-kie-issues#7121
+ final String drl =
+ "import " + InputFact.class.getCanonicalName() + ";\n" +
+ "import " + SubFact1.class.getCanonicalName() + ";\n" +
+ "import " + SubFact2.class.getCanonicalName() + ";\n" +
+ "import " + OtherFact.class.getCanonicalName() + ";\n" +
+ "import " + Anchor.class.getCanonicalName() + ";\n" +
+ "\n" +
+ // seed rule: each Anchor inserts two SubFact1 values logically
+ "rule Seed when\n" +
+ " Anchor()\n" +
+ "then\n" +
+ " insertLogical(new SubFact1(1));\n" +
+ " insertLogical(new SubFact1(2));\n" +
+ " insertLogical(new SubFact2(0));\n" +
+ "end\n" +
+ "\n" +
+ // Rule A wraps the shared CE in not()
+ "rule RuleA when\n" +
+ " InputFact()\n" +
+ " not( SubFact1() and SubFact2() )\n" +
+ "then\n" +
+ "end\n" +
+ "\n" +
+ // Rule B wraps the identical CE in exists() — forces node
sharing
+ "rule RuleB when\n" +
+ " InputFact()\n" +
+ " exists( SubFact1() and SubFact2() )\n" +
+ " OtherFact()\n" +
+ "then\n" +
+ "end\n";
+
+ final KieBase kbase =
KieBaseUtil.getKieBaseFromKieModuleFromDrl("subnetwork-npe-test",
kieBaseTestConfiguration, drl);
+ final KieSession ksession = kbase.newKieSession();
+ try {
+ // Multiple InputFacts ensure multiple subnetwork tuples are staged
+ ksession.insert(new InputFact(1));
+ ksession.insert(new InputFact(2));
+ ksession.insert(new OtherFact());
+ final FactHandle anchor = ksession.insert(new Anchor());
+ ksession.fireAllRules();
+
+ // Retract the anchor — the Seed rule's logical inserts are
withdrawn,
+ // producing deletes on the shared subnetwork while inserts from
the
+ // previous cycle may still be staged: triggers the null-previous
NPE.
+ ksession.retract(anchor);
+ ksession.fireAllRules();
Review Comment:
This retraction occurs only after the preceding `fireAllRules()` has drained
the insertion cycle, so it does not create the same-batch INSERT/DELETE clash
required to call `removeInsert` with divergent right-staging links. The linked
issue also notes that equivalent small topologies did not reproduce the
failure. As written, this test can pass on the unfixed implementation; make
support withdrawal happen during the same evaluation cycle or add a focused
test that constructs the divergent staging state and fails before the fix.
##########
drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/SubnetworkTest.java:
##########
@@ -521,4 +521,81 @@ private void
testFromCollectInSubnetwork(KieBaseTestConfiguration kieBaseTestCon
kieSession.dispose();
}
}
+
+ // --- fact types for the not/exists shared-subnetwork NPE test ---
+ public static class InputFact { private final int id; public
InputFact(int id) { this.id = id; } public int getId() { return id; } }
+ public static class SubFact1 { private final int id; public
SubFact1(int id) { this.id = id; } public int getId() { return id; } }
+ public static class SubFact2 { private final int id; public
SubFact2(int id) { this.id = id; } public int getId() { return id; } }
+ public static class OtherFact {
}
+ public static class Anchor {
}
+
+ /**
+ * Regression test for apache/incubator-kie-issues#7121.
Review Comment:
This reference points to `apache/incubator-kie-issues#7121`, which does not
exist; the reported bug is `apache/incubator-kie#7121`. Update the reference so
future maintainers can reach the issue.
This issue also appears on line 546 of the same file.
##########
drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/SubnetworkTest.java:
##########
@@ -521,4 +521,81 @@ private void
testFromCollectInSubnetwork(KieBaseTestConfiguration kieBaseTestCon
kieSession.dispose();
}
}
+
+ // --- fact types for the not/exists shared-subnetwork NPE test ---
+ public static class InputFact { private final int id; public
InputFact(int id) { this.id = id; } public int getId() { return id; } }
+ public static class SubFact1 { private final int id; public
SubFact1(int id) { this.id = id; } public int getId() { return id; } }
+ public static class SubFact2 { private final int id; public
SubFact2(int id) { this.id = id; } public int getId() { return id; } }
+ public static class OtherFact {
}
+ public static class Anchor {
}
+
+ /**
+ * Regression test for apache/incubator-kie-issues#7121.
+ *
+ * When two rules wrap an identical CE in {@code not(...)} and {@code
exists(...)}
+ * respectively, the engine shares one TupleToObjectNode feeding both beta
nodes.
+ * A retraction that removes an insert staged on the shared subnetwork
while a
+ * second subnetwork tuple is also staged concurrently causes
+ * {@code TupleSetsImpl.removeInsert/removeDelete/removeUpdate} to call
+ * {@code setNextTuple(previous, next)} with {@code previous == null} —
NPE.
+ */
+ @ParameterizedTest(name = "KieBase type={0}")
+ @MethodSource("parameters")
+ @Timeout(10000)
+ public void
sharedSubnetworkNotAndExistsDoesNotNpeOnConcurrentStagedDelete(KieBaseTestConfiguration
kieBaseTestConfiguration) {
+ // apache/incubator-kie-issues#7121
+ final String drl =
+ "import " + InputFact.class.getCanonicalName() + ";\n" +
+ "import " + SubFact1.class.getCanonicalName() + ";\n" +
+ "import " + SubFact2.class.getCanonicalName() + ";\n" +
+ "import " + OtherFact.class.getCanonicalName() + ";\n" +
+ "import " + Anchor.class.getCanonicalName() + ";\n" +
+ "\n" +
+ // seed rule: each Anchor inserts two SubFact1 values logically
+ "rule Seed when\n" +
+ " Anchor()\n" +
+ "then\n" +
+ " insertLogical(new SubFact1(1));\n" +
+ " insertLogical(new SubFact1(2));\n" +
+ " insertLogical(new SubFact2(0));\n" +
+ "end\n" +
+ "\n" +
+ // Rule A wraps the shared CE in not()
+ "rule RuleA when\n" +
+ " InputFact()\n" +
+ " not( SubFact1() and SubFact2() )\n" +
+ "then\n" +
+ "end\n" +
+ "\n" +
+ // Rule B wraps the identical CE in exists() — forces node
sharing
+ "rule RuleB when\n" +
+ " InputFact()\n" +
+ " exists( SubFact1() and SubFact2() )\n" +
+ " OtherFact()\n" +
+ "then\n" +
+ "end\n";
+
+ final KieBase kbase =
KieBaseUtil.getKieBaseFromKieModuleFromDrl("subnetwork-npe-test",
kieBaseTestConfiguration, drl);
+ final KieSession ksession = kbase.newKieSession();
+ try {
+ // Multiple InputFacts ensure multiple subnetwork tuples are staged
+ ksession.insert(new InputFact(1));
+ ksession.insert(new InputFact(2));
+ ksession.insert(new OtherFact());
+ final FactHandle anchor = ksession.insert(new Anchor());
+ ksession.fireAllRules();
+
+ // Retract the anchor — the Seed rule's logical inserts are
withdrawn,
+ // producing deletes on the shared subnetwork while inserts from
the
+ // previous cycle may still be staged: triggers the null-previous
NPE.
+ ksession.retract(anchor);
+ ksession.fireAllRules();
+
+ // Re-insert anchor and fire again to exercise the update path
+ ksession.insert(new Anchor());
+ ksession.fireAllRules();
Review Comment:
Reinserting an `Anchor` creates new `SubFact1`/`SubFact2` insertions; this
test never calls `update` and the facts use identity equality, so this cycle
does not exercise the update path or `removeUpdate`. Either drive an actual
update clash or remove the inaccurate claim.
--
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]
