Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Initial Support for v3 Webhook Payloads #427
base: main
Are you sure you want to change the base?
Initial Support for v3 Webhook Payloads #427
Changes from all commits
4509fb3
6ca4799
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still personally like to see this become
func GetEventDataValue(e OutboundEvent, keys ...string) (string, error)
since it's treating the public fields of theOutboundEvent
as input, and is not modifying internal state of the value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One case I am still considering how we should handle is when this method is called using a set of
keys
that does not terminate in a string value. For example if the object was:and this function was called with
GetEventDataValue("service")
you would get the stringified version of theservice
object which isn't super useful.My gut feeling is to return an error for this case as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the
io.Reader
interface contract, you should try processing the data inb
before you handle theio.ReadAll
error. I think this block of code should be something like:This version does a bit more to ensure the
*http.Request.Body
will contain the request body we received, even if we only got a partial read of the data. That way consumers can read that partial body themselves and inspect it.The one case this does not handle is when the
io.LimitReader
is violated. In that case they will get the.Body
that we read so far, and the remaining bytes will be lost meaning the caller couldn't choose to fully read it even if they wanted to. That could be accomplished by usingio.MultiReader()
, but would require some reworking of my snippet above.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(IIRC, we got this code from the comments you provided in the previous PR.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those snippets were things I probably hammered out on my mobile phone while responding back to the comment(s). In hindsight, I should have done a better job at communicating that I hacked them together pretty rapidly as an example. I'm sorry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries but I would still like to get clarity on what is the "perfect" reusable HTTP body reader.
This is one of the reasons I was pushing for more focused functions that verify signatures and construct events in #326. I would like to focus my effort on providing solid support for V3 Webhooks rather than the details of how folks implement that functionality.