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

JSONata falls back to compatibility mode when there’s a string with ’ msg ’ in it #4532

Open
Steve-Mcl opened this issue Jan 19, 2024 · 8 comments
Milestone

Comments

@Steve-Mcl
Copy link
Contributor

Current Behavior

See https://discourse.nodered.org/t/jsonata-falls-back-to-compatibility-mode-when-theres-a-string-msg-in-it/84692

Expected Behavior

Should not fallback when it encounters msg in a string

The bigger question for me is, is it time to drop the legacy mode?

Steps To Reproduce

See https://discourse.nodered.org/t/jsonata-falls-back-to-compatibility-mode-when-theres-a-string-msg-in-it/84692

Example flow

paste your flow here

Environment

  • Node-RED version: 3.1.x
  • Node.js version: 18
  • npm version:
  • Platform/OS:
  • Browser:
@kuema
Copy link

kuema commented Jan 19, 2024

I see two options:

Deprecation

Mark it deprecated in the next major release (4.0) and remove it in 5.0.
To support that, extend the legacy mode warning in the editor with the deprecation note.

Immediate removal

Remove it in 4.0 directly and add a section with breaking changes in the release notes.

Breaking changes are unavoidable as software evolves. So this is something that has to be clarified and documented in general, so users know what to expect.

@rvangeijtenbeek
Copy link

I feel it's quite risky to remove compatibility mode. You only see this deprecation warning when opening a JSONata editor. The current one is quite subtle, I wouldn't be surprised if lots of people use compatibility mode without thinking about it.

When I raised the issue I was thinking more about tightening the regexp :)

@kuema
Copy link

kuema commented Jan 19, 2024

Well, it's risky, I agree. But you can't keep certain features in a software forever.

There has to be a well-documented plan on how to phase something out. The next best thing is a major release, where breaking changes can occur and should be documented, of course.

@E1cid
Copy link

E1cid commented Jan 19, 2024

Could you we create a flow to run on the flow.json file in the next major upgrade.

Replace any reference to msg. That is not between \"...\" and possible some other coditions, in the to property of a change node, where tot = "jsonata". Possibly replacing with$$.

So if some one upgrade and had issues they could just run the replace msg. flow and restarrt Node-red.

@knolleary
Copy link
Member

Replace any reference to msg. That is not between "..." and possible some other coditions, in the to property of a change node, where tot = "jsonata". Possibly replacing with$$.

The "possible some other conditions" is doing a lot of work in that sentence.

One of the reasons the regex check for msg is so loose is because there are lots of valid ways it could appear in an expression where it is referencing the msg object and not just a random string.

Creating a script that would safely migrate an existing flow is likely a hard task than trying to improve the regex that detects it in the first place.

@E1cid
Copy link

E1cid commented Jan 19, 2024

I was thinking if the msg. is not in quotes and at start after certain chars, it most likely needs to be removed and replaced with $$ . The only case I can think of that would be in a string is $eval().

[msg.payload.*.
        {
            "name": $split($.mymsg.test, "watch")[1],
            "version": $lookup($, (msg._msg ? "latest"  : "wanted"))
        }
]

Take the above anything that is between quotes(which in flow.json would have \ to) can be ignored, then any other val having msg. that does not have a . _ a-Z 0-9 in front could be replaced with $$ (there are other cases but like $.msg.test, but that starts with a $ and has a .in front so could be seen and ignored.
The flow could back up the flow json in case there are exceptions not thought of. Then a flow to replace old flow.json could be run. This flow could be run before update or after.

Just a thought, if you were to remove compatibility mode making it easy for a good portion of people to convert with out going through the flow.json manually.

@Steve-Mcl
Copy link
Contributor Author

If we were to remove legacy mode in v4 I'd likely advocate keeping the regex to inform (status message or node.warn/node.error) the user of possible differences.

I agree software needs to evolve and legacy processing should be dropped at some point but perhaps the safer option is to keep legacy processing and add a more clear more prominent node.status/node.warn in v4 that spells out the removal in v5 is the better option all things considered?

@knolleary knolleary added this to the 4.0 milestone Jan 19, 2024
@shrickus
Copy link

If I recall correctly, there was a code change early on to expose the msg.* properties individually in the JSONata context, instead of exposing the entire msg object? But since we started out writing our expressions using msg. the old "legacy" code was left intact, and the regex was added to check for that old syntax and advise people to migrate their code. I cannot recall the original reason for removing the root msg object -- was it:
a. to avoid exposing internal node-red properties
b. to save resources and speed up context initialization
c. to avoid naming conflicts (e.g. a msg.msg property would be ambiguous)
d. all of the above
e. ???

It has been years now, and I'm fine with dropping that support -- but perhaps there is another option, where references to msg. are treated the same as $$. meaning go to the context root? I cannot immediately see a way to bind that without adding the function/variable prefix (i.e. $msg which really doesn't help retain backward compatibility). Perhaps the best person to ask is @acoleman himself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants