[incubator-kie-issues#1474] Fix NPE when loading multiple Signavio MID

[samuel-beniamin]

Closes apache/incubator-kie-issues#1474

[pibizza]

Added @gitgabrio, he is working on this stuff at the moment.

[pibizza]

Maybe you can simply this and the previous statement using directly assertj assertions. Assertj has a powerful fluent interface to manipulate streams and perform stream operation. You can check extracting and flatExtracting, for example.

Suggested change

	assertThat(decisionNames).containsOnly("sumUp", "iterating", "determineModifier", "concatNames");
	assertThat(dmnRuntime.getModels()).flatExtracting(dmnModel -> dmnModel.getDecisions()).extracting(DMNNode::getName).containsOnly("sumUp", "iterating", "determineModifier", "concatNames");

[samuel-beniamin]

Looks good, applied ✅

[gitgabrio]

Hi @samuel-beniamin
Reading the original ticket, it seems there is some sort of issue in the implementation.
But IINW, this PR does not change the implementation at all (if just add a log), and rather modifies a test: could you clarify that ?

[samuel-beniamin]

Hi @samuel-beniamin Reading the original ticket, it seems there is some sort of issue in the implementation. But IINW, this PR does not change the implementation at all (if just add a log), and rather modifies a test: could you clarify that ?

Hello @gitgabrio, It is in the implementation, I can still see my change returning or exiting early incase the model is not the same.

So basically, the compiler calls this callback regardless of the model it is trying to evaluate, but in this case the extra check is basically the fix, it seems trivial but it is blocking if we need to load multiple models.

if (cModel != model) {
  return;
}

[samuel-beniamin]

@gitgabrio So skipping the execution of the callback in the specific case of Signavio's extension and in the case it is a MID executing callback for a different model, is what you are working on right now?

[gitgabrio]

@samuel-beniamin first of all, thanks for the PR ❤️
And then, thanks for explanation - my bad, I was confused by the log.
I see your point, and this PR is fine for me.
But TBH, we did not touch this module for some time, so we would need to dig in it a bit; and as you wrote

but maybe the proper fix should be in the DMNCompilerImpl.

this seems more a workaround for a symptom of a problem somewhere else.
So, if you may, I would kindly ask you to create another ticket (or duplicate the original one) so that we could focus on the root problem, eventually with your help : wdyt ?

[pibizza]

Looks good to me.

[samuel-beniamin]

@gitgabrio Thank you for approving the the PR.

this seems more a workaround for a symptom of a problem somewhere else.

Yes, I agree too, I think a proper fix should be to validate for which model is the call back being executed, by the compiler itself. I will create another ticket for it.

[samuel-beniamin]

Here is a ticket to address a proper fix in the future apache/incubator-kie-issues#1478

[gitgabrio]

Great @samuel-beniamin , many thanks!!!