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

Only verify FEEL expressions; warn that others are currently unsupported #98

Merged
merged 1 commit into from
Apr 18, 2022

Conversation

pSub
Copy link
Member

@pSub pSub commented Apr 10, 2022

Currently, dmn-check only supports analysis of FEEL expressions. With this
commit the expression language that is specified in the DMN file is honored and
a warning is issued for all other expression languages.

Support for other expression languages is on the roadmap but with no schedule so
far. This feature makes dmn-check useable if other expression languages are
used in a DMN file, see issue #88.

Closes issue #83.

@codecov
Copy link

codecov bot commented Apr 10, 2022

Codecov Report

Merging #98 (3bf6980) into master (4fc4d15) will increase coverage by 2.03%.
The diff coverage is 98.41%.

@@             Coverage Diff              @@
##             master      #98      +/-   ##
============================================
+ Coverage     84.88%   86.92%   +2.03%     
- Complexity      430      523      +93     
============================================
  Files            45       47       +2     
  Lines           900     1063     +163     
  Branches         46       55       +9     
============================================
+ Hits            764      924     +160     
- Misses          100      101       +1     
- Partials         36       38       +2     
Impacted Files Coverage Δ
.../main/java/de/redsix/dmncheck/feel/FeelParser.java 99.11% <85.71%> (-0.89%) ⬇️
.../main/java/de/redsix/dmncheck/util/Expression.java 100.00% <100.00%> (ø)
...dsix/dmncheck/util/TopLevelExpressionLanguage.java 100.00% <100.00%> (ø)
...validators/ConnectedRequirementGraphValidator.java 92.04% <100.00%> (+4.11%) ⬆️
...x/dmncheck/validators/InputEntryTypeValidator.java 100.00% <100.00%> (+6.25%) ⬆️
.../dmncheck/validators/InputValuesTypeValidator.java 94.44% <100.00%> (+3.53%) ⬆️
...tors/ItemDefinitionAllowedValuesTypeValidator.java 97.14% <100.00%> (+0.98%) ⬆️
.../dmncheck/validators/OutputEntryTypeValidator.java 100.00% <100.00%> (+6.66%) ⬆️
...dmncheck/validators/OutputValuesTypeValidator.java 94.44% <100.00%> (+3.53%) ⬆️
...six/dmncheck/validators/ShadowedRuleValidator.java 88.09% <100.00%> (-0.80%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fc4d15...3bf6980. Read the comment docs.

@pSub pSub force-pushed the feature/check-expression-language branch 2 times, most recently from 1a69f42 to 7d18105 Compare April 11, 2022 20:50
Currently, `dmn-check` only supports analysis of FEEL expressions. With this
commit the expression language that is specified in the DMN file is honored and
a warning is issued for all other expression languages.

Support for other expression languages is on the roadmap but with no schedule so
far. This feature makes `dmn-check` useable if other expression languages are
used in a DMN file, see issue #88.

Closes issue #83.
@pSub pSub force-pushed the feature/check-expression-language branch from 7d18105 to 3bf6980 Compare April 11, 2022 20:51
@sonarcloud
Copy link

sonarcloud bot commented Apr 11, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 11 Code Smells

98.2% 98.2% Coverage
0.0% 0.0% Duplication

@pSub pSub merged commit d7839ab into master Apr 18, 2022
@pSub pSub deleted the feature/check-expression-language branch April 18, 2022 20:52
@nairagit
Copy link

Hi @pSub,

Could you please provide an update if the validation is enabled for javascript as well or is there a plan for it?

Regards

@pSub
Copy link
Member Author

pSub commented Feb 10, 2023

Hi @nairagit, personally I haven't used JavaScript in DMN. I'll try to look into possible validations and create a ticket specifing what needs to be done to support basic JavaScript analysis.

Do you have any requirements on what should be validated / analyzed?

@nairagit
Copy link

hi @pSub ,

In the current dmn validator, if we have javascript in any of the columns in the DMN, the plugin errored out.
Then you updated the validator to provide warnings instead of errors.

The requirement is to validate the dmn even if it has javascript in any of the columns in the DMN

Please let me know if I need to open a separate ticket for this or it can be tracked here

@pSub
Copy link
Member Author

pSub commented Feb 10, 2023

Alright, I think I understand. If you have javascript all the way nearly every validation will back out with the warning that currently only FEEL is supported. That is not a helpful result.

The thing is, that javascript is much more expressive, which makes certain validations (that are implemented for FEEL) impossible or at least very hard. But I can think of some that would be helpful and not too hard to implement.

For example checking that the variables used in the javascript expression are bound by DMN Inputs. In this example the validation would check that the variables Mass and Height are indeed inputs to the literal expression.

An other thing that seems reasonable is to do static analysis of the javascript (eg. with https://github.com/cs-au-dk/TAJS or https://github.com/wala/WALA). I suspect this should be fairly straightforward as well.

Do those two validations sound helpful to you? Could you share a DMN file with a lot of javascript (either publicly or privately) so that I can see what a real world example looks like? As I said, I have no real experience with javascript inside DMN.

If those options sound good to you, please open a separate ticket in which we can document the requirements.

By the way: Thank you for using dmn-check and providing feeback! ❤️ 😍

For the sake of completeness: A validation that would certainly be hard to implement is the ShadowedRuleValidator. In this validation we have to check if the codomain (the range of values the expression potentially evaluates to) of an expression is included or equal to the codomain of an other expression. Solving this for javascript in general would lead to solving the halting problem (I think), which is impossible, so we would have to restrict ourselves to some non-touring complete subset of javascript for those validations. That is certainly doable, but sounds like a lot of work.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants