Skip to content

[JENKINS-39590] Switch to GitHub.parseEventPayload for event parsing #155

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.

Sign up for GitHub

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

Merged
lanwen merged 2 commits into from
Jan 12, 2017
Merged

[JENKINS-39590] Switch to GitHub.parseEventPayload for event parsing #155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c5db21
[FIXED JENKINS-39590] Switch to `GitHub.parseEventPayload` for event …
stephenc Nov 8, 2016
a685508
[JENKINS-39590] Narrow the exception handling to payload parsing
stephenc Jan 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>github-api</artifactId>
<version>1.69</version>
<version>1.80</version>
</dependency>

<dependency>
Expand Down
33 changes: 22 additions & 11 deletions ...in/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,14 @@
import hudson.Extension;
import hudson.model.Item;
import hudson.security.ACL;
import java.io.IOException;
import java.io.StringReader;
import java.net.URL;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import org.jenkinsci.plugins.github.extension.GHEventsSubscriber;
import org.kohsuke.github.GHEvent;
import org.kohsuke.github.GHEventPayload;
import org.kohsuke.github.GitHub;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -61,12 +65,17 @@ protected Set<GHEvent> events() {
*/
@Override
protected void onEvent(GHEvent event, String payload) {
JSONObject json = JSONObject.fromObject(payload);
String repoUrl = json.getJSONObject("repository").getString("url");
final String pusherName = json.getJSONObject("pusher").getString("name");

LOGGER.info("Received POST for {}", repoUrl);
final GitHubRepositoryName changedRepository = GitHubRepositoryName.create(repoUrl);
GHEventPayload.Push push;
try {
push = GitHub.offline().parseEventPayload(new StringReader(payload), GHEventPayload.Push.class);
} catch (IOException e) {
LOGGER.warn("Received malformed PushEvent: " + payload, e);
return;
}
URL repoUrl = push.getRepository().getUrl();
final String pusherName = push.getPusher().getName();
LOGGER.info("Received PushEvent for {}", repoUrl);
final GitHubRepositoryName changedRepository = GitHubRepositoryName.create(repoUrl.toExternalForm());

if (changedRepository != null) {
// run in high privilege to see all the projects anonymous users don't see.
Expand All @@ -78,13 +87,15 @@ public void run() {
for (Item job : Jenkins.getInstance().getAllItems(Item.class)) {
GitHubTrigger trigger = triggerFrom(job, GitHubPushTrigger.class);
if (trigger != null) {
LOGGER.debug("Considering to poke {}", job.getFullDisplayName());
if (GitHubRepositoryNameContributor.parseAssociatedNames(job).contains(changedRepository)) {
LOGGER.info("Poked {}", job.getFullDisplayName());
String fullDisplayName = job.getFullDisplayName();
LOGGER.debug("Considering to poke {}", fullDisplayName);
if (GitHubRepositoryNameContributor.parseAssociatedNames(job)
.contains(changedRepository)) {
Copy link
Member

@jglick jglick Jan 12, 2017

Choose a reason for hiding this comment

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

Please avoid gratuitous reformatting of otherwise unmodified lines.

LOGGER.info("Poked {}", fullDisplayName);
trigger.onPost(pusherName);
} else {
LOGGER.debug("Skipped {} because it doesn't have a matching repository.",
job.getFullDisplayName());
fullDisplayName);
}
}
}
Expand Down
32 changes: 20 additions & 12 deletions src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/PingGHEventSubscriber.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,21 @@
import com.cloudbees.jenkins.GitHubRepositoryName;
import hudson.Extension;
import hudson.model.Item;
import net.sf.json.JSONObject;
import java.io.IOException;
import java.io.StringReader;
import java.util.Set;
import javax.inject.Inject;
import org.jenkinsci.plugins.github.admin.GitHubHookRegisterProblemMonitor;
import org.jenkinsci.plugins.github.extension.GHEventsSubscriber;
import org.kohsuke.github.GHEvent;
import org.kohsuke.github.GHEventPayload;
import org.kohsuke.github.GHOrganization;
import org.kohsuke.github.GHRepository;
import org.kohsuke.github.GitHub;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.inject.Inject;
import java.util.Set;

import static com.google.common.collect.Sets.immutableEnumSet;
import static net.sf.json.JSONObject.fromObject;
import static org.kohsuke.github.GHEvent.PING;

/**
Expand All @@ -35,7 +38,6 @@ public class PingGHEventSubscriber extends GHEventsSubscriber {
* This subscriber is not applicable to any item
*
* @param project ignored
*
* @return always false
*/
@Override
Expand All @@ -59,15 +61,21 @@ protected Set<GHEvent> events() {
*/
@Override
protected void onEvent(GHEvent event, String payload) {
Copy link
Member

@KostyaSha KostyaSha Jan 9, 2017

Choose a reason for hiding this comment

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

Does onEvent report errors itself? Then catch not needed.

Copy link
Member

@KostyaSha KostyaSha Jan 9, 2017

Choose a reason for hiding this comment

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

github-plugin/src/main/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriber.java

Lines 208 to 214 in 93d4069

try {
subscriber.onEvent(event, payload);
} catch (Throwable t) {
LOGGER.error("Subscriber {} failed to process {} hook, skipping...",
subscriber.getClass().getName(), event, t);
}
return null;

so try-catch useless wrapper, undo change and check that parser reports meaningful error.

Copy link
Member Author

@stephenc stephenc Jan 9, 2017

Choose a reason for hiding this comment

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

That Throwable catch should be the last ditch catch. Propagation to it is not a good pattern.

I'll see if I can decipher what you mean tomorrow on computer not phone. But in general you should catch and log as close and as specific as possible while you know what sense to make...

Copy link
Member

@KostyaSha KostyaSha Jan 9, 2017

Choose a reason for hiding this comment

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

In just you can trust to any underlying extension as it may throw NPE, so catching throwables and reporting is standard save ass pattern. If exception throws meaningful information then additional exception steps doesn't add anything useful for debugging.

JSONObject parsedPayload = fromObject(payload);
JSONObject repository = parsedPayload.optJSONObject("repository");
GHEventPayload.Ping ping;
try {
ping = GitHub.offline().parseEventPayload(new StringReader(payload), GHEventPayload.Ping.class);
} catch (IOException e) {
LOGGER.warn("Received malformed PingEvent: " + payload, e);
return;
}
GHRepository repository = ping.getRepository();
if (repository != null) {
LOGGER.info("{} webhook received from repo <{}>!", event, repository.getString("html_url"));
monitor.resolveProblem(GitHubRepositoryName.create(repository.getString("html_url")));
LOGGER.info("{} webhook received from repo <{}>!", event, repository.getHtmlUrl());
monitor.resolveProblem(GitHubRepositoryName.create(repository.getHtmlUrl().toExternalForm()));
} else {
JSONObject organization = parsedPayload.optJSONObject("organization");
GHOrganization organization = ping.getOrganization();
if (organization != null) {
LOGGER.info("{} webhook received from org <{}>!", event, organization.getString("url"));
LOGGER.info("{} webhook received from org <{}>!", event, organization.getUrl());
} else {
LOGGER.warn("{} webhook received with unexpected payload", event);
}
Expand Down