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

Definition of Business Knowlege Model return type is wrong for 22 test cases #112

Closed
DanielThanner opened this issue Jan 19, 2018 · 8 comments

Comments

@DanielThanner
Copy link
Contributor

22 testcases listed in the attached file InvalidTestCases.txt are defining the function invocation return type using the Information Item of the Business Knowledge Model. This is wrong.

The specification says on page 50 for the variable/InformationItem: "The instance of InformationItem that is bound to the function. An invocation can reference this variable by name" and "This attribute defines a variable that holds the FunctionDefinition, allowing a Decision to invoke it by name".

The InformationItem of the Business Knowledge Model defines the name and typeRef corresponding to the included FunctionDefinition. The typeRef is always a Function. FEEL/DMN has no function type. Therefore the typeRef of the variable/InformationItem should be omitted.

To specify the type that is returned by the FunctionDefinition, the typeRef attribute of the body expression of the FunctionDefinition of the Business Knowledge Model should be used.

22 testcases are affected. The typeRef attribute must move from BusinessKnowledgeModel/Variable to BusinessKnowledgeModel/EncapsulatedLogic/Body expression.

Currently these 22 test cases do not comply with the DMN 1.1 specification.

There are OMG issues for DMN 1.2 about fixing the doubled description of variable/InformationItem in table 14. But the content is the same.

Please find attached:

  • a file listing the 22 wrong test cases
  • an example testcase that is invalid (renamed to *.txt) with an additional screenshot file
  • an example testcase that was modified (renamed to *.txt) and is valid with an additional screenshot file

InvalidTestCases.txt

INVALID_0009-invocation-arithmetic.txt
invalid_0009-invocation-arithmetic

VALID_0009-invocation-arithmetic.txt
valid_0009-invocation-arithmetic

@etirelli
Copy link
Contributor

@DanielThanner what you are pointing out matches my initial implementation/understanding of DMN, but after discussing it further with Gary, it seems that because there is currently no way to define in FEEL a type like "feel:function", that the implementation has to "implicitly" understand that the BKM is a function that returns a "feel:number" due to that type definition.

It is a mess, I would gladly change that if we can get it fixed in the spec. (or if my understanding of the issue turns out to be wrong)

@opatrascoiu
Copy link
Contributor

opatrascoiu commented Jan 20, 2018

I agree with Daniel. According to DMN spec the typeRef of the variable is a FunctionType. The type of the body is the return type of the function. The FunctionType doesn't have to be specified, it can be inferred from the types of the parameters and the return type.

We should raise the issue at the next RTF.

We have two options:

  • keep the spec as it is and change the tests
  • change the spec so that the type of the variable is the return type and keep the tests as they are.

My preference is to change the tests. I don't see anything wrong with the spec. The function type can be inferred from parameters and return type.

@DanielThanner
Copy link
Contributor Author

Our preference is also to change the DMN TCK tests. Currently there is nothing wrong with the specification. Only a feel:function type is missing, which can be introduced for DMN 1.2

--> To be compliant with the specification, we should change the DMN models of the affected test cases

@etirelli
Copy link
Contributor

etirelli commented Feb 2, 2018

Discussed this further with other people, Denis, Gary, Bruce. It seems like the best approach will be to remove the typeRef from the BKM variables as suggest already by @DanielThanner .

@opatrascoiu it would be good if you can include something like a "feel:function[...]" type on your type improvement proposal to the RTF for 1.2.

@agilepro
Copy link
Contributor

agilepro commented Feb 2, 2018

Agreed to update test cases. Leaving issue in to track until we get the changes make.

@DanielThanner
Copy link
Contributor Author

Created pull request #120 with changed dmn files.

@opatrascoiu
Copy link
Contributor

@etirelli I'll look into it next week. Hopefully, I'll have more time.

@agilepro
Copy link
Contributor

agilepro commented Feb 9, 2018

This is resolved by the PR that was merged today

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

No branches or pull requests

4 participants