bartbutenaers / node-red-dashboard-2-ui-video

A Node-RED node for playing video in dashboard D2
Apache License 2.0
1 stars 0 forks source link

Input msg validation #16

Closed bartbutenaers closed 2 months ago

bartbutenaers commented 2 months ago

There are a number of todo's in the js backend file, for input message validation. It is not clear to me if this is possible in the beforeSend function. In some dashboard core node, they use a custom backend onInput function to do that. But that looks weird to me, since I want to reuse the standard onInput function.

colinl commented 2 months ago

In mine I do everything in onInput. I had not thought of doing it in beforeSend. Where is the default onInput so I can see what it does? I need to massage the message before it is sent, and may not want to send it to the clients at all, so one question is, can one achieve that in beforeSend?

bartbutenaers commented 2 months ago

Thanks @colinl! You can find the default onInput handler here. I want to avoid overwriting it because then I need to keep my copy in sync with the original one over time.

Doesn't the dashboard decide which messages are being send to to which clients? Would not expect ui node developers having to do that.

colinl commented 2 months ago

You can find the default onInput handler here.

That's very helpful, thanks. I see there that beforeSend is actually called before onInput, which I had not realised. Also it looks as if it should not be a problem modifying the message in beforeSend and that also it copes with sending null back from there if one doesn't want the message stored or passed through or sent to the clients.

Therefore I can't see any problem with validating the message in beforeSend, and either correcting any issues or returning null to ignore the message. I think you are right, that is the place to do the validation. I plan do some major refactoring on one of my nodes now that I understand rather more about the framework, but it won't be in the next few weeks.

Since onInput is called after beforeSend that means that it is the validated (and/or massaged) message that is saved in the datastore, which is good.

Doesn't the dashboard decide which messages are being send to to which clients? Would not expect ui node developers having to do that.

Yes, I didn't mean sending to only some clients, I meant whether it should be sent to any clients. The answer to which seems to be that it can be achieved by returning null from beforeSend.

Having said all that though, I am just looking at the code again. I don't think I can use the default onInput anyway as it always stores the message in the datastore, and I don't always want to do that. For example, if a message with only ui_updates in it is received that should not be stored as that will destroy any previous payload data stored. Also I see that after calling the widget's onInput or running its own it emits the message to the clients, but I am doing that myself in onInput when necessary and I don't see repeated messages. Are you sure that code is run for third party nodes?

bartbutenaers commented 2 months ago

Therefore I can't see any problem with validating the message in beforeSend

Yes you were right Colin. I was still thinking in the way before the dynamic properties had been introduced: that I had to throw an error (which would not be catched correctly for the beforeSend function) in order to avoid the dynamic properties to be emit to the frontend.

However now I just need to skip storing the dynamic property into the statestore when it contains an invalid value. For example:

        const beforeSend = async function (msg) {
            // dynamic properties
            const updates = msg.ui_update
            // Store all the dynamic properties, to make sure their values are still available at browser refresh
            if (updates) {
                if (typeof updates.logType !== 'undefined') {
                    if (['none', 'console', 'msg'].contains(updates.logType)) {
                        base.stores.state.set(group.getBase(), node, msg, 'logType', updates.logType)
                    } else {
                        node.error('Property msg.ui_update.logType should be "none", "console" or "msg"')
                    }
                }
            }
           return msg
         }

Thanks for thinking out loud!!

bartbutenaers commented 2 months ago

Implemented