Updated models to match actual data structures transmitted by GitHub.

[itaibh]

Closes #1789

[ryangribble]

Unfortunately these changes will break the PushEventPayload class when used with the Events API

As mentioned in your linked issue, the problem is that the GitHub push webhook uses vastly different payload/objects to the push event in the events API.

What we need to do here is make "new" response models for the webhook side of things, rather than modifying the event side of things.

I'd suggest renaming all the "new" ones as PushWebhookXXX

So in other words

• Instead of changing the PushEventPayload class to have extra/conflicting fields, create a new PushWebhookPayload class
• Rename CommitPayload to PushWebhookCommit
• Instead of changing the Commiter class to have extra/conflicting fields, create a new PushWebhookCommitter class

[itaibh]

I just tried that, and I don't know when to create a new PushWebhookPayload instance.
I mean, I do need to deserialize it from a JSON coming from the server, but the name the server sends is the same as of a regular push event.
Am I missing something?

[itaibh]

@ryangribble Any news regarding my question?

[ryangribble]

im not really sure what you mean sorry. The push event that we already have, is from the events API activity stream calls, whereas the push event you want is from a webhook. We currently dont have any code to directly handle a webhook, however if you create the response class you can still use our deserializer to pass the json payload to, and populate the response model.

As a first step can you create the response models and push them up? Then we can see what the best way is to deserialize the webhook payload

[itaibh]

@ryangribble I merged with latest changes on master with my changes. Hopefully I did it right.

All the new PushWebhook* classes are here, but now I need to deserialize them, and I don't know how to distinguish between the PushWebhookEvent payload and the PushEvent payload in SimpleJsonSerializer.cs in the GetPayloadType(string activityType) method.

Can you review it please and try to help me figure it out?

[ryangribble]

Looks like you've polluted your master branch with merge commits and other stuff that isn't actually in our master. There are a few spurious files/changes still on this PR that need to be reverted, which ive marked up

[ryangribble]

In terms of the deserializer code, you might be getting hung up on some of the magic that is in there, for the events API endpoints (but that doesn't have any bearing on what you're trying to do with webhooks). The magic stuff is to handle Activity responses, which have a Payload property of type ActivtyPayload with some special deserializer code that actually deserializes these as the derived classes.

For a basic webhook, you won't use any of that stuff though... you would have your own API/webserver which receives the webhook. You would check the X-GitHub-Event http header, and based on the value you would know what webhook type it is and thus what webhook payload type to use. Then you would call the octokit deserializer, and provide the actual class you want to deserialize into.

Eg

var eventHeader = "push";
var payload = "<json payload here>";

if (eventHeader == "push")
{
  var push = new SimpleJsonSerializer().Deserialize<PushWebhookPayload>(payload);
}
else if (eventHeader == "check_run")
{
  var checkRun = new SimpleJsonSerializer().Deserialize<CheckRunEventPayload>(payload);
}

This is the bare bones way to do it... I'm keen to think about how we could add some helpers to the library to make it easier. The problem is it's not possible to at runtime dynamically deserialize the payload into a specific response type, that the user can know at compile time. We can do the ugly trick like we do with the event APIs, where the user only gets a base class, which the deserializer has populated with an inheritted/derived class, but the user has to somehow know this, and cast it to the correct type, but I really dont like that.

Another way I was thinking we could do it, is by allowing the user to register their delegate code for the given types they care about, and we could then run that. This has the advantage of being strongly typed at compile time. It might look something like this:

// Setup code
Octokit.Webhooks.RegisterWebhookHandler(WebhookType.Push, x => {
  // x is a PushWebhookPayload
});
Octokit.Webhooks.RegisterWebhookHandler(WebhookType.CheckRun, x => {
  // x is a CheckRunEventPayload
});

// When receiving a webhook
var headers = request.Headers;
var payload = request.Body;

Octokit.Webhooks.Process(header, payload); // this would switch on the hook type found in the headers, then call any registered handler, passing the deserialized/strongly typed response model into the delegate provided by the user

Now whether this stuff would belong in this core library or in an addon package Octokit.Webhooks Im still on the fence about. I'd actually love to provide some common middle ware type approach that could actually handle the webserver/listening part of things too, so that makes me lean towards a separate library to do this stuff

But in terms of this PR I think we can just get the response models defined, and see if you can confirm that the first code sample I provided (using the Octokit deserializer directly) is working for you. We also have heaps more webhook payloads to implement if you want to add any others while you're here 👍

[GMouron]

Hello, @ryangribble @itaibh , I'm actually interested in this PR.
May I fork it and make the modifications asked so it can land on master ?
I don't know exactly what's the etiquette in this kind of situation since there is no news since July 30th ...

[itaibh]

@GMouron fine by me 😃

[GMouron]

Actually, I see that @itaibh did make the modifications asked by the code review

[ryangribble]

Hmmm I must have missed that last commit that says it addresses the review comments, I will take a look at this review again

[GMouron]

Hello, any idea of when this could land on master ?
And another slightly related question, when do you usually make a new release from master ?

Thanks :)

[shiftkey]

This felt very close to landing, so I rebased the branch on master and addressed the last bit of feedback.

[codecov]

Codecov Report

Merging #1844 into master will decrease coverage by 0.21%.
The diff coverage is 35.55%.

@@            Coverage Diff             @@
##           master    #1844      +/-   ##
==========================================
- Coverage   67.05%   66.84%   -0.22%     
==========================================
  Files         538      542       +4     
  Lines       14141    14219      +78     
==========================================
+ Hits         9482     9504      +22     
- Misses       4659     4715      +56

Impacted Files	Coverage Δ
Octokit/Helpers/AcceptHeaders.cs	100% <ø> (ø)	⬆️
Octokit/Models/Response/TimelineEventInfo.cs	3.33% <0%> (-0.24%)	⬇️
Octokit/Models/Response/IssueEvent.cs	6.66% <0%> (-1.03%)	⬇️
.../Response/ActivityPayloads/PushWebhookCommitter.cs	0% <0%> (ø)
Octokit/Models/Response/IssueEventProjectCard.cs	0% <0%> (ø)
...els/Response/ActivityPayloads/PushWebhookCommit.cs	0% <0%> (ø)
...ls/Response/ActivityPayloads/PushWebhookPayload.cs	0% <0%> (ø)
...t.Reactive/Clients/ObservableIssuesEventsClient.cs	100% <100%> (ø)	⬆️
Octokit/Clients/IssueTimelineClient.cs	100% <100%> (ø)	⬆️
Octokit/Clients/IssuesEventsClient.cs	100% <100%> (ø)	⬆️
... and 4 more

[shiftkey]

@itaibh thanks for the contribution, and for working through the review with us!

[GMouron]

Great, thank you @shiftkey. I will now be able to remove my home-built version 😉

[shiftkey]

release_notes: Added new types for webhook commit payload