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

RuleBook first run unexpected result #55

Closed
typhoon1978 opened this issue Apr 27, 2017 · 4 comments
Closed

RuleBook first run unexpected result #55

typhoon1978 opened this issue Apr 27, 2017 · 4 comments
Assignees
Labels
Milestone

Comments

@typhoon1978
Copy link

Hi,

I'm trying to integrate RuleBook in an enterprise project, because it seems to be really interesting.

I'm using version 0.5 and found that first run returns a default value because headRule is not correctly valued in CoRRuleBook.run method. These are the steps I did:

  1. First I created a new RuleBook extending CoRRuleBook:
public class AutoEvaluationRuleBook extends CoRRuleBook<MyBean> {

	@Override
	public void defineRules() {
           // here I define all my business rules
	}

}
  1. Then I instantiate a new RuleBook:
RuleBook<AutoEvaluationResult> autoEvaluationRuleBook = RuleBookBuilder.create(AutoEvaluationRuleBook.class)
				.withResultType(AutoEvaluationResult.class)
				.withDefaultResult(new AutoEvaluationResult(null, false, false, null))
				.build();
  1. Then I create some test cases:
List<NameValueReferableMap<MyBean>> factsList = testCases();

factsList.forEach(x -> {
	count++;
	autoEvaluationRuleBook.run(x);
	autoEvaluationRuleBook.getResult().ifPresent(result -> {
	MyBean ref = result.getValue().getRef();
	System.out.println(String.format("%s. Item: %s, Autoevaluated: %s, Suspect: %s, Reason: %s", count,
		null != ref ? "item" : "NULL", result.getValue().isAutoEvaluated(),
		result.getValue().isSuspect(), null != result.getValue().getReason() ? result.getValue().getReason().name() : "NULL"));
	});
});

In this way, the first run returns an unexpected result because I noticed in the run method inside the CoRRuleBook class the object headRule is not valued.

The original code:

  @Override
  @SuppressWarnings("unchecked")
  public void run(NameValueReferableMap facts) {
   // here an Optional obj wraps the class property _headRule
    Optional<Handler<Rule>> headRule = Optional.ofNullable(_headRule);
    if (!headRule.isPresent()) {
      // if not present (first run), you call defineRules()
      defineRules();
    }
   // here headRule Optional value is still null, is it wanted ?
    headRule.ifPresent(ruleHandler -> {
        ruleHandler.getDelegate().setFacts(facts);
        getResult().ifPresent(result -> ruleHandler.getDelegate().setResult(result));
      });
    headRule.ifPresent(Handler::handleRequest);
  }

In the original code if the headRule is not present, it calls the defineRules, defining the book rules, but after that the method isPresent is called on the Optional<Handler> headRule object which will return false. Is this wanted ?

I modified this code in this way:

@Override
@SuppressWarnings("unchecked")
public void run(NameValueReferableMap facts) {
	Optional<Handler<Rule>> headRule = Optional.ofNullable(_headRule);
	if (!headRule.isPresent()) {
		defineRules();
                // new added line below, rules are defined but object is still not present
		headRule = Optional.ofNullable(_headRule);
	}		
	headRule.ifPresent(ruleHandler -> {
		ruleHandler.getDelegate().setFacts(facts);
		getResult().ifPresent(result -> ruleHandler.getDelegate().setResult(result));
	});
	headRule.ifPresent(Handler::handleRequest);
}

In this way I have all the objects valued correctly (also the first one).

Let me know.

Thanks and best regards,
Alessandro Torrisi.

@Clayton7510
Copy link
Collaborator

You are correct. It looks like that is a bug that was probably introduced in v0.5. I just put the fix in 0.5.1-SNAPSHOT. I used the same basic approach as you. The only difference was I checked for null in the first statement.Why? Because another contributor pointed out that it's not considered a good use of Optional just to check isPresent() on an existing variable - not really a big difference tho, IMO.

