Issue: new node properties don't show up after upgrade

I've hit an issue where inconsistent stuff happens in my nodes after I add a new parameter and I tracked it down to the fact that existing nodes in flows don't get upgraded. Take a very simple node definition:

	RED.nodes.registerType('test',{
		category: 'test',
		color: '#A12100',
		defaults: {
			name: {value: ''},
			param_one: {value: "123"},
		},
		inputs: 1,
		outputs: 1,
		label: 'test'
	})

This node has one parameter (other than name): param_one
On the node.js side I have:

  function test(config) {
    RED.nodes.createNode(this, config)
    RED.log.info(`Test node config has: ${Object.keys(config).join(' ')}`)
  }

If I instantiate one of these nodes, it prints the expected:

10 Aug 17:12:34 - [info] Test node config has: id type z name param_one x y wires

If I now add a new parameter, e.g.:

		defaults: {
			name: {value: ''},
			param_one: {value: "123"},
			param_two: {value: "456"},
		},

Restart Node-RED and I still get:

10 Aug 17:12:34 - [info] Test node config has: id type z name param_one x y wires

So if in other parts of my code I expect config.param_two to exist or to have the default value 456 then that will fail for nodes created prior to the addition of param_two.
If I now reload the flow editor, add some random other node, and do a full deploy, there is no change, the flow editor did not add the default param_two to the pre-existing test nodes. The only way to get param_two added is to manually edit every test node, change something, and then deploy.

I understand why all this happens, but IMHO it's quite bad behavior. It leads to very subtle bugs that are basically impossible to track down once in the field because they depend on prior flow configuration and time of node upgrades.

In addition, to defend against this type of issue, one has to explicitly repeat the default values, which is also a great source for errors down the line. E.g., on the node.js side the constructor for the test node would have to include something like

if (!('param_two' in config)) config.param_two = 456

IMHO the best solution would be for the runtime to add the missing default values at start-up. But the runtime doesn't have the data for this... The next best thing would be for the flow-editor to add the missing default values to existing nodes and mark them as dirty.

Yes. You need to do both of those things. The editor can check and add a default value, but only if the node is edited. If the underlying node is just upgraded and the flow is run then the editor never gets a look in. So you need to set any new defaults in the js (server) side as well. Or at least handle them being missing in some way.

I obviously realized that I need to do both of those two things given the current state. My point is that this is a bug and most likely a cause for quite some issues out there.

It's also 100% undocumented as far as I can tell... A good place would be Node properties : Node-RED under 'Property Validation'. It would be good to at least mention there that (a) validators don't guarantee anything, i.e, the user can enter an invalid value and deploy, thus all validations have to be repeated in the node.js code, and (b) on upgrade no defaults are re-set and nothing is revalidated, thus the node.js has to duplicate all that.

All pretty bad stuff IMHO.

A few times we have made changes to core nodes that caused them to be marked dirty when the user opens the edit dialog, does nothing, then closes the dialog.

Every time that has happened, we have had issues raised and complaints because users have no idea what has changed.

Based on that feedback, in general it has been established as a bad practise for nodes to do that.

It is always true to say that the runtime code will always need to be able to handle a node configuration with newer properties missing. You do not know what flows will be given to the runtime. You cannot assume that users will open the flows in the editor after updating the nodes.

It really is no hardship to set sensible defaults in the runtime side.

I don't know what you call a 'hardship'... I highly value DRY. Hardship or not, it's not DRY.
Also, when a framework lets me write validators I expect those to actually be enforced. Node-RED's validators don't. This means I have to duplicate the validators. Again, not DRY.

At the end of the day the goal is to have reliable operation, isn't it? DRY is one little piece of achieving that. Input validation done correctly is another one.

I do take your point. Being able to more readily share code between the runtime and editor for that sort of shared view is certainly something that could be looked at.

But it doesn't exist today. So I wanted to share what the recommended best practice is for nodes to follow today.

With all due respect, you may be making too much of this. I admit to having been annoyed when I first encountered the issue, but recognizing and dealing with it was no great effort. The only consequences I encountered resulted from the new properties being undefined for previously deployed instances of the node, but if there are other, more serious effects, I could change my view. Still, this NOT a bug, since everything works as intended. I can't speak for the developers, but the basic structure of NR (editor and runtime) corresponding to that of the nodes (html and js) is a fundamental design choice, and the plan seems to be to separate the two rather than link them closer together.

On the other hand, you are correct to point out that the documentation should make some reference to the issue. The very beginning of the section you cite deals with exactly this situation:

In this section, we’ll add a new property called prefix to the node

All that is needed is a comment saying that the new property will be undefined in any instances of the node that were created before the change. Then the section on using the property in the node should have the line

this.prefix = config.prefix || "";

since the default value is an empty string. This is equivalent to the fix you propose, and unless I am seriously mistaken, it should take care of the problem.

2 Likes

You're just saying that it's a design bug :laughing:

This "basic structure", how it's represented, and more importantly all the resulting effects are all undocumented as far as I can tell. Throw in subflows, disabled flows and some of the idiosyncrasies around config nodes as well as dependencies between nodes and config nodes and nothing is simple or obvious anymore...

So this stuff is sooo obvious that even the one model example for creating a node doesn't do it right? I looked at some "random" contributed node and see 9 params, none of which has any defaulting nor validation: node-red-contrib-simple-gate/gate.js at master · drmibell/node-red-contrib-simple-gate · GitHub :wink:

I don't thing we mean the same thing by "bug." I'm sure the developers would welcome an issue (or better yet, a PR) on GitHub showing how to fix the bug.

I may well be mistaken, but the defaults defined in the html file and the conventions of JavaScript should be enough to prevent a user from entering an input that causes an error. If you find such a case, I will address it immediately. There are situations where the node can behave in ways that the user may not expect, but these cannot be changed by using defaults or validations of individual parameters.