Skip to content

Commit

Permalink
[Fix JENKINS-28792] GitLab browser edit panel allow more values for v…
Browse files Browse the repository at this point in the history 
…ersion

When saving a job defined to use the GitLab browser, if the version field
is empty or contains a value which cannot be converted to a double,
the save operation fails with a stack trace that the Double.valueOf()
threw a NumberFormatException.

Silently use a default value if the user provided value cannot be
converted to a Java double.
  • Loading branch information
@MarkEWaite
MarkEWaite committed Jun 9, 2015
1 parent 0c1a59a commit 2962ecc
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 45 deletions.
24 changes: 18 additions & 6 deletions src/main/java/hudson/plugins/git/browser/GitLab.java
View file
Expand Up @@ -11,7 +11,6 @@
import org.kohsuke.stapler.StaplerRequest;

import java.io.IOException;
import java.net.MalformedURLException;
import java.net.URL;

/**
Expand All @@ -22,11 +21,26 @@ public class GitLab extends GitRepositoryBrowser {
private static final long serialVersionUID = 1L;

private final double version;

/* package */
static final double DEFAULT_VERSION = 7.11;

@DataBoundConstructor
public GitLab(String repoUrl, String version) {
super(repoUrl);
this.version = Double.valueOf(version);
double tmpVersion;
try {
tmpVersion = Double.valueOf(version);
if (tmpVersion < 0
|| tmpVersion > DEFAULT_VERSION

This comment has been minimized.

Sign in to view

Copy link
@MarkEWaite

MarkEWaite Jun 15, 2015

Author Contributor

This line (checking tmpVersion is not greater than DEFAULT_VERSION) introduces a compatibility change. Previously, a valid version larger than the default version (like 9.87) was accepted and retained. With this line, a valid version larger than the default version is accepted, but silently forced to the default version.

A fix for that compatibility change is being developed in my forked copy of the repo.

The logic in the class is unharmed by that change from valid larger version to default version, but users will be needlessly perplexed at the change of their data from the value they entered to some other, seemingly unrelated value.
|| Double.isNaN(tmpVersion)
|| Double.isInfinite(tmpVersion)) {
tmpVersion = DEFAULT_VERSION;
}
} catch (NumberFormatException nfe) {
tmpVersion = DEFAULT_VERSION;
}
this.version = tmpVersion;
}

