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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 17 commits into from
Jun 22, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a4f6afe
DROOLS-7475 Proposed feature for ['key'] accessor
tarilabs Jun 19, 2023
1cd8cf9
WIP: broken but progressing
tarilabs Jun 19, 2023
7493fde
WIP: broken but null != UNDEF
tarilabs Jun 19, 2023
7cd6073
WIP: broken but progressing
tarilabs Jun 19, 2023
941af73
ALL tests passing
tarilabs Jun 20, 2023
7db369b
small refactor
tarilabs Jun 20, 2023
b1cf0d7
cleanup RulesModelUtil
tarilabs Jun 20, 2023
5f6004c
refactor, small fix for array access, tests
tarilabs Jun 20, 2023
2ebf1f4
fix grammar typo
tarilabs Jun 20, 2023
bc9cb36
refactor ExistsField as discussed with Mario
tarilabs Jun 20, 2023
0f41f50
implement code review feedback
tarilabs Jun 20, 2023
f6ccd37
implement code review feedback
tarilabs Jun 20, 2023
b536459
code review feedback impl about ORIGINAL_MAP_FIELD
tarilabs Jun 20, 2023
b2602bb
BREAKING to investigate indexing issue
tarilabs Jun 20, 2023
08c78d3
revert the disabled commit
tarilabs Jun 20, 2023
4dc4656
implement code review feedback
tarilabs Jun 21, 2023
896b925
code review feedback
tarilabs Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...egration-api/src/main/java/org/drools/ansible/rulebook/integration/api/RulesExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import org.drools.ansible.rulebook.integration.api.rulesengine.RulesExecutorSession;
import org.drools.ansible.rulebook.integration.api.rulesengine.SessionStats;
import org.drools.base.facttemplates.Fact;
import org.kie.api.runtime.KieSession;
import org.kie.api.runtime.rule.Match;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -95,4 +96,8 @@ public String getAllFactsAsJson() {
public CompletableFuture<List<Match>> advanceTime(long amount, TimeUnit unit ) {
return rulesEvaluator.advanceTime(amount, unit );
}

public KieSession asKieSession() {
return rulesEvaluator.asKieSession();
}
}
6 changes: 6 additions & 0 deletions .../java/org/drools/ansible/rulebook/integration/api/rulesengine/AbstractRulesEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.drools.ansible.rulebook.integration.api.io.RuleExecutorChannel;
import org.drools.base.facttemplates.Fact;
import org.drools.core.common.InternalFactHandle;
import org.kie.api.runtime.KieSession;
import org.kie.api.runtime.rule.Match;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -247,4 +248,9 @@ private List<Match> atomicRuleEvaluation(boolean processEventInsertion, Supplier

return matches;
}

@Override
public KieSession asKieSession() {
return rulesExecutorSession.asKieSession();
}
}
3 changes: 3 additions & 0 deletions ...src/main/java/org/drools/ansible/rulebook/integration/api/rulesengine/RulesEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.drools.ansible.rulebook.integration.api.rulesengine;

import org.drools.ansible.rulebook.integration.api.RulesExecutorContainer;
import org.kie.api.runtime.KieSession;
import org.kie.api.runtime.rule.Match;

import java.util.Collection;
Expand Down Expand Up @@ -41,4 +42,6 @@ public interface RulesEvaluator {
static RulesEvaluator createRulesEvaluator( RulesExecutorSession rulesExecutorSession, boolean async ) {
return async ? new AsyncRulesEvaluator(rulesExecutorSession) : new SyncRulesEvaluator(rulesExecutorSession);
}

KieSession asKieSession();
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe is package-protected for testing puposes? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

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.

}
4 changes: 4 additions & 0 deletions ...in/java/org/drools/ansible/rulebook/integration/api/rulesengine/RulesExecutorSession.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,4 +164,8 @@ void setExecuteActions(boolean executeActions) {
public boolean isMatchMultipleRules() {
return rulesSet.isMatchMultipleRules();
}

public KieSession asKieSession() {
return kieSession;
}
}
49 changes: 45 additions & 4 deletions ...-integration-api/src/test/java/org/drools/ansible/rulebook/integration/api/AlphaTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,27 @@
package org.drools.ansible.rulebook.integration.api;

import java.util.List;

import org.drools.ansible.rulebook.integration.api.rulesmodel.PrototypeFactory;
import org.drools.base.definitions.InternalKnowledgePackage;
import org.drools.base.facttemplates.FactTemplate;
import org.drools.base.facttemplates.FactTemplateObjectType;
import org.drools.base.rule.IndexableConstraint;
import org.drools.base.util.index.ConstraintTypeOperator;
import org.drools.core.reteoo.AlphaNode;
import org.drools.core.reteoo.EntryPointNode;
import org.drools.core.reteoo.ObjectSink;
import org.drools.core.reteoo.ObjectTypeNode;
import org.drools.core.reteoo.Rete;
import org.drools.kiesession.rulebase.InternalKnowledgeBase;
import org.drools.model.Prototype;
import org.drools.modelcompiler.facttemplate.FactFactory;
import org.junit.Test;
import org.kie.api.definition.rule.Rule;
import org.kie.api.runtime.rule.Match;

import java.util.List;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class AlphaTest {

Expand All @@ -31,14 +47,17 @@ public void testEqualsWithFixedValue() {
" ]\n" +
" },\n" +
" \"enabled\": true,\n" +
" \"name\": null\n" +
" \"name\": \"R1\"\n" +
" }\n" +
" }\n" +
" ]\n" +
"}";

RulesExecutor rulesExecutor = RulesExecutorFactory.createFromJson(json);

Rete rete = ((InternalKnowledgeBase) rulesExecutor.asKieSession().getKieBase()).getRete();
assertConstraintType(rete, "R1", ConstraintTypeOperator.EQUAL);

List<Match> matchedRules = rulesExecutor.processEvents("{ \"i\": 3 }").join();
assertEquals(0, matchedRules.size());

Expand All @@ -47,11 +66,33 @@ public void testEqualsWithFixedValue() {

matchedRules = rulesExecutor.processEvents("{ \"i\": 3, \"j\": 3 }").join();
assertEquals(1, matchedRules.size());
assertEquals("r_0", matchedRules.get(0).getRule().getName());
assertEquals("R1", matchedRules.get(0).getRule().getName());

rulesExecutor.dispose();
}

private void assertConstraintType(Rete rete, String ruleName, ConstraintTypeOperator expectedType) {
boolean asserted = false;
assertEquals("expecting only 1 pkg.", 1, rete.getRuleBase().getPackages().length);
InternalKnowledgePackage ipkg = rete.getRuleBase().getPackages()[0];
Prototype default_proto = PrototypeFactory.getPrototype(PrototypeFactory.DEFAULT_PROTOTYPE_NAME);
FactTemplate factTemplate = FactFactory.prototypeToFactTemplate(default_proto, ipkg);
EntryPointNode epn = rete.getEntryPointNodes().values().iterator().next();
ObjectTypeNode otn = epn.getObjectTypeNodes().get(new FactTemplateObjectType(factTemplate));
ObjectSink[] sinks = otn.getObjectSinkPropagator().getSinks();
for (ObjectSink objectSink : sinks) {
AlphaNode alphaNode = (AlphaNode) objectSink;
assertEquals("expecting that one rule has one AlphaNode.", 1, alphaNode.getAssociatedRules().length);
Rule rule = alphaNode.getAssociatedRules()[0];
if (rule.getName().equals(ruleName)) {
IndexableConstraint constraint = (IndexableConstraint) alphaNode.getConstraint();
assertEquals(expectedType, constraint.getConstraintType());
asserted = true;
}
}
assertTrue(asserted);
}

@Test
public void testEqualsOn2Fields() {
String json =
Expand Down
2 changes: 2 additions & 0 deletions ...tegration-api/src/test/java/org/drools/ansible/rulebook/integration/api/TimedOutTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.drools.ansible.rulebook.integration.api.rulesengine.SessionStats;
import org.drools.base.facttemplates.Fact;
import org.json.JSONObject;
import org.junit.Ignore;
danielezonca marked this conversation as resolved.
Show resolved Hide resolved
import org.junit.Test;
import org.kie.api.runtime.rule.Match;

Expand Down Expand Up @@ -183,6 +184,7 @@ public void testSimpleTimedOut() {
assertEquals(0, stats.getEventsSuppressed());
}

@Ignore // TODO NOOOOOO
tarilabs marked this conversation as resolved.
Show resolved Hide resolved
@Test
public void testTimedOutWithAutomaticClockAdvance() throws IOException {
String json =
Expand Down
4 changes: 2 additions & 2 deletions ...s/ansible/rulebook/integration/protoextractor/prototype/ExtractorPrototypeExpression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@
import org.drools.model.Prototype;
import org.drools.model.PrototypeDSL;
import org.drools.model.PrototypeExpression;
import org.drools.model.PrototypeExpression.IndexableExpression;
import org.drools.model.PrototypeFact;
import org.drools.model.functions.Function1;

public class ExtractorPrototypeExpression implements PrototypeExpression {
public class ExtractorPrototypeExpression implements PrototypeExpression, IndexableExpression {
/**
* 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
Expand All @@ -40,7 +41,6 @@ public Function1<PrototypeFact, Object> asFunction(Prototype prototype) {
};
}

// TODO used for indexing, normalizing chunks.
public String getFieldName() {
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

done with

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

are semantically guarding JUST-and-only the indexing.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

implemented #56 (comment)

with:

return this.computedFieldName;
}
Expand Down