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

JEXL expressions in custom notifications #4511

Merged
merged 113 commits into from
May 30, 2024
Merged

Conversation

fgalan
Copy link
Member

@fgalan fgalan commented Feb 12, 2024

Issue #4004

When this PR is created (at commit 9dfdccc) this PR has been locally tested with the following test cases (which are the ones that are more focused in macro replacement):

  • 2560_custom_notification_json
  • 2015_notification_templates
  • 4085_custom_notifications_ngsi_payload
  • 4159_custom_notif_extra_macros

What's pending

  • CI Docker
  • Jexl* -> Expr* (to make it more "neutral")
  • Python library version in GET /version ?
  • Subscription field to choose legacy/jexl evaluation (CRUD in csub) + legacy std::map based expressions mode (see here)
  • Add tranformation functions (+ ftest)
  • Test cases for JEXL calculations (based in different types of attributes, i.e. strings, numbers, etc.)
  • TextUnrestricted should not be needed when JEXL expression is used in ngsi element - 2cc2424
  • Test thread safeness model and memory profile (it's ok with cjexl)
  • Sem management (not needed with cjexl)
  • Use ExprResult::release() in the proper places - 49aa74c
  • Add times to time counters - ecf7fbd
    • JEXL context build
    • JEXL evaluation
    • Time waiting at sem (no sems at the end)
  • Check we have this covered in the tests:
    • Unknown attribute in context - jexl_not_defined_attrs.test
    • Unknown tranformation (e.g. c|patata) - jexl_transformation_unknown.test
    • Syntax errors (eg. c[0|fun, c|fun1('foo)|fun2, c|fun1('foo'|fun2) - jexl_syntax_errors.test
    • Use two expressions in the same notification, reusing context (to ensure the modifications to JsonHelper str() to allow it re-calleable are working properly) - jexl_transformation_in_id_and_type.test (id and type) and jexl_several_expressions.test (sum and mul)
  • Deployment Docker (maybe use pipx?)
    • master (although tested using the branch for this PR) - e789afc
    • tags - 7281e1f
  • Deal with cjexl lib absence
  • Analyze ftest differences (see here and here)
    • 2015_notification_templates/notification_templates_many_notifications.test ( + _legacy.test)
    • 3693_covered_notifications_in_subscriptions/covered_custom_notification.test (+ _legacy.test)
  • libcjexl 0.1.0
  • Check FIXME PR marks
  • Documentation
    • orion-api.md (to explain how it works now)
    • Admin (new build from sources, new CLI for the expressions mode, etc.)
    • Devel (as we have created a new library directory)
    • Add a warning about existing attributes in expressions with names using - or : (with basic replacement they work, but with JEXL no, see test/functionalTest/cases/4004_jexl_expressions_in_subs/jexl_expr_attrs_weird_syntax.test) - 9e04d8c
  • Functional pass with basic replacement, in separate PR [Not to merge] FIX test new implementation with basic expressions #4552
  • Valgrind pass PR Valgrind regression for JEXL implementation #4553
  • thMapper transformation (+tests)
  • CNR

@fgalan
Copy link
Member Author

fgalan commented Feb 13, 2024

Trying to install pyjexl in my local (Debian) environment:

fermin@bodoque:~/src/fiware-orion$ sudo pip install pyjexl==0.3.0
error: externally-managed-environment

× This environment is externally managed
╰─> To install Python packages system-wide, try apt install
    python3-xyz, where xyz is the package you are trying to
    install.
    
    If you wish to install a non-Debian-packaged Python package,
    create a virtual environment using python3 -m venv path/to/venv.
    Then use path/to/venv/bin/python and path/to/venv/bin/pip. Make
    sure you have python3-full installed.
    
    If you wish to install a non-Debian packaged Python application,
    it may be easiest to use pipx install xyz, which will manage a
    virtual environment for you. Make sure you have pipx installed.
    
    See /usr/share/doc/python3.11/README.venv for more information.

note: If you believe this is a mistake, please contact your Python installation or OS distribution provider. You can override this, at the risk of breaking your Python installation or OS, by passing --break-system-packages.
hint: See PEP 668 for the detailed specification.

fermin@bodoque:~/src/fiware-orion$ sudo apt-get install python3-pyjexl
Leyendo lista de paquetes... Hecho
Creando árbol de dependencias... Hecho
Leyendo la información de estado... Hecho
E: No se ha podido localizar el paquete python3-pyjexl

fermin@bodoque:~/src/fiware-orion$ sudo apt-cache search pyjexl
fermin@bodoque:~/src/fiware-orion$ 

Virtual env is ok for Docker CI, but in regular Docker probably we should install in the system. So, let's try with pipx.

fermin@bodoque:~/src/fiware-orion$ sudo apt-get install pipx
...
fermin@bodoque:~/src/fiware-orion$ sudo pipx install pyjexl==0.3.0
Note: Dependent package 'future' contains 2 apps
  - futurize
  - pasteurize

No apps associated with package pyjexl. Try again with '--include-deps' to include apps of dependent packages, which are listed above. If you are attempting to install a library, pipx should not be used.
Consider using pip or a similar tool instead."

No luck so far...

Edit: I have posted a question at SOF about this: https://stackoverflow.com/questions/77986950/better-way-of-installing-pyjexl-python-package-in-debian-systems-without-using

@fgalan
Copy link
Member Author

fgalan commented Feb 15, 2024

At commit ae3d83b the fails happening in 2560_custom_notification_json (8) and 4085_custom_notifications_ngsi_payload (16) are due to slight differences in the JSON rendering like this ones:

imagen

imagen

This is due to the usage of json.dumps() in JEXL evaluation for JSON Arrays and objects and will be solved if we use the CompoundNode based rendering. To do that, the PyDic/PyList used at JEXL evaluation stage should be tranformed in a CompounNode tree. For the same price, we will avoid the call to json.dumps() in the Python interpreter (which seems to be a bit overkill).

With regards to the changes in this ftest

imagen

they are due to the change in behaviour when a given attribute doesn't exist in the payload rendering. It seems before this we use null and now we are using the empty string. We need to analyse this in detail (not sure if it is even an improvement...).

@fgalan fgalan changed the title [WIP] JEXL expressions in custom notifications JEXL expressions in custom notifications May 28, 2024
CHANGES_NEXT_RELEASE Outdated Show resolved Hide resolved
doc/manuals/orion-api.md Outdated Show resolved Hide resolved
src/lib/rest/rest.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@danielvillalbamota danielvillalbamota left a comment

Choose a reason for hiding this comment

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

LGTM

doc/manuals/orion-api.md Outdated Show resolved Hide resolved
Co-authored-by: mapedraza <40356341+mapedraza@users.noreply.github.com>
Copy link
Collaborator

@mapedraza mapedraza left a comment

Choose a reason for hiding this comment

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

LGTM

@mapedraza mapedraza merged commit 33eef76 into master May 30, 2024
11 of 13 checks passed
@mapedraza mapedraza deleted the feature/4004_jexl_expressions branch May 30, 2024 09:54
@fgalan
Copy link
Member Author

fgalan commented May 30, 2024

@fisuda this PR does some modifications in the .md of the English documentation. It would be great if you could do a PR with the corresponding Japanese modifications. Thanks in advance!

@fisuda
Copy link
Contributor

fisuda commented May 31, 2024

I sent the PR #4559. Thanks.

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.

4 participants