public double getVersion() {
Expand All @@ -44,9 +58,7 @@ public double getVersion() {
*/
@Override
public URL getChangeSetLink(GitChangeSet changeSet) throws IOException {
String commitPrefix;

return new URL(getUrl(), calculatePrefix() + changeSet.getId().toString());
return new URL(getUrl(), calculatePrefix() + changeSet.getId());
}

/**
Expand All @@ -62,7 +74,7 @@ public URL getChangeSetLink(GitChangeSet changeSet) throws IOException {
@Override
public URL getDiffLink(Path path) throws IOException {
final GitChangeSet changeSet = path.getChangeSet();
return new URL(getUrl(), calculatePrefix() + changeSet.getId().toString() + "#" + path.getPath());
return new URL(getUrl(), calculatePrefix() + changeSet.getId() + "#" + path.getPath());
}

/**
Expand Down
106 changes: 67 additions & 39 deletions src/test/java/hudson/plugins/git/browser/GitLabTest.java
View file
Expand Up @@ -7,7 +7,6 @@

import java.io.File;
import java.io.IOException;
import java.net.URL;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
Expand All @@ -23,30 +22,52 @@ public class GitLabTest extends TestCase {
private final GitLab gitlab42 = new GitLab(GITLAB_URL, "4.2");
private final GitLab gitlab50 = new GitLab(GITLAB_URL, "5.0");
private final GitLab gitlab51 = new GitLab(GITLAB_URL, "5.1");
private final GitLab gitlab711 = new GitLab(GITLAB_URL, "7.11");
private final GitLab gitlab7114ee = new GitLab(GITLAB_URL, "7.11.4.ee");
private final GitLab gitlabDefault = new GitLab(GITLAB_URL, "");
private final GitLab gitlabNaN = new GitLab(GITLAB_URL, "NaN");
private final GitLab gitlabInfinity = new GitLab(GITLAB_URL, "Infinity");
private final GitLab gitlabNegative = new GitLab(GITLAB_URL, "-1");
private final GitLab gitlabGreater = new GitLab(GITLAB_URL, "9999");

private final String SHA1 = "396fc230a3db05c427737aa5c2eb7856ba72b05d";
private final String fileName = "src/main/java/hudson/plugins/git/browser/GithubWeb.java";

/**
* Test method for {@link hudson.plugins.git.browser.GitLab#getVersion()}.
*/
public void testGetVersion() {
assertEquals(gitlab29.getVersion(), 2.9);
assertEquals(gitlab42.getVersion(), 4.2);
assertEquals(gitlab50.getVersion(), 5.0);
assertEquals(gitlab51.getVersion(), 5.1);
assertEquals(2.9, gitlab29.getVersion());
assertEquals(4.2, gitlab42.getVersion());
assertEquals(5.0, gitlab50.getVersion());
assertEquals(5.1, gitlab51.getVersion());
assertEquals(GitLab.DEFAULT_VERSION, gitlab711.getVersion());
assertEquals(GitLab.DEFAULT_VERSION, gitlab7114ee.getVersion());
assertEquals(GitLab.DEFAULT_VERSION, gitlabDefault.getVersion());
assertEquals(GitLab.DEFAULT_VERSION, gitlabNaN.getVersion());
assertEquals(GitLab.DEFAULT_VERSION, gitlabInfinity.getVersion());
assertEquals(GitLab.DEFAULT_VERSION, gitlabNegative.getVersion());
assertEquals(GitLab.DEFAULT_VERSION, gitlabGreater.getVersion());
}

/**
* Test method for {@link hudson.plugins.git.browser.GitLab#getChangeSetLink(hudson.plugins.git.GitChangeSet)}.
* @throws IOException
*/
public void testGetChangeSetLinkGitChangeSet() throws IOException, SAXException {
assertEquals(GITLAB_URL + "commits/396fc230a3db05c427737aa5c2eb7856ba72b05d",
gitlab29.getChangeSetLink(createChangeSet("rawchangelog")).toString());
assertEquals(GITLAB_URL + "commit/396fc230a3db05c427737aa5c2eb7856ba72b05d",
gitlab42.getChangeSetLink(createChangeSet("rawchangelog")).toString());
assertEquals(GITLAB_URL + "commit/396fc230a3db05c427737aa5c2eb7856ba72b05d",
gitlab50.getChangeSetLink(createChangeSet("rawchangelog")).toString());
assertEquals(GITLAB_URL + "commit/396fc230a3db05c427737aa5c2eb7856ba72b05d",
gitlab51.getChangeSetLink(createChangeSet("rawchangelog")).toString());
final GitChangeSet changeSet = createChangeSet("rawchangelog");
final String expectedURL = GITLAB_URL + "commit/" + SHA1;
assertEquals(expectedURL.replace("commit/", "commits/"), gitlab29.getChangeSetLink(changeSet).toString());
assertEquals(expectedURL, gitlab42.getChangeSetLink(changeSet).toString());
assertEquals(expectedURL, gitlab50.getChangeSetLink(changeSet).toString());
assertEquals(expectedURL, gitlab51.getChangeSetLink(changeSet).toString());
assertEquals(expectedURL, gitlab711.getChangeSetLink(changeSet).toString());
assertEquals(expectedURL, gitlab7114ee.getChangeSetLink(changeSet).toString());
assertEquals(expectedURL, gitlabDefault.getChangeSetLink(changeSet).toString());
assertEquals(expectedURL, gitlabNaN.getChangeSetLink(changeSet).toString());
assertEquals(expectedURL, gitlabInfinity.getChangeSetLink(changeSet).toString());
assertEquals(expectedURL, gitlabNegative.getChangeSetLink(changeSet).toString());
assertEquals(expectedURL, gitlabGreater.getChangeSetLink(changeSet).toString());
}

/**
Expand All @@ -55,15 +76,19 @@ public void testGetChangeSetLinkGitChangeSet() throws IOException, SAXException
*/
public void testGetDiffLinkPath() throws IOException, SAXException {
final HashMap<String, Path> pathMap = createPathMap("rawchangelog");
final Path modified1 = pathMap.get("src/main/java/hudson/plugins/git/browser/GithubWeb.java");
assertEquals(GITLAB_URL + "commits/396fc230a3db05c427737aa5c2eb7856ba72b05d#src/main/java/hudson/plugins/git/browser/GithubWeb.java",
gitlab29.getDiffLink(modified1).toString());
assertEquals(GITLAB_URL + "commit/396fc230a3db05c427737aa5c2eb7856ba72b05d#src/main/java/hudson/plugins/git/browser/GithubWeb.java",
gitlab42.getDiffLink(modified1).toString());
assertEquals(GITLAB_URL + "commit/396fc230a3db05c427737aa5c2eb7856ba72b05d#src/main/java/hudson/plugins/git/browser/GithubWeb.java",
gitlab50.getDiffLink(modified1).toString());
assertEquals(GITLAB_URL + "commit/396fc230a3db05c427737aa5c2eb7856ba72b05d#src/main/java/hudson/plugins/git/browser/GithubWeb.java",
gitlab51.getDiffLink(modified1).toString());
final Path modified1 = pathMap.get(fileName);
final String expectedURL = GITLAB_URL + "commit/" + SHA1 + "#" + fileName;
assertEquals(expectedURL.replace("commit/", "commits/"), gitlab29.getDiffLink(modified1).toString());
assertEquals(expectedURL, gitlab42.getDiffLink(modified1).toString());
assertEquals(expectedURL, gitlab50.getDiffLink(modified1).toString());
assertEquals(expectedURL, gitlab51.getDiffLink(modified1).toString());
assertEquals(expectedURL, gitlab711.getDiffLink(modified1).toString());
assertEquals(expectedURL, gitlab7114ee.getDiffLink(modified1).toString());
assertEquals(expectedURL, gitlabDefault.getDiffLink(modified1).toString());
assertEquals(expectedURL, gitlabNaN.getDiffLink(modified1).toString());
assertEquals(expectedURL, gitlabInfinity.getDiffLink(modified1).toString());
assertEquals(expectedURL, gitlabNegative.getDiffLink(modified1).toString());
assertEquals(expectedURL, gitlabGreater.getDiffLink(modified1).toString());
}

/**
Expand All @@ -72,15 +97,21 @@ public void testGetDiffLinkPath() throws IOException, SAXException {
*/
public void testGetFileLinkPath() throws IOException, SAXException {
final HashMap<String,Path> pathMap = createPathMap("rawchangelog");
final Path path = pathMap.get("src/main/java/hudson/plugins/git/browser/GithubWeb.java");
assertEquals(GITLAB_URL + "tree/396fc230a3db05c427737aa5c2eb7856ba72b05d/src/main/java/hudson/plugins/git/browser/GithubWeb.java",
gitlab29.getFileLink(path).toString());
assertEquals(GITLAB_URL + "tree/396fc230a3db05c427737aa5c2eb7856ba72b05d/src/main/java/hudson/plugins/git/browser/GithubWeb.java",
gitlab42.getFileLink(path).toString());
assertEquals(GITLAB_URL + "396fc230a3db05c427737aa5c2eb7856ba72b05d/tree/src/main/java/hudson/plugins/git/browser/GithubWeb.java",
gitlab50.getFileLink(path).toString());
assertEquals(GITLAB_URL + "blob/396fc230a3db05c427737aa5c2eb7856ba72b05d/src/main/java/hudson/plugins/git/browser/GithubWeb.java",
gitlab51.getFileLink(path).toString());
final Path path = pathMap.get(fileName);
final String expectedURL = GITLAB_URL + "blob/396fc230a3db05c427737aa5c2eb7856ba72b05d/" + fileName;
final String expectedV29 = expectedURL.replace("blob/", "tree/");
final String expectedV50 = GITLAB_URL + "396fc230a3db05c427737aa5c2eb7856ba72b05d/tree/" + fileName;
assertEquals(expectedV29, gitlab29.getFileLink(path).toString());
assertEquals(expectedV29, gitlab42.getFileLink(path).toString());
assertEquals(expectedV50, gitlab50.getFileLink(path).toString());
assertEquals(expectedURL, gitlab51.getFileLink(path).toString());
assertEquals(expectedURL, gitlab711.getFileLink(path).toString());
assertEquals(expectedURL, gitlab7114ee.getFileLink(path).toString());
assertEquals(expectedURL, gitlabDefault.getFileLink(path).toString());
assertEquals(expectedURL, gitlabNaN.getFileLink(path).toString());
assertEquals(expectedURL, gitlabInfinity.getFileLink(path).toString());
assertEquals(expectedURL, gitlabNegative.getFileLink(path).toString());
assertEquals(expectedURL, gitlabGreater.getFileLink(path).toString());
}

/**
Expand All @@ -90,14 +121,11 @@ public void testGetFileLinkPath() throws IOException, SAXException {
public void testGetFileLinkPathForDeletedFile() throws IOException, SAXException {
final HashMap<String,Path> pathMap = createPathMap("rawchangelog-with-deleted-file");
final Path path = pathMap.get("bar");
assertEquals(GITLAB_URL + "commits/fc029da233f161c65eb06d0f1ed4f36ae81d1f4f#bar",
gitlab29.getFileLink(path).toString());
assertEquals(GITLAB_URL + "commit/fc029da233f161c65eb06d0f1ed4f36ae81d1f4f#bar",
gitlab42.getFileLink(path).toString());
assertEquals(GITLAB_URL + "commit/fc029da233f161c65eb06d0f1ed4f36ae81d1f4f#bar",
gitlab50.getFileLink(path).toString());
assertEquals(GITLAB_URL + "commit/fc029da233f161c65eb06d0f1ed4f36ae81d1f4f#bar",
gitlab51.getFileLink(path).toString());
final String expectedURL = GITLAB_URL + "commit/fc029da233f161c65eb06d0f1ed4f36ae81d1f4f#bar";
assertEquals(expectedURL.replace("commit/", "commits/"), gitlab29.getFileLink(path).toString());
assertEquals(expectedURL, gitlab42.getFileLink(path).toString());
assertEquals(expectedURL, gitlab50.getFileLink(path).toString());
assertEquals(expectedURL, gitlab51.getFileLink(path).toString());
}

private GitChangeSet createChangeSet(String rawchangelogpath) throws IOException, SAXException {
Expand Down

0 comments on commit 2962ecc

Please sign in to comment.