Prevent raw curies from being sent in a request. Allow removing empty… #153
Conversation
| if (req.removeMissingValuesFrom.params) { | ||
| pruneMissingQueryParams(req.params); | ||
| } | ||
| Object.entries(req.params).forEach(normalizeEmptyParamField(req)); |
There was a problem hiding this comment.
Toyed with using reduce to return a new object that could get piped to later operations. In the end, we're mutating request many other places, so stuck with that pattern.
src/tools/cleaner.js
Outdated
| @@ -110,9 +110,39 @@ const createBundleBank = (appRaw, event = {}) => { | |||
|
|
|||
| const maskOutput = output => _.pick(output, 'results', 'status'); | |||
|
|
|||
| const normalizeEmptyRequestField = (shouldCleanup, field) => req => { | |||
There was a problem hiding this comment.
Kept this as a callback to an array iterator function like forEach. We could pull that part in here and have a simple call to normalize that loops for the supplied field from request.
There was a problem hiding this comment.
I think I like the idea of centralizing the call to Object.entries so we don't need to keep re-typing that. Doesn't look like there's any place where the calling function needs to change that callback directly.
stevelikesmusic
force-pushed
the
fix/handle-empty-body-fields
branch
from
f953cf7 to
914941a
Compare
There was a problem hiding this comment.
So the code itself looks good, but i'm a little worried about changing default behavior (even if it's bad). I get that we need to abstract this some for godzilla use, but I think we should be careful. ¯\_(ツ)_/¯
Also, you sure do like your function factories 😀
|
I go back and forth between factories and partial application 😆 Definitely a fan of some type of curried pattern. I agree about being careful. What would you suggest? This is a CLI problem as well. Godzilla is just a CLI perform using the request shorthand. If a CLI app uses curlies for an input that comes back empty in a shorthand request, won't we do the same? We could totally limit the surface area and check for |
|
Totally. I think this is a good way to do it, just that "changing default behavior" sets off warning bells. 😁 should be good though! |
|
I think this all checks out. I too was worried about default behavior change, but thinking a bit more on it, this is just being helpful. There is never a reason to send curlies, so replacing those with empty string by default now is a slight improvement. And then the option is there to strip the key out completely if the dev needs it. |
Ticket
https://zapierorg.atlassian.net/browse/PDE-889 from Issue
From issue: https://github.com/zapier/zapier/issues/25928
This PR addresses two issues:
A couple considerations, we don't remove unresolved curlies from url path. This might allow something like
https://api.com/resouce/{{bundle.inputData.id}}Some context from slack