fix: handle gcf, verify already provided request.body

[Uzlopak]

Resolves #935

As explained in #935 in gcf the payload is already deserialized to be an object, resulting that the payload can not be verified.

Can this be please fast-tracked?

@wolfy1339
@gr2m

---

Before the change?

After the change?

Pull request checklist

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes
• No

---

[wolfy1339]

Can you make sure that this doesn't effect values that end in a zero decimal, ex: 123.10

[Uzlopak]

@wolfy1339

I think I can do it even better.

[Uzlopak]

@wolfy1339
I could not follow, what you mean with handling numbers correctly.

But now it is trying to avoid unecessary serializiation and deserialization based on the input. GCF has body and rawBody, while rawBody is the Buffer.

Let me test this on GCF and give you feedback :).

[Uzlopak]

@wolfy1339

It works now properly. Can you please explain what your concern is regarding numbers?

[wolfy1339]

NodeJS likes to remove trailing zeroes on numbers, and thus it changes the signature of the payload

[Uzlopak]

@wolfy1339

In this case, I just handle the specific case of GCF because there is no way I can ensure, that JSON.stringify and .parse will respect that.

PTAL

[Uzlopak]

Tested on GCF and works as expected ;)

[Uzlopak]

@wolfy1339

It is hard to provide a benchmark for body attribute, as it is gcf specific. Passing the body object is only relevant for gcf. In other cases it is just undefined and does not effect significantly the performance. In gcf it actually avoids one JSON.parse.

I really need this PR to land, so i beg you to approve and merge it.

My benchmarks show a significant performance gain from 73k ops/ 10s to 83k ops/ 10s.

before:

┌─────────┬──────┬──────┬───────┬──────┬────────┬─────────┬───────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg    │ Stdev   │ Max   │
├─────────┼──────┼──────┼───────┼──────┼────────┼─────────┼───────┤
│ Latency │ 0 ms │ 1 ms │ 4 ms  │ 5 ms │ 0.8 ms │ 1.08 ms │ 31 ms │
└─────────┴──────┴──────┴───────┴──────┴────────┴─────────┴───────┘
┌───────────┬────────┬────────┬────────┬────────┬────────┬─────────┬────────┐
│ Stat      │ 1%     │ 2.5%   │ 50%    │ 97.5%  │ Avg    │ Stdev   │ Min    │
├───────────┼────────┼────────┼────────┼────────┼────────┼─────────┼────────┤
│ Req/Sec   │ 3287   │ 3287   │ 7767   │ 7919   │ 7255.7 │ 1364.07 │ 3286   │
├───────────┼────────┼────────┼────────┼────────┼────────┼─────────┼────────┤
│ Bytes/Sec │ 411 kB │ 411 kB │ 971 kB │ 990 kB │ 907 kB │ 171 kB  │ 411 kB │
└───────────┴────────┴────────┴────────┴────────┴────────┴─────────┴────────┘

Req/Bytes counts sampled once per second.
# of samples: 10

73k requests in 10.02s, 9.07 MB read

after:

┌─────────┬──────┬──────┬───────┬──────┬─────────┬─────────┬───────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg     │ Stdev   │ Max   │
├─────────┼──────┼──────┼───────┼──────┼─────────┼─────────┼───────┤
│ Latency │ 0 ms │ 0 ms │ 4 ms  │ 4 ms │ 0.46 ms │ 0.99 ms │ 29 ms │
└─────────┴──────┴──────┴───────┴──────┴─────────┴─────────┴───────┘
┌───────────┬────────┬────────┬─────────┬─────────┬─────────┬─────────┬────────┐
│ Stat      │ 1%     │ 2.5%   │ 50%     │ 97.5%   │ Avg     │ Stdev   │ Min    │
├───────────┼────────┼────────┼─────────┼─────────┼─────────┼─────────┼────────┤
│ Req/Sec   │ 4711   │ 4711   │ 8623    │ 8959    │ 8254.8  │ 1231.47 │ 4708   │
├───────────┼────────┼────────┼─────────┼─────────┼─────────┼─────────┼────────┤
│ Bytes/Sec │ 589 kB │ 589 kB │ 1.08 MB │ 1.12 MB │ 1.03 MB │ 154 kB  │ 589 kB │
└───────────┴────────┴────────┴─────────┴─────────┴─────────┴─────────┴────────┘

Req/Bytes counts sampled once per second.
# of samples: 10

83k requests in 10.02s, 10.3 MB read

[wolfy1339]

I'd rather not. It's better to fix this at the source

[Uzlopak]

see octokit/webhooks-methods.js#211

[wolfy1339]

Please let other maintainers the time to review this. Can you split the performance patches from this PR and only handle GCF compatibility please

[wolfy1339]

We have 3 active PRs, of which 2 were split off from this one.

Can you incorporate the feedback of this PR into the other PRs?

#939 for the switch to using Buffer instead of string
#937 for the changes to fix compatibility with Google Cloud Functions.

Let's continue on in those more specific PRs

[Uzlopak]

k