Repository navigation
Description
GitHub event spec declares common properties: action, sender, repository, organization, installation. But current implementation of GHEventPayload has only one common field sender, other fields declared in subclasses. Most probably because it's based on real life examples. E.g. common field action is not present in event push, that's true.
But it raises a problem. When you implement webhook handler (e.g. webmvc controller) that validates payload signature (header X-Hub-Signature), you need to resolve the secret first. In trivial case secret is the same for an instance of controller, but if it depends on repository (or even on event type) it may differ.
Imagine the method that resolves secret:
/**
* Get secret to validate payload signature by repository and action.
*
* @return secret or empty in case when validation may be omitted
*/
Optional<String> getSecret(GHRepository repository, String action);
To call this method you should repeat extraction on each type of payload. Like this:
... postHook(
@RequestHeader("X-Hub-Signature") signature: String,
@RequestHeader("X-GitHub-Event") event: String,
@RequestBody payload: String
) {
if ("push".equals(event)) {
Push push = GitHub.offline().parseEventPayload(new StringReader(payload), Push.class);
validateSignature(push.getRepository(), null /*action*/, payload, signature);
return handlePush(push);
} else if ("pull_request".equals(event)) {
...
}
}
There are few ways to address it:
- Introduce base event in the controller project (not the library) that has all required fields (workaround):
package org.kohsuke.github;
@Getter
public class BaseEventPayload extends GHEventPayload {
private String action;
private GHRepository repository;
}
- Move all mentioned fields and getters into
GHEventPayload
It has a problem: it will be not that clear where the field/getter is nullable - Move only getters to
GHEventPayload(likeabstract getRepository()), almost the same as option above - Introduce interfaces like
public interface EventRepository {
GHRepository getRepository();
}
and mark event classes where applicable.
If this problem is addressed - there should be an enhancement: you can have a method that parses an event and validates it at once. Later the common class to handle events (some kind of http servlet doGet, doPost, etc.) may be suggested on base of this change.
What do you guys think about it?
Activity
bitwiseman commentedon Nov 5, 2020
@seregamorph
I like this option. Unclear nullability is a bug/feature of the underlying API. I don't think it is a hard requirement of this library to try to mitigate that.
However, there are any number of ways to mitigate this.
senderis already in the base, just leave it. Foraction, we can provide reasonable default values when the field isnull-push.getAction()always returns"push". Forrepository,organization,installation, they are always nullable and that's fine.This sounds great. Please submit a PR.
seregamorph commentedon Nov 6, 2020
Ok, will try to make it, thanks.
hub4j#947 base event payload
hub4j#947 base event payload
seregamorph commentedon Nov 9, 2020
@bitwiseman #976
hub4j#947 address review comments
Merge pull request #976 from seregamorph/feature/base-event-payload