remove request from legacy request handler (#7233)

Co-authored-by: repo-ranger[bot] <39074581+repo-ranger[bot]@users.noreply.github.com>

core/base-service/base.js

@@ -444,7 +444,7 @@ class BaseService {
       regex,
       handleRequest(cacheHeaderConfig, {
         queryParams,
-        handler: async (queryParams, match, sendBadge, request) => {
+        handler: async (queryParams, match, sendBadge) => {
           const metricHandle = metricHelper.startRequest()
 
           const namedParams = namedParamsForMatch(captureNames, match, this)
@@ -473,7 +473,6 @@ class BaseService {
           metricHandle.noteResponseSent()
         },
         cacheLength: this._cacheLength,
-        fetchLimitBytes,
       })
     )
   }

core/base-service/got.js

@@ -94,4 +94,4 @@ function fetchFactory(fetchLimitBytes = TEN_MB) {
   return sendRequest.bind(sendRequest, gotWithLimit)
 }
 
-export { requestOptions2GotOptions, fetchFactory }
+export { requestOptions2GotOptions, fetchFactory, userAgent }

core/base-service/legacy-request-handler.js

@@ -1,12 +1,8 @@
-import request from 'request'
 import makeBadge from '../../badge-maker/lib/make-badge.js'
 import { setCacheHeaders } from './cache-headers.js'
-import { Inaccessible, InvalidResponse, ShieldsRuntimeError } from './errors.js'
 import { makeSend } from './legacy-result-sender.js'
 import coalesceBadge from './coalesce-badge.js'
 
-const userAgent = 'Shields.io/2003a'
-
 // These query parameters are available to any badge. They are handled by
 // `coalesceBadge`.
 const globalQueryParams = new Set([
@@ -32,32 +28,12 @@ function flattenQueryParams(queryParams) {
   return Array.from(union).sort()
 }
 
-function promisify(cachingRequest) {
-  return (uri, options) =>
-    new Promise((resolve, reject) => {
-      cachingRequest(uri, options, (err, res, buffer) => {
-        if (err) {
-          if (err instanceof ShieldsRuntimeError) {
-            reject(err)
-          } else {
-            // Wrap the error in an Inaccessible so it can be identified
-            // by the BaseService handler.
-            reject(new Inaccessible({ underlyingError: err }))
-          }
-        } else {
-          resolve({ res, buffer })
-        }
-      })
-    })
-}
-
 // handlerOptions can contain:
 // - handler: The service's request handler function
 // - queryParams: An array of the field names of any custom query parameters
 //   the service uses
 // - cacheLength: An optional badge or category-specific cache length
 //   (in number of seconds) to be used in preference to the default
-// - fetchLimitBytes: A limit on the response size we're willing to parse
 //
 // For safety, the service must declare the query parameters it wants to use.
 // Only the declared parameters (and the global parameters) are provided to
@@ -77,8 +53,7 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
   }
 
   const allowedKeys = flattenQueryParams(handlerOptions.queryParams)
-  const { cacheLength: serviceDefaultCacheLengthSeconds, fetchLimitBytes } =
-    handlerOptions
+  const { cacheLength: serviceDefaultCacheLengthSeconds } = handlerOptions
 
   return (queryParams, match, end, ask) => {
     /*
@@ -139,40 +114,6 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
       makeSend(extension, ask.res, end)(svg)
     }, 25000)
 
-    function cachingRequest(uri, options, callback) {
-      if (typeof options === 'function' && !callback) {
-        callback = options
-      }
-      if (options && typeof options === 'object') {
-        options.uri = uri
-      } else if (typeof uri === 'string') {
-        options = { uri }
-      } else {
-        options = uri
-      }
-      options.headers = options.headers || {}
-      options.headers['User-Agent'] = userAgent
-
-      let bufferLength = 0
-      const r = request(options, callback)
-      r.on('data', chunk => {
-        bufferLength += chunk.length
-        if (bufferLength > fetchLimitBytes) {
-          r.abort()
-          r.emit(
-            'error',
-            new InvalidResponse({
-              prettyMessage: 'Maximum response size exceeded',
-            })
-          )
-        }
-      })
-    }
-
-    // Wrapper around `cachingRequest` that returns a promise rather than needing
-    // to pass a callback.
-    cachingRequest.asPromise = promisify(cachingRequest)
-
     const result = handlerOptions.handler(
       filteredQueryParams,
       match,
@@ -187,8 +128,7 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
         const svg = makeBadge(badgeData)
         setCacheHeadersOnResponse(ask.res, badgeData.cacheLengthSeconds)
         makeSend(format, ask.res, end)(svg)
-      },
-      cachingRequest
+      }
     )
     // eslint-disable-next-line promise/prefer-await-to-then
     if (result && result.catch) {
@@ -200,4 +140,4 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
   }
 }
 
-export { handleRequest, promisify, userAgent }
+export { handleRequest }

core/base-service/legacy-request-handler.spec.js

@@ -1,5 +1,4 @@
 import { expect } from 'chai'
-import nock from 'nock'
 import portfinder from 'portfinder'
 import Camp from '@shields_io/camp'
 import got from '../got-test-client.js'
@@ -42,28 +41,6 @@ function createFakeHandlerWithCacheLength(cacheLengthSeconds) {
   }
 }
 
-function fakeHandlerWithNetworkIo(queryParams, match, sendBadge, request) {
-  const [, someValue, format] = match
-  request('https://www.google.com/foo/bar', (err, res, buffer) => {
-    let message
-    if (err) {
-      message = err.prettyMessage
-    } else {
-      message = someValue
-    }
-    const badgeData = coalesceBadge(
-      queryParams,
-      {
-        label: 'testing',
-        message,
-        format,
-      },
-      {}
-    )
-    sendBadge(format, badgeData)
-  })
-}
-
 describe('The request handler', function () {
   let port, baseUrl
   beforeEach(async function () {
@@ -133,60 +110,6 @@ describe('The request handler', function () {
     })
   })
 
-  describe('the response size limit', function () {
-    beforeEach(function () {
-      camp.route(
-        /^\/testing\/([^/]+)\.(svg|png|gif|jpg|json)$/,
-        handleRequest(standardCacheHeaders, {
-          handler: fakeHandlerWithNetworkIo,
-          fetchLimitBytes: 100,
-        })
-      )
-    })
-
-    it('should not throw an error if the response <= fetchLimitBytes', async function () {
-      nock('https://www.google.com')
-        .get('/foo/bar')
-        .once()
-        .reply(200, 'x'.repeat(100))
-      const { statusCode, body } = await got(`${baseUrl}/testing/123.json`, {
-        responseType: 'json',
-      })
-      expect(statusCode).to.equal(200)
-      expect(body).to.deep.equal({
-        name: 'testing',
-        value: '123',
-        label: 'testing',
-        message: '123',
-        color: 'lightgrey',
-        link: [],
-      })
-    })
-
-    it('should throw an error if the response is > fetchLimitBytes', async function () {
-      nock('https://www.google.com')
-        .get('/foo/bar')
-        .once()
-        .reply(200, 'x'.repeat(101))
-      const { statusCode, body } = await got(`${baseUrl}/testing/123.json`, {
-        responseType: 'json',
-      })
-      expect(statusCode).to.equal(200)
-      expect(body).to.deep.equal({
-        name: 'testing',
-        value: 'Maximum response size exceeded',
-        label: 'testing',
-        message: 'Maximum response size exceeded',
-        color: 'lightgrey',
-        link: [],
-      })
-    })
-
-    afterEach(function () {
-      nock.cleanAll()
-    })
-  })
-
   describe('caching', function () {
     describe('standard query parameters', function () {
       function register({ cacheHeaderConfig }) {

doc/code-walkthrough.md

@@ -96,7 +96,7 @@ test this kind of logic through unit tests (e.g. of `render()` and
     is created in a legacy helper function in
     [`legacy-request-handler.js`][legacy-request-handler]. This callback
     delegates to a callback in `BaseService.register` with four different
-    parameters `( queryParams, match, sendBadge, request )`, which
+    parameters `( queryParams, match, sendBadge )`, which
     then runs `BaseService.invoke`. `BaseService.invoke` instantiates the
     service and runs `BaseService#handle`.
 
@@ -129,12 +129,12 @@ test this kind of logic through unit tests (e.g. of `render()` and
     handle unresponsive service code and the next callback is invoked: the
     legacy handler function.
 3.  The legacy handler function receives
-    `( queryParams, match, sendBadge, request )`. Its job is to extract data
-    from the regex `match` and `queryParams`, invoke `request` to fetch
-    whatever data it needs, and then invoke `sendBadge` with the result.
+    `( queryParams, match, sendBadge )`. Its job is to extract data
+    from the regex `match` and `queryParams`, and then invoke `sendBadge`
+    with the result.
 4.  The implementation of this function is in `BaseService.register`. It
     works by running `BaseService.invoke`, which instantiates the service,
-    injects more dependencies, and invokes `BaseService#handle` which is
+    injects more dependencies, and invokes `BaseService.handle` which is
     implemented by the service subclass.
 5.  The job of `handle()`, which should be implemented by each service
     subclass, is to return an object which partially describes a badge or

services/github/auth/acceptor.js

@@ -1,6 +1,6 @@
 import queryString from 'query-string'
 import request from 'request'
-import { userAgent } from '../../../core/base-service/legacy-request-handler.js'
+import { userAgent } from '../../../core/base-service/got.js'
 import log from '../../../core/server/log.js'
 
 function setRoutes({ server, authHelper, onTokenAccepted }) {

services/github/github-api-provider.js

@@ -1,7 +1,7 @@
 import Joi from 'joi'
 import log from '../../core/server/log.js'
 import { TokenPool } from '../../core/token-pooling/token-pool.js'
-import { userAgent } from '../../core/base-service/legacy-request-handler.js'
+import { userAgent } from '../../core/base-service/got.js'
 import { nonNegativeInteger } from '../validators.js'
 import { ImproperlyConfigured } from '../index.js'
 

services/librariesio/librariesio-api-provider.js

@@ -1,7 +1,7 @@
 import { ImproperlyConfigured } from '../index.js'
 import log from '../../core/server/log.js'
 import { TokenPool } from '../../core/token-pooling/token-pool.js'
-import { userAgent } from '../../core/base-service/legacy-request-handler.js'
+import { userAgent } from '../../core/base-service/got.js'
 
 // Provides an interface to the Libraries.io API.
 export default class LibrariesIoApiProvider {