Merge pull request #28814 from overleaf/jel-v1api-promisify

[web] v1 API decaf cleanup and remove unused code

GitOrigin-RevId: 706506ca12df20be74c7c3b35bec4d74baa7f130

services/web/app/src/Features/V1/V1Api.mjs

@@ -1,12 +1,3 @@
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS101: Remove unnecessary use of Array.from
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 import request from 'request'
 
 import settings from '@overleaf/settings'
@@ -27,82 +18,61 @@ const DEFAULT_V1_PARAMS = {
 
 const v1Request = request.defaults(DEFAULT_V1_PARAMS)
 
-const DEFAULT_V1_OAUTH_PARAMS = {
-  baseUrl: settings.apis.v1.url,
-  json: true,
-  timeout: settings.apis.v1.timeout,
+function makeRequest(options, callback) {
+  if (!callback) {
+    return request(options)
+  }
+  v1Request(options, (error, response, body) =>
+    _responseHandler(options, error, response, body, callback)
+  )
 }
 
-const v1OauthRequest = request.defaults(DEFAULT_V1_OAUTH_PARAMS)
-
-const V1Api = {
-  request(options, callback) {
-    if (callback == null) {
-      return request(options)
-    }
-    return v1Request(options, (error, response, body) =>
-      V1Api._responseHandler(options, error, response, body, callback)
+function _responseHandler(options, error, response, body, callback) {
+  const { expectedStatusCodes } = options
+  if (error) {
+    return callback(
+      new Errors.V1ConnectionError('error from V1 API').withCause(error)
     )
-  },
-
-  oauthRequest(options, token, callback) {
-    if (options.uri == null) {
-      return callback(new Error('uri required'))
-    }
-    if (options.method == null) {
-      options.method = 'GET'
-    }
-    options.auth = { bearer: token }
-    return v1OauthRequest(options, (error, response, body) =>
-      V1Api._responseHandler(options, error, response, body, callback)
+  }
+  if (response && response.statusCode >= 500) {
+    return callback(
+      new Errors.V1ConnectionError({
+        message: 'error from V1 API',
+        info: { status: response.statusCode, body },
+      })
     )
-  },
+  }
+  if (
+    (response && response.statusCode >= 200 && response.statusCode < 300) ||
+    (expectedStatusCodes || []).includes(response?.statusCode)
+  ) {
+    return callback(null, response, body)
+  } else if (response?.statusCode === 403) {
+    error = new Errors.ForbiddenError('overleaf v1 returned forbidden')
+    error.statusCode = response.statusCode
+    return callback(error)
+  } else if (response?.statusCode === 404) {
+    error = new Errors.NotFoundError(
+      `overleaf v1 returned non-success code: ${response.statusCode} ${options.method} ${options.uri}`
+    )
+    error.statusCode = response.statusCode
+    return callback(error)
+  } else {
+    error = new OError('overleaf v1 returned non-success code', {
+      status: response?.statusCode,
+      method: options.method,
+      url: options.uri,
+    })
+    error.statusCode = response?.statusCode
+    return callback(error)
+  }
+}
 
-  _responseHandler(options, error, response, body, callback) {
-    if (error != null) {
-      return callback(
-        new Errors.V1ConnectionError('error from V1 API').withCause(error)
-      )
-    }
-    if (response && response.statusCode >= 500) {
-      return callback(
-        new Errors.V1ConnectionError({
-          message: 'error from V1 API',
-          info: { status: response.statusCode, body },
-        })
-      )
-    }
-    if (
-      (response && response.statusCode >= 200 && response.statusCode < 300) ||
-      Array.from(options.expectedStatusCodes || []).includes(
-        response?.statusCode
-      )
-    ) {
-      return callback(null, response, body)
-    } else if (response?.statusCode === 403) {
-      error = new Errors.ForbiddenError('overleaf v1 returned forbidden')
-      error.statusCode = response.statusCode
-      return callback(error)
-    } else if (response?.statusCode === 404) {
-      error = new Errors.NotFoundError(
-        `overleaf v1 returned non-success code: ${response.statusCode} ${options.method} ${options.uri}`
-      )
-      error.statusCode = response.statusCode
-      return callback(error)
-    } else {
-      error = new OError('overleaf v1 returned non-success code', {
-        status: response?.statusCode,
-        method: options.method,
-        url: options.uri,
-      })
-      error.statusCode = response?.statusCode
-      return callback(error)
-    }
-  },
+const V1Api = {
+  request: makeRequest,
 }
 
 V1Api.promises = {
   request: promisifyMultiResult(V1Api.request, ['response', 'body']),
-  oauthRequest: promisifyMultiResult(V1Api.oauthRequest, ['response', 'body']),
 }
 export default V1Api

services/web/test/acceptance/src/V1ApiTests.mjs

@@ -0,0 +1,111 @@
+import { expect } from 'chai'
+import V1Api from '../../../app/src/Features/V1/V1Api.mjs'
+import MockV1ApiClass from './mocks/MockV1Api.mjs'
+import Features from '../../../app/src/infrastructure/Features.mjs'
+
+describe('V1Api', function () {
+  const testPath = '/api/v1/overleaf/fake_route_api_handler_tests'
+
+  function testPathWithResponse(statusCode, body = '') {
+    const encodedBody = encodeURIComponent(JSON.stringify(body))
+    return `${testPath}?expectedStatus=${statusCode}&expectedBody=${encodedBody}`
+  }
+
+  beforeEach(function () {
+    if (!Features.hasFeature('saas')) {
+      this.skip()
+    }
+
+    MockV1ApiClass.instance()
+  })
+
+  it('returns errors from request handling', async function () {
+    let error
+    try {
+      await V1Api.promises.request({})
+    } catch (e) {
+      error = e
+    }
+    expect(error).to.exist
+    expect(error.cause.message).to.equal(
+      'options.uri must be a string when using options.baseUrl'
+    )
+  })
+
+  it('returns 500 errors', async function () {
+    let error
+    const expectedBody = { testing: 'test' }
+    const url = testPathWithResponse(500, expectedBody)
+
+    try {
+      await V1Api.promises.request({ url })
+    } catch (e) {
+      error = e
+    }
+    expect(error).to.exist
+    expect(error.message).to.equal('error from V1 API')
+    expect(error.info.status).to.equal(500)
+    expect(error.info.body).to.deep.equal(expectedBody)
+  })
+
+  it('returns 2xx status responses', async function () {
+    const expectedBody = { testing: 'success' }
+    const url = testPathWithResponse(200, expectedBody)
+    const response = await V1Api.promises.request({ url })
+    expect(response.response.statusCode).to.equal(200)
+    expect(response.body).to.deep.equal(expectedBody)
+  })
+
+  it('returns 403 errors', async function () {
+    let error
+    const url = testPathWithResponse(403)
+
+    try {
+      await V1Api.promises.request({ url })
+    } catch (e) {
+      error = e
+    }
+    expect(error).to.exist
+    expect(error.message).to.equal('overleaf v1 returned forbidden')
+    expect(error.statusCode).to.equal(403)
+  })
+
+  it('returns 404 errors', async function () {
+    let error
+    const url = testPathWithResponse(404)
+
+    try {
+      await V1Api.promises.request({ uri: url, method: 'GET' })
+    } catch (e) {
+      error = e
+    }
+    expect(error).to.exist
+    expect(error.message).to.equal(
+      `overleaf v1 returned non-success code: 404 GET ${url}`
+    )
+    expect(error.statusCode).to.equal(404)
+  })
+
+  it('returns errors', async function () {
+    let error
+    const url = testPathWithResponse(402)
+
+    try {
+      await V1Api.promises.request({ uri: url, method: 'GET' })
+    } catch (e) {
+      error = e
+    }
+    expect(error).to.exist
+    expect(error.message).to.equal('overleaf v1 returned non-success code')
+    expect(error.statusCode).to.equal(402)
+  })
+
+  it('does not return error if expected status', async function () {
+    const url = testPathWithResponse(404)
+    const response = await V1Api.promises.request({
+      url,
+      expectedStatusCodes: [404],
+    })
+    expect(response.response.statusCode).to.equal(404)
+  })
+})

services/web/test/acceptance/src/mocks/MockV1Api.mjs

@@ -476,6 +476,25 @@ class MockV1Api extends AbstractMockApi {
       }
       res.json(template)
     })
+
+    this.app.get(
+      '/api/v1/overleaf/fake_route_api_handler_tests',
+      (req, res) => {
+        const expectedStatus = req.query.expectedStatus
+        const expectedBody = req.query.expectedBody
+        const statusCode = Number(expectedStatus)
+        if (
+          !Number.isInteger(statusCode) ||
+          statusCode < 100 ||
+          statusCode > 599
+        ) {
+          return res
+            .status(500)
+            .json({ error: 'Invalid expectedStatus query parameter' })
+        }
+        return res.status(statusCode).json(JSON.parse(expectedBody))
+      }
+    )
   }
 }