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.

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

Using InputData with imports, and imports of imports #274

Closed
StrayAlien opened this issue Mar 25, 2019 · 12 comments · Fixed by #328
Closed

Using InputData with imports, and imports of imports #274

StrayAlien opened this issue Mar 25, 2019 · 12 comments · Fixed by #328

Comments

@StrayAlien
Copy link
Contributor

All, imports can be pretty confusing, so bear with me here.

This is not so much as issue - but more of a ‘let’s agree on how we do this’. It is about how to supply input data to a model when the inputData is an import, and import of an import, or deeper …. and so on. And we don’t have anything guaranteed to be unique.

For TCK test model execution we have to supply the required input data as input nodes.

Say we have three models - A, B, C. A imports B as foo, and B imports C also as foo. The import name property is model-local. We could have used different names. Let’s say all three models have an InputData element with id fooInput.

We need to supply three things called fooInput to the test, and they all mean something different.

Does the test input data look something like this (as JSON for clarity) ?:

{
  "fooInput": "for model A",
  "foo": {
    "fooInput": "for model B",
    "foo": {
      "fooInput": "for model C"
    }
  }
}

Get my drift? When A references fooInput it means the data in its own model. When A references foo.fooInput, it means the data for model B, and when B references fooInput it means its own data, and when B references foo.fooInput it means model C and so on.

As nothing in the spec guarantees uniqueness between models somehow we have to distinguish between what input data is meant for what model.

The only real thing we could have possibly used to make things unique would be import locationURI#elementId …. But locationURI is optional ….

Hopefully that makes some small sense.

Happy for comments.

Greg.

PS. I did have a look at the RH import unit tests and they seem to assume that an import name is globally unique. If I read that right, I don't actually think that is correct. But I am not familiar with that code base so I could be wrong.

@agilepro
Copy link
Contributor

agilepro commented Mar 29, 2019

There was a discussion on this in the meeting today. Simon explained how this problem was solved by the implementations. (I didn't exactly follow it.) It might be nice to write this down because the agreement was that this had to be figured out, and it was not in the spec anywhere.

@StrayAlien
Copy link
Contributor Author

StrayAlien commented Apr 1, 2019

@agilepro @SimonRinguette @tarilabs

I'll summarise. Yes - the spec is very light here on what is a complex thing. Wait until we start testing decision services with import inputdata - but that will be another separate issue I raise for discussion.

The discussion agreed the input data in my previous post here should look like this:

{
  "fooInput": "for model A",
  "foo.fooInput": "for model B",
  "foo.foo.fooInput": "for model C"
}

(personally I prefer the nested data model - easier to group data and less effort to process etc.)

Effectively, the test input data uses a 'dot-separated' path notation where the final path element is the id of the drg element being referenced and all the previous path elements are the import names along the dependency path that leads to it.

The implication is, as agreed by Simon, is that if the same model is imported by different models then there will be multiple 'dot path' access to elements within it.

Using the example in my previous post say

A --(foo)-->B--(foo)-->C

Input nodes in the test data that are for 'C' must be prefixed with foo.foo. A never directly references C - it doesn't even know it exists* (* - back to this point at the end)

However, what if A also references C directly say as bar so now C is referenced both by A and B.

For those times in A when C is used directly then the test data must also have a bar entry such that the test input data looks like this:

{
  "fooInput": "for model A",
  "foo.fooInput": "for model B",
  "foo.foo.fooInput": "for model C",
  "bar.fooInput": "for model C also"
}

Personally, I think its a bit scrappy. But it seems that is what we might have to do. Sounds like RH and Trisotech have taken this approach and given that the spec does not cover how model data is structured I think we run with it.

I have submitted a PR for a test that shows it. Hopefully. #280

  • On a final note, Simon mentioned that, say, a decision could refer to imports of imports (of imports) using a dot-path notation inside literal expressions and not have a <informationRequirement> href referring to it. IMO, I don't think that should be permitted as it circumvents the 'explicit drg requirements' aspect of DMN. Visually, I should be able to see all requirement of a drg so I have not created tests for this.

Happy to discuss.

@tarilabs
Copy link
Member

tarilabs commented Apr 1, 2019

"My" preference would be for structured input data, instead of dot-notation.

@StrayAlien
Copy link
Contributor Author

"My" preference would be for structured input data, instead of dot-notation.

Hi, Matteo - apols - I thought this is what you and Simon had agreed upon.

I also think structured is better .

@StrayAlien
Copy link
Contributor Author

@tarilabs - regarding:

"My" preference would be for structured input data, instead of dot-notation.

mine too. I that something we can agree upon for tests? @SimonRinguette - is there are particular reason for having them dot separated?

@tarilabs
Copy link
Member

tarilabs commented May 9, 2019

I that something we can agree upon for tests?

Before providing an answer, eventually by means of a pragmatic PR with test case, I prefer to double check we are aligned on what is --in my view-- even more of a priority in the context of testing for DMN imports.

For this reason @StrayAlien can you kindly feedback on #294 please?

Thx

@agilepro
Copy link
Contributor

@StrayAlien has responded to #294 in general agreement, so Matteo is going to flesh out a proposal on that, and then we can revisit this with a clear resolution.

@agilepro
Copy link
Contributor

PR #294 will be merged at the end of the month, and that will open the way to resolve this issue.

@agilepro
Copy link
Contributor

agilepro commented Jun 7, 2019

Are there any other issues? The changes are merged to allow DMN import. Matteo is working on an example. If that is clear we should close this.

@agilepro
Copy link
Contributor

Still working on the examples for this. Leaving ticket in until fully resolved.

@agilepro
Copy link
Contributor

almost ready

@tarilabs
Copy link
Member

Sorry I have this PR still in backlog, will keep posted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants