feat: Support for DMN model imports #215

tracyhires · 2023-04-25T20:31:01Z

Description

Imported DMN models can be loaded via URI reference, or by namespace if already loaded. This does not fully address issue #71, as a model cannot be loaded from Zeebe

Related issues

contributes to #71

Bumps [log4j-core](https://github.com/apache/logging-log4j2) from 2.17.1 to 2.20.0. - [Release notes](https://github.com/apache/logging-log4j2/releases) - [Changelog](https://github.com/apache/logging-log4j2/blob/release-2.x/CHANGELOG.adoc) - [Commits](apache/logging-log4j2@rel/2.17.1...rel/2.20.0) --- updated-dependencies: - dependency-name: org.apache.logging.log4j:log4j-core dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [log4j-slf4j-impl](https://github.com/apache/logging-log4j2) from 2.19.0 to 2.20.0. - [Release notes](https://github.com/apache/logging-log4j2/releases) - [Changelog](https://github.com/apache/logging-log4j2/blob/release-2.x/CHANGELOG.adoc) - [Commits](apache/logging-log4j2@rel/2.19.0...rel/2.20.0) --- updated-dependencies: - dependency-name: org.apache.logging.log4j:log4j-slf4j-impl dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [log4j-api](https://github.com/apache/logging-log4j2) from 2.17.1 to 2.20.0. - [Release notes](https://github.com/apache/logging-log4j2/releases) - [Changelog](https://github.com/apache/logging-log4j2/blob/release-2.x/CHANGELOG.adoc) - [Commits](apache/logging-log4j2@rel/2.17.1...rel/2.20.0) --- updated-dependencies: - dependency-name: org.apache.logging.log4j:log4j-api dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps scala-maven-plugin from 4.8.0 to 4.8.1. --- updated-dependencies: - dependency-name: net.alchim31.maven:scala-maven-plugin dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [maven-surefire-plugin](https://github.com/apache/maven-surefire) from 2.22.2 to 3.0.0. - [Release notes](https://github.com/apache/maven-surefire/releases) - [Commits](apache/maven-surefire@surefire-2.22.2...surefire-3.0.0) --- updated-dependencies: - dependency-name: org.apache.maven.plugins:maven-surefire-plugin dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [slf4j-api](https://github.com/qos-ch/slf4j) from 2.0.6 to 2.0.7. - [Release notes](https://github.com/qos-ch/slf4j/releases) - [Commits](https://github.com/qos-ch/slf4j/commits) --- updated-dependencies: - dependency-name: org.slf4j:slf4j-api dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps [feel-engine](https://github.com/camunda/feel-scala) from 1.15.3 to 1.16.0. - [Release notes](https://github.com/camunda/feel-scala/releases) - [Commits](camunda/feel-scala@1.15.3...1.16.0) --- updated-dependencies: - dependency-name: org.camunda.feel:feel-engine dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Add a test case to verify the failure if a decision is invoked with a non-existing decision id.

If a decision is invoked with a non-existing decision id, the engine returns a failure with an empty audit log. This caused an exception because the audit log can't handle an empty list. Remove the public properties `rootEntry` and `requiredEntries` from the audit log. These properties doesn't work if the audit log entries are empty. Note that the property `rootEntry` may contained an audit log entry but no from the root decision. If the evaluation of a required decision failed, the engine doesn't add an entry for the root decision.

Adjust the test case for the audit log after removing the audit log properties.

Add another test case to verify the failure if a decision is invoked with a non-existing name (in addition to the case for the id).

The engine fails the parsing of a DMN if it contains a cyclic dependency between BKMs.

Remove duplicated logic.

Rename variables to improve their meaning.

Morph the dependency check into a tail-recursive function.

Extract the cyclic dependency check into a method for a better overview.

The dependency `log4j-slf4j-impl` is not compatible with `slf4j2`. As a result, the DMN engine doesn't print any logging. Replace the dependency with the compatible version `log4j-slf4j2-impl`.

Bumps license-maven-plugin from 4.1 to 4.2. --- updated-dependencies: - dependency-name: com.mycila:license-maven-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Bumps camunda-dmn-model from 7.18.0 to 7.19.0. --- updated-dependencies: - dependency-name: org.camunda.bpm.model:camunda-dmn-model dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

When a decision C has a dependency on decision A and B, and decision B also has a dependency on decision A, this is not a cyclic dependency. The DMN should be parsed correctly. Regression test for #213.

When a decision C has a dependency on decision A and B, and decision B also has a dependency on decision A, this is not a cyclic dependency. The DMN should be parsed correctly.

Issue # 71 - Dmn imports

CLAassistant · 2023-04-25T20:31:10Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ tracyhires
❌ actions-user
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

saig0 · 2023-04-26T08:48:57Z

@tracyhires awesome. 🥳 Thank you for your contribution. 🚀

Before I review, please rebase your PR on the latest changes. And, check the #215 (comment) from the SLA bot. 🍪

Issue # 71 - Dmn imports

saig0

@tracy-hires I finally reviewed your changes. You found a nice and simple solution to support the importing of DMN files. 🎉

I have a few minor comments about the changes. But a major concern about the general idea of processing the imported DMN files during the parsing.

On one side, it fits nicely with the current parsing and evaluation logic. But it has some downsides:

The imported DMNs must be parsed first. So, the order of parsing the DMNs is important. This is a hard requirement, for example, when integrating this into Zeebe. As a user, I (or the process application) deploy all DMNs at once.
The imported DMNs can't be updated. They are parsed directly into the importing DMN. As a user, I can't deploy only the imported DMNs but I must deploy also the importing DMN.

To get rid of the downsides, I would suggest processing the imports during the evaluation. I drafted a PoC here #222.

What do you think? Do you agree with my concern?

saig0 · 2023-06-15T03:51:35Z

src/main/scala/org/camunda/dmn/DmnEngine.scala

@@ -137,6 +148,7 @@ class DmnEngine(configuration: DmnEngine.Configuration =

  private val valueMapper = loadValueMapper()
  private val functionProvider = loadFunctionProvider()
+  private val loadedModels: Map[String, DmnModelInstance] = Map.empty


🔧 This variable is never used. We can delete it.

saig0 · 2023-06-15T04:04:40Z

src/main/scala/org/camunda/dmn/model/xml/instance/ImportAwareDmnModelInstanceImpl.scala

+                                       modelBuilder: ModelBuilder,
+                                       document: DomDocument,
+                                       private val dmnModelInstanceProvider: DmnModelInstanceProvider)
+  extends DmnModelInstanceImpl(model, modelBuilder, document) {


🤔 This is an interesting idea. By extending the DmnModelInstanceImpl, we can hide the importing logic in this class transparently from the caller of the model API.

However, I would prefer composition over inheritance. The DmnModelInstanceImpl is a core class of the model API.

saig0 · 2023-06-15T04:10:53Z

src/main/scala/org/camunda/dmn/model/xml/instance/StatefulDmnModelInstanceProvider.scala

+ * A DmnModelInstanceProvider that maintains a list of all models that it has already been used to load,
+ * which can then be retrieved by namespace
+ */
+class StatefulDmnModelInstanceProvider extends DmnParser with DmnModelInstanceProvider {


🔧 We should favor composition over inheritance. For example, by injecting the provider in the parser.

saig0 · 2023-06-15T11:06:33Z

src/main/scala/org/camunda/dmn/model/xml/instance/ImportAwareDmnModelInstanceImpl.scala

+  implicit class ModelQualifiedElementName(modelElement: NamedElement) {
+    def qualifiedName: String = {


❌ We should avoid implicit classes. They can be hard to understand and need to be migrated for Scala 3.

saig0 · 2023-06-27T04:46:47Z

src/main/scala/org/camunda/dmn/model/xml/instance/URILocatorDmnModelInstanceProvider.scala

+  override def loadModel[T](namespace: String, locator: Option[T]): Option[DmnModelInstance] = {
+    super.loadModel(namespace, locator)
+      .orElse(locator.map {
+        case s: String => readModelFromStream(URI.create(s).inputStream)
+        case uri: URI => readModelFromStream(uri.inputStream)
+        case _ => null
+      })
+  }


❌ It feels insecure to load an arbitrary URL from the DMN XML.

tracyhires · 2023-06-27T16:27:20Z

Your solution to the problem is more elegant than mine, making obsolete both the need to extend the DmnModelInstanceImpl, and the accompanying implicit class which was used to overcome the associated issues.

It seems, then, that we should close this PR, and finish the work you've outlined that is yet to be done on #222. Would you like help completing those items?

saig0 · 2023-07-03T09:22:35Z

I'm closing this PR in favor of #222.

@tracyhires this topic doesn't have the highest priority for me. But I'll work on it eventually (maybe in Q4 2023). Feel free to contribute based on #222. 🚀

dependabot bot and others added 30 commits April 25, 2023 15:13

test: verify failure for non-existing decision

f86643c

Add a test case to verify the failure if a decision is invoked with a non-existing decision id.

test: remove the audit log properties from the test

db11fce

Adjust the test case for the audit log after removing the audit log properties.

test: verify failure for non-existing decision by name

472b1cc

Add another test case to verify the failure if a decision is invoked with a non-existing name (in addition to the case for the id).

test: verify cyclic dependencies in BKMs

c163785

fix: detect cyclic dependencies between BKMs

5a209d5

The engine fails the parsing of a DMN if it contains a cyclic dependency between BKMs.

refactor: cyclic dependency check

fa99e31

Remove duplicated logic.

refactor: cyclic dependency check

3881147

Rename variables to improve their meaning.

refactor: cyclic dependency check

78e6295

Morph the dependency check into a tail-recursive function.

refactor: extract cyclic dependency check

c1f9893

Extract the cyclic dependency check into a method for a better overview.

deps: replace log4j-slf4j-impl by log4j-slf4j2-impl

ed57bc7

The dependency `log4j-slf4j-impl` is not compatible with `slf4j2`. As a result, the DMN engine doesn't print any logging. Replace the dependency with the compatible version `log4j-slf4j2-impl`.

Create CODEOWNERS

3607bba

refactor: move variable to its usage

c8949d9

build: change version to 1.7.5-SNAPSHOT

9375b2d

build: change version to 1.8.0-SNAPSHOT

d7b25da

build: ignore AuditLog API change

7e1f6c9

release(v1.8.0)

526b96b

release(v1.8.0): prepare for next development iteration

9553a49

test: verify nested non-cyclic dependencies in decisions

52bedf3

When a decision C has a dependency on decision A and B, and decision B also has a dependency on decision A, this is not a cyclic dependency. The DMN should be parsed correctly. Regression test for #213.

fix: incorrect cyclic dependency detection

ab9e039

When a decision C has a dependency on decision A and B, and decision B also has a dependency on decision A, this is not a cyclic dependency. The DMN should be parsed correctly.

release(v1.8.1)

b42c9a7

actions-user and others added 2 commits April 25, 2023 15:13

release(v1.8.1): prepare for next development iteration

b9ea01a

Issue 71 dmn import (#2)

6bc26be

Issue # 71 - Dmn imports

tracyhires requested a review from saig0 as a code owner April 25, 2023 20:31

saig0 changed the title ~~Support for dmn model imports~~ feat: Support for DMN model imports Apr 26, 2023

tracyhires added 3 commits April 26, 2023 08:30

Issue 71 dmn import (#2)

c4956df

Issue # 71 - Dmn imports

Merge branch 'camunda-main'

832519f

more special characters to search for when escaping names in expressions

e33641c

saig0 requested changes Jun 27, 2023

View reviewed changes

saig0 mentioned this pull request Jun 27, 2023

Import and evaluate a decision from another DMN #71

Open

saig0 closed this Oct 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support for DMN model imports #215

feat: Support for DMN model imports #215

tracyhires commented Apr 25, 2023 •

edited by saig0

Loading

CLAassistant commented Apr 25, 2023 •

edited

Loading

saig0 commented Apr 26, 2023

saig0 left a comment

saig0 Jun 15, 2023

saig0 Jun 15, 2023

saig0 Jun 15, 2023

saig0 Jun 15, 2023

saig0 Jun 27, 2023

tracyhires commented Jun 27, 2023

saig0 commented Jul 3, 2023

		implicit class ModelQualifiedElementName(modelElement: NamedElement) {
		def qualifiedName: String = {

feat: Support for DMN model imports #215

feat: Support for DMN model imports #215

Conversation

tracyhires commented Apr 25, 2023 • edited by saig0 Loading

Description

Related issues

CLAassistant commented Apr 25, 2023 • edited Loading

saig0 commented Apr 26, 2023

saig0 left a comment

Choose a reason for hiding this comment

saig0 Jun 15, 2023

Choose a reason for hiding this comment

saig0 Jun 15, 2023

Choose a reason for hiding this comment

saig0 Jun 15, 2023

Choose a reason for hiding this comment

saig0 Jun 15, 2023

Choose a reason for hiding this comment

saig0 Jun 27, 2023

Choose a reason for hiding this comment

tracyhires commented Jun 27, 2023

saig0 commented Jul 3, 2023

tracyhires commented Apr 25, 2023 •

edited by saig0

Loading

CLAassistant commented Apr 25, 2023 •

edited

Loading