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

Name of RuleParameter is ignored if the type is recognized #384

Closed
peeveen opened this issue Sep 1, 2022 · 2 comments
Closed

Name of RuleParameter is ignored if the type is recognized #384

peeveen opened this issue Sep 1, 2022 · 2 comments

Comments

@peeveen
Copy link
Contributor

peeveen commented Sep 1, 2022

Evaluating a rule twice with differently-named objects of the same type gives unexpected results.

Here's my test code:

using Newtonsoft.Json;
using RulesEngine.Models;
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Dynamic;
using System.Linq;
using System.Threading.Tasks;
using Xunit;

namespace RulesEngine.UnitTest
{
  [ExcludeFromCodeCoverage]
  public class RunTwiceTest
  {
    [Fact]
    public async Task RunTwiceTest_ReturnsExpectedResults()
    {
      var workflow = new Workflow {
        WorkflowName = "RunTwiceWorkflow",
        Rules = new Rule[] {
            new Rule {
                RuleName = "RunTwiceRule",
                RuleExpressionType = RuleExpressionType.LambdaExpression,
                Expression = "test.blah == 1"
            }
        }
      };
      var reSettings = new ReSettings { };
      var engine = new RulesEngine(reSettings: reSettings);
      engine.AddOrUpdateWorkflow(workflow);

      const string json_pass = @"
      {
        ""test"": {
          ""blah"": 1
        },
      }
      ";
      const string json_fail = @"
      {
        ""SOME_OTHER_OBJECT_NAME"": {
          ""blah"": 1
        },
      }
      ";

      JsonSerializerSettings SerializerSettings = new JsonSerializerSettings {
        DateParseHandling = DateParseHandling.None
      };
      RuleParameter[] FromJson(string json)
      {
        var input = JsonConvert.DeserializeObject<ExpandoObject>(json, SerializerSettings) ?? throw new Exception("Failed to deserialize inputs.");
        var inputDictionary = input.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);
        return inputDictionary.Where(kvp => kvp.Value != null).Cast<KeyValuePair<string, object>>().Select(kvp => new RuleParameter(kvp.Key, kvp.Value)).ToArray();
      }

      // Create two RuleParameter[]s
      // They both only contain one RuleParameter
      // inputs_pass contains a RuleParameter called "test"
      // inputs_fail contains a RuleParameter called "SOME_OTHER_OBJECT_NAME"
      var inputs_pass = FromJson(json_pass);
      var inputs_fail = FromJson(json_fail);
      Assert.Equal("test", inputs_pass.First().Name);
      Assert.Equal("SOME_OTHER_OBJECT_NAME", inputs_fail.First().Name);
      // So I would expect my rule (test.blah==1) to pass for input_pass
      // And to fail for inputs_fail.
      // It passes for both.
      var pass_results = await engine.ExecuteAllRulesAsync("RunTwiceWorkflow", inputs_pass);
      var fail_results = await engine.ExecuteAllRulesAsync("RunTwiceWorkflow", inputs_fail);
      Assert.True(pass_results.First().IsSuccess);
      Assert.False(fail_results.First().IsSuccess);
    }
  }
}
@peeveen
Copy link
Contributor Author

peeveen commented Sep 1, 2022

I've had a dig around, and it seems like the main problem is GetCompiledRulesKey in RulesEngine.cs.

private string GetCompiledRulesKey(string workflowName, RuleParameter[] ruleParams)
{
    var key = $"{workflowName}-" + string.Join("-", ruleParams.Select(c => c.Type.Name));
    return key;
}

At first glance, it looks like the engine plucks a rule out of the cache if the inputs are of the same type ... doesn't matter if they are the same name. Also, I'm guessing that if a rule takes multiple inputs of the same type, the order that they are supplied to the cached rule will be anyone's guess.

A quick fix might be to include the param name in that key calculation (perhaps sort the ruleParams by name beforehand?). At the moment I'm not sure what the side-effects would be. I do know that it fixes that test, although I would expect fail_results to then contain some kind of "Unknown identifier" message, but it instead contains this:

Exception while parsing expression `test.blah == 1` - The binary operator Equal is not defined for the types 'System.Object' and 'System.Int32'

EDIT: from some quick tests, it looks like if the rule expression references a non-existent object/field, you get that error message if your inputs contain any dynamic objects ... but that's for another day.

abbasc52 pushed a commit that referenced this issue Sep 5, 2022
* fix for ruleparameter name changes against cached rules

* fixed indentation to match rest of code

* suggested change implemented
@peeveen
Copy link
Contributor Author

peeveen commented Sep 5, 2022

Closing, fixed by this PR: #386

@peeveen peeveen closed this as completed Sep 5, 2022
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

No branches or pull requests

1 participant