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

feat(dpf-18714): inNextMinutes operator #835

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

vscaiceanu-1a
Copy link
Member

@vscaiceanu-1a vscaiceanu-1a commented Sep 28, 2023

Proposed change

Computing the input facts takes:

  • 3.7748750001192093 ms for 1k rules
  • 38.763249998912215 ms for 10k rules

@vscaiceanu-1a vscaiceanu-1a requested a review from a team as a code owner September 28, 2023 14:57
@vscaiceanu-1a vscaiceanu-1a marked this pull request as draft October 3, 2023 16:52
@vscaiceanu-1a vscaiceanu-1a force-pushed the feature/in-next-minutes-operator branch 3 times, most recently from 9b5ff69 to f37ddb4 Compare October 16, 2023 16:20
@vscaiceanu-1a vscaiceanu-1a force-pushed the feature/in-next-minutes-operator branch from f37ddb4 to ccdcda0 Compare October 19, 2023 14:55
@vscaiceanu-1a vscaiceanu-1a force-pushed the feature/in-next-minutes-operator branch from ccdcda0 to 29b6a71 Compare October 23, 2023 14:49
@vscaiceanu-1a vscaiceanu-1a force-pushed the feature/in-next-minutes-operator branch 3 times, most recently from 316b732 to 4ed8707 Compare October 25, 2023 09:26
@vscaiceanu-1a vscaiceanu-1a marked this pull request as ready for review October 25, 2023 09:33
@vscaiceanu-1a vscaiceanu-1a force-pushed the feature/in-next-minutes-operator branch from 4ed8707 to ccf2ae4 Compare October 25, 2023 12:56
@vscaiceanu-1a vscaiceanu-1a force-pushed the feature/in-next-minutes-operator branch 2 times, most recently from 3f33473 to 011dc8b Compare October 25, 2023 14:23
@vscaiceanu-1a
Copy link
Member Author

Missing:

  • JSON Schema update
  • Documentation
  • (metadata extraction)

@kpanot which JSON Schema are you referring to? The ruleset one hasn't been modified in this PR.

@vscaiceanu-1a vscaiceanu-1a force-pushed the feature/in-next-minutes-operator branch from c8f106c to 7642364 Compare October 31, 2023 15:22
@vscaiceanu-1a vscaiceanu-1a force-pushed the feature/in-next-minutes-operator branch 3 times, most recently from 3f2b2d8 to 47784fb Compare November 2, 2023 13:39
docs/rules-engine/operators.md Outdated Show resolved Hide resolved
docs/rules-engine/operators.md Show resolved Hide resolved
packages/@o3r/rules-engine/src/engine/ruleset-executor.ts Outdated Show resolved Hide resolved
const rulesWithContext: Rule[] = [];
const rulesWithoutContext: Rule[] = [];
const inputFactsForRule: Record<string, string[]> = {};
const exploreObject = (currentObj: any, ruleInputFacts: Set<string>) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure to understand the purpose of this part :S.
Why do you need it as you already have the list of facts available for the ruleset?

Copy link
Member Author

Choose a reason for hiding this comment

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

The goal is to compute the list of input facts and no longer take into account the value of the inputFacts coming from the CMS.
We have even removed them from the showcase app ruleset https://github.com/AmadeusITGroup/otter/pull/835/files#diff-1c83c4284e0d72e53c44790c5bdd50daedb84c30b8e703c4e0552ecf2ab870ae

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, then the field should be flagged as deprecated in the JSON schema.
The function may needs also a better name, comments ans proper input typings

Copy link
Member Author

Choose a reason for hiding this comment

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

Concerning the types for the input, we support nested rules and the method will iterate on many types of Objects or Arrays.

