opendev/gerrit
Code Issues Proposed changes

Merge "Preload REST API calls for change page and diff page"

Browse Source
This commit is contained in:
Milutin Kristofic 2020-02-05 18:14:17 +00:00 committed by Gerrit Code Review
commit d7a2bf53de
9 changed files with 211 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
java/com/google/gerrit/extensions/client/ListOption.java
View File

@ -16,6 +16,7 @@ package com.google.gerrit.extensions.client;
import java.lang.reflect.InvocationTargetException;
import java.util.EnumSet;
import java.util.Set;
/** Enum that can be expressed as a bitset in query parameters. */
public interface ListOption {
@ -46,4 +47,13 @@ public interface ListOption {
}
return r;
}
static String toHex(Set<ListChangesOption> options) {
int v = 0;
for (ListChangesOption option : options) {
v |= 1 << option.getValue();
}
return Integer.toHexString(v);
}
}

83
java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
View File

@ -18,15 +18,20 @@ import static com.google.template.soy.data.ordainers.GsonOrdainer.serializeObjec
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.UsedAt;
import com.google.gerrit.common.UsedAt.Project;
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.api.accounts.AccountApi;
import com.google.gerrit.extensions.api.config.Server;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.ListOption;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.json.OutputFormat;
import com.google.gson.Gson;
import com.google.template.soy.data.SanitizedContent;
@ -34,11 +39,46 @@ import java.net.URI;
import java.net.URISyntaxException;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.function.Function;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
/** Helper for generating parts of {@code index.html}. */
public class IndexHtmlUtil {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
static final String changeCanonicalUrl = ".*/c/(?<project>.+)/\\+/(?<changeNum>\\d+)";
static final String basePatchNumUrlPart = "(/(-?\\d+|edit)(\\.\\.(\\d+|edit))?)";
static final Pattern changeUrlPattern =
Pattern.compile(changeCanonicalUrl + basePatchNumUrlPart + "?" + "/?$");
static final Pattern diffUrlPattern =
Pattern.compile(changeCanonicalUrl + basePatchNumUrlPart + "(/(.+))" + "/?$");
public static String getDefaultChangeDetailHex() {
Set<ListChangesOption> options =
ImmutableSet.of(
ListChangesOption.ALL_COMMITS,
ListChangesOption.ALL_REVISIONS,
ListChangesOption.CHANGE_ACTIONS,
ListChangesOption.DETAILED_LABELS,
ListChangesOption.DOWNLOAD_COMMANDS,
ListChangesOption.MESSAGES,
ListChangesOption.SUBMITTABLE,
ListChangesOption.WEB_LINKS,
ListChangesOption.SKIP_DIFFSTAT);
return ListOption.toHex(options);
}
public static String getDefaultDiffDetailHex() {
Set<ListChangesOption> options =
ImmutableSet.of(
ListChangesOption.ALL_COMMITS,
ListChangesOption.ALL_REVISIONS,
ListChangesOption.SKIP_DIFFSTAT);
return ListOption.toHex(options);
}
/**
* Returns both static and dynamic parameters of {@code index.html}. The result is to be used when
@ -50,12 +90,18 @@ public class IndexHtmlUtil {
String cdnPath,
String faviconPath,
Map<String, String[]> urlParameterMap,
Function<String, SanitizedContent> urlInScriptTagOrdainer)
Function<String, SanitizedContent> urlInScriptTagOrdainer,
String requestedURL)
throws URISyntaxException, RestApiException {
return ImmutableMap.<String, Object>builder()
.putAll(
staticTemplateData(
canonicalURL, cdnPath, faviconPath, urlParameterMap, urlInScriptTagOrdainer))
canonicalURL,
cdnPath,
faviconPath,
urlParameterMap,
urlInScriptTagOrdainer,
requestedURL))
.putAll(dynamicTemplateData(gerritApi))
.build();
}
@ -98,7 +144,8 @@ public class IndexHtmlUtil {
String cdnPath,
String faviconPath,
Map<String, String[]> urlParameterMap,
Function<String, SanitizedContent> urlInScriptTagOrdainer)
Function<String, SanitizedContent> urlInScriptTagOrdainer,
String requestedURL)
throws URISyntaxException {
String canonicalPath = computeCanonicalPath(canonicalURL);
@ -133,9 +180,39 @@ public class IndexHtmlUtil {
if (urlParameterMap.containsKey("gf")) {
data.put("useGoogleFonts", "true");
}
if (urlParameterMap.containsKey("pl") && requestedURL != null) {
data.put("defaultChangeDetailHex", getDefaultChangeDetailHex());
data.put("defaultDiffDetailHex", getDefaultDiffDetailHex());
String changeRequestsPath = computeChangeRequestsPath(requestedURL, changeUrlPattern);
if (changeRequestsPath != null) {
data.put("preloadChangePage", "true");
} else {
changeRequestsPath = computeChangeRequestsPath(requestedURL, diffUrlPattern);
data.put("preloadDiffPage", "true");
}
if (changeRequestsPath != null) {
data.put("changeRequestsPath", changeRequestsPath);
}
}
return data.build();
}
static String computeChangeRequestsPath(String requestedURL, Pattern pattern) {
Matcher matcher = pattern.matcher(requestedURL);
if (matcher.matches()) {
Integer changeId = Ints.tryParse(matcher.group("changeNum"));
if (changeId != null) {
return "changes/" + Url.encode(matcher.group("project")) + "~" + changeId;
}
}
return null;
}
private static String computeCanonicalPath(@Nullable String canonicalURL)
throws URISyntaxException {
if (Strings.isNullOrEmpty(canonicalURL)) {

3
java/com/google/gerrit/httpd/raw/IndexServlet.java
View File

@ -70,10 +70,11 @@ public class IndexServlet extends HttpServlet {
SoySauce.Renderer renderer;
try {
Map<String, String[]> parameterMap = req.getParameterMap();
String requestUrl = req.getRequestURL() == null ? null : req.getRequestURL().toString();
// TODO(hiesel): Remove URL ordainer as parameter once Soy is consistent
ImmutableMap<String, Object> templateData =
IndexHtmlUtil.templateData(
gerritApi, canonicalUrl, cdnPath, faviconPath, parameterMap, urlOrdainer);
gerritApi, canonicalUrl, cdnPath, faviconPath, parameterMap, urlOrdainer, requestUrl);
renderer = soySauce.renderTemplate("com.google.gerrit.httpd.raw.Index").setData(templateData);
} catch (URISyntaxException | RestApiException e) {
throw new IOException(e);

1
javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@ -4637,7 +4637,6 @@ public class ChangeIT extends AbstractDaemonTest {
ListChangesOption.ALL_COMMITS,
ListChangesOption.ALL_REVISIONS,
ListChangesOption.CHANGE_ACTIONS,
ListChangesOption.CURRENT_ACTIONS,
ListChangesOption.DETAILED_LABELS,
ListChangesOption.DOWNLOAD_COMMANDS,
ListChangesOption.MESSAGES,

61
javatests/com/google/gerrit/httpd/raw/IndexHtmlUtilTest.java
View File

@ -15,6 +15,9 @@
package com.google.gerrit.httpd.raw;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.httpd.raw.IndexHtmlUtil.changeUrlPattern;
import static com.google.gerrit.httpd.raw.IndexHtmlUtil.computeChangeRequestsPath;
import static com.google.gerrit.httpd.raw.IndexHtmlUtil.diffUrlPattern;
import static com.google.gerrit.httpd.raw.IndexHtmlUtil.staticTemplateData;
import com.google.template.soy.data.SanitizedContent;
@ -29,7 +32,12 @@ public class IndexHtmlUtilTest {
public void noPathAndNoCDN() throws Exception {
assertThat(
staticTemplateData(
"http://example.com/", null, null, new HashMap<>(), IndexHtmlUtilTest::ordain))
"http://example.com/",
null,
null,
new HashMap<>(),
IndexHtmlUtilTest::ordain,
null))
.containsExactly("canonicalPath", "", "staticResourcePath", ordain(""));
}
@ -41,7 +49,8 @@ public class IndexHtmlUtilTest {
null,
null,
new HashMap<>(),
IndexHtmlUtilTest::ordain))
IndexHtmlUtilTest::ordain,
null))
.containsExactly("canonicalPath", "/gerrit", "staticResourcePath", ordain("/gerrit"));
}
@ -53,7 +62,8 @@ public class IndexHtmlUtilTest {
"http://my-cdn.com/foo/bar/",
null,
new HashMap<>(),
IndexHtmlUtilTest::ordain))
IndexHtmlUtilTest::ordain,
null))
.containsExactly(
"canonicalPath", "", "staticResourcePath", ordain("http://my-cdn.com/foo/bar/"));
}
@ -66,7 +76,8 @@ public class IndexHtmlUtilTest {
"http://my-cdn.com/foo/bar/",
null,
new HashMap<>(),
IndexHtmlUtilTest::ordain))
IndexHtmlUtilTest::ordain,
null))
.containsExactly(
"canonicalPath", "/gerrit", "staticResourcePath", ordain("http://my-cdn.com/foo/bar/"));
}
@ -77,11 +88,51 @@ public class IndexHtmlUtilTest {
urlParms.put("gf", new String[0]);
assertThat(
staticTemplateData(
"http://example.com/", null, null, urlParms, IndexHtmlUtilTest::ordain))
"http://example.com/", null, null, urlParms, IndexHtmlUtilTest::ordain, null))
.containsExactly(
"canonicalPath", "", "staticResourcePath", ordain(""), "useGoogleFonts", "true");
}
@Test
public void usePreloadRest() throws Exception {
Map<String, String[]> urlParms = new HashMap<>();
urlParms.put("pl", new String[0]);
assertThat(
staticTemplateData(
"http://example.com/",
null,
null,
urlParms,
IndexHtmlUtilTest::ordain,
"/c/project/+/123"))
.containsExactly(
"canonicalPath", "",
"staticResourcePath", ordain(""),
"defaultChangeDetailHex", "916314",
"defaultDiffDetailHex", "800014",
"preloadChangePage", "true",
"changeRequestsPath", "changes/project~123");
}
@Test
public void computeChangePath() throws Exception {
assertThat(computeChangeRequestsPath("/c/project/+/123", changeUrlPattern))
.isEqualTo("changes/project~123");
assertThat(computeChangeRequestsPath("/c/project/+/124/2", changeUrlPattern))
.isEqualTo("changes/project~124");
assertThat(computeChangeRequestsPath("/c/project/src/+/23", changeUrlPattern))
.isEqualTo("changes/project%2Fsrc~23");
assertThat(computeChangeRequestsPath("/q/project/src/+/23", changeUrlPattern)).isEqualTo(null);
assertThat(computeChangeRequestsPath("/c/Scripts/+/232/1//COMMIT_MSG", changeUrlPattern))
.isEqualTo(null);
assertThat(computeChangeRequestsPath("/c/Scripts/+/232/1//COMMIT_MSG", diffUrlPattern))
.isEqualTo("changes/Scripts~232");
}
private static SanitizedContent ordain(String s) {
return UnsafeSanitizedContentOrdainer.ordainAsSafe(
s, SanitizedContent.ContentKind.TRUSTED_RESOURCE_URI);

54
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
View File

@ -990,6 +990,20 @@
* @param {function()=} opt_cancelCondition
*/
getChangeDetail(changeNum, opt_errFn, opt_cancelCondition) {
return this.getConfig(false).then(config => {
const optionsHex = this._getChangeOptionsHex(config);
return this._getChangeDetail(
changeNum, optionsHex, opt_errFn, opt_cancelCondition)
.then(GrReviewerUpdatesParser.parse);
});
}
_getChangeOptionsHex(config) {
if (window.DEFAULT_DETAIL_HEXES && window.DEFAULT_DETAIL_HEXES.changePage
&& !(config.receive && config.receive.enable_signed_push)) {
return window.DEFAULT_DETAIL_HEXES.changePage;
}
// This list MUST be kept in sync with
// ChangeIT#changeDetailsDoesNotRequireIndex
const options = [
@ -1003,15 +1017,10 @@
this.ListChangesOption.WEB_LINKS,
this.ListChangesOption.SKIP_DIFFSTAT,
];
return this.getConfig(false).then(config => {
if (config.receive && config.receive.enable_signed_push) {
options.push(this.ListChangesOption.PUSH_CERTIFICATES);
}
const optionsHex = this.listChangesOptionsToHex(...options);
return this._getChangeDetail(
changeNum, optionsHex, opt_errFn, opt_cancelCondition)
.then(GrReviewerUpdatesParser.parse);
});
if (config.receive && config.receive.enable_signed_push) {
options.push(this.ListChangesOption.PUSH_CERTIFICATES);
}
return this.listChangesOptionsToHex(...options);
}
/**
@ -1020,11 +1029,16 @@
* @param {function()=} opt_cancelCondition
*/
getDiffChangeDetail(changeNum, opt_errFn, opt_cancelCondition) {
const optionsHex = this.listChangesOptionsToHex(
this.ListChangesOption.ALL_COMMITS,
this.ListChangesOption.ALL_REVISIONS,
this.ListChangesOption.SKIP_DIFFSTAT
);
let optionsHex = '';
if (window.DEFAULT_DETAIL_HEXES && window.DEFAULT_DETAIL_HEXES.diffPage) {
optionsHex = window.DEFAULT_DETAIL_HEXES.diffPage;
} else {
optionsHex = this.listChangesOptionsToHex(
this.ListChangesOption.ALL_COMMITS,
this.ListChangesOption.ALL_REVISIONS,
this.ListChangesOption.SKIP_DIFFSTAT
);
}
return this._getChangeDetail(changeNum, optionsHex, opt_errFn,
opt_cancelCondition);
}
@ -1040,7 +1054,7 @@
const urlWithParams = this._restApiHelper
.urlWithParams(url, optionsHex);
const params = {O: optionsHex};
let req = {
const req = {
url,
errFn: opt_errFn,
cancelCondition: opt_cancelCondition,
@ -1048,7 +1062,6 @@
fetchOptions: this._etags.getOptions(urlWithParams),
anonymizedUrl: '/changes/*~*/detail?O=' + optionsHex,
};
req = this._restApiHelper.addAcceptJsonHeader(req);
return this._restApiHelper.fetchRawJSON(req).then(response => {
if (response && response.status === 304) {
return Promise.resolve(this._restApiHelper.parsePrefixedJSON(
@ -2039,12 +2052,15 @@
* @param {string|number=} opt_patchNum
* @return {!Promise<!Object>} Diff comments response.
*/
// We don't want to add accept header, since preloading of comments is
// working only without accept header.
const noAcceptHeader = true;
const fetchComments = opt_patchNum => this._getChangeURLAndFetch({
changeNum,
endpoint,
patchNum: opt_patchNum,
reportEndpointAsIs: true,
});
}, noAcceptHeader);
if (!opt_basePatchNum && !opt_patchNum && !opt_path) {
return fetchComments();
@ -2609,7 +2625,7 @@
* @param {Gerrit.ChangeFetchRequest} req
* @return {!Promise<!Object>}
*/
_getChangeURLAndFetch(req) {
_getChangeURLAndFetch(req, noAcceptHeader) {
const anonymizedEndpoint = req.reportEndpointAsIs ?
req.endpoint : req.anonymizedEndpoint;
const anonymizedBaseUrl = req.patchNum ?
@ -2622,7 +2638,7 @@
fetchOptions: req.fetchOptions,
anonymizedUrl: anonymizedEndpoint ?
(anonymizedBaseUrl + anonymizedEndpoint) : undefined,
}));
}, noAcceptHeader));
}
/**

10
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
View File

@ -1036,16 +1036,6 @@ limitations under the License.
});
});
test('_getChangeDetail accepts only json', () => {
const authFetchStub = sandbox.stub(element._auth, 'fetch')
.returns(Promise.resolve());
const errFn = sinon.stub();
element._getChangeDetail(123, '516714', errFn);
assert.isTrue(authFetchStub.called);
assert.equal(authFetchStub.lastCall.args[1].headers.get('Accept'),
'application/json');
});
test('_getChangeDetail populates _projectLookup', () => {
sandbox.stub(element, 'getChangeActionURL')
.returns(Promise.resolve(''));

7
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper.js
View File

@ -204,9 +204,12 @@
* Same as {@link fetchRawJSON}, plus error handling.
*
* @param {Gerrit.FetchJSONRequest} req
* @param {boolean} noAcceptHeader - don't add default accept json header
*/
fetchJSON(req) {
req = this.addAcceptJsonHeader(req);
fetchJSON(req, noAcceptHeader) {
if (!noAcceptHeader) {
req = this.addAcceptJsonHeader(req);
}
return this.fetchRawJSON(req).then(response => {
if (!response) {
return;

23
resources/com/google/gerrit/httpd/raw/PolyGerritIndexHtml.soy
View File

@ -28,6 +28,11 @@
{@param? polyfillSD: ?}
{@param? polyfillSC: ?}
{@param? useGoogleFonts: ?}
{@param? changeRequestsPath: ?}
{@param? defaultChangeDetailHex: ?}
{@param? defaultDiffDetailHex: ?}
{@param? preloadChangePage: ?}
{@param? preloadDiffPage: ?}
<!DOCTYPE html>{\n}
<html lang="en">{\n}
<meta charset="utf-8">{\n}
@ -43,6 +48,14 @@
// Disable extra font load from paper-styles
window.polymerSkipLoadingFontRoboto = true;
window.CLOSURE_NO_DEPS = true;
window.DEFAULT_DETAIL_HEXES = {lb}
{if $defaultChangeDetailHex}
changePage: '{$defaultChangeDetailHex}',
{/if}
{if $defaultDiffDetailHex}
diffPage: '{$defaultDiffDetailHex}',
{/if}
{rb};
{if $canonicalPath != ''}window.CANONICAL_PATH = '{$canonicalPath}';{/if}
{if $versionInfo}window.VERSION_INFO = '{$versionInfo}';{/if}
{if $staticResourcePath != ''}window.STATIC_RESOURCE_PATH = '{$staticResourcePath}';{/if}
@ -68,6 +81,16 @@
{else}
<link rel="icon" type="image/x-icon" href="{$canonicalPath}/favicon.ico">{\n}
{/if}
{if $changeRequestsPath}
{if $preloadChangePage and $defaultChangeDetailHex}
<link rel="preload" href="{$canonicalPath}/{$changeRequestsPath}/detail?O={$defaultChangeDetailHex}" as="fetch" type="application/json" crossorigin="anonymous"/>{\n}
{/if}
{if $preloadDiffPage and $defaultDiffDetailHex}
<link rel="preload" href="{$canonicalPath}/{$changeRequestsPath}/detail?O={$defaultDiffDetailHex}" as="fetch" type="application/json" crossorigin="anonymous"/>{\n}
{/if}
<link rel="preload" href="{$canonicalPath}/{$changeRequestsPath}/comments" as="fetch" type="application/json" crossorigin="anonymous"/>{\n}
<link rel="preload" href="{$canonicalPath}/{$changeRequestsPath}/robotcomments" as="fetch" type="application/json" crossorigin="anonymous"/>{\n}
{/if}
// RobotoMono fonts are used in styles/fonts.css
{if $useGoogleFonts}