dmn-tck / tck

Decision Model and Notation Technology Compatibility Kit
https://dmn-tck.github.io/tck
51 stars 36 forks source link

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

Closed DanielThanner closed 6 years ago

DanielThanner commented 6 years ago

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:

InvalidTestCases.txt

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

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

etirelli commented 6 years ago

@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 commented 6 years ago

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:

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

DanielThanner commented 6 years ago

Created pull request #120 with changed dmn files.

opatrascoiu commented 6 years ago

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

agilepro commented 6 years ago

This is resolved by the PR that was merged today