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.

Sign up for GitHub

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

Jump to bottom

[incubator-kie-issues#1478] Register DRG callbacks to assigned models from DMNCompilerImpl #6105

Open

samuel-beniamin wants to merge 7 commits into apache:main from samuel-beniamin:incubator-kie-issues#1478

+76 −17

Contributor

samuel-beniamin commented Sep 30, 2024

Closes incubator-kie-issues/1478

samuel-beniamin added 4 commits

September 30, 2024 13:34


          [incubator-kie-issues#1478] Regiser DRG callbacks to assigned models …

a713f41

…from DMNCompilerImpl


          Merge branch 'apache:main' into incubator-kie-issues#1478

41cb9a1


          Merge branch 'apache:main' into incubator-kie-issues#1478

b497d72


          Merge branch 'apache:main' into incubator-kie-issues#1478

7ebf79b

yesamer requested review from gitgabrio, baldimir and yesamer

October 14, 2024 06:11

Contributor

gitgabrio commented Oct 14, 2024

👀

yesamer added the DMN label

Contributor

gitgabrio commented Oct 15, 2024

Hi @samuel-beniamin thanks for the PR.
I tried to understand better the overall context, and I'm sharing my thoughts: please correct me if/where I'm wrong: I may misread something.

by itself, I think the fix could be right
but those AfterProcessDrgElements / addCallback / addCallbackForModel are used/needed only by the signavio module
but being defined/ implemented in the "core" DMNCompilerImpl, those somehow "pollute" it with something that, IMO, has no reason to be there

So, I was thinking to

change DMNCompilerImpl.processDrgElements from private to protected, to be overridable
get rid of all signavio-specific stuff from it (double check also the tests)
create a signavio-specific subclass of DMNCompilerImpl that add the behavior required by signavio
put your modifications in that subclass

The idea would be to have DMNCompilerImpl dealing only with "core" responsibilities, and move implementation-specific ones (e.g. signavio) to dedicated subclasses

Wdyt ? Does this make sense ? Do you think you could do that ?

@baldimir @yesamer


          Merge branch 'apache:main' into incubator-kie-issues#1478

c4bf372

Contributor Author

samuel-beniamin commented Nov 4, 2024

Hi @samuel-beniamin thanks for the PR. I tried to understand better the overall context, and I'm sharing my thoughts: please correct me if/where I'm wrong: I may misread something.

by itself, I think the fix could be right

but those AfterProcessDrgElements / addCallback / addCallbackForModel are used/needed only by the signavio module

but being defined/ implemented in the "core" DMNCompilerImpl, those somehow "pollute" it with something that, IMO, has no reason to be there

So, I was thinking to

change DMNCompilerImpl.processDrgElements from private to protected, to be overridable

get rid of all signavio-specific stuff from it (double check also the tests)

create a signavio-specific subclass of DMNCompilerImpl that add the behavior required by signavio

put your modifications in that subclass

The idea would be to have DMNCompilerImpl dealing only with "core" responsibilities, and move implementation-specific ones (e.g. signavio) to dedicated subclasses

Wdyt ? Does this make sense ? Do you think you could do that ?

@baldimir @yesamer

Hello @gitgabrio,

Thank you for taking the time to review the code and for your suggestions.

I think your suggestions make sense, I believe I understood what need to be done, and I will be happy to make these changes in this PR as well, if that is okay.

gitgabrio requested changes

View reviewed changes

kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/compiler/DMNCompilerImpl.java

                   public interface AfterProcessDrgElements {
                       void callback(DMNCompilerImpl compiler, DMNCompilerContext ctx, DMNModelImpl model);
                   }
+                  /**

Contributor

gitgabrio Nov 4, 2024

Hi @samuel-beniamin
IINW this is signavio-only behaviour that get to the core code: if so, could you please remove and move to signavio module ? Thanks!

kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/compiler/DMNCompilerImpl.java

-                  private DMNCompilerConfiguration dmnCompilerConfig;
-                  private Deque<DRGElementCompiler> drgCompilers = new LinkedList<>();
+                  private final DMNCompilerConfiguration dmnCompilerConfig;
+                  private final Deque<DRGElementCompiler> drgCompilers = new LinkedList<>();

Contributor

gitgabrio Nov 4, 2024

Hi @samuel-beniamin
IINW this is signavio-only behaviour that get to the core code: if so, could you please remove and move to signavio module ? Thanks!

kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/compiler/DMNCompilerImpl.java

@@ @@ -123,6 +121,7 @@ public class DMNCompilerImpl implements DMNCompiler { @@
                       drgCompilers.add( new KnowledgeSourceCompiler() ); // keep last as it's a void compiler
                   }
                   private final List<AfterProcessDrgElements> afterDRGcallbacks = new ArrayList<>();
+                  private final Map<DMNModel, List<AfterProcessDrgElements>> afterDRGcallbacksForModel = new HashMap<>();

Contributor

gitgabrio Nov 4, 2024

Hi @samuel-beniamin
IINW this is signavio-only behaviour that get to the core code: if so, could you please remove and move to signavio module ? Thanks!

kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/compiler/DMNCompilerImpl.java

+                  /**
+                   * Adds a callback that will be invoked after the DRG elements have been processed, for a given model.
+                   */
+                  public void addCallbackForModel(AfterProcessDrgElements callback, DMNModel model) {

Contributor

gitgabrio Nov 4, 2024

Hi @samuel-beniamin
IINW this is signavio-only behaviour that get to the core code: if so, could you please remove and move to signavio module ? Thanks!

kie-dmn/kie-dmn-core/src/test/java/org/kie/dmn/core/compiler/DMNCompilerImplTest.java

                   @BeforeAll
                   static void setup() {
                       String modelName = "LocalHrefs";
                       parent = new TDefinitions();
                       parent.setName(modelName);
                       parent.setNamespace(nameSpace);
+                      DMNCompilerConfiguration config = DMNFactory.newCompilerConfiguration();

Contributor

gitgabrio Nov 4, 2024

Hi @samuel-beniamin
IINW this is signavio-only behaviour that get to the core code: if so, could you please remove and move to signavio module ? Thanks!

kie-dmn/kie-dmn-core/src/test/java/org/kie/dmn/core/compiler/DMNCompilerImplTest.java

@@ @@ -76,4 +98,27 @@ void getRootElement() { @@
                       retrieved = DMNCompilerImpl.getRootElement(elementReference);
                       assertThat(retrieved).isNotNull().isEqualTo(parent);
                   }

Contributor

gitgabrio Nov 4, 2024

Hi @samuel-beniamin
IINW this is signavio-only behaviour that get to the core code: if so, could you please remove and move to signavio module ? Thanks!


          Merge branch 'apache:main' into incubator-kie-issues#1478

723eff3

Contributor Author

samuel-beniamin commented Nov 13, 2024

Hello @gitgabrio

Well, I tried the approach of the a separate subclass for Signavio specific things, I have some observation and a conclusion:

I ended up not able to inject that Signavio specific compiler easily into the code.
DMNCompilerConfiguration is not Signavio specific, this is related to the core compiler for extensions in general.
drgCompilers are not Signavio specific, they are also used by other extensions, (e.g. trisotech is also using that).
- This makes me think that AfterProcessDrgElements follows the same reasoning, is not Signavio specific (since drgCompilers are not), AfterProcessDrgElements is indeed used only now by Signavio, but that doesn't limit the need to Signavio only.

I have a question, do you know a way with which I can integrate the new compiler into the code by any means, I had the impression that the compiler is not intended to be changed but extended through the DMNCompilerConfiguration only, am I correct?

If I am correct on the previous point, I would actually close this ticket after rethinking this part and also close the ticket, since this is the way the compiler was meant to be extended actually.

Contributor

gitgabrio commented Nov 13, 2024

First of all, many thanks @samuel-beniamin for all that.
I'll need a bit of time to double check, but it is very possible that you are right.
I've answered immediately to first of all thanks you again, I'll let you know ASAP!


          Merge branch 'apache:main' into incubator-kie-issues#1478

099528a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DMN