Browse Source

ref(js): Use `fetch` in API client (#23679)

Evan Purkhiser 4 years ago
parent
commit
673430e5a4

+ 3 - 7
src/sentry/static/sentry/app/__mocks__/api.tsx

@@ -1,5 +1,3 @@
-import $ from 'jquery';
-
 import * as ImportedClient from 'app/api';
 
 const RealClient: typeof ImportedClient = jest.requireActual('app/api');
@@ -150,9 +148,7 @@ class Client {
       if (response.statusCode !== 200) {
         response.callCount++;
 
-        const deferred = $.Deferred();
-
-        const errorResponse: JQueryXHR = Object.assign(
+        const errorResponse = Object.assign(
           {
             status: response.statusCode,
             responseText: JSON.stringify(body),
@@ -164,16 +160,16 @@ class Client {
             then: () => {},
             error: () => {},
           },
-          deferred,
           new XMLHttpRequest()
         );
+
         this.handleRequestError(
           {
             id: '1234',
             path: url,
             requestOptions: options,
           },
-          errorResponse,
+          errorResponse as any,
           'error',
           'error'
         );

+ 125 - 37
src/sentry/static/sentry/app/api.tsx

@@ -5,53 +5,59 @@ import Cookies from 'js-cookie';
 import isUndefined from 'lodash/isUndefined';
 
 import {openSudo, redirectToProject} from 'app/actionCreators/modal';
+import {CSRF_COOKIE_NAME, EXPERIMENTAL_SPA} from 'app/constants';
 import {
   PROJECT_MOVED,
   SUDO_REQUIRED,
   SUPERUSER_REQUIRED,
 } from 'app/constants/apiErrorCodes';
-import ajaxCsrfSetup from 'app/utils/ajaxCsrfSetup';
 import {metric} from 'app/utils/analytics';
 import {run} from 'app/utils/apiSentryClient';
+import getCookie from 'app/utils/getCookie';
 import {uniqueId} from 'app/utils/guid';
 import createRequestError from 'app/utils/requestError/createRequestError';
 
-import {EXPERIMENTAL_SPA} from './constants';
-
 export class Request {
   /**
    * Is the request still in flight
    */
   alive: boolean;
-  xhr: JQueryXHR;
+  /**
+   * Promise which will be resolved when the request has completed
+   */
+  requestPromise: Promise<Response>;
+  /**
+   * AbortController to cancel the in-flight request
+   */
+  aborter: AbortController;
 
-  constructor(xhr: JQueryXHR) {
-    this.xhr = xhr;
+  constructor(requestPromise: Promise<Response>, aborter: AbortController) {
+    this.requestPromise = requestPromise;
+    this.aborter = aborter;
     this.alive = true;
   }
 
   cancel() {
     this.alive = false;
-    this.xhr.abort();
+    this.aborter.abort();
     metric('app.api.request-abort', 1);
   }
 }
 
 /**
- * Setup the CSRF + other client early initalization.
+ * Setup the CSRF + other client early initialization.
+ *
+ * TODO(epurkhiser): This can be removed now that we are using fetch, there is
+ * no initialization that needs to happen
  */
-export function initApiClient() {
-  jQuery.ajaxSetup({
-    // jQuery won't allow using the ajaxCsrfSetup function directly
-    beforeSend: ajaxCsrfSetup,
-    // Completely disable evaluation of script responses using jQuery ajax
-    // Typically the `text script` converter will eval the text [1]. Instead we
-    // just immediately return.
-    // [1]: https://github.com/jquery/jquery/blob/8969732518470a7f8e654d5bc5be0b0076cb0b87/src/ajax/script.js#L39-L42
-    converters: {
-      'text script': (value: any) => value,
-    },
-  });
+export function initApiClient() {}
+
+/**
+ * Check if the requested method does not require CSRF tokens
+ */
+function csrfSafeMethod(method?: string) {
+  // these HTTP methods do not require CSRF protection
+  return /^(GET|HEAD|OPTIONS|TRACE)$/.test(method ?? '');
 }
 
 // TODO: Need better way of identifying anonymous pages that don't trigger redirect
@@ -62,8 +68,10 @@ const ALLOWED_ANON_PAGES = [
   /^\/join-request\//,
 ];
 
-export function initApiClientErrorHandling() {
-  jQuery(document).ajaxError(function (_evt, jqXHR) {
+const globalErrorHandlers: ((jqXHR: JQueryXHR) => void)[] = [];
+
+export const initApiClientErrorHandling = () =>
+  globalErrorHandlers.push((jqXHR: JQueryXHR) => {
     const pageAllowsAnon = ALLOWED_ANON_PAGES.find(regex =>
       regex.test(window.location.pathname)
     );
@@ -97,7 +105,6 @@ export function initApiClientErrorHandling() {
       window.location.reload();
     }
   });
-}
 
 /**
  * Construct a full request URL
@@ -160,7 +167,7 @@ export function hasProjectBeenRenamed(response: JQueryXHR) {
   return true;
 }
 
-// TODO: move this somewhere
+// TODO(ts): move this somewhere
 export type APIRequestMethod = 'POST' | 'GET' | 'DELETE' | 'PUT';
 
 type FunctionCallback<Args extends any[] = any[]> = (...args: Args) => void;
@@ -209,7 +216,7 @@ type HandleRequestErrorOptions = {
 /**
  * The API client is used to make HTTP requests to Sentry's backend.
  *
- * This is the prefered way to talk to the backend.
+ * This is they preferred way to talk to the backend.
  */
 export class Client {
   baseUrl: string;
@@ -299,13 +306,25 @@ export class Client {
    */
   request(path: string, options: Readonly<RequestOptions> = {}): Request {
     const method = options.method || (options.data ? 'POST' : 'GET');
+
+    let fullUrl = buildRequestUrl(this.baseUrl, path, options.query);
+
     let data = options.data;
 
     if (!isUndefined(data) && method !== 'GET') {
       data = JSON.stringify(data);
     }
 
-    const fullUrl = buildRequestUrl(this.baseUrl, path, options.query);
+    // TODO(epurkhiser): Mimicking the old jQuery API, data could be a string /
+    // object for GET requets. jQuery just sticks it onto the URL as query
+    // parameters
+    if (method === 'GET' && data) {
+      const queryString = typeof data === 'string' ? data : jQuery.param(data);
+
+      if (queryString.length > 0) {
+        fullUrl = fullUrl + (fullUrl.indexOf('?') !== -1 ? '&' : '?') + queryString;
+      }
+    }
 
     const id = uniqueId();
     const startMarker = `api-request-start-${id}`;
@@ -383,20 +402,89 @@ export class Client {
         true
       )(jqXHR, textStatus);
 
-    const xhrRequest = jQuery.ajax({
-      url: fullUrl,
+    const aborter = new AbortController();
+
+    // GET requests may not have a body
+    const body = method !== 'GET' ? data : undefined;
+
+    const headers = new Headers({
+      Accept: 'application/json; charset=utf-8',
+      'Content-Type': 'application/json',
+    });
+
+    // Do not set the X-CSRFToken header when making a request outside of the
+    // current domain
+    const absoluteUrl = new URL(fullUrl, window.location.origin);
+    const isSameOrigin = window.location.origin === absoluteUrl.origin;
+
+    if (!csrfSafeMethod(method) && isSameOrigin) {
+      headers.set('X-CSRFToken', getCookie(CSRF_COOKIE_NAME) ?? '');
+    }
+
+    const fetchRequest = fetch(fullUrl, {
       method,
-      data,
-      contentType: 'application/json',
-      headers: {
-        Accept: 'application/json; charset=utf-8',
-      },
-      success: successHandler,
-      error: errorHandler,
-      complete: completeHandler,
+      body,
+      headers,
+      credentials: 'same-origin',
+      signal: aborter.signal,
     });
 
-    const request = new Request(xhrRequest);
+    // XXX(epurkhiser): We're migrating off of jquery, so for now we have a
+    // compatibility layer which mimics that of the jquery response objects.
+    fetchRequest
+      .then(async response => {
+        let responseJSON: any;
+        let responseText: any;
+
+        // Try to get JSON out of the response no matter the status
+        try {
+          responseJSON = await response.json();
+        } catch {
+          // No json came out.. too bad
+        }
+
+        // Try to get text out of the response no matter the status
+        try {
+          responseText = await response.text();
+        } catch {
+          // No text came out.. too bad
+        }
+
+        const {ok, status, statusText} = response;
+
+        const emulatedJQueryXHR: any = {
+          status,
+          statusText,
+          responseJSON,
+          responseText,
+          getResponseHeader: (header: string) => response.headers.get(header),
+        };
+
+        if (ok) {
+          successHandler(responseJSON, statusText, emulatedJQueryXHR);
+        } else {
+          globalErrorHandlers.forEach(handler => handler(emulatedJQueryXHR));
+          errorHandler(emulatedJQueryXHR, statusText, 'Request not OK');
+        }
+
+        completeHandler(emulatedJQueryXHR, statusText);
+      })
+      .catch(err => {
+        // Aborts are expected
+        if (err?.name === 'AbortError') {
+          return;
+        }
+
+        // The request failed for other reason
+        run(Sentry =>
+          Sentry.withScope(scope => {
+            scope.setLevel(Severity.Warning);
+            Sentry.captureException(err);
+          })
+        );
+      });
+
+    const request = new Request(fetchRequest, aborter);
     this.activeRequests[id] = request;
 
     return request;

+ 0 - 3
src/sentry/static/sentry/app/bootstrap.tsx

@@ -17,7 +17,6 @@ import moment from 'moment';
 import PropTypes from 'prop-types';
 import Reflux from 'reflux';
 
-import {initApiClient} from 'app/api';
 import {DISABLE_RR_WEB, NODE_ENV, SPA_DSN} from 'app/constants';
 import Main from 'app/main';
 import plugins from 'app/plugins';
@@ -108,8 +107,6 @@ Sentry.setTag('rrweb.active', hasReplays ? 'yes' : 'no');
 // bundle was loaded by browser.
 metric.mark({name: 'sentry-app-init'});
 
-initApiClient();
-
 const ROOT_ELEMENT = 'blk_router';
 
 const render = (Component: React.ComponentType) => {

+ 3 - 0
src/sentry/static/sentry/app/utils/ajaxCsrfSetup.tsx

@@ -6,6 +6,9 @@ function csrfSafeMethod(method?: string) {
   return /^(GET|HEAD|OPTIONS|TRACE)$/.test(method ?? '');
 }
 
+/**
+ * TODO(epurkhiser): This can be removed now that we are using fetch for API requests
+ */
 export default function ajaxCsrfSetup(
   this: JQueryAjaxSettings,
   xhr: JQueryXHR,

+ 7 - 14
tests/js/spec/api.spec.jsx

@@ -1,5 +1,3 @@
-import $ from 'jquery';
-
 import {Client, Request} from 'app/api';
 import {PROJECT_MOVED} from 'app/constants/apiErrorCodes';
 
@@ -13,18 +11,13 @@ describe('api', function () {
   });
 
   describe('Client', function () {
-    beforeEach(function () {
-      jest.spyOn($, 'ajax');
-    });
-
     describe('cancel()', function () {
       it('should abort any open XHR requests', function () {
-        const req1 = new Request({
-          abort: jest.fn(),
-        });
-        const req2 = new Request({
-          abort: jest.fn(),
-        });
+        const abort1 = jest.fn();
+        const abort2 = jest.fn();
+
+        const req1 = new Request(new Promise(() => null), {abort: abort1});
+        const req2 = new Request(new Promise(() => null), {abort: abort2});
 
         api.activeRequests = {
           1: req1,
@@ -33,8 +26,8 @@ describe('api', function () {
 
         api.clear();
 
-        expect(req1.xhr.abort).toHaveBeenCalledTimes(1);
-        expect(req2.xhr.abort).toHaveBeenCalledTimes(1);
+        expect(req1.aborter.abort).toHaveBeenCalledTimes(1);
+        expect(req2.aborter.abort).toHaveBeenCalledTimes(1);
       });
     });
   });