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 capability to walk through 'link' nodes and avoid crash when newNode is null #61

Closed
abruno06 opened this issue Nov 7, 2018 · 3 comments

Comments

@abruno06
Copy link

abruno06 commented Nov 7, 2018

changing the function in swagger.js by this will add the two capabilities

function checkWiresForHttpResponse(node) {
var wires = node.wires[0];
// Add link following
if (wires === undefined || wires.length == 0) {
wires = node.links; // in case this links are in use on the scenario
}
for (var i in wires) {
var newNode = RED.nodes.getNode(wires[i]);
if (newNode === null) {
// in case newNode is null will avoid process to crash
return false;
}
if (newNode.type == "http response") {
return true;
} else if (checkWiresForHttpResponse(newNode)) {
return true;
}
}
return false;
}

@shrickus
Copy link

shrickus commented Jan 4, 2019

I'm running into this same issue with the checkWiresForHttpResponse function, which has at least 3 bugs in it...

As Aurelien noted, the logic does not allow for flows that pass through a pair of link in/link out nodes. This is because the link out node is "connected" to the link in node using the links property, not the wires[] array (at least, this used to be the case... but maybe that's not true anymore?). While misleading, at least it doesn't throw an exception -- it just stops traversing that path and does not list that endpoint as available in the swagger sidebar.

Secondly, the wires are only followed if they are connected to the first output port -- due to this code:

        var wires = node.wires[0]; // only checks the first output port here?
        for (var i in wires) {

Instead, the list of downstream nodes needs to be a concatenation of all the output wires, with any "link" ids.

The bigger issue is the runtime exception which causes the swagger sidebar rendering to fail. This is caused by RED.nodes.getNode(wires[i]) returning a null for nodes that are inside a disabled flow. I cannot tell if this is working by design, or was overlooked when the "tab disable" feature was added -- but either way the code should not be checking the node's type without first making sure there is a node matching the id.

Here is a more robust version of that function, which I've been using locally for a while:

    function checkWiresForHttpResponse(node) {
        var wires = node.wires.reduce((x,a) => x.concat(a), []);
        for (var i = 0; i < wires.length; i++) {
            var newNode = RED.nodes.getNode(wires[i]);
            if (newNode) {
                if (newNode.type == "http response") {
                    return true;
                }
                else if (checkWiresForHttpResponse(newNode)) {
                    return true;
                }
            }
        }
        return false;
    }

Please let me know if you want me to open a PR, or if you have all you need.

@knolleary
Copy link
Member

@shrickus this is a very unloved node that hasn't had any changes in 3 years. I'm unlikely to spend any time on it given all of the higher priority work on the core of Node-RED. So if you wanted to raise a PR with all the fixes, then it would be very welcome.

@knolleary
Copy link
Member

#67 has been merged that should address this issue - version 0.1.9 of the node published to npm.

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

No branches or pull requests

3 participants