From cfaa0c44e2d87ff287146f45392eef2b93db451d Mon Sep 17 00:00:00 2001 From: Allan Burdajewicz Date: Tue, 2 May 2023 21:23:03 +1000 Subject: [PATCH 1/2] [JENKINS-60738] Fix global configuration submission from UI --- .../github/config/GitHubPluginConfig.java | 28 ++++++++++++++++--- .../config/GitHubPluginConfig/config.groovy | 3 +- .../jenkins/GlobalConfigSubmitTest.java | 25 ++++------------- 3 files changed, 32 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java b/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java index f3bb9304f..b8f7a211e 100644 --- a/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java +++ b/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java @@ -4,6 +4,7 @@ import com.google.common.base.Function; import com.google.common.base.Predicate; import com.google.common.base.Predicates; +import hudson.BulkChange; import hudson.Extension; import hudson.Util; import hudson.XmlFile; @@ -190,17 +191,36 @@ protected XmlFile getConfigFile() { @Override public boolean configure(StaplerRequest req, JSONObject json) throws FormException { - hookSecretConfigs = null; // form binding might omit empty lists try { - req.bindJSON(this, json); + BulkChange bc = new BulkChange(this); + try { + if (json.has("configs")) { + setConfigs(req.bindJSONToList(GitHubServerConfig.class, json.getJSONObject("configs"))); + } else { + setConfigs(Collections.emptyList()); + } + if (json.has("hookSecretConfigs")) { + setHookSecretConfigs(req.bindJSONToList(HookSecretConfig.class, + json.getJSONObject("hookSecretConfigs"))); + } else { + setHookSecretConfigs(Collections.emptyList()); + } + if (json.optBoolean("isOverrideHookUrl", false) && (json.has("hookUrl"))) { + setHookUrl(json.optString("hookUrl")); + } else { + setHookUrl(null); + } + req.bindJSON(this, json); + clearRedundantCaches(configs); + } finally { + bc.commit(); + } } catch (Exception e) { LOGGER.debug("Problem while submitting form for GitHub Plugin ({})", e.getMessage(), e); LOGGER.trace("GH form data: {}", json.toString()); throw new FormException( format("Malformed GitHub Plugin configuration (%s)", e.getMessage()), e, "github-configuration"); } - save(); - clearRedundantCaches(configs); return true; } diff --git a/src/main/resources/org/jenkinsci/plugins/github/config/GitHubPluginConfig/config.groovy b/src/main/resources/org/jenkinsci/plugins/github/config/GitHubPluginConfig/config.groovy index fdc8fad55..96077fbb5 100644 --- a/src/main/resources/org/jenkinsci/plugins/github/config/GitHubPluginConfig/config.groovy +++ b/src/main/resources/org/jenkinsci/plugins/github/config/GitHubPluginConfig/config.groovy @@ -27,10 +27,11 @@ f.section(title: descriptor.displayName) { f.entry(title: _("Override Hook URL")) { g.blockWrapper { f.optionalBlock(title: _("Specify another hook URL for GitHub configuration"), + name: "isOverrideHookUrl", inline: true, checked: instance.isOverrideHookUrl()) { f.entry(field: "hookUrl") { - f.textbox(checkMethod: "post") + f.textbox(checkMethod: "post", name: "hookUrl") } } } diff --git a/src/test/java/com/cloudbees/jenkins/GlobalConfigSubmitTest.java b/src/test/java/com/cloudbees/jenkins/GlobalConfigSubmitTest.java index 847268cf3..cf8320b58 100644 --- a/src/test/java/com/cloudbees/jenkins/GlobalConfigSubmitTest.java +++ b/src/test/java/com/cloudbees/jenkins/GlobalConfigSubmitTest.java @@ -3,7 +3,6 @@ import com.gargoylesoftware.htmlunit.html.HtmlForm; import com.gargoylesoftware.htmlunit.html.HtmlPage; import org.jenkinsci.plugins.github.GitHubPlugin; -import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; @@ -14,17 +13,18 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.startsWith; /** * Test Class for {@link GitHubPushTrigger}. * * @author Seiji Sogabe */ -@Ignore("Have troubles with memory consumption") +//@Ignore("Have troubles with memory consumption") public class GlobalConfigSubmitTest { - public static final String OVERRIDE_HOOK_URL_CHECKBOX = "_.isOverrideHookUrl"; - public static final String HOOK_URL_INPUT = "_.hookUrl"; + public static final String OVERRIDE_HOOK_URL_CHECKBOX = "isOverrideHookUrl"; + public static final String HOOK_URL_INPUT = "hookUrl"; private static final String WEBHOOK_URL = "http://jenkinsci.example.com/jenkins/github-webhook/"; @@ -43,7 +43,7 @@ public void shouldSetHookUrl() throws Exception { } @Test - public void shouldNotSetHookUrl() throws Exception { + public void shouldResetHookUrlIfNotChecked() throws Exception { GitHubPlugin.configuration().setHookUrl(WEBHOOK_URL); HtmlForm form = globalConfig(); @@ -52,20 +52,7 @@ public void shouldNotSetHookUrl() throws Exception { form.getInputByName(HOOK_URL_INPUT).setValueAttribute("http://foo"); jenkins.submit(form); - assertThat(GitHubPlugin.configuration().getHookUrl(), equalTo(new URL(WEBHOOK_URL))); - } - - @Test - public void shouldNotOverrideAPreviousHookUrlIfNotChecked() throws Exception { - GitHubPlugin.configuration().setHookUrl(WEBHOOK_URL); - - HtmlForm form = globalConfig(); - - form.getInputByName(OVERRIDE_HOOK_URL_CHECKBOX).setChecked(false); - form.getInputByName(HOOK_URL_INPUT).setValueAttribute(""); - jenkins.submit(form); - - assertThat(GitHubPlugin.configuration().getHookUrl(), equalTo(new URL(WEBHOOK_URL))); + assertThat(GitHubPlugin.configuration().getHookUrl().toString(), startsWith(jenkins.jenkins.getRootUrl())); } public HtmlForm globalConfig() throws IOException, SAXException { From 3a5d46fa1f0725cf68b0e9cbde26249cd2c9d146 Mon Sep 17 00:00:00 2001 From: Allan Burdajewicz Date: Wed, 3 May 2023 10:43:42 +1000 Subject: [PATCH 2/2] [JENKINS-60738] Fix json binding for arrays --- .../jenkinsci/plugins/github/config/GitHubPluginConfig.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java b/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java index b8f7a211e..a0a82a164 100644 --- a/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java +++ b/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java @@ -195,13 +195,12 @@ public boolean configure(StaplerRequest req, JSONObject json) throws FormExcepti BulkChange bc = new BulkChange(this); try { if (json.has("configs")) { - setConfigs(req.bindJSONToList(GitHubServerConfig.class, json.getJSONObject("configs"))); + setConfigs(req.bindJSONToList(GitHubServerConfig.class, json.get("configs"))); } else { setConfigs(Collections.emptyList()); } if (json.has("hookSecretConfigs")) { - setHookSecretConfigs(req.bindJSONToList(HookSecretConfig.class, - json.getJSONObject("hookSecretConfigs"))); + setHookSecretConfigs(req.bindJSONToList(HookSecretConfig.class, json.get("hookSecretConfigs"))); } else { setHookSecretConfigs(Collections.emptyList()); }