DROOLS-7475 Proposed feature for ['key'] accessor #56

tarilabs · 2023-06-20T06:26:49Z

this PR is actually merge-able without having to necessarily wait for ansible-side (ie: retrofit and backward compatible)

to review this PR, I believe will be easier at times to see each commit individually "as a step", maybe is more easier to digest that way 🙃
to avoid bloating further the PR, I've tried to minimize the refactor on existing code.

I remain available also via chat/quickcall if preferred.

JIRA: https://issues.redhat.com/browse/DROOLS-7475

referenced Pull Requests:

nesting the protoextractor into the maven build

Results : Failed tests: testWithSelectAttr(org.drools.ansible.rulebook.integration.api.NullTest): expected:<1> but was:<0> test(org.drools.ansible.rulebook.integration.api.NullTest): expected:<1> but was:<0> testRepeatedOnceWithin(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinWithOr(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinWithAnd(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testSelectOnNull(org.drools.ansible.rulebook.integration.api.SelectTest): expected:<1> but was:<0> Tests in error: testOnceWithinInRule(org.drools.ansible.rulebook.integration.api.OnceWithinTest): Cannot invoke "java.util.Map.get(Object)" because "map" is null testOnceWithinInCondition(org.drools.ansible.rulebook.integration.api.OnceWithinTest): Cannot invoke "java.util.Map.get(Object)" because "map" is null testOnceAfterWithOr(org.drools.ansible.rulebook.integration.api.OnceAfterTest): Cannot invoke "Object.toString()" because the return value of "org.drools.base.facttemplates.Fact.get(String)" is null Tests run: 138, Failures: 6, Errors: 3, Skipped: 0

Results : Failed tests: testRepeatedOnceWithin(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinWithOr(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinWithAnd(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> Tests in error: testOnceWithinInRule(org.drools.ansible.rulebook.integration.api.OnceWithinTest): Cannot invoke "java.util.Map.get(Object)" because "map" is null testOnceWithinInCondition(org.drools.ansible.rulebook.integration.api.OnceWithinTest): Cannot invoke "java.util.Map.get(Object)" because "map" is null testOnceAfterWithOr(org.drools.ansible.rulebook.integration.api.OnceAfterTest): Cannot invoke "Object.toString()" because the return value of "org.drools.base.facttemplates.Fact.get(String)" is null Tests run: 138, Failures: 3, Errors: 3, Skipped: 0

Failed tests: testRepeatedOnceWithin(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinInRule(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinWithOr(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinWithAnd(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinInCondition(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceAfterWithOr(org.drools.ansible.rulebook.integration.api.OnceAfterTest): expected:<2> but was:<1> Tests run: 138, Failures: 6, Errors: 0, Skipped: 0

tkobayas

Thank you for the great work!

Btw, does it make sense to add a few integration tests (under drools-ansible-rulebook-integration-api) to demonstrate the new capability?

tarilabs · 2023-06-20T09:24:16Z

Thank you for the great work!
Btw, does it make sense to add a few integration tests (under drools-ansible-rulebook-integration-api) to demonstrate the new capability?

thanks @tkobayas 🙇
That is correct observation and thank you for raising, nevertheless I believe would be helpful to do e2e test only after ansible-side approve of their related parser changes (I want to minimize the number of e2e tests which are not actually "requested").
In other words, I believe would make sense to enrich e2e testing in following PRs

tkobayas · 2023-06-20T09:28:08Z

In other words, I believe would make sense to enrich e2e testing in following PRs

Got it, thanks!

...org/drools/ansible/rulebook/integration/protoextractor/prototype/ValueExtractionVisitor.java

...ible/rulebook/integration/protoextractor/prototype/CompleteExtractorPrototypeExpression.java

mariofusco · 2023-06-20T11:56:13Z

...ools/ansible/rulebook/integration/protoextractor/prototype/ExtractorPrototypeExpression.java

+    }
+
+    // TODO used for indexing, normalizing chunks.
+    public String getFieldName() {


Unfortunately this is breaking indexing, see https://github.com/kiegroup/drools/blob/main/drools-model/drools-canonical-model/src/main/java/org/drools/model/PrototypeDSL.java#L135 What I suggest is introducing an interface named something like FieldAccessingExpression declaring a getFieldName() method, making both this class and PrototypeFieldValue to implement it and changing the downcasts in PrototypeDSL to use it. The only problem is that this will of course require also a change in Drools, but this seems unavoidable (or at least I don't have any better idea).

Also it would be nice to add a few assertions in some tests, for instance here, to make sure that the alpha node is indexed as expected.

seems no other way indeed given PrototypeFieldValue cannot be instantiated (due to constructor visibility)

done with

https://github.com/kiegroup/drools/pull/5333

b2602bb

What about just adding the extractor method to the PrototypeExpression interface returning an Optional? Like
Optional<String> getIndexingKey();

In this way we can avoid the instanceof and the downcasting: each expression can be indexable returning a value or not with an empty option

thanks for the comment @danielezonca
I believe this pends a confirmation from Mario these 2 guards:

https://github.com/kiegroup/drools/pull/5333/files#diff-3f9ca0220c837c33d5a557b4af7727094a6c21475c926ce19dfc1bffd02c3c90R135

https://github.com/kiegroup/drools/pull/5333/files#diff-3f9ca0220c837c33d5a557b4af7727094a6c21475c926ce19dfc1bffd02c3c90R186

are semantically guarding JUST-and-only the indexing.

@danielezonca I don't see any semantic difference between the method returning an Optional and the new interface and actually I still prefer to use the second to have clearer marker between which expression is indexable and which isn't.

implemented #56 (comment)

with:

kiegroup/drools@a26fce1

4dc4656

...api/src/main/java/org/drools/ansible/rulebook/integration/api/rulesmodel/RulesModelUtil.java

mariofusco · 2023-06-20T12:20:45Z

...-integration-api/src/test/java/org/drools/ansible/rulebook/integration/api/AdditionTest.java

@@ -70,7 +71,7 @@ public void testExecuteRules() {
    public void testExecuteRulesSet() {
        RulesSet rulesSet = new RulesSet();
        rulesSet.addRule().withCondition().all()
-                .addSingleCondition(prototypeField("nested.i"), Index.ConstraintType.EQUAL, prototypeField("nested.j").add(fixedValue(1)));


Just a note: when we will move the extractor module into Drools I think we could also deprecate (or completely remove) the old prototypeField.

one step at a time 🙃
the correct place to mark this would be another ticket 😉

mariofusco

Great job! Except for a few minor comments the only real problem is that this pull request as it is breaks indexing. Please fix it as discussed.

see also kiegroup/drools-ansible-rulebook-integration#56 (comment) make IndexableExpression interface so downstream can leverage this rather than specific concrete class

...-integration-api/src/test/java/org/drools/ansible/rulebook/integration/api/TimedOutTest.java

tarilabs · 2023-06-21T10:26:05Z

@danielezonca @mariofusco implemented #56 (comment)

with:

danielezonca

Thanks for the PR, few comments but they are all minor or clarification, well done 👍

danielezonca · 2023-06-21T10:34:24Z

...pi/src/main/java/org/drools/ansible/rulebook/integration/api/rulesengine/RulesEvaluator.java

@@ -41,4 +42,6 @@ public interface RulesEvaluator {
    static RulesEvaluator createRulesEvaluator( RulesExecutorSession rulesExecutorSession, boolean async ) {
        return async ? new AsyncRulesEvaluator(rulesExecutorSession) : new SyncRulesEvaluator(rulesExecutorSession);
    }
+
+    KieSession asKieSession();


I'm not familiar with this RulesEvaluator interface (even if we have probably similar concepts in Drools repo) but I guess it is a "facade" to hide the Session (a bit similar to the RuleEvaluator in Drools). I don't think we want to expose a asKieSession.

@mariofusco
Wdyt?

I believe is package-protected for testing puposes? 🤔

Yes, this is only for testing purpose and unfortunately unavoidable. In particular the need is to access the KieSession and then the KieBase and the Rete network to check that a node was indexed as expected. I don't see any other way to do this.

danielezonca · 2023-06-21T11:26:29Z

pom.xml

@@ -33,6 +34,7 @@
    <version.assertj>3.20.2</version.assertj>
    <version.jackson>2.13.1</version.jackson>
    <version.json>20230227</version.json>
+    <version.org.antlr4>4.10.1</version.org.antlr4>


Do you think this version and Drools repo antlr versions can conflict or this is not possible?
In general if they must stay aligned it might be useful to create a followup ticket to introduce the usage of drools-parent or similar to centralise the declaration.
Wdyt?

We could consider drools-ansible-rulebook-intergration to inherit the same parent (or even jboss-patent) in a subsequent task/pr ?

Yes I think something like that in another PR 👍

...ctor/src/test/java/org/drools/ansible/rulebook/integration/protoextractor/ExtractorTest.java

...ansible/rulebook/integration/protoextractor/prototype/ExtractorPrototypeExpressionUtils.java

danielezonca · 2023-06-21T11:30:00Z

...ools/ansible/rulebook/integration/protoextractor/prototype/ExtractorPrototypeExpression.java

+    /**
+     * This is okay for ansible-integration work as prototype do not define accessor programmatically,
+     * but having prototype definition ignored in {@link #asFunction(Prototype)} call, ought to be revised 
+     * if porting this module into Drools and generalizing beyond usage in ansible-integration.
+     */
+    public static final Prototype IGNORED = PrototypeDSL.prototype("IGNORED");


It should be ideal to have a tracking ticket and here the id/link

...-integration-api/src/test/java/org/drools/ansible/rulebook/integration/api/TimedOutTest.java

.../org/drools/ansible/rulebook/integration/api/domain/temporal/OnceAbstractTimeConstraint.java

danielezonca · 2023-06-21T11:35:16Z

...tor/src/main/antlr4/org/drools/ansible/rulebook/integration/protoextractor/Protoextractor.g4

@@ -0,0 +1,26 @@
+grammar Protoextractor;


@mariofusco
Is native compilation a requirement/nice to have/planned scenario for this integration module?
If so we should verify if this antlr part is compatible

Not sure why we would think this is unsupported at native-image? 🤔
To my knowledge, we are not codegen at parsing time, so this is just standard java code in some way, don't you agree?

Yes indeed, it might be necessary to list some classes for reflection but nothing more than that overall

* DROOLS-7475 Proposed feature for ['key'] accessor see also kiegroup/drools-ansible-rulebook-integration#56 (comment) make IndexableExpression interface so downstream can leverage this rather than specific concrete class * fix PrototypeDSL * implement code review feedback --------- Co-authored-by: mariofusco <[email protected]>

This reverts commit 28b41e5.

work-in-progress as i'm volunteering to work on this e2e, so this is a quite-complete draft for the ansible-rulebook side; i'll keep posted as i will progress on drools-ansible-rulebook-integration side. EDIT: demonstrate working locally and added e2e test as requested **JIRA**: https://issues.redhat.com/browse/DROOLS-7475 **referenced Pull Requests**: * #539 * ansible/drools_jpy#50 * kiegroup/drools-ansible-rulebook-integration#56 * https://github.com/kiegroup/drools/pull/5333 * https://issues.redhat.com/browse/AAP-10738 * kiegroup/drools-ansible-rulebook-integration#65 * ansible/drools_jpy#51 --------- Co-authored-by: Madhu Kanoor <[email protected]> Co-authored-by: Alex <[email protected]>

* DROOLS-7475 Proposed feature for ['key'] accessor nesting the protoextractor into the maven build * WIP: broken but progressing Results : Failed tests: testWithSelectAttr(org.drools.ansible.rulebook.integration.api.NullTest): expected:<1> but was:<0> test(org.drools.ansible.rulebook.integration.api.NullTest): expected:<1> but was:<0> testRepeatedOnceWithin(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinWithOr(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinWithAnd(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testSelectOnNull(org.drools.ansible.rulebook.integration.api.SelectTest): expected:<1> but was:<0> Tests in error: testOnceWithinInRule(org.drools.ansible.rulebook.integration.api.OnceWithinTest): Cannot invoke "java.util.Map.get(Object)" because "map" is null testOnceWithinInCondition(org.drools.ansible.rulebook.integration.api.OnceWithinTest): Cannot invoke "java.util.Map.get(Object)" because "map" is null testOnceAfterWithOr(org.drools.ansible.rulebook.integration.api.OnceAfterTest): Cannot invoke "Object.toString()" because the return value of "org.drools.base.facttemplates.Fact.get(String)" is null Tests run: 138, Failures: 6, Errors: 3, Skipped: 0 * WIP: broken but null != UNDEF Results : Failed tests: testRepeatedOnceWithin(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinWithOr(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinWithAnd(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> Tests in error: testOnceWithinInRule(org.drools.ansible.rulebook.integration.api.OnceWithinTest): Cannot invoke "java.util.Map.get(Object)" because "map" is null testOnceWithinInCondition(org.drools.ansible.rulebook.integration.api.OnceWithinTest): Cannot invoke "java.util.Map.get(Object)" because "map" is null testOnceAfterWithOr(org.drools.ansible.rulebook.integration.api.OnceAfterTest): Cannot invoke "Object.toString()" because the return value of "org.drools.base.facttemplates.Fact.get(String)" is null Tests run: 138, Failures: 3, Errors: 3, Skipped: 0 * WIP: broken but progressing Failed tests: testRepeatedOnceWithin(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinInRule(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinWithOr(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinWithAnd(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceWithinInCondition(org.drools.ansible.rulebook.integration.api.OnceWithinTest): expected:<0> but was:<1> testOnceAfterWithOr(org.drools.ansible.rulebook.integration.api.OnceAfterTest): expected:<2> but was:<1> Tests run: 138, Failures: 6, Errors: 0, Skipped: 0 * ALL tests passing * small refactor * cleanup RulesModelUtil * refactor, small fix for array access, tests * fix grammar typo * refactor ExistsField as discussed with Mario * implement code review feedback * implement code review feedback * code review feedback impl about ORIGINAL_MAP_FIELD * BREAKING to investigate indexing issue * revert the disabled commit * implement code review feedback * code review feedback (cherry picked from commit 28b41e5)

tarilabs added 8 commits June 19, 2023 09:29

DROOLS-7475 Proposed feature for ['key'] accessor

a4f6afe

nesting the protoextractor into the maven build

ALL tests passing

941af73

small refactor

7db369b

cleanup RulesModelUtil

b1cf0d7

refactor, small fix for array access, tests

5f6004c

tarilabs requested review from danielezonca, mariofusco, tkobayas, pibizza and dupliaka June 20, 2023 06:26

tarilabs mentioned this pull request Jun 20, 2023

AAP-10738 DROOLS-7475 Proposed feature for ['key'] accessor ansible/ansible-rulebook#539

Merged

tarilabs added 2 commits June 20, 2023 08:46

fix grammar typo

2ebf1f4

refactor ExistsField as discussed with Mario

bc9cb36

tkobayas approved these changes Jun 20, 2023

View reviewed changes