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

EnableExceptionAsErrorMessage = false does not actually throw exception, but ignores/supresses it #65

Open
rklec opened this issue Aug 14, 2024 · 9 comments

Comments

@rklec
Copy link

rklec commented Aug 14, 2024

STR

Have any invalid rule or one that somehow else throws an exception.

And have the settings configured as such:

      var engineSettings = new ReSettings()
        {
            CustomActions = // ...
            CustomTypes = // ...
            EnableExceptionAsErrorMessage = false
        };
     
        new RulesEngine.RulesEngine(mailWorkflows, engineSettings);     

What should happen

EnableExceptionAsErrorMessage = false is explained on https://microsoft.github.io/RulesEngine/#settings should do the following:

Otherwise [= if false], throws an exception. This setting is only applicable if IgnoreException is set to false.

Notably IgnoreException is not touched, i.e. it is (as per https://microsoft.github.io/RulesEngine/#settings aka the default set to false)

What happens

Errors are silently ignored.

More information

I have also indicated this in microsoft#623, see this for a practical example.

System

    <TargetFramework>net8.0</TargetFramework>
<!-- ... -->
    <PackageReference Include="RulesEngine" Version="5.0.3" /> 

aka .NET 8

Windows 10


copied/transferred from microsoft#624 (more details there)

@RenanCarlosPereira
Copy link
Collaborator

hey @rklec

we have some tests to cover this here:

are they valid?

please take a look and check if that make sense.

@rklec
Copy link
Author

rklec commented Aug 20, 2024

Well as far as I see they use the file rules9.json which only is a "traditional" workflow. In my case it is an action workflow,l maybe that makes a difference (here) (too).

@asulwer
Copy link
Owner

asulwer commented Aug 22, 2024

@rklec Those two properties work together.

var workflows = new Workflow[] {
    new() {
        WorkflowName = "Test Workflow1",
        Rules = new List<Rule> {
            new() {
                RuleName = "Test Rule1",
                SuccessEvent = "Count is less",
                ErrorMessage = "Over Expected",
                Expression = "counts < 3"
            },
            new() {
                RuleName = "Test Rule2",
                SuccessEvent = "Count is more",
                ErrorMessage = "Under Expected",
                Expression = "count > 3"
            }
        }
    }
};

Notice the Expression for each of the two rules. one contains count and the other counts. one should fail if input is count as follows

var settings = new ReSettings {
    //IgnoreException = true,
    EnableExceptionAsErrorMessage = false
};

var bre = new RulesEngine.RulesEngine(workflows, settings);

var inputs = new RuleParameter[] {new("input1", new {count = 1})};
  • EnableExceptionAsErrorMessage
    • true = runtime exception occurs
    • false = RuleResultTree->ExceptionMessage changes to formatted exception and no runtime exception
  • IgnoreException
    • EnableExceptionAsErrorMessage = false AND IgnoreException = true does not alter RuleResultTree with the formatted exception, basically its ignored in favor of the ErrorMessage string in the Rule

@asulwer
Copy link
Owner

asulwer commented Aug 22, 2024

@rklec using actions the results are not the same. regardless of how EnableExceptionAsErrorMessage is set the exception is formatted and stored in action->exception, no runtime exception ever occurs

var workflows = new Workflow[] {
   new() {
        WorkflowName = "Test Workflow4",
        Rules = new List<Rule> {
            new() {
                RuleName = "Test Rule",
                Expression = "1 == 1",
                Actions = new RuleActions {
                    OnSuccess = new ActionInfo {
                        Name = "OutputExpression",
                        Context = new Dictionary<string, object> {{"expression", "counts"}}
                    }
                }
            }
        }
    }
};

var settings = new ReSettings {
    IgnoreException = true,
    EnableExceptionAsErrorMessage = true
};

var bre = new RulesEngine.RulesEngine(workflows, settings);

var inputs = new RuleParameter[] {new("input1", new {count = 1})};

@asulwer
Copy link
Owner

asulwer commented Aug 22, 2024

i believe the original author was removing all usage of Rule->Expression in favor of Rule->Actions

@rklec
Copy link
Author

rklec commented Aug 23, 2024

AFAIK you cannot use them together, can you? In any case, yes it's action rules, only.

using actions the results are not the same. regardless of how EnableExceptionAsErrorMessage is set the exception is formatted and stored in action->exception, no runtime exception ever occurs

In case I understand it correctly, that's exactly the issue I am facing, After all, why is it though? Why can EnableExceptionAsErrorMessage=false not also throw in a action?
In any case, IMHO it's bad that this behavior is not documented. It should not matter what workflow type I am using, all settings should be applied, unless one setting says it does not apply. So at the very least, the doc needs improvement here.

EnableExceptionAsErrorMessage

  • true = runtime exception occurs
  • false = RuleResultTree->ExceptionMessage changes to formatted exception and no runtime exception

You meant to inverse true and false, did not you?
(If so, maybe an inversed ThrowException property would have been the clearer API.

@asulwer
Copy link
Owner

asulwer commented Aug 23, 2024

@rklec those properties affect a normal workflow without actions. it is indeed an issue that actions only format the exception and not have the option to throw it as well. i will look into adjusting that for the next release, next couple of days.

leaving this issue open so it can be referenced with the solution

@rklec
Copy link
Author

rklec commented Aug 29, 2024

Also asked a related question about this, because it totally confuses me how many places there are to store some exceptions (or exception messages): microsoft#628

@asulwer
Copy link
Owner

asulwer commented Aug 30, 2024

the code has loads of areas that need improvements. i am working on major changes. in my latest tests i went from 45 minute runtime to less than a minute, just with a couple of changes, major changes. coming soon

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

3 participants