Show 404 when project lookup fails
Following Ic9c9f0071e, looking up the project of a nonexistent change
would result in a 404 in the getChange REST call. This network failure
would propagate to a visual not-found message automatically and the
router merely needed to stop attempting to redirect when it recognized
the failure.
In I7336ae90d7, the getChange method is updated to use a query because
the former endpoint had been deprecated. However, because the call
became a query, nonexistent changes would yield an empty set rather than
a 404 network error -- consequently, no visual 404 would appear and the
app would appear to hang.
With this change, instead of merely halting when recognizing a project
lookup failure, the router will display a 404.
The new project lookup query is updated to use the change ID operator
query as Ibe82272104 does.
Bug: Issue 8152
Change-Id: I37a816c41485c077d28f051de281225c20ed495b
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
index 8d91ef8..6a7966b 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
@@ -55,7 +55,7 @@
stub('gr-rest-api-interface', {
getConfig() { return Promise.resolve({}); },
getAccount() { return Promise.resolve({}); },
- getChange() { return Promise.resolve([{}]); },
+ getChange() { return Promise.resolve({}); },
getChangeSuggestedReviewers() { return Promise.resolve([]); },
});
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.js b/polygerrit-ui/app/elements/core/gr-router/gr-router.js
index dc264f2..d85b810 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.js
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.js
@@ -498,9 +498,12 @@
return this.$.restAPI.getFromProjectLookup(params.changeNum)
.then(project => {
- // Do nothing if the lookup request failed. This avoids an infinite
- // loop of project lookups.
- if (!project) { return; }
+ // Show a 404 and terminate if the lookup request failed. Attempting
+ // to redirect after failing to get the project loops infinitely.
+ if (!project) {
+ this._show404();
+ return;
+ }
params.project = project;
this._normalizePatchRangeParams(params);
@@ -1333,6 +1336,10 @@
* Catchall route for when no other route is matched.
*/
_handleDefaultRoute() {
+ this._show404();
+ },
+
+ _show404() {
// Note: the app's 404 display is tightly-coupled with catching 404
// network responses, so we simulate a 404 response status to display it.
// TODO: Decouple the gr-app error view from network responses.
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html
index 7011c65..4af8130 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html
@@ -430,11 +430,13 @@
suite('_normalizeLegacyRouteParams', () => {
let rangeStub;
let redirectStub;
+ let show404Stub;
setup(() => {
rangeStub = sandbox.stub(element, '_normalizePatchRangeParams')
.returns(Promise.resolve());
redirectStub = sandbox.stub(element, '_redirect');
+ show404Stub = sandbox.stub(element, '_show404');
});
test('w/o changeNum', () => {
@@ -445,6 +447,7 @@
assert.isFalse(rangeStub.called);
assert.isNotOk(params.project);
assert.isFalse(redirectStub.called);
+ assert.isFalse(show404Stub.called);
});
});
@@ -456,18 +459,19 @@
assert.isTrue(rangeStub.called);
assert.equal(params.project, 'foo/bar');
assert.isTrue(redirectStub.calledOnce);
+ assert.isFalse(show404Stub.called);
});
});
test('halts on project lookup failure', () => {
projectLookupStub.returns(Promise.resolve(undefined));
-
const params = {changeNum: 1234};
return element._normalizeLegacyRouteParams(params).then(() => {
assert.isTrue(projectLookupStub.called);
assert.isFalse(rangeStub.called);
assert.isUndefined(params.project);
assert.isFalse(redirectStub.called);
+ assert.isTrue(show404Stub.calledOnce);
});
});
});
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index b5bb35a..3f19f7e 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -1993,7 +1993,11 @@
*/
getChange(changeNum, opt_errFn) {
// Cannot use _changeBaseURL, as this function is used by _projectLookup.
- return this.fetchJSON(`/changes/?q=${changeNum}`, opt_errFn);
+ return this.fetchJSON(`/changes/?q=change:${changeNum}`, opt_errFn)
+ .then(res => {
+ if (!res || !res.length) { return null; }
+ return res[0];
+ });
},
/**
@@ -2026,8 +2030,7 @@
this.fire('page-error', {response});
};
- return this.getChange(changeNum, onError).then(res => {
- const change = res[0];
+ return this.getChange(changeNum, onError).then(change => {
if (!change || !change.project) { return; }
this.setInProjectLookup(changeNum, change.project);
return change.project;
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
index a8c09d1..33eb181 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
@@ -1036,7 +1036,7 @@
suite('getFromProjectLookup', () => {
test('getChange fails', () => {
sandbox.stub(element, 'getChange')
- .returns(Promise.resolve([]));
+ .returns(Promise.resolve(null));
return element.getFromProjectLookup().then(val => {
assert.strictEqual(val, undefined);
assert.deepEqual(element._projectLookup, {});
@@ -1044,7 +1044,7 @@
});
test('getChange succeeds, no project', () => {
- sandbox.stub(element, 'getChange').returns(Promise.resolve([]));
+ sandbox.stub(element, 'getChange').returns(Promise.resolve(null));
return element.getFromProjectLookup().then(val => {
assert.strictEqual(val, undefined);
assert.deepEqual(element._projectLookup, {});
@@ -1053,7 +1053,7 @@
test('getChange succeeds with project', () => {
sandbox.stub(element, 'getChange')
- .returns(Promise.resolve([{project: 'project'}]));
+ .returns(Promise.resolve({project: 'project'}));
return element.getFromProjectLookup('test').then(val => {
assert.equal(val, 'project');
assert.deepEqual(element._projectLookup, {test: 'project'});
