gitgabrio commented on code in PR #6535:
URL:
https://github.com/apache/incubator-kie-drools/pull/6535#discussion_r2569369734
##########
kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/compiler/ItemDefinitionDependenciesSorter.java:
##########
@@ -37,66 +38,80 @@ public ItemDefinitionDependenciesSorter(String
modelNamespace) {
/**
* Return a new list of ItemDefinition sorted by dependencies (required
dependencies comes first)
+ * @param itemDefinitions list of ItemDefinitions available in the model
+ * @param dmnVersion version of dmn in the current model
*/
- public List<ItemDefinition> sort(List<ItemDefinition> ins) {
+ public List<ItemDefinition> sort(List<ItemDefinition> itemDefinitions,
DMNVersion dmnVersion) {
// In a graph A -> B -> {C, D}
// showing that A requires B, and B requires C,D
// then a depth-first visit would satisfy required ordering, for
example a valid depth first visit is also a valid sort here: C, D, B, A.
- Collection<ItemDefinition> visited = new ArrayList<>(ins.size());
- List<ItemDefinition> dfv = new ArrayList<>(ins.size());
+ Collection<ItemDefinition> visited = new
ArrayList<>(itemDefinitions.size());
+ List<ItemDefinition> sortedItemDefinitions = new
ArrayList<>(itemDefinitions.size());
Review Comment:
👍
##########
kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/compiler/ItemDefinitionDependenciesSorter.java:
##########
@@ -37,66 +38,80 @@ public ItemDefinitionDependenciesSorter(String
modelNamespace) {
/**
* Return a new list of ItemDefinition sorted by dependencies (required
dependencies comes first)
+ * @param itemDefinitions list of ItemDefinitions available in the model
+ * @param dmnVersion version of dmn in the current model
*/
- public List<ItemDefinition> sort(List<ItemDefinition> ins) {
+ public List<ItemDefinition> sort(List<ItemDefinition> itemDefinitions,
DMNVersion dmnVersion) {
// In a graph A -> B -> {C, D}
// showing that A requires B, and B requires C,D
// then a depth-first visit would satisfy required ordering, for
example a valid depth first visit is also a valid sort here: C, D, B, A.
- Collection<ItemDefinition> visited = new ArrayList<>(ins.size());
- List<ItemDefinition> dfv = new ArrayList<>(ins.size());
+ Collection<ItemDefinition> visited = new
ArrayList<>(itemDefinitions.size());
+ List<ItemDefinition> sortedItemDefinitions = new
ArrayList<>(itemDefinitions.size());
- for (ItemDefinition node : ins) {
+ for (ItemDefinition node : itemDefinitions) {
if (!visited.contains(node)) {
- dfVisit(node, ins, visited, dfv);
+ populateSortedItemDefinitions(node, itemDefinitions, visited,
sortedItemDefinitions, dmnVersion);
}
}
- return dfv;
+ return sortedItemDefinitions;
}
/**
* Performs a depth first visit, but keeping a separate reference of
visited/visiting nodes, _also_ to avoid potential issues of circularities.
*/
- private void dfVisit(ItemDefinition node, List<ItemDefinition> allNodes,
Collection<ItemDefinition> visited, List<ItemDefinition> dfv) {
+ private void populateSortedItemDefinitions(ItemDefinition node,
List<ItemDefinition> allNodes, Collection<ItemDefinition> visited,
List<ItemDefinition> sortedItemDefinitions, DMNVersion dmnVersion) {
visited.add(node);
List<ItemDefinition> neighbours = allNodes.stream()
.filter(n ->
!n.getName().equals(node.getName())) // filter out `node`
- .filter(n ->
recurseFind(node, new QName(modelNamespace, n.getName()))) // I pick from
allNodes, those referenced by this `node`. Only neighbours of `node`, because N
is referenced by NODE.
-
.collect(Collectors.toList());
+ .filter(n ->
recurseFind(node, new QName(modelNamespace, n.getName()), dmnVersion)) // I
pick from allNodes, those referenced by this `node`. Only neighbours of `node`,
because N is referenced by NODE.
+ .toList();
for (ItemDefinition n : neighbours) {
if (!visited.contains(n)) {
- dfVisit(n, allNodes, visited, dfv);
+ populateSortedItemDefinitions(n, allNodes, visited,
sortedItemDefinitions, dmnVersion);
}
}
- dfv.add(node);
+ sortedItemDefinitions.add(node);
}
-
- private static boolean recurseFind(ItemDefinition o1, QName qname2) {
+
+ static QName retrieveTypeRef(ItemDefinition o1, DMNVersion dmnVersion) {
if ( o1.getTypeRef() != null ) {
- return extFastEqUsingNSPrefix(o1, qname2);
+ return o1.getTypeRef();
}
- for ( ItemDefinition ic : o1.getItemComponent() ) {
- if ( recurseFind(ic, qname2) ) {
- return true;
+ if (dmnVersion != null && dmnVersion.getDmnVersion() >
DMNVersion.V1_2.getDmnVersion()) {
Review Comment:
Hi @AthiraHari77 : what about ->
```java
QName toReturn = o1.getTypeRef();
if (toReturn == null
&& dmnVersion.getDmnVersion() > DMNVersion.V1_2.getDmnVersion()
&& o1.getFunctionItem() != null) {
toReturn = o1.getFunctionItem().getOutputTypeRef();
}
return toReturn;
```
(we should avoid the `dmnVersion != null ` check because, it it is null,
then there is a bug somewhere else, and we should be aware of that)
?
##########
kie-dmn/kie-dmn-core/src/test/java/org/kie/dmn/core/compiler/ItemDefinitionDependenciesTest.java:
##########
@@ -227,4 +230,53 @@ void circular3() {
assertThat(orderedList.subList(0, 2)).contains(fhirAge, fhirExtension);
assertThat(orderedList.subList(2, 3)).contains(fhirT1);
}
+
+ @Test
+ void testTypeRefWhenPresent() {
+ QName expected = new QName("ns", "date");
+ ItemDefinition item = new TItemDefinition();
+ item.setTypeRef(expected);
+
+ QName result = retrieveTypeRef(item, DMNVersion.V1_2);
+ assertThat(expected).isEqualTo(result);
+ }
+
+ @Test
+ void testTypeRefNull() {
+ ItemDefinition item = new TItemDefinition();
+
+ QName result = retrieveTypeRef(item, DMNVersion.V1_2);
+ assertThat(result).isNull();
+ }
+
+ @Test
+ void testRetrieveTypeRef_dmnVersionNull() {
+ ItemDefinition item = new TItemDefinition();
+
+ QName result = retrieveTypeRef(item, null);
Review Comment:
As in other comment, dmn version should never be `null`.
If you saw that during debug, it means there is a bug somewhere (and an
exception should be thrown)
##########
kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/compiler/ItemDefinitionDependenciesSorter.java:
##########
@@ -37,66 +38,80 @@ public ItemDefinitionDependenciesSorter(String
modelNamespace) {
/**
* Return a new list of ItemDefinition sorted by dependencies (required
dependencies comes first)
+ * @param itemDefinitions list of ItemDefinitions available in the model
+ * @param dmnVersion version of dmn in the current model
*/
- public List<ItemDefinition> sort(List<ItemDefinition> ins) {
+ public List<ItemDefinition> sort(List<ItemDefinition> itemDefinitions,
DMNVersion dmnVersion) {
// In a graph A -> B -> {C, D}
// showing that A requires B, and B requires C,D
// then a depth-first visit would satisfy required ordering, for
example a valid depth first visit is also a valid sort here: C, D, B, A.
- Collection<ItemDefinition> visited = new ArrayList<>(ins.size());
- List<ItemDefinition> dfv = new ArrayList<>(ins.size());
+ Collection<ItemDefinition> visited = new
ArrayList<>(itemDefinitions.size());
+ List<ItemDefinition> sortedItemDefinitions = new
ArrayList<>(itemDefinitions.size());
- for (ItemDefinition node : ins) {
+ for (ItemDefinition node : itemDefinitions) {
if (!visited.contains(node)) {
- dfVisit(node, ins, visited, dfv);
+ populateSortedItemDefinitions(node, itemDefinitions, visited,
sortedItemDefinitions, dmnVersion);
Review Comment:
❤️
##########
kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/compiler/ItemDefinitionDependenciesSorter.java:
##########
@@ -57,46 +58,55 @@ public List<ItemDefinition> sort(List<ItemDefinition> ins) {
/**
* Performs a depth first visit, but keeping a separate reference of
visited/visiting nodes, _also_ to avoid potential issues of circularities.
*/
- private void dfVisit(ItemDefinition node, List<ItemDefinition> allNodes,
Collection<ItemDefinition> visited, List<ItemDefinition> dfv) {
+ private void dfVisit(ItemDefinition node, List<ItemDefinition> allNodes,
Collection<ItemDefinition> visited, List<ItemDefinition> dfv, DMNVersion
dmnVersion) {
visited.add(node);
List<ItemDefinition> neighbours = allNodes.stream()
.filter(n ->
!n.getName().equals(node.getName())) // filter out `node`
- .filter(n ->
recurseFind(node, new QName(modelNamespace, n.getName()))) // I pick from
allNodes, those referenced by this `node`. Only neighbours of `node`, because N
is referenced by NODE.
-
.collect(Collectors.toList());
+ .filter(n ->
recurseFind(node, new QName(modelNamespace, n.getName()), dmnVersion)) // I
pick from allNodes, those referenced by this `node`. Only neighbours of `node`,
because N is referenced by NODE.
+ .toList();
for (ItemDefinition n : neighbours) {
if (!visited.contains(n)) {
- dfVisit(n, allNodes, visited, dfv);
+ dfVisit(n, allNodes, visited, dfv, dmnVersion);
}
}
dfv.add(node);
}
- private static boolean recurseFind(ItemDefinition o1, QName qname2) {
+ private static boolean recurseFind(ItemDefinition o1, QName qname2,
DMNVersion dmnVersion) {
if ( o1.getTypeRef() != null ) {
- return extFastEqUsingNSPrefix(o1, qname2);
+ return extFastEqUsingNSPrefix(o1, o1.getTypeRef(), qname2);
+ }
+ if (dmnVersion != null && dmnVersion.getDmnVersion() >
DMNVersion.V1_2.getDmnVersion()) {
+ FunctionItem fi = o1.getFunctionItem();
+ if (fi != null && fi.getOutputTypeRef() != null) {
+ return extFastEqUsingNSPrefix(o1,
o1.getFunctionItem().getOutputTypeRef(), qname2);
Review Comment:
Could you please apply, @AthiraHari77 ?
--
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]
