Copilot commented on code in PR #7264:
URL: https://github.com/apache/incubator-kie/pull/7264#discussion_r4477666292
##########
drools-core/src/main/java/org/drools/core/common/AgendaGroupQueueImpl.java:
##########
@@ -131,6 +131,11 @@ public void setFocus() {
reteEvaluator.addPropagation( new SetFocusAction( this.name ) );
}
+ @Override
+ public void activate() {
+ ((InternalAgenda)
reteEvaluator.getActivationsManager()).activateRuleFlowGroup(this.name);
+ }
+
Review Comment:
`setFocus()` enqueues a propagation (`reteEvaluator.addPropagation(...)`),
but `activate()` directly calls into the activations manager. This can break
the propagation/ordering/thread-safety model by executing immediately in the
caller thread. Please align `activate()` with `setFocus()` by enqueuing a
propagation entry/action that performs `activateRuleFlowGroup(name)` during
propagation processing.
##########
kie-api/src/main/java/org/kie/api/runtime/rule/AgendaGroup.java:
##########
@@ -36,5 +36,9 @@ public interface AgendaGroup {
void clear();
+ /**
+ * This method will be deprecated in a future version.
+ * Use {@link RuleFlowGroup#activate()} instead.
Review Comment:
The deprecation guidance is ambiguous because `RuleFlowGroup#activate()` is
not a direct replacement for `AgendaGroup#setFocus()` without explaining how to
obtain the target `RuleFlowGroup` (and the naming relationship between
agenda-group vs ruleflow-group). Consider updating the javadoc to point to a
concrete replacement call pattern such as
`getAgenda().getRuleFlowGroup(name).activate()` and/or
`KieCommands.newActivateRuleFlowGroup(name)`.
##########
drools-xml-support/src/test/java/org/drools/xml/support/XStreamXMLTest.java:
##########
@@ -88,6 +89,15 @@ public void
testUnMarshallStartProcessCmdWithNoOutIdentifier() {
assertThat(cmd.getProcessId()).isEqualTo("some-process-id");
}
+ @Test
+ public void testMarshallActivateRuleFlowGroupCommand() {
+ ActivateRuleFlowGroupCommand cmd = new
ActivateRuleFlowGroupCommand("foo-group");
+ String xmlString = xstream.toXML( cmd );
+ assertThat(xmlString).isEqualTo("<activate-ruleflow-group
name=\"foo-group\"/>");
+ ActivateRuleFlowGroupCommand cmd2 = (ActivateRuleFlowGroupCommand)
xstream.fromXML( xmlString );
+ assertThat(cmd2.getName()).isEqualTo(cmd.getName());
+ }
Review Comment:
This assertion is brittle because it requires an exact XML string match
(including formatting and attribute ordering). To reduce test fragility across
XStream/version/configuration changes, prefer asserting XML equivalence
(canonicalized) or asserting key properties (e.g., root node name and `name`
attribute) rather than the entire literal string.
##########
drools-commands/src/main/java/org/drools/commands/fluent/KieSessionFluentImpl.java:
##########
@@ -66,6 +66,13 @@ public KieSessionFluent delete(FactHandle handle) {
return this;
}
+ /**
+ * {@inheritDoc}
+ *
+ * @implNote This method currently delegates to {@link
#setActiveAgendaGroup(String)}
+ * and does not fire {@code RuleFlowGroupActivated}/{@code
RuleFlowGroupDeactivated} events.
+ * A future version will align its behaviour with {@link
org.kie.api.runtime.rule.RuleFlowGroup#activate()}.
+ */
@Override
public KieSessionFluent setActiveRuleFlowGroup(String ruleFlowGroup) {
return setActiveAgendaGroup(ruleFlowGroup);
Review Comment:
`setActiveRuleFlowGroup` currently delegates to `setActiveAgendaGroup`, and
the added javadoc notes it doesn't fire `RuleFlowGroupActivated/Deactivated`
events. With the new public `RuleFlowGroup#activate()` and
`newActivateRuleFlowGroup(String)` command, this is a user-visible semantic
mismatch: fluent code using 'ruleflow-group' won't behave like the new API.
Consider introducing/using an `ActivateRuleFlowGroupCommand` in the fluent
pipeline (or otherwise ensuring it calls the same activation path as
`RuleFlowGroup#activate()`) so the fluent API matches the new public contract.
##########
drools-commands/src/main/java/org/drools/commands/runtime/rule/ActivateRuleFlowGroupCommand.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.drools.commands.runtime.rule;
+
+import jakarta.xml.bind.annotation.XmlAccessType;
+import jakarta.xml.bind.annotation.XmlAccessorType;
+import jakarta.xml.bind.annotation.XmlAttribute;
+import jakarta.xml.bind.annotation.XmlRootElement;
+
+import org.kie.api.command.ExecutableCommand;
+import org.kie.api.runtime.Context;
+import org.kie.api.runtime.KieSession;
+import org.kie.internal.command.RegistryContext;
+
+@XmlRootElement
+@XmlAccessorType(XmlAccessType.NONE)
+public class ActivateRuleFlowGroupCommand implements ExecutableCommand<Void> {
+
+ @XmlAttribute(required = true)
+ private String name;
+
+ public ActivateRuleFlowGroupCommand() { }
+
+ public ActivateRuleFlowGroupCommand(String name) {
+ this.name = name;
+ }
+
+ public String getName() {
+ return name;
+ }
+
+ public void setName(String name) {
+ this.name = name;
+ }
+
+ public Void execute(Context context) {
+ KieSession ksession = ((RegistryContext) context).lookup(
KieSession.class );
+ ksession.getAgenda().getRuleFlowGroup( this.name ).activate();
+ return null;
+ }
+
+ public String toString() {
+ return "session.getAgenda().getRuleFlowGroup(" + name +
").activate();";
Review Comment:
Add `@Override` on `execute` and `toString()` for compile-time checking.
Also, `toString()` currently omits quotes around the group name; output like
`getRuleFlowGroup(foo bar)` can be misleading. Consider quoting/escaping the
name in the returned string to make logs/debugging clearer.
--
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]
