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.

Sign up for GitHub

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

Jump to bottom

fix: only parse payload once #921

Merged
merged 5 commits into from
Nov 16, 2023
Merged

fix: only parse payload once #921

merged 5 commits into from
Nov 16, 2023

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Nov 16, 2023 •
edited
Loading

Closes #922

We are parsing the JSON twice. Also it is quite dangerous to parse unverified data. So it is better to first verify and then parse it with the very low risk that it is broken payload.

Improves the performance of the valid path in probot beta from 63 k ops/s to 71 k ops/s.

@wolfy1339 wolfy1339 added the Type: Bug Something isn't working as documented, or is being fixed label Nov 16, 2023
wolfy1339
wolfy1339 previously approved these changes Nov 16, 2023
View reviewed changes
@wolfy1339
Copy link
Member

I'd like to wait for others to review as well

@G-Rath
Copy link
Member

G-Rath commented Nov 16, 2023

I personally don't feel like this change is needed and really doubt this is an actual bottleneck for probot (if it is, I'd want to see a more detailed breakdown of the benchmarking before making exploring more of these kind of changes).

Having said that iirc getPayload is an internal function and it would be fair to have it just return string without any validation (it already does anyway if request.body is present).

G-Rath
G-Rath reviewed Nov 16, 2023
View reviewed changes
src/verify-and-receive.ts Outdated Show resolved Hide resolved
gr2m
gr2m previously approved these changes Nov 16, 2023
View reviewed changes
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes look good to me, just some nits

src/verify-and-receive.ts Outdated Show resolved Hide resolved
src/middleware/node/get-payload.ts Outdated Show resolved Hide resolved
@Uzlopak Uzlopak dismissed stale reviews from gr2m and wolfy1339 via 3e85511 November 16, 2023 08:49
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 16, 2023

@wolfy1339
@G-Rath
@gr2m
@oscard0m
PTAL ;)

@gr2m gr2m merged commit a12200f into octokit:main Nov 16, 2023
6 checks passed
@github-actions GitHub Actions
Copy link
Contributor

🎉 This PR is included in version 12.0.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@G-Rath G-Rath mentioned this pull request Nov 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wolfy1339 wolfy1339 wolfy1339 left review comments

@G-Rath G-Rath G-Rath left review comments

@oscard0m oscard0m oscard0m approved these changes

@gr2m gr2m gr2m approved these changes

Assignees
No one assigned
Labels
released Type: Bug Something isn't working as documented, or is being fixed
Projects
🧰 Octokit Active
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Avoid parsing JSON payload twice.
5 participants
@Uzlopak @wolfy1339 @G-Rath @gr2m @oscard0m