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: improve typing of RuleResult #390

Merged

Conversation

emilefokkemanavara
Copy link
Contributor

This PR closes #352 by improving the typing of RuleResult.

@chris-pardy
Copy link
Collaborator

@emilefokkemanavara thanks for putting this up. I'll take a look later today or over the weekend.
One thing to note right away is that the condition reference can end up in the result if the conditions it's referencing isn't defined and if the "allowUndefinedConditions" option is specified.

@emilefokkemanavara
Copy link
Contributor Author

@chris-pardy Thanks for the heads up!

types/index.d.ts Outdated Show resolved Hide resolved
@chris-pardy
Copy link
Collaborator

@emilefokkemanavara This looks great, expect for one issue.

The a evaluation of JSON Rules Engine follows these steps:

  1. Clone all the conditions
  2. Attach the cloned conditions to the rule result
  3. Evaluate each cloned condition
  4. Modify the cloned condition to have new properties such as result

This means that if a condition doesn't get evaluated it will still be in the rule result but it won't have properties like result or factResult. The main reason conditions can be skipped is in Any/All conditions. These group by priority and short-circuit between priorities so for instance:

This set of conditions

{
  "all": [
     { "fact": "age", "operator": "greaterThanInclusive", "value": 21, "priority": 1 },
     { "fact": "likesOranges", "operator": "equal", "value": true, "priority": 2 }
  ]
}

The result can look like this.

{
  "all": [
     {"fact": "age", "operator": "greaterThanInclusive", "value": 21, "priority": 1, "factResult": 20 },
     { "fact": "likesOranges", "operator": "equal", "value": true, "priority": 2 }
  ]
}

@emilefokkemanavara
Copy link
Contributor Author

@emilefokkemanavara This looks great, expect for one issue.

The a evaluation of JSON Rules Engine follows these steps:

  1. Clone all the conditions
  2. Attach the cloned conditions to the rule result
  3. Evaluate each cloned condition
  4. Modify the cloned condition to have new properties such as result

This means that if a condition doesn't get evaluated it will still be in the rule result but it won't have properties like result or factResult. The main reason conditions can be skipped is in Any/All conditions. These group by priority and short-circuit between priorities so for instance:

This set of conditions

{
  "all": [
     { "fact": "age", "operator": "greaterThanInclusive", "value": 21, "priority": 1 },
     { "fact": "likesOranges", "operator": "equal", "value": true, "priority": 2 }
  ]
}

The result can look like this.

{
  "all": [
     {"fact": "age", "operator": "greaterThanInclusive", "value": 21, "priority": 1, "factResult": 20 },
     { "fact": "likesOranges", "operator": "equal", "value": true, "priority": 2 }
  ]
}

@chris-pardy So this means result and factResult are possibly undefined? (see latest commit)

Copy link
Collaborator

@chris-pardy chris-pardy left a comment

Choose a reason for hiding this comment

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

Looks good.
Before running the tests could you rebase this and squash the commits to make it a bit easier to read down the line.

types/index.d.ts Show resolved Hide resolved
@chris-pardy
Copy link
Collaborator

@emilefokkemanavara This looks great, expect for one issue.

The a evaluation of JSON Rules Engine follows these steps:

  1. Clone all the conditions
  2. Attach the cloned conditions to the rule result
  3. Evaluate each cloned condition
  4. Modify the cloned condition to have new properties such as result

This means that if a condition doesn't get evaluated it will still be in the rule result but it won't have properties like result or factResult. The main reason conditions can be skipped is in Any/All conditions. These group by priority and short-circuit between priorities so for instance:

This set of conditions

{
  "all": [
     { "fact": "age", "operator": "greaterThanInclusive", "value": 21, "priority": 1 },
     { "fact": "likesOranges", "operator": "equal", "value": true, "priority": 2 }
  ]
}

The result can look like this.

{
  "all": [
     {"fact": "age", "operator": "greaterThanInclusive", "value": 21, "priority": 1, "factResult": 20 },
     { "fact": "likesOranges", "operator": "equal", "value": true, "priority": 2 }
  ]
}

@chris-pardy So this means result and factResult are possibly undefined? (see latest commit)

Yes, that's correct.

commit 1356b48
Author: Emile Fokkema <[email protected]>
Date:   Sun Nov 10 20:08:27 2024 +0100

    result and factResult are not always there

commit 725fb66
Author: Emile Fokkema <[email protected]>
Date:   Sat Nov 9 20:07:13 2024 +0100

    condition reference can be part of the result

commit cb5f3c7
Author: Emile Fokkema <[email protected]>
Date:   Fri Nov 8 16:27:34 2024 +0000

    there are no condition references in the result

commit c688692
Author: Emile Fokkema <[email protected]>
Date:   Fri Nov 8 16:27:09 2024 +0000

    begin to add condition result types

commit 7a90a81
Author: Emile Fokkema <[email protected]>
Date:   Fri Nov 8 16:26:42 2024 +0000

    add toJSON method to RuleResult
@chris-pardy chris-pardy self-requested a review November 12, 2024 15:26
@chris-pardy chris-pardy merged commit 9d51178 into CacheControl:master Nov 12, 2024
3 checks passed
@emilefokkemanavara emilefokkemanavara deleted the fix/352/rule-result-json branch November 13, 2024 18:46
chris-pardy added a commit that referenced this pull request Nov 15, 2024
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

Successfully merging this pull request may close these issues.

RuleResult class missing from type declarations
2 participants