Here's an example of a nested rule:

        {
          'id': '6e8t54h6s4e-6erth46sre8th4-d46t8s13t5j2',
          'name': 'the second rule',
          'inputRuntimeFacts': ['UI_FACT_2'],
          'inputFacts': ['cart'],
          'outputRuntimeFacts': [],
          'rootElement': {
            'elementType': 'RULE_BLOCK',
            'blockType': 'IF_ELSE',
            'condition': {
              'any': [
                {
                  'lhs': {
                    'type': 'RUNTIME_FACT',
                    'value': 'UI_FACT_2'
                  },
                  'operator': 'equals',
                  'rhs': {
                    'type': 'LITERAL',
                    'value': false
                  }
                }
              ]
            },
            'successElements': [
              {
                'elementType': 'ACTION',
                'actionType': 'UPDATE_LOCALISATION',
                'key': 'my.loc.key2.success',
                'value': 'my.loc.value2.success'
              },
              {
                'elementType': 'ACTION',
                'actionType': 'UPDATE_LOCALISATION',
                'key': 'my.loc.key3.success',
                'value': 'my.loc.value3.success'
              },
              {
                'elementType': 'RULE_BLOCK',
                'blockType': 'IF_ELSE',
                'condition': {
                  'any': [
                    {
                      'lhs': {
                        'type': 'FACT',
                        'value': 'cart',
                        'path': '$.xmasHampers[0].hamperItems[1].id'
                      },
                      'operator': 'equals',
                      'rhs': {
                        'type': 'LITERAL',
                        'value': 'foieGras'
                      }
                    }
                  ]
                },
                'successElements': [
                  {
                    'elementType': 'ACTION',
                    'actionType': 'UPDATE_LOCALISATION',
                    'key': 'my.loc.key4.success',
                    'value': 'my.loc.value4.success'
                  }
                ],
                'failureElements': [
                  {
                    'elementType': 'ACTION',
                    'actionType': 'UPDATE_LOCALISATION',
                    'key': 'my.loc.key5.failure',
                    'value': 'my.loc.value5.failure'
                  }
                ]
              },
              {
                'elementType': 'ACTION',
                'actionType': 'UPDATE_LOCALISATION',
                'key': 'my.loc.key6.success',
                'value': 'my.loc.value6.success'
              }
            ],
            'failureElements': [
              {
                'elementType': 'ACTION',
                'actionType': 'SET_FACT',
                'fact': 'UI_FACT_2',
                'value': false
              }
            ]
          }
        }

The method will find the cart fact in a successElements array.

@vscaiceanu-1a vscaiceanu-1a force-pushed the feature/in-next-minutes-operator branch 2 times, most recently from f130333 to 6b143ea Compare November 10, 2023 13:58
@vscaiceanu-1a vscaiceanu-1a force-pushed the feature/in-next-minutes-operator branch from 6b143ea to 90006c8 Compare November 13, 2023 15:35
packages/@o3r/rules-engine/src/engine/ruleset-executor.ts Outdated Show resolved Hide resolved
* @param currentObject The current object being explored.
* @param ruleInputFacts A set to store the identified input facts for the rule.
*/
const collectRuleInputFacts = (currentObject: any, ruleInputFacts: Set<string>) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

currentObject should be properly typed.
(this function can also be a private field to not be redefined at each execution)

@vscaiceanu-1a vscaiceanu-1a force-pushed the feature/in-next-minutes-operator branch 2 times, most recently from 19060e4 to 93f68ac Compare November 13, 2023 18:03
@@ -277,6 +277,14 @@ export class RulesEngineExtractor {
nbValues: 1
}
};
if (declaration.initializer && ts.isObjectLiteralExpression(declaration.initializer)) {
declaration.initializer.properties.forEach((property: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

: any should be avoid, how do you guaranty that property.name exists otherwise?

* @param currentObject The current object being explored.
* @param ruleInputFacts A set to store the identified input facts for the rule.
*/
private collectRuleInputFacts(currentObject: any, ruleInputFacts: Set<string>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid : any

@vscaiceanu-1a vscaiceanu-1a force-pushed the feature/in-next-minutes-operator branch 2 times, most recently from 5c11621 to eb4c10c Compare November 15, 2023 11:57
kpanot
kpanot previously approved these changes Nov 16, 2023
Comment on lines +54 to +64
if (!operatorFactValues) {
throw new Error('No operatorFactValues. Unable to retrieve the current time.');
}
if (typeof operatorFactValues.o3rCurrentTime !== 'number') {
throw new Error('o3rCurrentTime value is not a number');
}
const currentTimeValue = operatorFactValues.o3rCurrentTime;
const now = new Date(currentTimeValue);
const leftDate = new Date(leftDateInput);
const targetDate = new Date(new Date(currentTimeValue).setMinutes(now.getMinutes() + +minutes));
return leftDate >= now && leftDate > targetDate;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!operatorFactValues) {
throw new Error('No operatorFactValues. Unable to retrieve the current time.');
}
if (typeof operatorFactValues.o3rCurrentTime !== 'number') {
throw new Error('o3rCurrentTime value is not a number');
}
const currentTimeValue = operatorFactValues.o3rCurrentTime;
const now = new Date(currentTimeValue);
const leftDate = new Date(leftDateInput);
const targetDate = new Date(new Date(currentTimeValue).setMinutes(now.getMinutes() + +minutes));
return leftDate >= now && leftDate > targetDate;
return !dateInNextMinutes.evaluator(leftDateInput, minutes, operatorFactValues);

?

@vscaiceanu-1a vscaiceanu-1a added this pull request to the merge queue Nov 20, 2023
Merged via the queue into main with commit 5a8dc9f Nov 20, 2023
25 checks passed
@vscaiceanu-1a vscaiceanu-1a deleted the feature/in-next-minutes-operator branch November 20, 2023 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants