Skip to content

Commit ea95434

Browse files
jglickjglick
committed
[FIXED JENKINS-19310] Provide correct links for build history inside a folder.
The basic fix is to use ${rootURL} plus full model object URLs rather than relying on ${jobBaseUrl}. This is made trickier by the fact that the model object URLs are computed inside ProgressiveRendering.compute, and therefore will not be correct when nondefault views are in the crumb trail unless the original request information is present. So modifying ProgressiveRendering to preserve a copy of the original request for use during computation. (This could probably be used to simplify parts of AsynchPeople as well.) Also improving AbstractItem.getUrl to properly construct a URL including views even when the current page is not inside the item; it should suffice for some ancestor of the current item (or a view thereof) to be in the ancestor list of this page.
1 parent 00ab601 commit ea95434

File tree

10 files changed

+132
-42
lines changed

10 files changed

+132
-42
lines changed

‎changelog.html

+3
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@
6464
<li class=bug>
6565
Incorrect redirect after deleting a folder.
6666
(<a href="https://issues.jenkins-ci.org/browse/JENKINS-23375">issue 23375</a>)
67+
<li class=bug>
68+
Incorrect links from Build History page inside a folder.
69+
(<a href="https://issues.jenkins-ci.org/browse/JENKINS-19310">issue 19310</a>)
6770
<li class=rfe>
6871
API changes allowing new job types to use SCM plugins.
6972
(<a href="https://issues.jenkins-ci.org/browse/JENKINS-23365">issue 23365</a>)

‎core/src/main/java/hudson/model/AbstractItem.java

+32-2
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,8 @@
5454
import java.util.Collection;
5555
import java.util.List;
5656
import java.util.ListIterator;
57+
import java.util.logging.Level;
58+
import java.util.logging.Logger;
5759
import javax.annotation.Nonnull;
5860