Thanks for that! I'll update the tests to catch that condition and push it out as a release in the next couple of days. But for now, it's in the current SNAPSHOT release and in the develop branch.

@Clayton7510
Copy link
Collaborator

BTW, I can see that you create a RuleBook with the a result type of MyBean here.

public class AutoEvaluationRuleBook extends CoRRuleBook<MyBean> {

	@Override
	public void defineRules() {
           // here I define all my business rules
	}
}

But then you try to instantiate that RuleBook using a different result type of AutoEvaluationResult here.

RuleBook<AutoEvaluationResult> autoEvaluationRuleBook = RuleBookBuilder.create(AutoEvaluationRuleBook.class)
				.withResultType(AutoEvaluationResult.class)
				.withDefaultResult(new AutoEvaluationResult(null, false, false, null))
				.build();

I'm not 100% sure why. But here's an example of what I think you are trying to do. And once again, thanks for finding that bug!

/**
 * Custom RuleBook with a MyBean result type.
 */
public class AutoEvaluationRuleBook extends CoRRuleBook<MyBean> {
  @Override
  public void defineRules() {
    addRule(RuleBuilder.create().withFactType(String.class)
        .withResultType(MyBean.class) //should match the result type of the RuleBook, which is MyBean
        .when(facts -> facts.containsKey("value1"))
        .using("value1")
        .then((facts, result) -> result.getValue().setStrValue1(facts.getOne()))
        .build()
    );
    addRule(RuleBuilder.create().withFactType(String.class)
        .withResultType(MyBean.class) //should match the result type of the RuleBook, which is MyBean
        .when(facts -> facts.containsKey("value2"))
        .using("value2")
        .then((facts, result) -> result.getValue().setStrValue2(facts.getOne()))
        .build()
    );
  }
}
package com.deliveredtechnologies.rulebook.example;

public class MyBean {
  private String strValue1;
  private String strValue2;

  public String getStrValue1() {
    return strValue1;
  }

  public void setStrValue1(String strValue1) {
    this.strValue1 = strValue1;
  }

  public String getStrValue2() {
    return strValue2;
  }

  public void setStrValue2(String strValue2) {
    this.strValue2 = strValue2;
  }
}
public class Application {
  public static void main(String args[]) {
    AutoEvaluationRuleBook ruleBook = new AutoEvaluationRuleBook();
    ruleBook.setDefaultResult(new MyBean());

    NameValueReferableMap factMap = new FactMap();
    factMap.setValue("value1", "First Value");
    factMap.setValue("value2", "Second Value");

    ruleBook.run(factMap);

    ruleBook.getResult().ifPresent(result ->
    {
      MyBean bean = result.getValue();
      System.out.println(bean.getStrValue1());
      System.out.println(bean.getStrValue2());
    });
  }
}

@Clayton7510 Clayton7510 added this to the Release 0.5.1 milestone Apr 28, 2017
@typhoon1978
Copy link
Author

Hi Clayton,

sorry my sample is wrong, because I deleted the real bean name and put at last a generic MyBean.

My real class is declared as follows:

public class AutoEvaluationRuleBook extends CoRRuleBook<AutoEvaluationResult> {
	
	private static final String DATE_PATTERN = "(\\d{4})-(\\d{2})-(\\d{2})";

	private static final int YEAR_GROUP = 1;

	@Override
	public void defineRules() {

		addRule(RuleBuilder.create().withFactType(CalculatedMatching.class).withResultType(AutoEvaluationResult.class)
				.when(personTypeMismatch()).then(unevaluated()).stop().build());
  // and so on...

          }
}

I preferred using the bean approach rather than the map approach.

Thanks and best regards,
Alessandro.

@Clayton7510 Clayton7510 self-assigned this Apr 28, 2017
@Clayton7510
Copy link
Collaborator

I updated the tests and just pushed 0.5.1 to Maven with this bug fix.

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

No branches or pull requests

2 participants