Add offline support to the API to make parsing events easier #306

stephenc · 2016-11-08T12:58:01Z

When we receive events from a webhook, it is non-trivial to determine which GitHub instance the event came from
or for that matter even if the event actually came from GitHub or GitHub Enterprise.
In order to ensure that the logic for parsing events does not get replicated in clients, we need to be
able to call GitHub.parseEventPayload(Reader,Class) without knowing which GitHub the event originates from
and without the resulting objects triggering API calls back to a GitHub
Thus we add GitHub.offline() to provide an off-line connection
Thus we modify some of the object classes to return best-effort objects when off-line
Add support for more of the event types into GHEventPayload
Add tests of the event payload and accessing critical fields when using GitHub.offline()

@reviewbybees

- When we receive events from a webhook, it is non-trivial to determine which GitHub instance the event came from or for that matter even if the event actually came from GitHub or GitHub Enterprise. - In order to ensure that the logic for parsing events does not get replicated in clients, we need to be able to call GitHub.parseEventPayload(Reader,Class) without knowing which GitHub the event originates from and without the resulting objects triggering API calls back to a GitHub - Thus we add GitHub.offline() to provide an off-line connection - Thus we modify some of the object classes to return best-effort objects when off-line - Add support for more of the event types into GHEventPayload - Add tests of the event payload and accessing critical fields when using GitHub.offline()

ghost · 2016-11-08T13:01:38Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

jglick

🐝 (assuming a downstream PR is in the making)

jglick · 2016-11-08T15:17:35Z

src/main/java/org/kohsuke/github/GHIssueComment.java


    private String body, gravatar_id;
-    private GHUser user;
+    private GHUser user; // not fully populated. beware.


Seems like this sort of comment would better be placed in the Javadoc of the getter.

yep... just copying existing code style

jglick · 2016-11-08T15:18:20Z

src/main/java/org/kohsuke/github/GitHub.java

        return new GitHubBuilder().withEndpoint(apiUrl).build();
    }

+    /**


offline-only

jglick · 2016-11-08T15:22:21Z

src/test/java/org/kohsuke/github/PayloadRule.java

+            return IOUtils.toByteArray(input);
+        } finally {
+            IOUtils.closeQuietly(input);
+        }


try (InputStream input = asInputStream()) { return IOUtils.toByteArray(input); }

Java 5 (yes WAT!)

jglick · 2016-11-08T15:22:56Z

src/test/java/org/kohsuke/github/PayloadRule.java

+    }
+
+    public Reader asReader() throws FileNotFoundException {
+        return new InputStreamReader(asInputStream(), Charset.defaultCharset());


Since this is being used for JSON, I would think UTF-8 would be more appropriate.

jglick · 2016-11-08T15:24:39Z

src/test/resources/org/kohsuke/github/GHEventPayloadTest/delete.json

+    "avatar_url": "https://avatars.githubusercontent.com/u/6752317?v=3",
+    "gravatar_id": "",
+    "url": "https://api.github.com/users/baxterthehacker",
+    "html_url": "https://github.com/baxterthehacker",


Who are you really @baxterthehacker? :-)

These are the samples from github's api docs

jglick · 2016-11-08T15:25:43Z

src/test/java/org/kohsuke/github/PayloadRule.java

+/**
+ * @author Stephen Connolly
+ */
+public class PayloadRule implements TestRule {


Not going to use jenkinsci/jenkins-test-harness#39?

This is a kohsuke project not a jenkins project (would be nice if it was something we could release without KK)

…b-plugin

jglick

re:bee:

lanwen · 2016-11-08T18:41:20Z

src/main/java/org/kohsuke/github/GHEventPayload.java

+     * @see <a href="http://developer.github.com/v3/activity/events/types/#commitcommentevent">authoritative source</a>
+     */
+    @SuppressFBWarnings(value = {"UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR", "NP_UNWRITTEN_FIELD" },
+            justification = "Constructed by JSON deserialization")


maybe its better to place this classes in separate files?

it would have been better if the design was originally that way... but needs to be in same package due to current API design (FTR I would not have designed this API this way as it is very tricky to unpick the stuff that requires a connection from the stuff that doesn't... but we are where we are)

lanwen · 2016-11-08T18:44:20Z

src/test/java/org/kohsuke/github/GHEventPayloadTest.java

+    public void pull_request() throws Exception {
+        GHEventPayload.PullRequest event =
+                GitHub.offline().parseEventPayload(payload.asReader(), GHEventPayload.PullRequest.class);
+        assertThat(event.getAction(), is("opened"));


you can create expected object and use haveSameProperties matcher

That runs the risk of following requests for the partial objects that are not available from the event objects. safer to just test the paths

oleg-nenashev

👍 Should be a nice improvement. And 🐝

oleg-nenashev · 2016-11-08T17:38:55Z

src/main/java/org/kohsuke/github/GHEventPayload.java

+     * Gets the sender or {@code null} if accessed via the events API.
+     * @return the sender or {@code null} if accessed via the events API.
+     */
+    public GHUser getSender() {


CheckForNull. Nobody reads Javadoc

But generally applies to all deserialized fields :(

oleg-nenashev · 2016-11-09T09:54:00Z

src/main/java/org/kohsuke/github/GHEventPayload.java

            return before;
        }

+        @JsonSetter // alias


Maybe it worth to add getter as well. Just to avoid confusing people

It's private just using this as an alias field name... the documented sample event payload says "after" the documented event says "head" and my tests say "head" but the alias is here as a "just in case"

… multibranch projects

stephenc added 3 commits November 8, 2016 15:04

Fix findbugs false alarms

5b92d4b

Ensure a use case required by github-plugin is valid

ce3f742

Need the pusher details for github-plugin

7bf8621

jglick approved these changes Nov 8, 2016

View reviewed changes

stephenc added 2 commits November 8, 2016 15:26

Seems there is an undocumented but important PING event used by githu…

66145e1

…b-plugin

More details emerge on the PingEvent payload

7dc620a

stephenc mentioned this pull request Nov 8, 2016

[JENKINS-39590] Switch to GitHub.parseEventPayload for event parsing jenkinsci/github-plugin#155

Merged

Typos spotted by Jesse

498d63e

stephenc force-pushed the offline-support branch from 210b1f7 to 498d63e Compare November 8, 2016 16:04

jglick approved these changes Nov 8, 2016

View reviewed changes

lanwen reviewed Nov 8, 2016

View reviewed changes

oleg-nenashev approved these changes Nov 9, 2016

View reviewed changes

stephenc added 3 commits November 11, 2016 14:18

Need to be able to tell if this is a creation / deletion of a ref for…

d36e145

… multibranch projects

Add some more tests

9988a09

Fix the push event payload

e544c7a

kohsuke merged commit d1378a0 into hub4j:master Nov 17, 2016

jglick mentioned this pull request Dec 16, 2016

Fix syntactically malformed test JSON #323

Merged

stephenc deleted the offline-support branch November 1, 2017 15:50

jenkins-infra-bot mentioned this pull request Dec 5, 2017

[JENKINS-39590] Switch to GitHub.parseEventPayload for event parsing jenkinsci/github-plugin#609

Closed

Add offline support to the API to make parsing events easier #306

Add offline support to the API to make parsing events easier #306

Uh oh!

Conversation

stephenc commented Nov 8, 2016

Uh oh!

ghost commented Nov 8, 2016

Uh oh!

jglick left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jglick 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!

oleg-nenashev 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jglick left a comment •

edited

Loading