#947 base event payload by seregamorph · Pull Request #976 · hub4j/github-api · GitHub

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".

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.

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.

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

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

replaced with wrapUp on the super-class

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

@seregamorph
Yes, please make these private.

done

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

moved to superclass

test for already addressed #943

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

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

Thanks for contributing!

@seregamorph
Yes, please make these private.

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.

done (typo in suggestion fixed)

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);

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

…payload

inherited

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

fully inherited - hence removed

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.

hub4j#947 base event payload

929d9fb

seregamorph commented Nov 9, 2020

View reviewed changes

seregamorph mentioned this pull request Nov 9, 2020

Base event payload fields in GHEventPayload #947

Closed

bitwiseman requested changes Nov 12, 2020

View reviewed changes

seregamorph added 2 commits November 13, 2020 10:27

Merge remote-tracking branch 'origin/master' into feature/base-event-…

74496d3

…payload

hub4j#947 address review comments

b51d655

seregamorph commented Nov 13, 2020

View reviewed changes

bitwiseman approved these changes Nov 17, 2020

View reviewed changes

bitwiseman merged commit 8ababb6 into hub4j:master Nov 17, 2020

seregamorph deleted the feature/base-event-payload branch November 18, 2020 15:25

+                    <groupId>org.slf4j</groupId>
+                    <artifactId>slf4j-simple</artifactId>
+                    <version>1.7.2</version>
+                    <scope>test</scope>

                *      and payloads</a>
                */
               @SuppressWarnings("UnusedDeclaration")
-              public abstract class GHEventPayload {

-                       * @return the repository
-                       */
-                      public GHRepository getRepository() {
-                          repository.root = root;

+                  // Webhook payload object common properties: action, sender, repository, organization, installation
+                  private String action;
                   private GHUser sender;
+                  GHRepository repository;

                   public static class PullRequestReview extends GHEventPayload {
-                      private String action;
                       private GHPullRequestReview review;
-                      private GHPullRequest pull_request;

                       if (sender != null) {
                           sender.wrapUp(root);
                       }
+                      if (repository != null) {

                       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"));

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

-                       */
-                      public GHRepository getRepository() {
-                          return repository;
+                          pullRequest.root = root;

		@@ -0,0 +1,149 @@
		{
		"zen": "Design for failure.",

#947 base event payload by seregamorph · Pull Request #976 · hub4j/github-api · GitHub

Uh oh!

#947 base event payload - #976

#947 base event payload#976bitwiseman merged 3 commits intohub4j:masterhub4j/github-api:masterfrom seregamorph:feature/base-event-payloadseregamorph/github-api:feature/base-event-payloadCopy head branch name to clipboard

Conversation

seregamorph commented Nov 9, 2020

Description

Before submitting a PR:

When creating a PR:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitwiseman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

#947 base event payload#976
bitwiseman merged 3 commits into
hub4j:masterfrom
seregamorph:feature/base-event-payload