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

Validate Expression.Values #46

Open
sobolewskikamil opened this issue Jul 30, 2018 · 1 comment
Open

Validate Expression.Values #46

sobolewskikamil opened this issue Jul 30, 2018 · 1 comment

Comments

@sobolewskikamil
Copy link
Collaborator

sobolewskikamil commented Jul 30, 2018

Problem description
It is possible to specify Expression.Values which contains other expressions. Validations that are missing in this scenario:

  1. Expressions which are inside are not validated - missing validation propagation to expressions inside Expression.Values.
  2. Values returned from different expressions are not validated (e.g. values(String.class, value("${fact.longValue}")) - longValue is not String)

Suggested solution

  • Add missing Expression.Values traversing in ReferenceValidator (with tests)
  • Add validation for types in ReferenceValidator. Placeholders types should be resolved by the current logic. Remember to use isAssignableFrom. Cover null cases. Cover nested values() - might require collections checks. If logic with isAssignableFrom is too complicated consider using commons-lang utils.

Acceptance criteria

  • Problems 1 and 2 are successfully reported by ReferenceValidator
  • Tests are prepared
  • Documentation is updated
  • Code review is done
@adamszadkowski
Copy link
Contributor

I have thought about this issue. There is no type validation for value("${...}") in Expression. It would be probably better to implement this in more generic way. Implementing this only for Expession.Values would be not generic enough to extend it further for Expression.Value for other Expression uses. By other Expression uses I mean for example:

RuleDsl.ruleBuilder()
  .predicate(
    value("${booleanParameter}")
  )

Above example will fail on runtime in current implementation.
Also it is not always possible to check the correct type, e.g.:

RuleDsl.ruleBuilder()
  .action("action",
    param("context", value("${ctx}")),
  )

In above example it is not possible to know what type "context" should be. In such cases we could assume Object type and fail on runtime. The same will happen on function calls in predicate.

I would suggest to implement this in more generic way by creating new tree structure basing on predicate in Rule model. This tree would contain types. By traversing this tree we could find conflicts in types.

What do you think about such solution?

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

No branches or pull requests

2 participants