migrate github badges to use got instead of request; affects [github librariesio] (#7212)

* migrate github badges to use got instead of request

* simplify creation of requestFetcher in libraries.io base

* improve libraries.io connection error test

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

core/base-service/base.js

@@ -450,8 +450,7 @@ class BaseService {
           const namedParams = namedParamsForMatch(captureNames, match, this)
           const serviceData = await this.invoke(
             {
-              sendAndCacheRequest: fetcher,
-              sendAndCacheRequestWithCallbacks: request,
+              sendAndCacheRequest: fetcher, // TODO: rename sendAndCacheRequest
               githubApiProvider,
               librariesIoApiProvider,
               metricHelper,

core/base-service/got.js

@@ -64,7 +64,8 @@ async function sendRequest(gotWrapper, url, options) {
   }
 }
 
-function fetchFactory(fetchLimitBytes) {
+const TEN_MB = 10485760
+function fetchFactory(fetchLimitBytes = TEN_MB) {
   const gotWithLimit = got.extend({
     handlers: [
       (options, next) => {

services/github/github-api-provider.integration.js

@@ -1,7 +1,8 @@
 import { expect } from 'chai'
 import config from 'config'
-import request from 'request'
+import { fetchFactory } from '../../core/base-service/got.js'
 import GithubApiProvider from './github-api-provider.js'
+const requestFetcher = fetchFactory()
 
 describe('Github API provider', function () {
   const baseUrl = process.env.GITHUB_URL || 'https://api.github.com'
@@ -30,8 +31,8 @@ describe('Github API provider', function () {
     it('should be able to run 10 requests', async function () {
       this.timeout('20s')
       for (let i = 0; i < 10; ++i) {
-        await githubApiProvider.requestAsPromise(
-          request,
+        await githubApiProvider.fetch(
+          requestFetcher,
           '/repos/rust-lang/rust',
           {}
         )
@@ -52,8 +53,8 @@ describe('Github API provider', function () {
 
     const headers = []
     async function performOneRequest() {
-      const { res } = await githubApiProvider.requestAsPromise(
-        request,
+      const { res } = await githubApiProvider.fetch(
+        requestFetcher,
         '/repos/rust-lang/rust',
         {}
       )

services/github/github-api-provider.js

@@ -3,6 +3,7 @@ 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 { nonNegativeInteger } from '../validators.js'
+import { ImproperlyConfigured } from '../index.js'
 
 const headerSchema = Joi.object({
   'x-ratelimit-limit': nonNegativeInteger,
@@ -139,10 +140,7 @@ class GithubApiProvider {
     }
   }
 
-  // Act like request(), but tweak headers and query to avoid hitting a rate
-  // limit. Inject `request` so we can pass in `cachingRequest` from
-  // `request-handler.js`.
-  request(request, url, options = {}, callback) {
+  async fetch(requestFetcher, url, options = {}) {
     const { baseUrl } = this
 
     let token
@@ -151,8 +149,10 @@ class GithubApiProvider {
       try {
         token = this.tokenForUrl(url)
       } catch (e) {
-        callback(e)
-        return
+        log.error(e)
+        throw new ImproperlyConfigured({
+          prettyMessage: 'Unable to select next Github token from pool',
+        })
       }
       tokenString = token.id
     } else {
@@ -162,8 +162,6 @@ class GithubApiProvider {
     const mergedOptions = {
       ...options,
       ...{
-        url,
-        baseUrl,
         headers: {
           'User-Agent': userAgent,
           Authorization: `token ${tokenString}`,
@@ -171,31 +169,15 @@ class GithubApiProvider {
         },
       },
     }
-
-    request(mergedOptions, (err, res, buffer) => {
-      if (err === null) {
-        if (this.withPooling) {
-          if (res.statusCode === 401) {
-            this.invalidateToken(token)
-          } else if (res.statusCode < 500) {
-            this.updateToken({ token, url, res })
-          }
-        }
+    const response = await requestFetcher(`${baseUrl}${url}`, mergedOptions)
+    if (this.withPooling) {
+      if (response.res.statusCode === 401) {
+        this.invalidateToken(token)
+      } else if (response.res.statusCode < 500) {
+        this.updateToken({ token, url, res: response.res })
       }
-      callback(err, res, buffer)
-    })
-  }
-
-  requestAsPromise(request, url, options) {
-    return new Promise((resolve, reject) => {
-      this.request(request, url, options, (err, res, buffer) => {
-        if (err) {
-          reject(err)
-        } else {
-          resolve({ res, buffer })
-        }
-      })
-    })
+    }
+    return response
   }
 }
 

services/github/github-api-provider.spec.js

@@ -21,47 +21,35 @@ describe('Github API provider', function () {
   })
 
   context('a search API request', function () {
-    const mockRequest = (options, callback) => {
-      callback()
-    }
-    it('should obtain an appropriate token', function (done) {
-      provider.request(mockRequest, '/search', {}, (err, res, buffer) => {
-        expect(err).to.be.undefined
-        expect(provider.searchTokens.next).to.have.been.calledOnce
-        expect(provider.standardTokens.next).not.to.have.been.called
-        expect(provider.graphqlTokens.next).not.to.have.been.called
-        done()
-      })
+    it('should obtain an appropriate token', async function () {
+      const mockResponse = { res: { headers: {} } }
+      const mockRequest = sinon.stub().resolves(mockResponse)
+      await provider.fetch(mockRequest, '/search', {})
+      expect(provider.searchTokens.next).to.have.been.calledOnce
+      expect(provider.standardTokens.next).not.to.have.been.called
+      expect(provider.graphqlTokens.next).not.to.have.been.called
     })
   })
 
   context('a graphql API request', function () {
-    const mockRequest = (options, callback) => {
-      callback()
-    }
-    it('should obtain an appropriate token', function (done) {
-      provider.request(mockRequest, '/graphql', {}, (err, res, buffer) => {
-        expect(err).to.be.undefined
-        expect(provider.searchTokens.next).not.to.have.been.called
-        expect(provider.standardTokens.next).not.to.have.been.called
-        expect(provider.graphqlTokens.next).to.have.been.calledOnce
-        done()
-      })
+    it('should obtain an appropriate token', async function () {
+      const mockResponse = { res: { headers: {} } }
+      const mockRequest = sinon.stub().resolves(mockResponse)
+      await provider.fetch(mockRequest, '/graphql', {})
+      expect(provider.searchTokens.next).not.to.have.been.called
+      expect(provider.standardTokens.next).not.to.have.been.called
+      expect(provider.graphqlTokens.next).to.have.been.calledOnce
     })
   })
 
   context('a core API request', function () {
-    const mockRequest = (options, callback) => {
-      callback()
-    }
-    it('should obtain an appropriate token', function (done) {
-      provider.request(mockRequest, '/repo', {}, (err, res, buffer) => {
-        expect(err).to.be.undefined
-        expect(provider.searchTokens.next).not.to.have.been.called
-        expect(provider.standardTokens.next).to.have.been.calledOnce
-        expect(provider.graphqlTokens.next).not.to.have.been.called
-        done()
-      })
+    it('should obtain an appropriate token', async function () {
+      const mockResponse = { res: { headers: {} } }
+      const mockRequest = sinon.stub().resolves(mockResponse)
+      await provider.fetch(mockRequest, '/repo', {})
+      expect(provider.searchTokens.next).not.to.have.been.called
+      expect(provider.standardTokens.next).to.have.been.calledOnce
+      expect(provider.graphqlTokens.next).not.to.have.been.called
     })
   })
 
@@ -70,40 +58,32 @@ describe('Github API provider', function () {
     const remaining = 7955
     const nextReset = 123456789
     const mockResponse = {
-      statusCode: 200,
-      headers: {
-        'x-ratelimit-limit': rateLimit,
-        'x-ratelimit-remaining': remaining,
-        'x-ratelimit-reset': nextReset,
+      res: {
+        statusCode: 200,
+        headers: {
+          'x-ratelimit-limit': rateLimit,
+          'x-ratelimit-remaining': remaining,
+          'x-ratelimit-reset': nextReset,
+        },
+        buffer: Buffer.alloc(0),
       },
     }
-    const mockBuffer = Buffer.alloc(0)
-    const mockRequest = (...args) => {
-      const callback = args.pop()
-      callback(null, mockResponse, mockBuffer)
-    }
+    const mockRequest = sinon.stub().resolves(mockResponse)
 
-    it('should invoke the callback', function (done) {
-      provider.request(mockRequest, '/foo', {}, (err, res, buffer) => {
-        expect(err).to.equal(null)
-        expect(Object.is(res, mockResponse)).to.be.true
-        expect(Object.is(buffer, mockBuffer)).to.be.true
-        done()
-      })
+    it('should return the response', async function () {
+      const res = await provider.fetch(mockRequest, '/repo', {})
+      expect(Object.is(res, mockResponse)).to.be.true
     })
 
-    it('should update the token with the expected values', function (done) {
-      provider.request(mockRequest, '/foo', {}, (err, res, buffer) => {
-        expect(err).to.equal(null)
-        const expectedUsesRemaining =
-          remaining - Math.ceil(reserveFraction * rateLimit)
-        expect(mockStandardToken.update).to.have.been.calledWith(
-          expectedUsesRemaining,
-          nextReset
-        )
-        expect(mockStandardToken.invalidate).not.to.have.been.called
-        done()
-      })
+    it('should update the token with the expected values', async function () {
+      await provider.fetch(mockRequest, '/foo', {})
+      const expectedUsesRemaining =
+        remaining - Math.ceil(reserveFraction * rateLimit)
+      expect(mockStandardToken.update).to.have.been.calledWith(
+        expectedUsesRemaining,
+        nextReset
+      )
+      expect(mockStandardToken.invalidate).not.to.have.been.called
     })
   })
 
@@ -112,9 +92,10 @@ describe('Github API provider', function () {
     const remaining = 7955
     const nextReset = 123456789
     const mockResponse = {
-      statusCode: 200,
-      headers: {},
-      body: `{
+      res: {
+        statusCode: 200,
+        headers: {},
+        body: `{
         "data": {
           "rateLimit": {
             "limit": 12500,
@@ -124,67 +105,46 @@ describe('Github API provider', function () {
           }
         }
       }`,
+      },
     }
-    const mockBuffer = Buffer.alloc(0)
-    const mockRequest = (...args) => {
-      const callback = args.pop()
-      callback(null, mockResponse, mockBuffer)
-    }
+    const mockRequest = sinon.stub().resolves(mockResponse)
 
-    it('should invoke the callback', function (done) {
-      provider.request(mockRequest, '/graphql', {}, (err, res, buffer) => {
-        expect(err).to.equal(null)
-        expect(Object.is(res, mockResponse)).to.be.true
-        expect(Object.is(buffer, mockBuffer)).to.be.true
-        done()
-      })
+    it('should return the response', async function () {
+      const res = await provider.fetch(mockRequest, '/graphql', {})
+      expect(Object.is(res, mockResponse)).to.be.true
     })
 
-    it('should update the token with the expected values', function (done) {
-      provider.request(mockRequest, '/graphql', {}, (err, res, buffer) => {
-        expect(err).to.equal(null)
-        const expectedUsesRemaining =
-          remaining - Math.ceil(reserveFraction * rateLimit)
-        expect(mockGraphqlToken.update).to.have.been.calledWith(
-          expectedUsesRemaining,
-          nextReset
-        )
-        expect(mockGraphqlToken.invalidate).not.to.have.been.called
-        done()
-      })
+    it('should update the token with the expected values', async function () {
+      await provider.fetch(mockRequest, '/graphql', {})
+      const expectedUsesRemaining =
+        remaining - Math.ceil(reserveFraction * rateLimit)
+      expect(mockGraphqlToken.update).to.have.been.calledWith(
+        expectedUsesRemaining,
+        nextReset
+      )
+      expect(mockGraphqlToken.invalidate).not.to.have.been.called
     })
   })
 
   context('an unauthorized response', function () {
-    const mockResponse = { statusCode: 401 }
-    const mockBuffer = Buffer.alloc(0)
-    const mockRequest = (...args) => {
-      const callback = args.pop()
-      callback(null, mockResponse, mockBuffer)
-    }
-
-    it('should invoke the callback and update the token with the expected values', function (done) {
-      provider.request(mockRequest, '/foo', {}, (err, res, buffer) => {
-        expect(err).to.equal(null)
-        expect(mockStandardToken.invalidate).to.have.been.calledOnce
-        expect(mockStandardToken.update).not.to.have.been.called
-        done()
-      })
+    it('should invoke the callback and update the token with the expected values', async function () {
+      const mockResponse = { res: { statusCode: 401, headers: {} } }
+      const mockRequest = sinon.stub().resolves(mockResponse)
+      await provider.fetch(mockRequest, '/foo', {})
+      expect(mockStandardToken.invalidate).to.have.been.calledOnce
+      expect(mockStandardToken.update).not.to.have.been.called
     })
   })
 
   context('a connection error', function () {
-    const mockRequest = (...args) => {
-      const callback = args.pop()
-      callback(Error('connection timeout'))
-    }
-
-    it('should pass the error to the callback', function (done) {
-      provider.request(mockRequest, '/foo', {}, (err, res, buffer) => {
-        expect(err).to.be.an.instanceof(Error)
-        expect(err.message).to.equal('connection timeout')
-        done()
-      })
+    it('should throw an exception', function () {
+      const msg = 'connection timeout'
+      const requestError = new Error(msg)
+      const mockRequest = sinon.stub().rejects(requestError)
+      return expect(provider.fetch(mockRequest, '/foo', {})).to.be.rejectedWith(
+        Error,
+        'connection timeout'
+      )
     })
   })
 })

services/github/github-auth-service.js

@@ -2,21 +2,15 @@ import gql from 'graphql-tag'
 import { mergeQueries } from '../../core/base-service/graphql.js'
 import { BaseGraphqlService, BaseJsonService } from '../index.js'
 
-function createRequestFetcher(context, config) {
-  const { sendAndCacheRequestWithCallbacks, githubApiProvider } = context
-
-  return async (url, options) =>
-    githubApiProvider.requestAsPromise(
-      sendAndCacheRequestWithCallbacks,
-      url,
-      options
-    )
+function createRequestFetcher(context) {
+  const { sendAndCacheRequest, githubApiProvider } = context
+  return githubApiProvider.fetch.bind(githubApiProvider, sendAndCacheRequest)
 }
 
 class GithubAuthV3Service extends BaseJsonService {
   constructor(context, config) {
     super(context, config)
-    this._requestFetcher = createRequestFetcher(context, config)
+    this._requestFetcher = createRequestFetcher(context)
     this.staticAuthConfigured = true
   }
 }
@@ -30,7 +24,7 @@ class ConditionalGithubAuthV3Service extends BaseJsonService {
   constructor(context, config) {
     super(context, config)
     if (context.githubApiProvider.globalToken) {
-      this._requestFetcher = createRequestFetcher(context, config)
+      this._requestFetcher = createRequestFetcher(context)
       this.staticAuthConfigured = true
     } else {
       this.staticAuthConfigured = false
@@ -41,7 +35,7 @@ class ConditionalGithubAuthV3Service extends BaseJsonService {
 class GithubAuthV4Service extends BaseGraphqlService {
   constructor(context, config) {
     super(context, config)
-    this._requestFetcher = createRequestFetcher(context, config)
+    this._requestFetcher = createRequestFetcher(context)
     this.staticAuthConfigured = true
   }
 

services/github/github-auth-service.spec.js

@@ -14,7 +14,7 @@ describe('GithubAuthV3Service', function () {
         schema: Joi.object({
           requiredString: Joi.string().required(),
         }).required(),
-        url: 'https://github-api.example.com/repos/badges/shields/check-runs',
+        url: '/repos/badges/shields/check-runs',
         options: {
           headers: {
             Accept: 'application/vnd.github.antiope-preview+json',
@@ -26,10 +26,17 @@ describe('GithubAuthV3Service', function () {
   }
 
   it('forwards custom Accept header', async function () {
-    const sendAndCacheRequestWithCallbacks = sinon.stub().returns(
+    const sendAndCacheRequest = sinon.stub().returns(
       Promise.resolve({
         buffer: '{"requiredString": "some-string"}',
-        res: { statusCode: 200 },
+        res: {
+          statusCode: 200,
+          headers: {
+            'x-ratelimit-limit': 12500,
+            'x-ratelimit-remaining': 7955,
+            'x-ratelimit-reset': 123456789,
+          },
+        },
       })
     )
     const githubApiProvider = new GithubApiProvider({
@@ -39,18 +46,19 @@ describe('GithubAuthV3Service', function () {
     sinon.stub(githubApiProvider.standardTokens, 'next').returns(mockToken)
 
     DummyGithubAuthV3Service.invoke({
-      sendAndCacheRequestWithCallbacks,
+      sendAndCacheRequest,
       githubApiProvider,
     })
 
-    expect(sendAndCacheRequestWithCallbacks).to.have.been.calledOnceWith({
-      headers: {
-        'User-Agent': 'Shields.io/2003a',
-        Accept: 'application/vnd.github.antiope-preview+json',
-        Authorization: 'token undefined',
-      },
-      url: 'https://github-api.example.com/repos/badges/shields/check-runs',
-      baseUrl: 'https://github-api.example.com',
-    })
+    expect(sendAndCacheRequest).to.have.been.calledOnceWith(
+      'https://github-api.example.com/repos/badges/shields/check-runs',
+      {
+        headers: {
+          'User-Agent': 'Shields.io/2003a',
+          Accept: 'application/vnd.github.antiope-preview+json',
+          Authorization: 'token undefined',
+        },
+      }
+    )
   })
 })

services/librariesio/librariesio-api-provider.spec.js

@@ -119,14 +119,10 @@ describe('LibrariesIoApiProvider', function () {
     const requestError = new Error(msg)
     const mockRequest = sinon.stub().rejects(requestError)
 
-    it('should pass the error to the callback', async function () {
-      try {
-        await provider.fetch(mockRequest, '/npm/badge-maker')
-        expect(false).to.be.true
-      } catch (err) {
-        expect(err).to.be.an.instanceof(Error)
-        expect(err.message).to.equal(msg)
-      }
+    it('should throw an exception', async function () {
+      return expect(
+        provider.fetch(mockRequest, '/npm/badge-maker', {})
+      ).to.be.rejectedWith(Error, 'connection timeout')
     })
   })
 })

services/librariesio/librariesio-base.js

@@ -10,17 +10,14 @@ const projectSchema = Joi.object({
   rank: anyInteger,
 }).required()
 
-function createRequestFetcher(context, config) {
-  const { sendAndCacheRequest, librariesIoApiProvider } = context
-
-  return async (url, options) =>
-    await librariesIoApiProvider.fetch(sendAndCacheRequest, url, options)
-}
-
 export default class LibrariesIoBase extends BaseJsonService {
   constructor(context, config) {
     super(context, config)
-    this._requestFetcher = createRequestFetcher(context, config)
+    const { sendAndCacheRequest, librariesIoApiProvider } = context
+    this._requestFetcher = librariesIoApiProvider.fetch.bind(
+      librariesIoApiProvider,
+      sendAndCacheRequest
+    )
   }
 
   async fetchProject({ platform, scope, packageName }) {

services/suggest.js

@@ -5,7 +5,8 @@
 // This endpoint is called from frontend/components/suggestion-and-search.js.
 
 import { URL } from 'url'
-import request from 'request'
+import { fetchFactory } from '../core/base-service/got.js'
+const requestFetcher = fetchFactory()
 
 function twitterPage(url) {
   if (url.protocol === null) {
@@ -75,8 +76,8 @@ async function githubLicense(githubApiProvider, user, repo) {
 
   let link = `https://github.com/${repoSlug}`
 
-  const { buffer } = await githubApiProvider.requestAsPromise(
-    request,
+  const { buffer } = await githubApiProvider.fetch(
+    requestFetcher,
     `/repos/${repoSlug}/license`
   )
   try {