Unhandled exception when dash in event name

myllynen commented 1 year ago

Please confirm the following

[X] I agree to follow this project's code of conduct.
[X] I have checked the current issues for duplicates.
[X] I understand that ansible-rulebook is open source software provided for free and that I might not receive a timely response.

Bug Summary

An unexpected exception is thrown when using a dash (minus) as part of the condition string:

condition: event.payload.vendor-product.rule == "Test"

Environment

__version__ = '0.12.0'
  Executable location = /root/eda-venv/bin/ansible-rulebook
  Drools_jpy version = 0.3.0
  Java home = /etc/alternatives/jre
  Java version = 17.0.7
  Python version = 3.9.14 (main, Jan  9 2023, 00:00:00) [GCC 11.3.1 20220421 (Red Hat 11.3.1-2)]

Steps to reproduce

Trivial to reproduce:

# cat rulebook.yml 
---
- name: Listen for events on a webhook
  hosts: all
  sources:
    - ansible.eda.webhook:
        host: 0.0.0.0
        port: 5000
  rules:
    - name: Check performance events
      condition: event.payload.vendor-product.rule == "Test"
      action:
        run_playbook:
          name: action.yml
# ansible-rulebook --inventory inventory.yml --rulebook rulebook.yml
2023-04-24 09:27:28,539 - ansible_rulebook.cli - ERROR - Unexpected exception
Traceback (most recent call last):
  File "/root/eda-venv/lib64/python3.9/site-packages/ansible_rulebook/condition_parser.py", line 320, in parse_condition
    return condition.parseString(condition_string, parse_all=True)[0]
  File "/root/eda-venv/lib64/python3.9/site-packages/pyparsing/core.py", line 1141, in parse_string
    raise exc.with_traceback(None)
pyparsing.exceptions.ParseException: Expected end of text, found '.'  (at char 28), (line:1, col:29)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/root/eda-venv/lib64/python3.9/site-packages/ansible_rulebook/cli.py", line 223, in main
    asyncio.run(app.run(args))
  File "/usr/lib64/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/usr/lib64/python3.9/asyncio/base_events.py", line 647, in run_until_complete
    return future.result()
  File "/root/eda-venv/lib64/python3.9/site-packages/ansible_rulebook/app.py", line 71, in run
    startup_args.rulesets = load_rulebook(
  File "/root/eda-venv/lib64/python3.9/site-packages/ansible_rulebook/app.py", line 171, in load_rulebook
    rulesets = rules_parser.parse_rule_sets(data, variables)
  File "/root/eda-venv/lib64/python3.9/site-packages/ansible_rulebook/rules_parser.py", line 72, in parse_rule_sets
    rules=parse_rules(rule_set.get("rules", {}), variables),
  File "/root/eda-venv/lib64/python3.9/site-packages/ansible_rulebook/rules_parser.py", line 143, in parse_rules
    condition=parse_condition(rule["condition"]),
  File "/root/eda-venv/lib64/python3.9/site-packages/ansible_rulebook/rules_parser.py", line 177, in parse_condition
    return rt.Condition("all", [parse_condition_value(condition)])
  File "/root/eda-venv/lib64/python3.9/site-packages/ansible_rulebook/condition_parser.py", line 324, in parse_condition
    raise ConditionParsingException(msg)
ansible_rulebook.exception.ConditionParsingException: Error parsing: event.payload.vendor-product.rule == "Test". Expected end of text, found '.'  (at char 28), (line:1, col:29)

Actual results

Unexcepted exception.

Expected results

No exception.

Additional information

While the code will likely be easy to fix I think the documentation should already at this point provide instruction how vendors should setup "namespace" for their products. I've now seen one project to use a generic condition name (event.payload.<rule-name> e.g. event.payload.lifecycle) while another tried to create a namespace with event.payload.project-component.rule type approach.

Since in most enterprises where only one port is expected for each service (such as EDA) instead of a separate port for each event source there needs to a way how to avoid overalapping names with different vendors and event sources. Perhaps we should a have separate doc issue for this.

Thanks.

evgeni commented 1 year ago

would event.payload['vendor-product'].rule == "Test" or event.payload['vendor-product']['rule'] == "Test" work?

myllynen commented 1 year ago

I tested that and it seems that using [ leads to ConditionParsingException.

Perhaps the docs should be clarified first and then we could tweak our rulebooks accordingly. But in any case a quality implementation should not throw unexpected expectations for cases which are not completely unreasonable.

Alex-Izquierdo commented 1 year ago

Hello. There are some constraints for event keys that are not properly documented yet. The way to deal with this kind of situation would be to use a source filter to transform the incoming events. For this specific case, we already provide a source filter that replaces dashes to underscores: https://github.com/ansible/event-driven-ansible/blob/main/extensions/eda/plugins/event_filters/dashes_to_underscores.py

natoscott commented 1 year ago

FWIW, we'll tweak the "pcp-pmie" JSON field name that triggered this issue to split it up into {"pcp":{"pmie":{ ... }}} - on discussing with @myllynen we think this will be a cleaner approach in general anyway.

ansible / ansible-rulebook