node-red / node-red-node-swagger

A set of tools for generating Swagger api documentation based on the HTTP nodes deployed in a flow
Apache License 2.0
63 stars 48 forks source link

Add capability to walk through 'link' nodes and avoid crash when newNode is null #61

Closed abruno06 closed 4 years ago

abruno06 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 4 years ago

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