Skip to content
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

DMNCompilerImpl class code refactoring and Test cases #6267

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
+292 −68
Open

DMNCompilerImpl class code refactoring and Test cases #6267

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
6fbd51c
TestCases for DMNCompilerImpl class
ChinchuAjith Feb 19, 2025
3e7d044
Merge branch 'apache:main' into incubator-kie-issues#1748
ChinchuAjith Feb 19, 2025
c952e19
TestCases for DMNCompilerImpl class
ChinchuAjith Feb 20, 2025
85ae9a7
Merge branch 'apache:main' into incubator-kie-issues#1748
ChinchuAjith Feb 21, 2025
3bc52b9
TestCases for DMNCompilerImpl class - changes in assertions
ChinchuAjith Feb 21, 2025
e9f25ca
TestCases for DMNCompilerImpl class - changes in assertions
ChinchuAjith Feb 21, 2025
91ecdbd
Changing Testcase names
ChinchuAjith Feb 24, 2025
9eee2d8
Refactoring Compile method
ChinchuAjith Feb 24, 2025
d540ece
Refactoring Compile method
ChinchuAjith Feb 24, 2025
459a605
Refactoring Compile method
ChinchuAjith Feb 25, 2025
86e64da
Merge branch 'apache:main' into incubator-kie-issues#1748
ChinchuAjith Feb 25, 2025
5f830ac
Refactoring Compile method-Incorporating review comments, code cleanup
ChinchuAjith Feb 26, 2025
6c28863
Test cases for refactored method
ChinchuAjith Feb 27, 2025
6dfae4e
Merge branch 'apache:main' into incubator-kie-issues#1748
ChinchuAjith Feb 28, 2025
84a703e
Merge branch 'apache:main' into incubator-kie-issues#1748
ChinchuAjith Mar 6, 2025
cc0d641
Merge branch 'apache:main' into incubator-kie-issues#1748
ChinchuAjith Mar 7, 2025
45658fe
Review Comments fix
ChinchuAjith Mar 7, 2025
bce987f
Review Comments fix:replace if/else with switch in DMNCompilerImpl class
ChinchuAjith Mar 7, 2025
753815b
[incubator-kie-issues#1748] Refactoring and unit testing DMNCompilerI…
Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 70 additions & 47 deletions kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/compiler/DMNCompilerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@

import javax.xml.namespace.QName;

import org.codehaus.plexus.util.CollectionUtils;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls remove this import

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

import org.drools.io.FileSystemResource;
import org.kie.api.io.Resource;
import org.kie.dmn.api.core.DMNCompiler;
Expand Down Expand Up @@ -209,60 +210,82 @@ public DMNModel compile(Definitions dmndefs, Collection<DMNModel> dmnModels, Res
}
DMNModelImpl model = new DMNModelImpl(dmndefs, resource);
model.setRuntimeTypeCheck(((DMNCompilerConfigurationImpl) dmnCompilerConfig).getOption(RuntimeTypeCheckOption.class).isRuntimeTypeCheck());
DMNCompilerContext ctx = configureDMNCompiler(model.getFeelDialect(), relativeResolver);
if (!dmndefs.getImport().isEmpty()) {
iterateImports(dmndefs, dmnModels, model, relativeResolver );
}
processItemDefinitions(ctx, model, dmndefs);
processDrgElements(ctx, model, dmndefs);
return model;
}

private DMNCompilerContext configureDMNCompiler(FEELDialect feeldialect, Function<String, Reader> relativeResolver) {

DMNCompilerConfigurationImpl cc = (DMNCompilerConfigurationImpl) dmnCompilerConfig;
List<FEELProfile> helperFEELProfiles = cc.getFeelProfiles();
DMNFEELHelper feel = new DMNFEELHelper(cc.getRootClassLoader(), helperFEELProfiles, model.getFeelDialect());
DMNFEELHelper feel = new DMNFEELHelper(cc.getRootClassLoader(), helperFEELProfiles, feeldialect);
DMNCompilerContext ctx = new DMNCompilerContext(feel);
ctx.setRelativeResolver(relativeResolver);
return ctx;
}

