Define a default errFn for REST API calls from plugins
The REST API service was recently fixed to fire error events on the
document instead of on itself. This is a change in behavior for plugins,
because they were using a component instance that was detached from the
DOM.
We want to revert back to the behavior where plugins would typically
not expect Gerrit to handle errors by showing error dialogs. So we are
defining a default errFn at least for the known case of a Chrome plugin
that would otherwise show a distracting dialog when no config was found.
We are swapping the catch() and then() clauses in the rest-api-helper
such that errFn can throw an error without the catch() clause catching
it. errFn throwing an error is the only situation where the swap
matters.
We are planning a lot of further clean-ups and want to ultimately remove
the errFn support. There are a lot of follow-up changes expected, but
here we just want to fix Gerrit and bring it into a releasable state.
Change-Id: I3f441a0a7587b791223c4ae89fe0547cb2ea090c
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-rest-api.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-rest-api.ts
index 00b2963..eb63a77 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-rest-api.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-rest-api.ts
@@ -120,14 +120,28 @@
errFn?: ErrorCallback,
contentType?: string
) {
+ // Plugins typically don't want Gerrit to show error dialogs for failed
+ // requests. So we are defining a default errFn here, even if it is not
+ // explicitly set by the caller.
+ // TODO: We are soon getting rid of the `errFn` altogether. There are only
+ // 2 known usages of errFn in plugins: delete-project and verify-status.
+ errFn =
+ errFn ??
+ ((response: Response | null | undefined, error?: Error) => {
+ if (error) throw error;
+ if (response) throw new Error(`${response.status}`);
+ throw new Error('Generic REST API error.');
+ });
return this.fetch(method, url, payload, errFn, contentType).then(
response => {
+ // Will typically not happen. The response can only be unset, if the
+ // errFn handles the error and then returns void or undefined or null.
+ // But the errFn above always throws.
if (!response) {
- // TODO(TS): Fix method definition
- // If errFn exists and doesn't throw an exception, the fetch method
- // returns empty response
- throw new Error('errFn must throw an exception');
+ throw new Error('plugin rest-api call failed');
}
+ // Will typically not happen. errFn will have dealt with that and the
+ // caller will get a rejected promise already.
if (response.status < 200 || response.status >= 300) {
return response.text().then(text => {
if (text) {
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper.ts
index fadd9f3..8aa1b0f 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper.ts
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper.ts
@@ -509,22 +509,6 @@
anonymizedUrl: req.reportUrlAsIs ? url : req.anonymizedUrl,
};
const xhr = this.fetch(fetchReq)
- .then(response => {
- if (!response.ok) {
- if (req.errFn) {
- req.errFn.call(undefined, response);
- return;
- }
- document.dispatchEvent(
- new CustomEvent('server-error', {
- detail: {request: fetchReq, response},
- composed: true,
- bubbles: true,
- })
- );
- }
- return response;
- })
.catch(err => {
document.dispatchEvent(
new CustomEvent('network-error', {
@@ -538,6 +522,22 @@
} else {
throw err;
}
+ })
+ .then(response => {
+ if (response && !response.ok) {
+ if (req.errFn) {
+ req.errFn.call(undefined, response);
+ return;
+ }
+ document.dispatchEvent(
+ new CustomEvent('server-error', {
+ detail: {request: fetchReq, response},
+ composed: true,
+ bubbles: true,
+ })
+ );
+ }
+ return response;
});
if (req.parseResponse) {
