Handle GWT line numbers in legacy and non-legacy diff routes
Rewriting GWT style line numbers was introduced in If57d28bb3c, before
project URLs were supported. When project URL support was added, the
line numbers rewriting logic was preserved only in the legacy (non
project relative) diff route, thereby breaking linenum links generated
by the GWT UI after it gained project relative URL support.
Because the project relative diff route handler did not expect such line
numbers, they would be interpreted as part of the file path, resulting
in requesting a nonexistent file.
With this change, GWT style line number rewriting is moved into its own
route handler, which works on both diff route forms.
Bug: Issue 7671
Change-Id: I5fb43a85300cf3883c2b372a6a3a582a41f16b4c
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 aefc3b9..8a59fa6 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.js
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.js
@@ -107,9 +107,15 @@
// eslint-disable-next-line max-len
DIFF_EDIT: /^\/c\/(.+)\/\+\/(\d+)\/edit\/(.+),edit$/,
- // Matches /c/<changeNum>/[<basePatchNum>..]<patchNum>/<path>.
+ // Matches non-project-relative
+ // /c/<changeNum>/[<basePatchNum>..]<patchNum>/<path>.
DIFF_LEGACY: /^\/c\/(\d+)\/((-?\d+|edit)(\.\.(\d+|edit))?)\/(.+)/,
+ // Matches diff routes using @\d+ to specify a file name (whether or not
+ // the project name is included).
+ // eslint-disable-next-line max-len
+ DIFF_LEGACY_LINENUM: /^\/c\/((.+)\/\+\/)?(\d+)(\/?((-?\d+|edit)(\.\.(\d+|edit))?\/(.+))?)@[ab]?\d+$/,
+
SETTINGS: /^\/settings\/?/,
SETTINGS_LEGACY: /^\/settings\/VE\/(\S+)/,
@@ -137,6 +143,11 @@
*/
const QUESTION_PATTERN = /^\?*/;
+ /**
+ * GWT UI would use @\d+ at the end of a path to indicate linenum.
+ */
+ const LEGACY_LINENUM_PATTERN = /@([ab]?\d+)$/;
+
// Polymer makes `app` intrinsically defined on the window by virtue of the
// custom element having the id "app", but it is made explicit here.
const app = document.querySelector('#app');
@@ -601,6 +612,8 @@
this._mapRoute(RoutePattern.QUERY, '_handleQueryRoute');
+ this._mapRoute(RoutePattern.DIFF_LEGACY_LINENUM, '_handleLegacyLinenum');
+
this._mapRoute(RoutePattern.CHANGE_NUMBER_LEGACY,
'_handleChangeNumberLegacyRoute');
@@ -1048,15 +1061,11 @@
this._normalizeLegacyRouteParams(params);
},
- _handleDiffLegacyRoute(ctx) {
- // Check if path has an '@' which indicates it was using GWT style line
- // numbers. Even if the filename had an '@' in it, it would have already
- // been URI encoded. Redirect to hash version of path.
- if (ctx.path.includes('@')) {
- this._redirect(ctx.path.replace('@', '#'));
- return;
- }
+ _handleLegacyLinenum(ctx) {
+ this._redirect(ctx.path.replace(LEGACY_LINENUM_PATTERN, '#$1'));
+ },
+ _handleDiffLegacyRoute(ctx) {
// Parameter order is based on the regex group number matched.
const params = {
changeNum: ctx.params[0],
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 c2b46f8..12d9dea 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
@@ -153,6 +153,7 @@
'_handleDefaultRoute',
'_handleChangeLegacyRoute',
'_handleDiffLegacyRoute',
+ '_handleLegacyLinenum',
'_handleImproperlyEncodedPlusRoute',
'_handlePassThroughRoute',
'_handleProjectAccessRoute',
@@ -1204,12 +1205,17 @@
});
});
- test('_handleDiffLegacyRoute w/ @', () => {
- const normalizeRouteStub = sandbox.stub(element,
- '_normalizeLegacyRouteParams');
+ test('_handleLegacyLinenum w/ @321', () => {
+ const ctx = {path: '/c/1234/3..8/foo/bar@321'};
+ element._handleLegacyLinenum(ctx);
+ assert.isTrue(redirectStub.calledOnce);
+ assert.isTrue(redirectStub.calledWithExactly(
+ '/c/1234/3..8/foo/bar#321'));
+ });
+
+ test('_handleLegacyLinenum w/ @b123', () => {
const ctx = {path: '/c/1234/3..8/foo/bar@b123'};
- element._handleDiffLegacyRoute(ctx);
- assert.isFalse(normalizeRouteStub.called);
+ element._handleLegacyLinenum(ctx);
assert.isTrue(redirectStub.calledOnce);
assert.isTrue(redirectStub.calledWithExactly(
'/c/1234/3..8/foo/bar#b123'));