private void iterateImports(Definitions dmndefs, Collection<DMNModel> dmnModels, DMNModelImpl model, Function<String, Reader> relativeResolver ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method need to be tested, at least verify that elements are correctly iterated over

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if/else inside this method have been replaced with switch case. Tested and is working fine now. Please review once and let me know if any further modification required here.

List<DMNModel> toMerge = new ArrayList<>();
if (!dmndefs.getImport().isEmpty()) {
for (Import i : dmndefs.getImport()) {
if (ImportDMNResolverUtil.whichImportType(i) == ImportType.DMN) {
Either<String, DMNModel> resolvedResult = ImportDMNResolverUtil.resolveImportDMN(i, dmnModels, (DMNModel m) -> new QName(m.getNamespace(), m.getName()));
DMNModel located = resolvedResult.cata(msg -> {
MsgUtil.reportMessage(logger,
DMNMessage.Severity.ERROR,
i,
model,
null,
null,
Msg.IMPORT_NOT_FOUND_FOR_NODE,
msg,
i);
return null;
}, Function.identity());
if (located != null) {
String iAlias = Optional.ofNullable(i.getName()).orElse(located.getName());
// incubator-kie-issues#852: The idea is to not treat the anonymous models as import, but to "merge" them
// with original one,
// because otherwise we would have to deal with clashing name aliases, or similar issues
if (iAlias != null && !iAlias.isEmpty()) {
model.setImportAliasForNS(iAlias, located.getNamespace(), located.getName());
importFromModel(model, located, iAlias);
} else {
toMerge.add(located);
}
}
} else if (ImportDMNResolverUtil.whichImportType(i) == ImportType.PMML) {
processPMMLImport(model, i, relativeResolver);
model.setImportAliasForNS(i.getName(), i.getNamespace(), i.getName());
} else {
MsgUtil.reportMessage(logger,
DMNMessage.Severity.ERROR,
null,
model,
null,
null,
Msg.IMPORT_TYPE_UNKNOWN,
i.getImportType());
}
for (Import i : dmndefs.getImport()) {
if (ImportDMNResolverUtil.whichImportType(i) == ImportType.DMN) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if/else could be replaced by switch

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with switch

resolveDMNImportType(i, dmnModels, model, toMerge );
} else if (ImportDMNResolverUtil.whichImportType(i) == ImportType.PMML) {
processPMMLImport(model, i, relativeResolver);
model.setImportAliasForNS(i.getName(), i.getNamespace(), i.getName());
} else {
logErrorMessage(model, i.getImportType());
}
}

toMerge.forEach(mergedModel -> processMergedModel(model, (DMNModelImpl) mergedModel));
processItemDefinitions(ctx, model, dmndefs);
processDrgElements(ctx, model, dmndefs);
return model;
}

static void resolveDMNImportType(Import i, Collection<DMNModel> dmnModels, DMNModelImpl model, List<DMNModel> toMerge) {
Either<String, DMNModel> resolvedResult = ImportDMNResolverUtil.resolveImportDMN(i, dmnModels, (DMNModel m) -> new QName(m.getNamespace(), m.getName()));
DMNModel located = resolvedResult.cata(msg -> {
MsgUtil.reportMessage(logger,
DMNMessage.Severity.ERROR,
i,
model,
null,
null,
Msg.IMPORT_NOT_FOUND_FOR_NODE,
msg,
i);
return null;
}, Function.identity());
checkLocatedDMNModel(i, located, model, toMerge);

}

private void logErrorMessage(DMNModelImpl model, String importType) {
MsgUtil.reportMessage(logger,
DMNMessage.Severity.ERROR,
null,
model,
null,
null,
Msg.IMPORT_TYPE_UNKNOWN,
importType);
}

static void checkLocatedDMNModel(Import i, DMNModel located, DMNModelImpl model, List<DMNModel> toMerge) {
if (located != null) {
String iAlias = Optional.ofNullable(i.getName()).orElse(located.getName());
// incubator-kie-issues#852: The idea is to not treat the anonymous models as import, but to "merge" them
// with original one,
// because otherwise we would have to deal with clashing name aliases, or similar issues
if (iAlias != null && !iAlias.isEmpty()) {
model.setImportAliasForNS(iAlias, located.getNamespace(), located.getName());
importFromModel(model, located, iAlias);
} else {
toMerge.add(located);
}
}
}

private void processPMMLImport(DMNModelImpl model, Import i, Function<String, Reader> relativeResolver) {
Expand Down Expand Up @@ -348,7 +371,7 @@ protected static URI resolveRelativeURI(DMNModelImpl model, String relative) thr
}
}

