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

Add support for nested objects with exclamation mark #375

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

KGuillemot
Copy link
Contributor

@KGuillemot KGuillemot commented Aug 9, 2023

Actual behavior

Currently, when using the following rulebase :

version=2
rule=:%a!b:rest%

And the following line to parse :

line to parse

The result is :

{ "a!b": "line to parse" }

New behavior

With the proposed improvement, when using the same rulebase and the same line to parse as above, the result is now :

{ "a": { "b": "line to parse" } }

Note that this is a breaking change in case of using exclamation mark in captured names.

Notes

I tried to add the new behavior as an option, for example by adding "nested=" at the beginning of rulebase files, but I encountered some issues by using file inclusion or with the Rsyslog mmnormalize module by using the directive "rule=".

Please let me know if you have a better option to add this new behavior as a option, or to prevent breaking change, but this new behavior makes sens regarding the access of nested variables in Rsyslog by using exclamation marks.

@KGuillemot
Copy link
Contributor Author

@rgerhards : What do you think of this PR please ?

@rgerhards
Copy link
Member

rgerhards commented Aug 9, 2023

What do you think of this PR please ?

I was busy, bear with me ;-)

But I've seen the proposal just 5 mins ago (reading mail sequentially).

I agree it is useful.

I don't like breaking changes. This one sounds reasonable. I would tend to say that ! inside names in existing rule bases is rare / exotic.

I see some solutions, though, for making it optional.

  • bump rulebase version to 3 which would mean "! is meaningful in names"
    if we take that path, we should define some chars which may have special meaning in field names and must
    not be used (e.g. /,.+-!&%" - along these lines). Unfortunately we did not do this in v2.

  • support that option in condensed or full json format, only
    that would also work with rule="...."
    doc: https://www.liblognorm.com/files/manual/configuration.html#full-json-format

What do you think? @davidelang what's your take on this?

@KGuillemot
Copy link
Contributor Author

Thank you for your answer :-)

If you really don't want breaking change, I like your first option "version=3" !

@rgerhards
Copy link
Member

If you really don't want breaking change, I like your first option "version=3" !

perfect, so be it :-)

Could you include that change in your PR?

@davidelang
Copy link
Contributor

sorry I missed this, yes this is a greatly desired functionality, I like the version 3 to avoid breaking (rare) existing configs. I like expanding the list of reserved characters.

As I understand it, some things (elasticsearch IIRC) use '.' instead of '!' for their separator character. reserving '.' is far more likely to break configs, but with the version change, we can get away with doing so.

I'm not thrilled at making it only apply to the json syntax, the old syntax is just so straightforward in many cases.

@KGuillemot
Copy link
Contributor Author

Thank you for your feedback.
Using exclamation mark seemed a good choice to me because in Rainerscript specifying "a!b" calls the key "b" of the object "a", which is exactly the newly added behavior for liblognorm in this PR.

The only "breaking change" I just add is this line :

ln_sampLoadFromString(ln_ctx ctx, const char *string)
<<< ctx->version = 2;
>>> ctx->version = 3;

Which is a breaking change for calling mmnormalize module in Rsyslog with rule=[""].

Is that ok for you both ?

@KGuillemot
Copy link
Contributor Author

@rgerhards , @davidelang : What's your opinion please ?

@rgerhards
Copy link
Member

Which is a breaking change for calling mmnormalize module in Rsyslog with rule=[""].

Is that ok for you both ?

sry - didn't notice the message. Yes, for mmnormalize this sounds acceptable to me. Unlikely it will hit hard there.

@rgerhards rgerhards self-assigned this Oct 25, 2023
@rgerhards
Copy link
Member

In any case, we need some doc update :-)

@davidelang
Copy link
Contributor

davidelang commented Oct 25, 2023 via email

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.

3 participants