-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JBPM-5541 - Case file conditional event fails other cases #724
Conversation
@@ -30,6 +30,7 @@ | |||
public abstract class CaseCommand<T> implements ExecutableCommand<T> { | |||
|
|||
private static final long serialVersionUID = 4116744986913465571L; | |||
protected static final String ENTRY_POINT_VAR_NAME = "_EntryPoint_"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have some doubts about using a variable for storing this, as this will result in an internal implementation detail be very visible in the process instance details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here but couldn't find anything better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative would be to make metadata persisted (like it is at definition side).
@@ -6,7 +6,7 @@ import org.jbpm.casemgmt.demo.insurance.ClaimReport; | |||
rule "classify-claim" ruleflow-group "claims" | |||
|
|||
when | |||
$caseData : CaseData() | |||
$caseData : CaseData() from entry-point "insurance-claims.CarInsuranceClaimCase" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this introduce backwards compatibility issues, and pushes the burden partially to the user as well (just in a different location, now the rules are specifying the entrypoint rather than the rules having to make sure they are independent from other cases if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does make it backward incompatible as it introduces complete isolation bwteen the processes
@@ -426,7 +438,7 @@ private String createSplitRule(Process process, | |||
connection.getToType() + "\" @Propagation(EAGER) \n" + | |||
" ruleflow-group \"DROOLS_SYSTEM\" \n" + | |||
" when \n" + | |||
" " + constraint + "\n" + | |||
" " + constraint + getEntryPoint(constraint, process) + "\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concern around backwards compatibility, doesn't this also make it difficult to actually try and share data across instances, which might be useful in some cases as well. Or are we going to consider this an anti-pattern since this will result in different behaviour depending on session strategy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only kbase separation as process instance is still mainly based on runtime strategy so I'd say it's an antipattern
@@ -201,6 +204,9 @@ public void internalTrigger(final NodeInstance from, String type) { | |||
// remove foreach input variable to avoid problems when running in variable strict mode | |||
parameters.remove(getSubProcessNode().getMetaData("MICollectionInput")); | |||
} | |||
if (getVariable(ENTRY_POINT_VAR_NAME) != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to happen at the end, so seems to override any value provided in mapping? Should we only do this if this param isn't set yet? Also would suggest just using getVariable(..) once and reus second time for efficiency.
In general my main concern is that this is too much constrained solution as it separates the hard way so to say. I think to close this and put the need of separation on users to create rules in the same kjar to be unique when this is desired. With improvement to include case def id (process id) to be available in CaseData/CaseFile type so it can be easily used in rules to separate them. Wdyt? |
closing in favor of #729 |
depends on apache/incubator-kie-drools#1060
@krisv here is a change to use entry points for case file that is named same as case definition (ad hoc process id) that way case definitions conditions are isolated to its own type/definition so they don't interfere.
The problem here is that all conditions (drools rules) are compiled into kbase and shared globally for all process definitions. This is not really a case mgmt issue but overall problem though it will be mostly visible in cases where conditions will usually refer to CaseData (aka CaseFile).
To be able to support layered case instances - case definitions + subprocesses to access easily same file the entry point (process id of the case def) is stored as process variable. That way it is always given to child processes and can be used to look up the right entry point to find case file.
What do you think? I couldn't find any other applicable solution than this... so open for any ideas.
Only other alternative is to put the requirement on end users to keep in mind that when there are multiple case definitions with conditions (which most likely be rather normal case) then conditions must be distinct like using case definition id as part of the conditions. Not always possible and easy to mis... especially that it can't be caught on compile time but on runtime.