node-red / node-red

Low-code programming for event-driven applications
http://nodered.org
Apache License 2.0
18.93k stars 3.31k forks source link

Undesirable behaviour when _bad_ JSON entered into TypedInput #4745

Open Steve-Mcl opened 3 weeks ago

Steve-Mcl commented 3 weeks ago

Current Behavior

When I paste JSON with duplicate key, then expand the editor, my JSON is unexpectantly changed (sanitised)

demo

chrome_HKBDD3RXWg

JSON I used

{     "info": "hello",     "abc": "123",     "fff": "www",     "info": "xxxxx" }

Expected Behavior

  1. JSON entered in text field should be faithfully presented in the editor warts n all
    1. This allows me to "see my error" and "fix it up" the way I want to (not take the first key)
  2. TypedInput validator should highlight this as an issue

Steps To Reproduce

  1. Add an inject --> debug
  2. set payload to { "info": "hello", "abc": "123", "fff": "www", "info": "xxxxx" }
  3. deploy and inject
  4. debug outputs { "info": "xxxxx", "abc": "123", "fff": "www" }

image

Example flow

[{"id":"df3b6e629014dda8","type":"inject","z":"06a2836c9f2ae124","name":"{     \"info\": \"hello\",     \"abc\": \"123\",     \"fff\": \"www\",     \"info\": \"xxxxx\" }","props":[{"p":"payload"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"{     \"info\": \"hello\",     \"abc\": \"123\",     \"fff\": \"www\",     \"info\": \"xxxxx\" }","payloadType":"json","x":1680,"y":60,"wires":[["9f6565a5dce8016d"]]},{"id":"9f6565a5dce8016d","type":"debug","z":"06a2836c9f2ae124","name":"debug 13","active":true,"tosidebar":true,"console":false,"tostatus":false,"complete":"false","statusVal":"","statusType":"auto","x":1810,"y":100,"wires":[]}]

Environment

hardillb commented 3 weeks ago

https://esdiscuss.org/topic/json-duplicate-keys

This behaves as I expect

Steve-Mcl commented 3 weeks ago

I (respectfully) kinda disagree (from a low-code POV)

It tripped me up 😉

I was scratching my head longer than I care to admit as to why the debug output was different to what i seen in the nodes text entry.

stoopid loose terms in the RFC causing confusion! "If a key is duplicated, a parser SHOULD reject." I read that as the parser "should reject". Hey ho.

I'm not entirely sure if we should (or how we would) do anything about it but it just doesnt feel right 🤷

Happy to be told its a "won't fix" item but maybe lets sit on it a while and see if any good ideas crop up?

Steve-Mcl commented 3 weeks ago

Mind you, i still think the editor should respect what is entered in the text box.

I was completely tripped by what the expanded editor was showing me matched the debug output - i think there is something to handle there. So I stand by the "JSON entered in text field should be faithfully presented in the editor warts n all"

hardillb commented 3 weeks ago

Does the expanded JSON editor flag it as a syntax exception of you try and add duplicate keys

knolleary commented 3 weeks ago

We use JSON.stringfy/parse to roundtrip between the single line input and the multiline editor. Duplicate keys, whilst not flagged as an error, will not survive that round trip. Any fix for this will require a custom json parser/formatter.

Steve-Mcl commented 3 weeks ago

Does the expanded JSON editor flag it as a syntax exception of you try and add duplicate keys

It does. image

We use JSON.stringfy/parse to roundtrip between the single line input and the multiline editor. Duplicate keys, whilst not flagged as an error, will not survive that round trip. Any fix for this will require a custom json parser/formatter.

It might not require a custom formatter.

If we pass whatever is in the text field directly into the editor it will be shown as "problematic" and the editor itself has a formatter built in that correctly preserves the original data.

chrome_sOIEvmYoqS

We can call the formatter function programmatically too like this:

function formatDocument() {
  editor.getAction('editor.action.formatDocument').run();
}

// on first initialisation
editor.onDidChangeModelLanguageConfiguration(formatDocument);

// on every initialisation
editor.onDidLayoutChange(formatDocument);

// manually
function setValue (v) {
    editor.setValue(v)
    formatDocument()
}

Here is a demo I knocked up: Monaco Playground Programmatically call formatDocument

knolleary commented 3 weeks ago

Regardless of what monaco could do here, if you want the TypedInput to flag an error, it will require a custom JSON parser to spot it and flag it. Not sure how much effort it's worth putting into this in the big scheme of things.

Steve-Mcl commented 1 week ago

if you want the TypedInput to flag an error, it will require a custom JSON parser to spot it and flag it.

Or just use jsonlint.

const jsonlint = require("@prantlf/jsonlint");
let jsonParsed
try {
    jsonParsed = jsonlint.parse(
      '{     "info": "hello",     "abc": "123",     "fff": "www",     "info": "xxxxx" }',
      { allowDuplicateObjectKeys: false })
    console.log(jsonParsed)
} catch (e) {
    console.error(e);
}

image

Working demo:

https://replit.com/@StephenMcLaughl/jsonlint#index.js

GogoVega commented 5 days ago

Interesting, it would allow TypedInput to flag an error and at the same time to highlight the spot by changing the color (because the location is available). There is probably a way to use the autoComplete logic (label) for this.