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

Fix evaluation of is not defined expression #108

Merged
merged 1 commit into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ public AnyCondition any() {
return anyCondition;
}

@Override
public boolean isSingleCondition() {
return rootCondition.isSingleCondition();
}

@Override
public ViewItem toPattern(RuleGenerationContext ruleContext) {
return rootCondition.toPattern(ruleContext);
Expand Down Expand Up @@ -93,6 +98,11 @@ public AllCondition(RuleGenerationContext ruleContext) {
protected org.drools.model.Condition.Type getConditionType() {
return org.drools.model.Condition.Type.AND;
}

@Override
public boolean isSingleCondition() {
return false;
}
}

public static class AnyCondition extends MultipleConditions<AnyCondition> {
Expand All @@ -115,6 +125,11 @@ protected void beforeBinding() {
protected void afterBinding() {
ruleContext.popContext();
}

@Override
public boolean isSingleCondition() {
return false;
}
}

public abstract static class PatternCondition implements Condition {
Expand Down Expand Up @@ -187,6 +202,11 @@ public PatternCondition negate(RuleGenerationContext ruleContext) {
.withLhs(lhs.negate(ruleContext))
.withRhs(rhs.negate(ruleContext));
}

@Override
public boolean isSingleCondition() {
return false;
}
}

public static class OrCondition extends CombinedPatternCondition {
Expand Down Expand Up @@ -221,6 +241,11 @@ public PatternCondition negate(RuleGenerationContext ruleContext) {
.withLhs(lhs.negate(ruleContext))
.withRhs(rhs.negate(ruleContext));
}

@Override
public boolean isSingleCondition() {
return false;
}
}

public static class SingleCondition<P extends MultipleConditions> extends PatternCondition {
Expand Down Expand Up @@ -283,5 +308,10 @@ public SingleCondition<P> addSingleCondition(PrototypeExpression left, Index.Con
public ParsedCondition getParsedCondition() {
return parsedCondition;
}

@Override
public boolean isSingleCondition() {
return true;
}
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@

public interface Condition {
ViewItem toPattern(RuleGenerationContext ruleContext);
boolean isSingleCondition();
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import org.drools.ansible.rulebook.integration.api.domain.constraints.ItemNotInListConstraint;
import org.drools.ansible.rulebook.integration.api.domain.constraints.ListContainsConstraint;
import org.drools.ansible.rulebook.integration.api.domain.constraints.ListNotContainsConstraint;
import org.drools.ansible.rulebook.integration.api.domain.constraints.NegatedExistsField;
import org.drools.ansible.rulebook.integration.api.domain.constraints.RulebookOperator;
import org.drools.ansible.rulebook.integration.api.domain.constraints.SearchMatchesConstraint;
import org.drools.ansible.rulebook.integration.api.domain.constraints.SelectAttrConstraint;
Expand Down Expand Up @@ -48,6 +49,39 @@ public void setMap(Map<?,?> map) {
this.map = map;
}

@Override
public boolean isSingleCondition() {
return isSingleCondition(map);
}

private boolean isSingleCondition(Map map) {
if (map.size() != 1) {
return false;
}
Object value = map.values().iterator().next();
if (value instanceof Map) {
return isSingleCondition((Map) value);
}
if (value instanceof List) {
return isSingleCondition((List) value);
}
return true;
}

private boolean isSingleCondition(List list) {
if (list.size() != 1) {
return false;
}
Object value = list.get(0);
if (value instanceof Map) {
return isSingleCondition((Map) value);
}
if (value instanceof List) {
return isSingleCondition((List) value);
}
return true;
}

private String getPatternBinding(RuleGenerationContext ruleContext) {
if (patternBinding == null) {
patternBinding = ruleContext.generateBinding();
Expand Down Expand Up @@ -183,7 +217,7 @@ private ParsedCondition parseSingle(RuleGenerationContext ruleContext, Map.Entry
}

private ParsedCondition parseExpression(RuleGenerationContext ruleContext, String expressionName, Map<?, ?> expression) {
RulebookOperator operator = decodeOperation(expressionName);
RulebookOperator operator = decodeOperation(ruleContext, expressionName);

if (operator instanceof ConditionFactory) {
if (expression.get("lhs") != null) {
Expand Down Expand Up @@ -224,7 +258,7 @@ private static void throwExceptionIfCannotInverse(RulebookOperator operator, Rul
}
}

private static RulebookOperator decodeOperation(String expressionName) {
private static RulebookOperator decodeOperation(RuleGenerationContext ruleContext, String expressionName) {
switch (expressionName) {
case "EqualsExpression":
return RulebookOperator.EQUAL;
Expand All @@ -239,8 +273,10 @@ private static RulebookOperator decodeOperation(String expressionName) {
case "LessThanOrEqualToExpression":
return RulebookOperator.LESS_OR_EQUAL;
case ExistsField.EXPRESSION_NAME:
case ExistsField.NEGATED_EXPRESSION_NAME:
return ExistsField.INSTANCE;
case ExistsField.NEGATED_EXPRESSION_NAME:
// IsNotDefinedExpression behaves as an existential not only when used alone
return ruleContext.getCondition().isSingleCondition() ? ExistsField.INSTANCE : NegatedExistsField.INSTANCE;
Copy link
Member Author

Choose a reason for hiding this comment

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

Here I'm clearly making a distinction between the case when the IsNotDefinedExpression is used alone, when it is evaluated as our not existential operator, or in conjuction with another constraint, when it is simply the negation of IsDefinedExpression.

case ListContainsConstraint.EXPRESSION_NAME:
return ListContainsConstraint.INSTANCE;
case ListNotContainsConstraint.EXPRESSION_NAME:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,11 @@ public Type getType() {
return Type.SINGLE;
}

@Override
public boolean isSingleCondition() {
return getType() == Type.SINGLE;
}

public String otherBinding() {
throw new UnsupportedOperationException();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
package org.drools.ansible.rulebook.integration.api.domain.constraints;

import org.drools.ansible.rulebook.integration.api.domain.RuleGenerationContext;
import org.drools.ansible.rulebook.integration.api.domain.conditions.ConditionExpression;
import org.drools.ansible.rulebook.integration.api.rulesmodel.ParsedCondition;
import org.drools.model.functions.Function1;
import org.drools.model.prototype.PrototypeExpression;
import org.drools.model.prototype.PrototypeVariable;
import org.kie.api.prototype.Prototype;
import org.kie.api.prototype.PrototypeFactInstance;

import java.util.Collection;
import java.util.Map;
import java.util.Optional;
import java.util.function.BiPredicate;

import static org.drools.ansible.rulebook.integration.api.domain.conditions.ConditionExpression.map2Expr;
import static org.drools.model.prototype.PrototypeExpression.thisPrototype;

public enum NegatedExistsField implements RulebookOperator, ConditionFactory {

INSTANCE;

public static final String EXPRESSION_NAME = "IsNotDefinedExpression";

private static final Object ADMITTED_UNDEFINED_VALUE = AdmittedUndefinedValue.INSTANCE;

@Override
public <T, V> BiPredicate<T, V> asPredicate() {
return (t, v) -> v == ADMITTED_UNDEFINED_VALUE;
}

@Override
public String toString() {
return "NEGATED_EXISTS_FIELD";
}

@Override
public ParsedCondition createParsedCondition(RuleGenerationContext ruleContext, String expressionName, Map<?, ?> expression) {
PrototypeExpression rightExpression = map2Expr(ruleContext, expression).getPrototypeExpression();
return new ParsedCondition(thisPrototype(), this, new PrototypeExpressionWithAdmittedUndefined(rightExpression));
}

private static class PrototypeExpressionWithAdmittedUndefined implements PrototypeExpression {
private final PrototypeExpression delegate;

private PrototypeExpressionWithAdmittedUndefined(PrototypeExpression delegate) {
this.delegate = delegate;
}

@Override
public Function1<PrototypeFactInstance, Object> asFunction(Prototype prototype) {
return delegate.asFunction(prototype).andThen( result -> result == Prototype.UNDEFINED_VALUE ? ADMITTED_UNDEFINED_VALUE : result );
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the main fix and it is necessary to workaround the fact that on the Drools side it prevents any constraint evaluation, returning false, when it meets an UNDEFINED_VALUE. I preferred to avoid modifying Drools and fix the problem only here. However I believe that, with this trick, it is not bad that an undefined value is actually evaluated only for this constraint (which is the only one that we know at the moment where this is necessary) and not for each and every constraint that at that point will have to deal with it for no reason.

}

@Override
public Collection<String> getImpactedFields() {
return delegate.getImpactedFields();
}

@Override
public Optional<String> getIndexingKey() {
return delegate.getIndexingKey();
}

@Override
public PrototypeExpression andThen(PrototypeExpression other) {
return delegate.andThen(other);
}

@Override
public boolean hasPrototypeVariable() {
return delegate.hasPrototypeVariable();
}

@Override
public Collection<PrototypeVariable> getPrototypeVariables() {
return delegate.getPrototypeVariables();
}

@Override
public PrototypeExpression composeWith(BinaryOperation.Operator op, PrototypeExpression right) {
return delegate.composeWith(op, right);
}

@Override
public PrototypeExpression add(PrototypeExpression right) {
return delegate.add(right);
}

@Override
public PrototypeExpression sub(PrototypeExpression right) {
return delegate.sub(right);
}

@Override
public PrototypeExpression mul(PrototypeExpression right) {
return delegate.mul(right);
}

@Override
public PrototypeExpression div(PrototypeExpression right) {
return delegate.div(right);
}
}

public static class AdmittedUndefinedValue {
static final AdmittedUndefinedValue INSTANCE = new AdmittedUndefinedValue();
static final UnsupportedOperationException HASHCODE_EXCEPTION = new UnsupportedOperationException();

public AdmittedUndefinedValue() {
}

public int hashCode() {
throw HASHCODE_EXCEPTION;
}

public boolean equals(Object obj) {
return false;
}

public String toString() {
return "$AdmittedUndefinedValue$";
}
}
}

This file was deleted.

Loading
Loading