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.

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

#947 base event payload #976

Merged
merged 3 commits into from
Nov 17, 2020

Conversation

seregamorph
Copy link
Contributor

Description

** Address issue #947: move common fields to base class GHEventPayload **
Reference: Webhook payload object common properties

Before submitting a PR:

We love getting PRs, but we hate asking people for the same basic changes every time.

  • Push your changes to a branch other than master. Create your PR from that branch.
  • Add JavaDocs and other comments
  • Write tests that run and pass in CI. See CONTRIBUTING.md for details on how to capture snapshot data.
  • Run mvn clean compile locally. This may reformat your code, commit those changes.
  • Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.

When creating a PR:

  • Fill in the "Description" above.
  • Enable "Allow edits from maintainers".

<groupId>org.slf4j</groupId>
<artifactId>slf4j-simple</artifactId>
<version>1.7.2</version>
<scope>test</scope>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to avoid this message on test execution:

SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.

It is out of scope, but not sure if this deserves separate PR.

@@ -16,14 +16,30 @@
* and payloads</a>
*/
@SuppressWarnings("UnusedDeclaration")
public abstract class GHEventPayload {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pay attention: class made non-abstract. Reasoning: it allow to instantiate common event object:

GitHub.offline()
                .parseEventPayload(new StringReader(payload), GHEventPayload.class)

Why this may be needed: if you want to validate the X-Hub-Signature before handling the payload, you may need common information like repository to resolve the secret.
Possible workaround in case if the class is still abstract: make an empty subclass (in your local project, but in org.kohsuke.github package) and instantiate it.
Possible incompatibility: not expected.

protected GitHub root;

// https://docs.github.com/en/free-pro-team@latest/developers/webhooks-and-events/webhook-events-and-payloads#webhook-payload-object-common-properties
// Webhook payload object common properties: action, sender, repository, organization, installation
private String action;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as declared in #947 moved from subclasses. Field may be null depending on the event type

void wrapUp(GitHub root) {
this.root = root;
if (sender != null) {
sender.wrapUp(root);
}
if (repository != null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

aggregated from overriden wrapUp methods and a getter with side effect (see below) because of fields lift

* @return the repository
*/
public GHRepository getRepository() {
repository.root = root;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced with wrapUp on the super-class

private GHUser sender;
GHRepository repository;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

some fields are package visible, because subclasses work with them directly. Considerable: make private, replace with accessors

Copy link
Member

Choose a reason for hiding this comment

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

@seregamorph
Yes, please make these private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

private GHPullRequestReview review;
private GHPullRequest pull_request;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: snake_case replaced with camelCase, mapper is set up to force snake_case, so it was redundant

@Override
void wrapUp(GitHub root) {
super.wrapUp(root);
if (repository != null)
repository.wrap(root);
if (organization != null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to superclass

@@ -232,6 +242,7 @@ public void pull_request_labeled() throws Exception {
assertThat(event.getPullRequest().getAdditions(), is(137));
assertThat(event.getPullRequest().getDeletions(), is(81));
assertThat(event.getPullRequest().getChangedFiles(), is(22));
assertThat(event.getPullRequest().getLabels().iterator().next().getName(), is("Ready for Review"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test for already addressed #943

@@ -0,0 +1,149 @@
{
"zen": "Design for failure.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the payload got from real requests (only some usernames were changed)

Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

Excellent! Theres some additional cleanup needed, but other than that this is looking great!

Thanks for contributing!

private GHUser sender;
GHRepository repository;
Copy link
Member

Choose a reason for hiding this comment

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

@seregamorph
Yes, please make these private.


GHEventPayload() {
}

/**
* Most webhook payloads contain an action property that contains the specific activity that triggered the event.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Most webhook payloads contain an action property that contains the specific activity that triggered the event.
* Gets the action for the trigged event.
*
* Most but not all webhook payloads contain an action property that contains the specific activity that triggered the event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done (typo in suggestion fixed)

Comment on lines 1240 to 1242
void wrapUp(GitHub root) {
super.wrapUp(root);
repository.wrap(root);
Copy link
Member

Choose a reason for hiding this comment

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

The super handles the repository.wrap(), right?

Suggested change
void wrapUp(GitHub root) {
super.wrapUp(root);
repository.wrap(root);
void wrapUp(GitHub root) {
super.wrapUp(root);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. the inherited method removed (contains only a call to super)

if (repository != null) {
repository.wrap(root);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

inherited

throw new IllegalStateException(
"Expected check_suite payload, but got something else. Maybe we've got another type of event?");
else
installation.wrapUp(root);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

inherited

*/
public GHRepository getRepository() {
return repository;
pullRequest.root = root;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW this logic kept, but this line looks redundant (see wrapUp)

}

@Override
void wrapUp(GitHub root) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fully inherited - hence removed

@@ -112,11 +113,14 @@ void wrapUp(GitHub root) {
sender.wrapUp(root);
}
if (repository != null) {
repository.root = root;
repository.wrap(root);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: repository.root = root; replaced with repository.wrap(root);, a more complex. This way we can inherit this action in subclasses and remove repository.wrap(root); line from them.

@bitwiseman bitwiseman merged commit 8ababb6 into hub4j:master Nov 17, 2020
@seregamorph seregamorph deleted the feature/base-event-payload branch November 18, 2020 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants