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

Enhance ConstraintOperator capability #6104

Merged

Conversation

tkobayas
Copy link
Contributor

@tkobayas tkobayas commented Sep 27, 2024

…ares incompatible types

This PR is to enhance ConstraintOperator to be able to use alpha/beta index.

@tkobayas
Copy link
Contributor Author

@mariofusco This PR is not a clear separation as you mentioned in kiegroup/drools-ansible-rulebook-integration#112 (comment)

This PR enhances ConstraintOperator to contain Index.ConstraintType, so a custom ConstraintOperator can have its own predicate while make use of alpha index. (I haven't implemented for beta index yet. I can do it later in this PR). Bare Index.ConstraintType can be both an operator and an index type. I leave it so that I can minimize the change. If you want further separation, feel free to let me know.

- Add unit test
@tkobayas
Copy link
Contributor Author

Updated with beta index support and unit test

Comment on lines +166 to +167
// alpha index hashing is effective, so the predicated is not called
assertThat(customConstraintOperator.counter).isZero();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If alpha index is effective, the predicate in the CustomConstraintOperator is not executed.

Comment on lines +227 to +228
// When beta index is used, the predicate in the custom operator is not actually called
assertThat(customConstraintOperator.counter).isZero();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If beta index is effective, the predicate in the CustomConstraintOperator is not executed.

@tkobayas
Copy link
Contributor Author

As commented above, if alpha/beta index is effective, the predicate in the CustomConstraintOperator is not executed. So implementing type check and error logging in the predicate doesn't work.

I guess alpha index is a more possible use case.

For example,

A user mistakenly wrote wrong rules like this.

condition: event.meta.headers == "AAA"
condition: event.meta.headers == "BBB"
condition: event.meta.headers == "CCC"

Then, an incoming event has meta.headers as a Map.

We can do type check in a predicate in a custom ConstraintOperator, but in the alpha index hashing case, the predicate is not executed and simply no rule would be fired. The user has no clue what is wrong in the rules.

If we want to rescue the situation, we need to implement type check at CompositeObjectSinkAdapter.propagateAssertObject, WDYT? @mariofusco

@tkobayas tkobayas marked this pull request as ready for review September 30, 2024 10:30
@tkobayas tkobayas merged commit f47946f into apache:main Oct 1, 2024
9 of 19 checks passed
rgdoliveira pushed a commit to rgdoliveira/drools that referenced this pull request Oct 3, 2024
* [DROOLS-7635] ansible-rulebook : Raise an error when a condition compares incompatible types

* - Add beta index support
- Add unit test

* changed method names
tkobayas added a commit to tkobayas/drools that referenced this pull request Oct 11, 2024
* [DROOLS-7635] ansible-rulebook : Raise an error when a condition compares incompatible types

* - Add beta index support
- Add unit test

* changed method names
rgdoliveira pushed a commit to kiegroup/drools that referenced this pull request Oct 17, 2024
* [DROOLS-7635] ansible-rulebook : Raise an error when a condition compares incompatible types

* - Add beta index support
- Add unit test

* changed method names
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 this pull request may close these issues.

3 participants