5961
import org.kohsuke.stapler.StaplerRequest;
@@ -84,6 +86,9 @@
8486
// Java doesn't let multiple inheritance.
8587
@ExportedBean
8688
public abstract class AbstractItem extends Actionable implements Item, HttpDeletable, AccessControlled, DescriptorByNameOwner {
89+
90+
private static final Logger LOGGER = Logger.getLogger(AbstractItem.class.getName());
91+
8792
/**
8893
* Project name.
8994
*/
@@ -395,16 +400,41 @@ public void onCopiedFrom(Item src) {
395400
public final String getUrl() {
396401
// try to stick to the current view if possible
397402
StaplerRequest req = Stapler.getCurrentRequest();
403+
String shortUrl = getShortUrl();
404+
String uri = req == null ? null : req.getRequestURI();
398405
if (req != null) {
399406
String seed = Functions.getNearestAncestorUrl(req,this);
407+
LOGGER.log(Level.FINER, "seed={0} for {1} from {2}", new Object[] {seed, this, uri});
400408
if(seed!=null) {
401409
// trim off the context path portion and leading '/', but add trailing '/'
402410
return seed.substring(req.getContextPath().length()+1)+'/';
403411
}
412+
List<Ancestor> ancestors = req.getAncestors();
413+
if (!ancestors.isEmpty()) {
414+
Ancestor last = ancestors.get(ancestors.size() - 1);
415+
if (last.getObject() instanceof View) {
416+
View view = (View) last.getObject();
417+
if (view.getOwnerItemGroup() == getParent() && !view.isDefault()) {
418+
// Showing something inside a view, so should use that as the base URL.
419+
String base = last.getUrl().substring(req.getContextPath().length() + 1) + '/';
420+
LOGGER.log(Level.FINER, "using {0}{1} for {2} from {3}", new Object[] {base, shortUrl, this, uri});
421+
return base + shortUrl;
422+
} else {
423+
LOGGER.log(Level.FINER, "irrelevant {0} for {1} from {2}", new Object[] {last.getObject(), this, uri});
424+
}
425+
} else {
426+
LOGGER.log(Level.FINER, "inapplicable {0} for {1} from {2}", new Object[] {last.getObject(), this, uri});
427+
}
428+
} else {
429+
LOGGER.log(Level.FINER, "no ancestors for {0} from {1}", new Object[] {this, uri});
430+
}
431+
} else {
432+
LOGGER.log(Level.FINER, "no current request for {0}", this);
404433
}
405-
406434
// otherwise compute the path normally
407-
return getParent().getUrl()+getShortUrl();
435+
String base = getParent().getUrl();
436+
LOGGER.log(Level.FINER, "falling back to {0}{1} for {2} from {3}", new Object[] {base, shortUrl, this, uri});
437+
return base + shortUrl;
408438
}
409439

410440
public String getShortUrl() {

‎core/src/main/java/jenkins/model/Jenkins.java

-20
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,6 @@
180180
import hudson.util.NamingThreadFactory;
181181
import hudson.util.RemotingDiagnostics;
182182
import hudson.util.RemotingDiagnostics.HeapDump;
183-
import hudson.util.StreamTaskListener;
184183
import hudson.util.TextFile;
185184
import hudson.util.TimeUnit2;
186185
import hudson.util.VersionNumber;
@@ -223,7 +222,6 @@
223222
import org.kohsuke.accmod.restrictions.NoExternalUse;
224223
import org.kohsuke.args4j.Argument;
225224
import org.kohsuke.args4j.Option;
226-
import org.kohsuke.stapler.Ancestor;
227225
import org.kohsuke.stapler.HttpRedirect;
228226
import org.kohsuke.stapler.HttpResponse;
229227
import org.kohsuke.stapler.HttpResponses;
@@ -666,24 +664,6 @@ protected void add(TopLevelItem item) {
666664
protected File getRootDirFor(String name) {
667665
return Jenkins.this.getRootDirFor(name);
668666
}
669-
670-
/**
671-
* Send the browser to the config page.
672-
* use View to trim view/{default-view} from URL if possible
673-
*/
674-
@Override
675-
protected String redirectAfterCreateItem(StaplerRequest req, TopLevelItem result) throws IOException {
676-
String redirect = result.getUrl()+"configure";
677-
List<Ancestor> ancestors = req.getAncestors();
678-
for (int i = ancestors.size() - 1; i >= 0; i--) {
679-
Object o = ancestors.get(i).getObject();
680-
if (o instanceof View) {
681-
redirect = req.getContextPath() + '/' + ((View)o).getUrl() + redirect;
682-
break;
683-
}
684-
}
685-
return redirect;
686-
}
687667
};
688668

689669

‎core/src/main/java/jenkins/util/ProgressiveRendering.java

+70-5
Original file line numberDiff line numberDiff line change
@@ -25,16 +25,28 @@
2525
package jenkins.util;
2626

2727
import edu.umd.cs.findbugs.annotations.SuppressWarnings;
28+
import hudson.model.AbstractItem;
29+
import java.lang.reflect.Field;
30+
import java.lang.reflect.InvocationHandler;
31+
import java.lang.reflect.Method;
32+
import java.lang.reflect.Proxy;
33+
import java.util.HashMap;
34+
import java.util.List;
35+
import java.util.Locale;
36+
import java.util.Map;
2837
import java.util.concurrent.ExecutorService;
2938
import java.util.logging.Level;
3039
import java.util.logging.Logger;
3140
import javax.annotation.Nonnull;
41+
import javax.servlet.http.HttpServletRequest;
3242
import net.sf.json.JSON;
3343
import net.sf.json.JSONObject;
3444
import org.acegisecurity.context.SecurityContext;
3545
import org.acegisecurity.context.SecurityContextHolder;
46+
import org.kohsuke.stapler.Ancestor;
47+
import org.kohsuke.stapler.RequestImpl;
3648
import org.kohsuke.stapler.Stapler;
37-
import org.kohsuke.stapler.StaplerRequest;
49+
import org.kohsuke.stapler.TokenList;
3850
import org.kohsuke.stapler.bind.JavaScriptMethod;
3951

4052
/**
@@ -68,13 +80,11 @@ public abstract class ProgressiveRendering {
6880
private double status = 0;
6981
private long lastNewsTime;
7082
/** just for logging */
71-
private final String uri;
83+
private String uri;
7284
private long start;
7385

7486
/** Constructor for subclasses. */
7587
protected ProgressiveRendering() {
76-
StaplerRequest currentRequest = Stapler.getCurrentRequest();
77-
uri = currentRequest != null ? currentRequest.getRequestURI() : "?";
7888
}
7989

8090
/**
@@ -83,9 +93,12 @@ protected ProgressiveRendering() {
8393
@SuppressWarnings("RV_RETURN_VALUE_IGNORED_BAD_PRACTICE")
8494
public final void start() {
8595
final SecurityContext securityContext = SecurityContextHolder.getContext();
96+
final RequestImpl request = createMockRequest();
97+
uri = request.getRequestURI();
8698
executorService().submit(new Runnable() {
87-
public void run() {
99+
@Override public void run() {
88100
lastNewsTime = start = System.currentTimeMillis();
101+
setCurrentRequest(request);
89102
SecurityContext orig = SecurityContextHolder.getContext();
90103
try {
91104
SecurityContextHolder.setContext(securityContext);
@@ -98,15 +111,67 @@ public void run() {
98111
status = ERROR;
99112
} finally {
100113
SecurityContextHolder.setContext(orig);
114+
setCurrentRequest(null);
101115
LOG.log(Level.FINE, "{0} finished in {1}msec with status {2}", new Object[] {uri, System.currentTimeMillis() - start, status});
102116
}
103117
}
104118
});
105119
}
106120

121+
/**
122+
* Copies important fields from the current HTTP request and makes them available during {@link #compute}.
123+
* This is necessary because some model methods such as {@link AbstractItem#getUrl} behave differently when called from a request.
124+
*/
125+
@java.lang.SuppressWarnings({"rawtypes", "unchecked"}) // public RequestImpl ctor requires List<AncestorImpl> yet AncestorImpl is not public! API design flaw
126+
private static RequestImpl createMockRequest() {
127+
RequestImpl currentRequest = (RequestImpl) Stapler.getCurrentRequest();
128+
HttpServletRequest original = (HttpServletRequest) currentRequest.getRequest();
129+
final Map<String,Object> getters = new HashMap<String,Object>();
130+
for (Method method : HttpServletRequest.class.getMethods()) {
131+
String m = method.getName();
132+
if ((m.startsWith("get") || m.startsWith("is")) && method.getParameterTypes().length == 0) {
133+
Class<?> type = method.getReturnType();
134+
// TODO could add other types which are known to be safe to copy: Cookie[], Principal, HttpSession, etc.
135+
if (type.isPrimitive() || type == String.class || type == Locale.class) {
136+
try {
137+
getters.put(m, method.invoke(original));
138+
} catch (Exception x) {
139+
LOG.log(Level.WARNING, "cannot mock Stapler request " + method, x);
140+
}
141+
}
142+
}
143+
}
144+
List/*<AncestorImpl>*/ ancestors = currentRequest.ancestors;
145+
LOG.log(Level.FINE, "mocking ancestors {0} using {1}", new Object[] {ancestors, getters});
146+
TokenList tokens = currentRequest.tokens;
147+
return new RequestImpl(Stapler.getCurrent(), (HttpServletRequest) Proxy.newProxyInstance(ProgressiveRendering.class.getClassLoader(), new Class<?>[] {HttpServletRequest.class}, new InvocationHandler() {
148+
@Override public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
149+
String m = method.getName();
150+
if (getters.containsKey(m)) {
151+
return getters.get(m);
152+
} else { // TODO implement other methods as needed
153+
throw new UnsupportedOperationException(m);
154+
}
155+
}
156+
}), ancestors, tokens);
157+
}
158+
159+
@java.lang.SuppressWarnings("unchecked")
160+
private static void setCurrentRequest(RequestImpl request) {
161+
try {
162+
Field field = Stapler.class.getDeclaredField("CURRENT_REQUEST");
163+
field.setAccessible(true);
164+
((ThreadLocal<RequestImpl>) field.get(null)).set(request);
165+
} catch (Exception x) {
166+
LOG.log(Level.WARNING, "cannot mock Stapler request", x);
167+
}
168+
}
169+
107170
/**
108171
* Actually do the work.
109172
* <p>The security context will be that in effect when the web request was made.
173+
* {@link Stapler#getCurrentRequest} will also be similar to that in effect when the web request was made;
174+
* at least, {@link Ancestor}s and basic request properties (URI, locale, and so on) will be available.
110175
* @throws Exception whenever you like; the progress bar will indicate that an error occurred but details go to the log only
111176
*/
112177
protected abstract void compute() throws Exception;

‎core/src/main/resources/hudson/model/Computer/builds.jelly

+1-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ THE SOFTWARE.
3535
<st:include page="control.jelly" it="${it.timeline}" />
3636
<div style="height:2em"/><!-- spacer -->
3737

38-
<t:buildListTable builds="${it.builds}" jobBaseUrl="${rootURL}/" />
38+
<t:buildListTable builds="${it.builds}"/>
3939
</l:main-panel>
4040
</l:layout>
4141
</j:jelly>

‎core/src/main/resources/hudson/model/User/builds.jelly

+1-1
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ THE SOFTWARE.
3131
${%title(it)}
3232
</h1>
3333
<!-- TODO consider adding a BuildTimelineWidget (cf. Job, View, Computer) -->
34-
<t:buildListTable builds="${it.builds}" jobBaseUrl="${rootURL}/" />
34+
<t:buildListTable builds="${it.builds}"/>
3535
</l:main-panel>
3636
</l:layout>
3737
</j:jelly>

‎core/src/main/resources/hudson/model/View/builds.jelly

+1-2
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,7 @@ THE SOFTWARE.
4141
<div>
4242
<a href="cc.xml">${%Export as plain XML}</a>
4343
</div>
44-
<!-- set @jobBaseUrl="" so that links to jobs will be under this view. -->
45-
<t:buildListTable builds="${it.builds}" jobBaseUrl="" />
44+
<t:buildListTable builds="${it.builds}"/>
4645
</l:main-panel>
4746
</l:layout>
4847
</j:jelly>

‎core/src/main/resources/lib/hudson/buildListTable.jelly

+4-7
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,6 @@ THE SOFTWARE.
2929
<st:attribute name="builds" use="required">
3030
A collection of builds to be displayed.
3131
</st:attribute>
32-
<st:attribute name="jobBaseUrl" use="required">
33-
The base URL of all job/build links. Normally ${rootURL}/
34-
</st:attribute>
3532
</st:documentation>
3633

3734
<t:setIconSize/>
@@ -42,20 +39,20 @@ THE SOFTWARE.
4239
var e = data[x];
4340
var tr = new Element('tr');
4441
tr.insert(new Element('td', {data: e.iconColorOrdinal}).
45-
insert(new Element('a', {href: '${jobBaseUrl}' + e.url}).
42+
insert(new Element('a', {href: '${rootURL}/' + e.url}).
4643
insert(new Element('img', {src: '${imagesURL}/${iconSize}/' + e.buildStatusUrl, alt: e.iconColorDescription, 'class': 'icon${iconSize}'}))));
4744
tr.insert(new Element('td').
48-
insert(new Element('a', {href: '${jobBaseUrl}' + e.parentUrl, 'class': 'model-link'}).
45+
insert(new Element('a', {href: '${rootURL}/' + e.parentUrl, 'class': 'model-link'}).
4946
update(e.parentFullDisplayName)).
5047
insert('\u00A0').
51-
insert(new Element('a', {href: '${jobBaseUrl}' + e.url, 'class': 'model-link inside'}).
48+
insert(new Element('a', {href: '${rootURL}/' + e.url, 'class': 'model-link inside'}).
5249
update(e.displayName.escapeHTML())));
5350
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 + '"))'}).
5451
update(e.timestampString.escapeHTML()));
5552
tr.insert(new Element('td', {style: e.buildStatusSummaryWorse ? 'color: red' : ''}).
5653
update(e.buildStatusSummaryMessage.escapeHTML()));
5754
tr.insert(new Element('td').
58-
insert(new Element('a', {href: '${jobBaseUrl}' + e.url + 'console'}).
55+
insert(new Element('a', {href: '${rootURL}/' + e.url + 'console'}).
5956
insert(new Element('img', {src: '${imagesURL}/${subIconSize}/terminal.png', alt: '${%Console output}', border: 0}))));
6057
p.insert(tr);
6158
Behaviour.applySubtree(tr);

‎test/src/test/java/hudson/model/MyViewTest.java

+13-2
Original file line numberDiff line numberDiff line change
@@ -25,15 +25,18 @@
2525

2626
import org.junit.Before;
2727
import org.junit.Test;
28-
import org.jvnet.hudson.test.JenkinsRule.WebClient;
2928
import com.gargoylesoftware.htmlunit.html.HtmlForm;
3029
import hudson.security.GlobalMatrixAuthorizationStrategy;
31-
import hudson.security.HudsonPrivateSecurityRealm;
3230
import java.io.IOException;
31+
import java.util.logging.ConsoleHandler;
32+
import java.util.logging.Handler;
33+
import java.util.logging.Level;
34+
import java.util.logging.Logger;
3335
import org.acegisecurity.context.SecurityContextHolder;
3436
import org.junit.Rule;
3537
import org.jvnet.hudson.test.JenkinsRule;
3638
import static org.junit.Assert.*;
39+
import org.junit.BeforeClass;
3740

3841
/**
3942
*
@@ -49,6 +52,14 @@ public void setup() {
4952
rule.jenkins.setSecurityRealm(rule.createDummySecurityRealm());
5053
}
5154

55+
private static final Logger logger = Logger.getLogger(AbstractItem.class.getName());
56+
@BeforeClass public static void logging() {
57+
logger.setLevel(Level.ALL);
58+
Handler handler = new ConsoleHandler();
59+
handler.setLevel(Level.ALL);
60+
logger.addHandler(handler);
61+
}
62+
5263
@Test
5364
public void testContains() throws IOException, Exception{
5465

‎test/src/test/java/jenkins/widgets/BuildListTableTest.java

+7-2
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232
import java.net.URL;
3333
import org.junit.Test;
3434
import static org.junit.Assert.*;
35-
import org.junit.Ignore;
3635
import org.junit.Rule;
3736
import org.jvnet.hudson.test.Issue;
3837
import org.jvnet.hudson.test.JenkinsRule;
@@ -42,7 +41,6 @@ public class BuildListTableTest {
4241

4342
@Rule public JenkinsRule r = new JenkinsRule();
4443

45-
@Ignore("TODO")
4644
@Issue("JENKINS-19310")
4745
@Test public void linksFromFolders() throws Exception {
4846
MockFolder d = r.createFolder("d");
@@ -62,8 +60,15 @@ public class BuildListTableTest {
6260
HtmlAnchor anchor = page.getAnchorByText("d » d2 » p");
6361
String href = anchor.getHrefAttribute();
6462
URL target = URI.create(page.getDocumentURI()).resolve(href).toURL();
63+
wc.getPage(target);
6564
assertEquals(href, r.getURL() + "view/v1/job/d/view/v2/job/d2/job/p/", target.toString());
65+
page = wc.goTo("job/d/view/All/builds?suppressTimelineControl=true");
66+
assertEquals(0, wc.waitForBackgroundJavaScript(120000));
67+
anchor = page.getAnchorByText("d » d2 » p");
68+
href = anchor.getHrefAttribute();
69+
target = URI.create(page.getDocumentURI()).resolve(href).toURL();
6670
wc.getPage(target);
71+
assertEquals(href, r.getURL() + "job/d/job/d2/job/p/", target.toString());
6772
}
6873

6974
}

0 commit comments

Comments
 (0)