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

[http-request] : Can't set payload handling when method is passed in via msg.method #3957

Open
marcus-j-davies opened this issue Nov 23, 2022 · 7 comments · May be fixed by #3961
Open

[http-request] : Can't set payload handling when method is passed in via msg.method #3957

marcus-j-davies opened this issue Nov 23, 2022 · 7 comments · May be fixed by #3961
Assignees

Comments

@marcus-j-davies
Copy link

marcus-j-davies commented Nov 23, 2022

Current Behavior

When - set by msg.method - is used for the method, it hides the payload handling dropdown on the node config editor.
Now then, If the user does not first choose the payload handling option prior to selecting - set by msg.method -

The users payload is ignored, i.e for GET requests, the payload is not passed in as a query string, as the default is to ignore it.

The user has to first set it back to GET, change the payload handling option, then set it back to - set by msg.method -.
The option is there initially, but this could trip up new users.

Expected Behavior

Iether allow payload handling options to be include in the message, or to not hide the option

Steps To Reproduce

  • Add a new http-request node
  • set it to - set by msg.method -
  • Note that the payload handling option has gone

Example flow

No response

Environment

  • Node-RED version: 3.0.0
  • Node.js version: 16.15.1
  • npm version: 8.11.0
  • Platform/OS: Debian
  • Browser: Safari

Discussed here:
https://discourse.nodered.org/t/http-request-node-returning-error

@Paul-Reed
Copy link

I have an example flow which I can share by forum DM if necessary.

@Steve-Mcl
Copy link
Contributor

@Paul-Reed could you share a minimal flow here please?

Ta.

@Paul-Reed
Copy link

Paul-Reed commented Nov 24, 2022

No, because it contains my api key/secret, but I can DM it to you, for your personal attention.
The error cannot be demonstrated without a valid api request which needs api auth.

EDIT - Now DM'd

@Steve-Mcl Steve-Mcl self-assigned this Nov 28, 2022
@Steve-Mcl
Copy link
Contributor

Steve-Mcl commented Nov 28, 2022

Issue confirmed.

ether allow payload handling options to be include in the message, or to not hide the option

@dceejay my preference would be to permit the payload handling option (for GET requests) to be set by the msg rather than always display the dropdown.

My reasoning is there may be only 1 HTTP Request Node on a flow, used for all GET/POST/ETC using msg.method and some of the routes may want payload in the QS, some may want payload in the body.

My current solution would be to replace the at lines 203~205
https://github.com/node-red/node-red/blob/master/packages/node_modules/@node-red/nodes/core/network/21-httprequest.js#L203-L205

... with a "per msg" (on the fly) determination...

        this.on("input",function(msg,nodeSend,nodeDone) {
            // ...
            // ... snipped for brevity ---
            // ...
            // ... 
            let paytoqs = false
            let paytobody = false
            if (msg.method && n.method && (n.method === "use")) {
                method = msg.method.toUpperCase(); // use the msg parameter
                // If the method is set to GET (by msg.method), we need to check what to do with the payload
                if (method === "GET") {
                    if (msg.payloadToQuerystring === true) { 
                        paytoqs = true
                    } else if (msg.payloadToBody === true) { 
                        paytobody = true
                    }
                }
            } else {
                  // use UI options (original code)
                if (n.paytoqs === true || n.paytoqs === "query") { 
                    paytoqs = true
                } else if (n.paytoqs === "body") { 
                    paytobody = true
                }
            }

Thoughts please Dave?

@marcus-j-davies
Copy link
Author

Makes complete sense to include it in the message
if I may add a slight alterantive to consider?

if (method === "GET") {
    switch (msg.payloadHandling.toLowerCase()) {
        case 'qs':
            paytoqs = true
            break
        case 'body':
            paytobody = true
            break
    }
} else {
  ...
}

Could make it easier to add more options in the future, using just one key to dictate handling.

@Steve-Mcl
Copy link
Contributor

Steve-Mcl commented Nov 28, 2022

@marcus-j-davies I like the single msg property idea - it does simplify things somewhat. But for compatibility reasons, I would suggest the value options are true, "query" and "body". This aligns with the current option values.

  • true and "query" would mean add payload to querystring
  • "body" would mean add payload to body

The built in docs would not bother to mention the true value but for compatibility reasons, would be supported.

            let paytoqs = false
            let paytobody = false
            let payloadHandling = n.paytoqs
            if (msg.method && n.method && (n.method === "use")) {
                method = msg.method.toUpperCase(); // use the msg parameter
                payloadHandling = msg.payloadHandling
            }

            // Test payloadHandling to see if it has been set
            if (payloadHandling === true || payloadHandling === "query") { 
                paytoqs = true
            } else if (payloadHandling === "body") { 
                paytobody = true
            }

NOTE: I realise I am not checking the method is a GET but it doesnt matter since that is checked further down in the code (paytoqs / paytobody are only ever used for GET requests)

@dceejay
Copy link
Member

dceejay commented Nov 28, 2022

Sounds sensible to me... no objections here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants