[BUG]: ProcessDeployProtectionRuleWebhookAsync seems to have the wrong argument type

[justinmchase]

What happened?

The second argument is of type DeploymentProtectionRuleEvent but it should be DeploymentProtectionRuleRequestedEvent.

There are a bunch of fields on the payload that are inaccessible as a result.

Bug is on this line:
https://github.com/octokit/webhooks.net/blob/main/src/Octokit.Webhooks/WebhookEventProcessor.cs#L178

Should be:

            WebhookEventType.DeploymentProtectionRule => JsonSerializer.Deserialize<DeploymentProtectionRuleRequestedEvent>(body)!,

Versions

webhooks.net latest version

Relevant log output

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[github-actions]

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

[justinmchase]

I was able to work around this with the following:

public override WebhookEvent DeserializeWebhookEvent(WebhookHeaders headers, string body)
{
  return headers.Event switch
  {
    "deployment_protection_rule" => JsonSerializer.Deserialize<DeploymentProtectionRuleRequestedEvent>(body),
    _ => base.DeserializeWebhookEvent(headers, body)
  };
}

protected override async Task ProcessDeployProtectionRuleWebhookAsync(
  WebhookHeaders headers,
  DeploymentProtectionRuleEvent deploymentProtectionRuleEvent,
  DeploymentProtectionRuleAction action
)
{
  if (deploymentProtectionRuleEvent is DeploymentProtectionRuleRequestedEvent deploymentProtectionRuleRequestedEvent)
  {
    await ProcessDeployProtectionRuleRequestedAsync(deploymentProtectionRuleRequestedEvent);
  }
}

protected async Task ProcessDeployProtectionRuleRequestedAsync(
  DeploymentProtectionRuleRequestedEvent evt
)
{
  // todo: implement...
}

[JamieMagee]

I don't think this is a bug. The pattern we're following is that we have a common abstract class that contains all the common properties between each different event action type, and each concrete action type only has the specific properties for the action.

So DeploymentProtectionRuleEvent is the abstract base class for the deployment_protection_rule event, and DeploymentProtectionRuleRequestedEvent is the concrete implementation for the requested action. It looks like in this case, all of the properties are on the concrete implementation. But I suspect that's because it's the only action for that event. We don't know what's common until we have 2 actions for an event.

I think your workaround is not so much a workaround, but how I would expect the SDK to be used. All you should need is:

protected override async Task ProcessDeployProtectionRuleWebhookAsync(
  WebhookHeaders headers,
  DeploymentProtectionRuleEvent deploymentProtectionRuleEvent,
  DeploymentProtectionRuleAction action
)
{
  if (deploymentProtectionRuleEvent is DeploymentProtectionRuleRequestedEvent deploymentProtectionRuleRequestedEvent)
  {
    // do whatever here
  }
}

I think we could also move the properties from DeploymentProtectionRuleRequestedEvent to DeploymentProtectionRuleEvent, and re-evaluate what is actually common when another action is added for this event.