private void importFromModel(DMNModelImpl model, DMNModel m, String iAlias) {
static void importFromModel(DMNModelImpl model, DMNModel m, String iAlias) {
model.addImportChainChild(((DMNModelImpl) m).getImportChain(), iAlias);
for (ItemDefNode idn : m.getItemDefinitions()) {
model.getTypeRegistry().registerType(idn.getType());
Expand Down
194 changes: 183 additions & 11 deletions kie-dmn/kie-dmn-core/src/test/java/org/kie/dmn/core/compiler/DMNCompilerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls remove this empty line

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

/* 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
Expand All @@ -18,20 +18,32 @@
*/
package org.kie.dmn.core.compiler;


import org.drools.io.ClassPathResource;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.kie.dmn.model.api.DMNElementReference;
import org.kie.dmn.model.api.Definitions;
import org.kie.dmn.model.api.InformationRequirement;
import org.kie.dmn.model.v1_5.TDMNElementReference;
import org.kie.dmn.model.v1_5.TDefinitions;
import org.kie.dmn.model.v1_5.TInformationRequirement;
import org.kie.api.io.Resource;
import org.kie.dmn.api.core.DMNCompiler;
import org.kie.dmn.api.core.DMNMessageType;
import org.kie.dmn.api.core.DMNModel;

import org.kie.dmn.core.impl.DMNModelImpl;
import org.kie.dmn.model.api.*;
import org.kie.dmn.model.v1_5.*;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.*;
import java.util.List;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType;

class DMNCompilerImplTest {

public static final Logger LOG = LoggerFactory.getLogger(DMNCompilerImplTest.class);

private static DMNCompiler dMNCompiler;
private static final String nameSpace = "http://www.montera.com.au/spec/DMN/local-hrefs";
private static Definitions parent;

Expand All @@ -41,6 +53,8 @@ static void setup() {
parent = new TDefinitions();
parent.setName(modelName);
parent.setNamespace(nameSpace);

dMNCompiler = new DMNCompilerImpl();
}

@Test
Expand Down Expand Up @@ -70,10 +84,168 @@ void getRootElement() {

InformationRequirement informationRequirement = new TInformationRequirement();
elementReference.setParent(informationRequirement);
assertThatExceptionOfType(RuntimeException.class).isThrownBy(() -> DMNCompilerImpl.getRootElement(elementReference));

assertThatExceptionOfType(RuntimeException.class).isThrownBy(
() -> DMNCompilerImpl.getRootElement(elementReference)).withMessageContaining
("Failed to get Definitions parent for org.kie.dmn.model.v1_5");
informationRequirement.setParent(parent);
retrieved = DMNCompilerImpl.getRootElement(elementReference);
assertThat(retrieved).isNotNull().isEqualTo(parent);
}
}

@Test
void compile() {
List<DMNModel> dmnModels = new ArrayList<>();
String nameSpace = "http://www.trisotech.com/dmn/definitions/_f27bb64b-6fc7-4e1f-9848-11ba35e0df44";
Resource resource = new ClassPathResource( "valid_models/DMNv1_5/Imported_Model_Unamed.dmn",
this.getClass());
DMNModel importedModel = dMNCompiler.compile( resource, dmnModels);
assertThat(importedModel).isNotNull();
assertThat(importedModel.getNamespace()).isNotNull().isEqualTo(nameSpace);
assertThat(importedModel.getMessages()).isEmpty();
}

@Test
void compileWithUnknownTypeModelImports() {
List<DMNModel> dmnModels = new ArrayList<>();
String nameSpace = "http://www.trisotech.com/dmn/definitions/_f27bb64b-6fc7-4e1f-9848-11ba35e0df44";
String modelName = "Imported Model";
String importType = String.valueOf(ImportDMNResolverUtil.ImportType.UNKNOWN);
Resource resource = new ClassPathResource( "valid_models/DMNv1_5/Imported_Model_Unamed.dmn",
this.getClass());
DMNModel model = dMNCompiler.compile( resource, dmnModels);
assertThat(model).isNotNull();
Definitions dmnDefn = model.getDefinitions();
addImport(dmnDefn, importType, nameSpace, modelName);
dmnModels.add(model);
model = dMNCompiler.compile(dmnDefn, resource, dmnModels);
assertThat(model).isNotNull();
assertThat(model.getName()).isNotNull().isEqualTo(modelName);
assertThat(model.getMessages()).isNotEmpty();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THis test is error prone. It rely on the assumption that there is only one message, without checking it before. Please add assertion on model.getMessages() size.
Please notice assertions on the same object could be chained

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

assertThat(model.getMessages().get(0).getText()).isEqualTo("DMN: Import type unknown: 'UNKNOWN'. (Invalid FEEL syntax on the referenced expression) ");

}

@Test
void compileWithImportingDmnModel() {
List<DMNModel> dmnModels = new ArrayList<>();
Resource resource = new ClassPathResource( "valid_models/DMNv1_5/Imported_Model_Unamed.dmn",
this.getClass());
DMNModel importedModel = dMNCompiler.compile( resource, dmnModels);
assertThat(importedModel).isNotNull();
dmnModels.add(importedModel);

//imported model - Importing_Named_Model.dmn
String nameSpace = "http://www.trisotech.com/dmn/definitions/_f79aa7a4-f9a3-410a-ac95-bea496edabgc";
resource = new ClassPathResource( "valid_models/DMNv1_5/Importing_Named_Model.dmn",
this.getClass());

DMNModel importingModel = dMNCompiler.compile(resource, dmnModels);
assertThat(importingModel).isNotNull();
assertThat(importingModel.getNamespace()).isNotNull().isEqualTo(nameSpace);
assertThat(importingModel.getMessages()).isEmpty();
}

@Test
void compileImportingModelWithoutImportedModel() {
List<DMNModel> dmnModels = new ArrayList<>();
String modelName = "Importing named Model";
Resource resource = new ClassPathResource( "valid_models/DMNv1_5/Importing_Named_Model.dmn",
this.getClass());
DMNModel model = dMNCompiler.compile( resource, dmnModels);
assertThat(model).isNotNull();
assertThat(model.getName()).isNotNull().isEqualTo(modelName);

Definitions dmnDefn = model.getDefinitions();
dmnModels.add(model);
model = dMNCompiler.compile(dmnDefn, resource, dmnModels);
assertThat(model).isNotNull();
assertThat(model.getName()).isNotNull().isEqualTo(modelName);
assertThat(model.getMessages()).isNotEmpty();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous comment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

assertThat(model.getMessages().get(0).getMessageType()).isEqualTo(DMNMessageType.IMPORT_NOT_FOUND);

}

@Test
void resolveDMNImportType() {
List<DMNModel> toMerge = new ArrayList<>();
List<DMNModel> dmnModels = new ArrayList<>();
Resource resource = new ClassPathResource( "valid_models/DMNv1_5/Imported_Model_Unamed.dmn",
this.getClass());
DMNModel importedModel = dMNCompiler.compile( resource, dmnModels);
assertThat(importedModel).isNotNull();
dmnModels.add(importedModel);

//imported model - Importing_Named_Model.dmn
String nameSpace = "http://www.trisotech.com/dmn/definitions/_f79aa7a4-f9a3-410a-ac95-bea496edabgc";
resource = new ClassPathResource( "valid_models/DMNv1_5/Importing_Named_Model.dmn",
this.getClass());
DMNModel importingModel = dMNCompiler.compile(resource, dmnModels);
assertThat(importingModel).isNotNull();
assertThat(importingModel.getNamespace()).isNotNull().isEqualTo(nameSpace);
assertThat(importingModel.getMessages()).isEmpty();

Import input = importingModel.getDefinitions().getImport().get(0);
DMNModelImpl model = new DMNModelImpl(importingModel.getDefinitions(), resource);
DMNCompilerImpl.resolveDMNImportType(input, dmnModels, model, toMerge);
assertThat(model.getMessages()).isEmpty();
assertThat(model.getImportAliasesForNS().entrySet().stream().findFirst().get().getValue().getLocalPart()).isNotNull().isEqualTo("Imported Model");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an assertion on the get() invocation, or throw an exception

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Added assertion on the get() invocation


}

@Test
void checkLocatedDMNModel() {
List<DMNModel> toMerge = new ArrayList<>();
List<DMNModel> dmnModels = new ArrayList<>();
String nameSpace = "http://www.trisotech.com/dmn/definitions/_f79aa7a4-f9a3-410a-ac95-bea496edabgc";
Resource resource = new ClassPathResource( "valid_models/DMNv1_5/Importing_Named_Model.dmn",
this.getClass());
DMNModel importingModel = dMNCompiler.compile(resource, dmnModels);
assertThat(importingModel).isNotNull();
assertThat(importingModel.getNamespace()).isNotNull().isEqualTo(nameSpace);

Import input = importingModel.getDefinitions().getImport().get(0);
DMNModelImpl model = new DMNModelImpl(importingModel.getDefinitions(), resource);
DMNModel located = new DMNModelImpl(importingModel.getDefinitions(), resource);
DMNCompilerImpl.checkLocatedDMNModel(input, located, model, toMerge);
assertThat(importingModel).isNotNull();
assertThat(importingModel.getNamespace()).isNotNull().isEqualTo(nameSpace);
assertThat(toMerge.isEmpty()).isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls replace with more idiomatic
assertThat(toMerge).isEmpty();

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected

}

@Test
void checkLocatedDMNModelWithAliasNull() {
String namespace="http://www.trisotech.com/dmn/definitions/_f79aa7a4-f9a3-410a-ac95-bea496edabgc";
List<DMNModel> toMerge = new ArrayList<>();
List<DMNModel> dmnModels = new ArrayList<>();
Resource resource = new ClassPathResource( "valid_models/DMNv1_5/Importing_EmptyNamed_Model_Without_Href_Namespace.dmn",
this.getClass());
DMNModel emptyNamedModel = dMNCompiler.compile( resource, dmnModels);
assertThat(emptyNamedModel).isNotNull();
dmnModels.add(emptyNamedModel);

Import input = emptyNamedModel.getDefinitions().getImport().get(0);
DMNModelImpl model = new DMNModelImpl(emptyNamedModel.getDefinitions(), resource);
DMNModel located = new DMNModelImpl(emptyNamedModel.getDefinitions(), resource);
DMNCompilerImpl.checkLocatedDMNModel(input, located, model, toMerge);
assertThat(emptyNamedModel).isNotNull();
assertThat(toMerge.isEmpty()).isFalse();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls replace with more idiomatic
assertThat(toMerge).isNotEmpty();

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

assertThat(toMerge.get(0).getNamespace()).isNotNull().isEqualTo(namespace);

}

private void addImport(Definitions dmnDefs, String importType, String nameSpace, String modelName) {
dmnDefs.setName(modelName);
Import import1 = new TImport();
import1.setNamespace(nameSpace);
import1.setName(modelName);
import1.setImportType(importType);
import1.setParent(dmnDefs);
import1.setLocationURI(importType);
dmnDefs.getImport().add(import1);
}

}