diff --git a/changelog.html b/changelog.html index cfc0e15719a925be9fcf456b8dff358a3155c40f..532960725db9be0ea4d6c2ae61a613d183652ff7 100644 --- a/changelog.html +++ b/changelog.html @@ -67,6 +67,9 @@ Upcoming changes
  • Incorrect redirect after deleting a folder. (issue 23375) +
  • + Incorrect links from Build History page inside a folder. + (issue 19310)
  • API changes allowing new job types to use SCM plugins. (issue 23365) diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java index fcbccad747e611a93a12a5d983d1028a5423958e..d33dd88b729cca6788a236c8d30321599b39ac43 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -54,6 +54,8 @@ import java.io.IOException; import java.util.Collection; import java.util.List; import java.util.ListIterator; +import java.util.logging.Level; +import java.util.logging.Logger; import javax.annotation.Nonnull; import org.kohsuke.stapler.StaplerRequest; @@ -84,6 +86,9 @@ import org.kohsuke.stapler.Ancestor; // Java doesn't let multiple inheritance. @ExportedBean public abstract class AbstractItem extends Actionable implements Item, HttpDeletable, AccessControlled, DescriptorByNameOwner { + + private static final Logger LOGGER = Logger.getLogger(AbstractItem.class.getName()); + /** * Project name. */ @@ -395,16 +400,41 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet public final String getUrl() { // try to stick to the current view if possible StaplerRequest req = Stapler.getCurrentRequest(); + String shortUrl = getShortUrl(); + String uri = req == null ? null : req.getRequestURI(); if (req != null) { String seed = Functions.getNearestAncestorUrl(req,this); + LOGGER.log(Level.FINER, "seed={0} for {1} from {2}", new Object[] {seed, this, uri}); if(seed!=null) { // trim off the context path portion and leading '/', but add trailing '/' return seed.substring(req.getContextPath().length()+1)+'/'; } + List ancestors = req.getAncestors(); + if (!ancestors.isEmpty()) { + Ancestor last = ancestors.get(ancestors.size() - 1); + if (last.getObject() instanceof View) { + View view = (View) last.getObject(); + if (view.getOwnerItemGroup() == getParent() && !view.isDefault()) { + // Showing something inside a view, so should use that as the base URL. + String base = last.getUrl().substring(req.getContextPath().length() + 1) + '/'; + LOGGER.log(Level.FINER, "using {0}{1} for {2} from {3}", new Object[] {base, shortUrl, this, uri}); + return base + shortUrl; + } else { + LOGGER.log(Level.FINER, "irrelevant {0} for {1} from {2}", new Object[] {last.getObject(), this, uri}); + } + } else { + LOGGER.log(Level.FINER, "inapplicable {0} for {1} from {2}", new Object[] {last.getObject(), this, uri}); + } + } else { + LOGGER.log(Level.FINER, "no ancestors for {0} from {1}", new Object[] {this, uri}); + } + } else { + LOGGER.log(Level.FINER, "no current request for {0}", this); } - // otherwise compute the path normally - return getParent().getUrl()+getShortUrl(); + String base = getParent().getUrl(); + LOGGER.log(Level.FINER, "falling back to {0}{1} for {2} from {3}", new Object[] {base, shortUrl, this, uri}); + return base + shortUrl; } public String getShortUrl() { diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 139dc5501ce6c773faa37d7205dda7c91511740c..6f11cb8f14404abf3fa67920fd3a57390b2c98d5 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -180,7 +180,6 @@ import hudson.util.MultipartFormDataParser; import hudson.util.NamingThreadFactory; import hudson.util.RemotingDiagnostics; import hudson.util.RemotingDiagnostics.HeapDump; -import hudson.util.StreamTaskListener; import hudson.util.TextFile; import hudson.util.TimeUnit2; import hudson.util.VersionNumber; @@ -223,7 +222,6 @@ import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.Option; -import org.kohsuke.stapler.Ancestor; import org.kohsuke.stapler.HttpRedirect; import org.kohsuke.stapler.HttpResponse; import org.kohsuke.stapler.HttpResponses; @@ -666,24 +664,6 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve protected File getRootDirFor(String name) { return Jenkins.this.getRootDirFor(name); } - - /** - * Send the browser to the config page. - * use View to trim view/{default-view} from URL if possible - */ - @Override - protected String redirectAfterCreateItem(StaplerRequest req, TopLevelItem result) throws IOException { - String redirect = result.getUrl()+"configure"; - List ancestors = req.getAncestors(); - for (int i = ancestors.size() - 1; i >= 0; i--) { - Object o = ancestors.get(i).getObject(); - if (o instanceof View) { - redirect = req.getContextPath() + '/' + ((View)o).getUrl() + redirect; - break; - } - } - return redirect; - } }; diff --git a/core/src/main/java/jenkins/util/ProgressiveRendering.java b/core/src/main/java/jenkins/util/ProgressiveRendering.java index 684197e8d8d3a0783b2061facbe3bbde6cd40f14..8685dd98890fa0d3139ba99ee7e9f5c99a252f31 100644 --- a/core/src/main/java/jenkins/util/ProgressiveRendering.java +++ b/core/src/main/java/jenkins/util/ProgressiveRendering.java @@ -25,16 +25,28 @@ package jenkins.util; import edu.umd.cs.findbugs.annotations.SuppressWarnings; +import hudson.model.AbstractItem; +import java.lang.reflect.Field; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.util.HashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; import java.util.concurrent.ExecutorService; import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.Nonnull; +import javax.servlet.http.HttpServletRequest; import net.sf.json.JSON; import net.sf.json.JSONObject; import org.acegisecurity.context.SecurityContext; import org.acegisecurity.context.SecurityContextHolder; +import org.kohsuke.stapler.Ancestor; +import org.kohsuke.stapler.RequestImpl; import org.kohsuke.stapler.Stapler; -import org.kohsuke.stapler.StaplerRequest; +import org.kohsuke.stapler.TokenList; import org.kohsuke.stapler.bind.JavaScriptMethod; /** @@ -68,13 +80,11 @@ public abstract class ProgressiveRendering { private double status = 0; private long lastNewsTime; /** just for logging */ - private final String uri; + private String uri; private long start; /** Constructor for subclasses. */ protected ProgressiveRendering() { - StaplerRequest currentRequest = Stapler.getCurrentRequest(); - uri = currentRequest != null ? currentRequest.getRequestURI() : "?"; } /** @@ -83,9 +93,12 @@ public abstract class ProgressiveRendering { @SuppressWarnings("RV_RETURN_VALUE_IGNORED_BAD_PRACTICE") public final void start() { final SecurityContext securityContext = SecurityContextHolder.getContext(); + final RequestImpl request = createMockRequest(); + uri = request.getRequestURI(); executorService().submit(new Runnable() { - public void run() { + @Override public void run() { lastNewsTime = start = System.currentTimeMillis(); + setCurrentRequest(request); SecurityContext orig = SecurityContextHolder.getContext(); try { SecurityContextHolder.setContext(securityContext); @@ -98,15 +111,67 @@ public abstract class ProgressiveRendering { status = ERROR; } finally { SecurityContextHolder.setContext(orig); + setCurrentRequest(null); LOG.log(Level.FINE, "{0} finished in {1}msec with status {2}", new Object[] {uri, System.currentTimeMillis() - start, status}); } } }); } + /** + * Copies important fields from the current HTTP request and makes them available during {@link #compute}. + * This is necessary because some model methods such as {@link AbstractItem#getUrl} behave differently when called from a request. + */ + @java.lang.SuppressWarnings({"rawtypes", "unchecked"}) // public RequestImpl ctor requires List yet AncestorImpl is not public! API design flaw + private static RequestImpl createMockRequest() { + RequestImpl currentRequest = (RequestImpl) Stapler.getCurrentRequest(); + HttpServletRequest original = (HttpServletRequest) currentRequest.getRequest(); + final Map getters = new HashMap(); + for (Method method : HttpServletRequest.class.getMethods()) { + String m = method.getName(); + if ((m.startsWith("get") || m.startsWith("is")) && method.getParameterTypes().length == 0) { + Class type = method.getReturnType(); + // TODO could add other types which are known to be safe to copy: Cookie[], Principal, HttpSession, etc. + if (type.isPrimitive() || type == String.class || type == Locale.class) { + try { + getters.put(m, method.invoke(original)); + } catch (Exception x) { + LOG.log(Level.WARNING, "cannot mock Stapler request " + method, x); + } + } + } + } + List/**/ ancestors = currentRequest.ancestors; + LOG.log(Level.FINE, "mocking ancestors {0} using {1}", new Object[] {ancestors, getters}); + TokenList tokens = currentRequest.tokens; + return new RequestImpl(Stapler.getCurrent(), (HttpServletRequest) Proxy.newProxyInstance(ProgressiveRendering.class.getClassLoader(), new Class[] {HttpServletRequest.class}, new InvocationHandler() { + @Override public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + String m = method.getName(); + if (getters.containsKey(m)) { + return getters.get(m); + } else { // TODO implement other methods as needed + throw new UnsupportedOperationException(m); + } + } + }), ancestors, tokens); + } + + @java.lang.SuppressWarnings("unchecked") + private static void setCurrentRequest(RequestImpl request) { + try { + Field field = Stapler.class.getDeclaredField("CURRENT_REQUEST"); + field.setAccessible(true); + ((ThreadLocal) field.get(null)).set(request); + } catch (Exception x) { + LOG.log(Level.WARNING, "cannot mock Stapler request", x); + } + } + /** * Actually do the work. *

    The security context will be that in effect when the web request was made. + * {@link Stapler#getCurrentRequest} will also be similar to that in effect when the web request was made; + * at least, {@link Ancestor}s and basic request properties (URI, locale, and so on) will be available. * @throws Exception whenever you like; the progress bar will indicate that an error occurred but details go to the log only */ protected abstract void compute() throws Exception; diff --git a/core/src/main/resources/hudson/model/Computer/builds.jelly b/core/src/main/resources/hudson/model/Computer/builds.jelly index 208b0c675943ce8a4aabb58be8f598f141273e76..2864dee11c289e22082f135a401c725cfb92bab3 100644 --- a/core/src/main/resources/hudson/model/Computer/builds.jelly +++ b/core/src/main/resources/hudson/model/Computer/builds.jelly @@ -35,7 +35,7 @@ THE SOFTWARE.

    - + diff --git a/core/src/main/resources/hudson/model/User/builds.jelly b/core/src/main/resources/hudson/model/User/builds.jelly index cc4a6f28dc1aecaef19be227ca55f13d113c2548..cae284074bd9e9262b8637fff45374ba41a80822 100644 --- a/core/src/main/resources/hudson/model/User/builds.jelly +++ b/core/src/main/resources/hudson/model/User/builds.jelly @@ -31,7 +31,7 @@ THE SOFTWARE. ${%title(it)} - + diff --git a/core/src/main/resources/hudson/model/View/builds.jelly b/core/src/main/resources/hudson/model/View/builds.jelly index c39729b5149052fea02b56a3c43717dfb9fe16db..a6f094503568d09dba68e0d7ac97c92a32d07f57 100644 --- a/core/src/main/resources/hudson/model/View/builds.jelly +++ b/core/src/main/resources/hudson/model/View/builds.jelly @@ -41,8 +41,7 @@ THE SOFTWARE. - - + diff --git a/core/src/main/resources/lib/hudson/buildListTable.jelly b/core/src/main/resources/lib/hudson/buildListTable.jelly index 17d59a4bef4c4941b362e8eec37142c6681bc44e..80cb00dad047c42afeae97db8ebd5a3bdd1a79b0 100644 --- a/core/src/main/resources/lib/hudson/buildListTable.jelly +++ b/core/src/main/resources/lib/hudson/buildListTable.jelly @@ -29,9 +29,6 @@ THE SOFTWARE. A collection of builds to be displayed. - - The base URL of all job/build links. Normally ${rootURL}/ - @@ -42,20 +39,20 @@ THE SOFTWARE. var e = data[x]; var tr = new Element('tr'); tr.insert(new Element('td', {data: e.iconColorOrdinal}). - insert(new Element('a', {href: '${jobBaseUrl}' + e.url}). + insert(new Element('a', {href: '${rootURL}/' + e.url}). insert(new Element('img', {src: '${imagesURL}/${iconSize}/' + e.buildStatusUrl, alt: e.iconColorDescription, 'class': 'icon${iconSize}'})))); tr.insert(new Element('td'). - insert(new Element('a', {href: '${jobBaseUrl}' + e.parentUrl, 'class': 'model-link'}). + insert(new Element('a', {href: '${rootURL}/' + e.parentUrl, 'class': 'model-link'}). update(e.parentFullDisplayName)). insert('\u00A0'). - insert(new Element('a', {href: '${jobBaseUrl}' + e.url, 'class': 'model-link inside'}). + insert(new Element('a', {href: '${rootURL}/' + e.url, 'class': 'model-link inside'}). update(e.displayName.escapeHTML()))); tr.insert(new Element('td', {data: e.timestampString2, tooltip: '${%Click to center timeline on event}', onclick: 'javascript:tl.getBand(0).scrollToCenter(Timeline.DateTime.parseGregorianDateTime("' + e.timestampString2 + '"))'}). update(e.timestampString.escapeHTML())); tr.insert(new Element('td', {style: e.buildStatusSummaryWorse ? 'color: red' : ''}). update(e.buildStatusSummaryMessage.escapeHTML())); tr.insert(new Element('td'). - insert(new Element('a', {href: '${jobBaseUrl}' + e.url + 'console'}). + insert(new Element('a', {href: '${rootURL}/' + e.url + 'console'}). insert(new Element('img', {src: '${imagesURL}/${subIconSize}/terminal.png', alt: '${%Console output}', border: 0})))); p.insert(tr); Behaviour.applySubtree(tr); diff --git a/test/src/test/java/hudson/model/MyViewTest.java b/test/src/test/java/hudson/model/MyViewTest.java index 3efa4e9ed04843866f5cc6590fed747ba7c201e2..274f8103342cb7a24274b391d4d4c7bf0cb012da 100644 --- a/test/src/test/java/hudson/model/MyViewTest.java +++ b/test/src/test/java/hudson/model/MyViewTest.java @@ -25,15 +25,18 @@ package hudson.model; import org.junit.Before; import org.junit.Test; -import org.jvnet.hudson.test.JenkinsRule.WebClient; import com.gargoylesoftware.htmlunit.html.HtmlForm; import hudson.security.GlobalMatrixAuthorizationStrategy; -import hudson.security.HudsonPrivateSecurityRealm; import java.io.IOException; +import java.util.logging.ConsoleHandler; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.Logger; import org.acegisecurity.context.SecurityContextHolder; import org.junit.Rule; import org.jvnet.hudson.test.JenkinsRule; import static org.junit.Assert.*; +import org.junit.BeforeClass; /** * @@ -49,6 +52,14 @@ public class MyViewTest { rule.jenkins.setSecurityRealm(rule.createDummySecurityRealm()); } + private static final Logger logger = Logger.getLogger(AbstractItem.class.getName()); + @BeforeClass public static void logging() { + logger.setLevel(Level.ALL); + Handler handler = new ConsoleHandler(); + handler.setLevel(Level.ALL); + logger.addHandler(handler); + } + @Test public void testContains() throws IOException, Exception{ diff --git a/test/src/test/java/jenkins/widgets/BuildListTableTest.java b/test/src/test/java/jenkins/widgets/BuildListTableTest.java index 61d79fae32895ebfc257468a54844118529f424c..bfdbaf60d166130d50d58e5b33d7835a8808909b 100644 --- a/test/src/test/java/jenkins/widgets/BuildListTableTest.java +++ b/test/src/test/java/jenkins/widgets/BuildListTableTest.java @@ -32,7 +32,6 @@ import java.net.URI; import java.net.URL; import org.junit.Test; import static org.junit.Assert.*; -import org.junit.Ignore; import org.junit.Rule; import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; @@ -42,7 +41,6 @@ public class BuildListTableTest { @Rule public JenkinsRule r = new JenkinsRule(); - @Ignore("TODO") @Issue("JENKINS-19310") @Test public void linksFromFolders() throws Exception { MockFolder d = r.createFolder("d"); @@ -62,8 +60,15 @@ public class BuildListTableTest { HtmlAnchor anchor = page.getAnchorByText("d » d2 » p"); String href = anchor.getHrefAttribute(); URL target = URI.create(page.getDocumentURI()).resolve(href).toURL(); + wc.getPage(target); assertEquals(href, r.getURL() + "view/v1/job/d/view/v2/job/d2/job/p/", target.toString()); + page = wc.goTo("job/d/view/All/builds?suppressTimelineControl=true"); + assertEquals(0, wc.waitForBackgroundJavaScript(120000)); + anchor = page.getAnchorByText("d » d2 » p"); + href = anchor.getHrefAttribute(); + target = URI.create(page.getDocumentURI()).resolve(href).toURL(); wc.getPage(target); + assertEquals(href, r.getURL() + "job/d/job/d2/job/p/", target.toString()); } }