Rewrite GitHub commit status (#3186)

* WIP

* Parse the error response

* Clarify

* Restore one test

* Add a schema

core/base-service/errors.js

@@ -34,6 +34,7 @@ class NotFound extends ShieldsRuntimeError {
         ? 'Not Found'
         : `Not Found: ${prettyMessage}`
     super(props, message)
+    this.response = props.response
   }
 }
 
@@ -50,6 +51,7 @@ class InvalidResponse extends ShieldsRuntimeError {
       ? `Invalid Response: ${props.underlyingError.message}`
       : 'Invalid Response'
     super(props, message)
+    this.response = props.response
   }
 }
 
@@ -66,6 +68,7 @@ class Inaccessible extends ShieldsRuntimeError {
       ? `Inaccessible: ${props.underlyingError.message}`
       : 'Inaccessible'
     super(props, message)
+    this.response = props.response
   }
 }
 
@@ -82,6 +85,7 @@ class InvalidParameter extends ShieldsRuntimeError {
       ? `Invalid Parameter: ${props.underlyingError.message}`
       : 'Invalid Parameter'
     super(props, message)
+    this.response = props.response
   }
 }
 

lib/error-helper.js

@@ -30,9 +30,10 @@ function checkErrorResponse(badgeData, err, res, errorMessages = {}) {
 
 checkErrorResponse.asPromise = function(errorMessages = {}) {
   return async function({ buffer, res }) {
+    let error
     errorMessages = { ...defaultErrorMessages, ...errorMessages }
     if (res.statusCode === 404) {
-      throw new NotFound({ prettyMessage: errorMessages[404] })
+      error = new NotFound({ prettyMessage: errorMessages[404] })
     } else if (res.statusCode !== 200) {
       const underlying = Error(
         `Got status code ${res.statusCode} (expected 200)`
@@ -42,11 +43,18 @@ checkErrorResponse.asPromise = function(errorMessages = {}) {
         props.prettyMessage = errorMessages[res.statusCode]
       }
       if (res.statusCode >= 500) {
-        throw new Inaccessible(props)
+        error = new Inaccessible(props)
+      } else {
+        error = new InvalidResponse(props)
       }
-      throw new InvalidResponse(props)
     }
-    return { buffer, res }
+    if (error) {
+      error.response = res
+      error.buffer = buffer
+      throw error
+    } else {
+      return { buffer, res }
+    }
   }
 }
 

lib/error-helper.spec.js

@@ -52,31 +52,40 @@ describe('Standard Error Handler', function() {
 })
 
 describe('async error handler', function() {
+  const buffer = Buffer.from('some stuff')
+
   context('when status is 200', function() {
     it('passes through the inputs', async function() {
-      const args = { buffer: 'buffer', res: { statusCode: 200 } }
+      const res = { statusCode: 200 }
-      expect(await checkErrorResponse.asPromise()(args)).to.deep.equal(args)
+      expect(
+        await checkErrorResponse.asPromise()({ res, buffer })
+      ).to.deep.equal({ res, buffer })
     })
   })
 
   context('when status is 404', function() {
+    const buffer = Buffer.from('some stuff')
     const res = { statusCode: 404 }
 
     it('throws NotFound', async function() {
       try {
-        await checkErrorResponse.asPromise()({ res })
+        await checkErrorResponse.asPromise()({ res, buffer })
         expect.fail('Expected to throw')
       } catch (e) {
         expect(e).to.be.an.instanceof(NotFound)
         expect(e.message).to.equal('Not Found')
         expect(e.prettyMessage).to.equal('not found')
+        expect(e.response).to.equal(res)
+        expect(e.buffer).to.equal(buffer)
       }
     })
 
     it('displays the custom not found message', async function() {
       const notFoundMessage = 'no goblins found'
       try {
-        await checkErrorResponse.asPromise({ 404: notFoundMessage })({ res })
+        await checkErrorResponse.asPromise({
+          404: notFoundMessage,
+        })({ res, buffer })
         expect.fail('Expected to throw')
       } catch (e) {
         expect(e).to.be.an.instanceof(NotFound)
@@ -90,7 +99,7 @@ describe('async error handler', function() {
     it('throws InvalidResponse', async function() {
       const res = { statusCode: 499 }
       try {
-        await checkErrorResponse.asPromise()({ res })
+        await checkErrorResponse.asPromise()({ res, buffer })
         expect.fail('Expected to throw')
       } catch (e) {
         expect(e).to.be.an.instanceof(InvalidResponse)
@@ -98,6 +107,8 @@ describe('async error handler', function() {
           'Invalid Response: Got status code 499 (expected 200)'
         )
         expect(e.prettyMessage).to.equal('invalid')
+        expect(e.response).to.equal(res)
+        expect(e.buffer).to.equal(buffer)
       }
     })
 
@@ -122,7 +133,7 @@ describe('async error handler', function() {
     it('throws Inaccessible', async function() {
       const res = { statusCode: 503 }
       try {
-        await checkErrorResponse.asPromise()({ res })
+        await checkErrorResponse.asPromise()({ res, buffer })
         expect.fail('Expected to throw')
       } catch (e) {
         expect(e).to.be.an.instanceof(Inaccessible)
@@ -130,6 +141,8 @@ describe('async error handler', function() {
           'Inaccessible: Got status code 503 (expected 200)'
         )
         expect(e.prettyMessage).to.equal('inaccessible')
+        expect(e.response).to.equal(res)
+        expect(e.buffer).to.equal(buffer)
       }
     })
 
@@ -138,7 +151,7 @@ describe('async error handler', function() {
       try {
         await checkErrorResponse.asPromise({
           500: 'server overloaded',
-        })({ res })
+        })({ res, buffer })
         expect.fail('Expected to throw')
       } catch (e) {
         expect(e).to.be.an.instanceof(Inaccessible)

services/github/github-commit-status.service.js

@@ -1,19 +1,16 @@
 'use strict'
 
-const LegacyService = require('../legacy-service')
+const Joi = require('joi')
-const { makeBadgeData: getBadgeData } = require('../../lib/badge-data')
+const { NotFound, InvalidParameter } = require('..')
-const {
+const { GithubAuthService } = require('./github-auth-service')
-  documentation,
+const { documentation, errorMessagesFor } = require('./github-helpers')
-  checkErrorResponse: githubCheckErrorResponse,
-} = require('./github-helpers')
 
-// This legacy service should be rewritten to use e.g. BaseJsonService.
+const schema = Joi.object({
-//
+  // https://stackoverflow.com/a/23969867/893113
-// Tips for rewriting:
+  status: Joi.equal('identical', 'ahead', 'behind', 'diverged'),
-// https://github.com/badges/shields/blob/master/doc/rewriting-services.md
+}).required()
-//
+
-// Do not base new services on this code.
+module.exports = class GithubCommitStatus extends GithubAuthService {
-module.exports = class GithubCommitStatus extends LegacyService {
   static get category() {
     return 'issue-tracking'
   }
@@ -35,71 +32,56 @@ module.exports = class GithubCommitStatus extends LegacyService {
           branch: 'master',
           commit: '5d4ab86b1b5ddfb3c4a70a70bd19932c52603b8c',
         },
-        staticPreview: {
+        staticPreview: this.render({
-          label: 'commit status',
+          isInBranch: true,
-          message: 'in master',
+          branch: 'master',
-          color: 'brightgreen',
+        }),
-        },
         keywords: ['branch'],
         documentation,
       },
     ]
   }
 
-  static registerLegacyRouteHandler({ camp, cache, githubApiProvider }) {
+  static get defaultBadgeData() {
-    camp.route(
+    return {
-      /^\/github\/commit-status\/([^/]+)\/([^/]+)\/([^/]+)\/([^/]+)\.(svg|png|gif|jpg|json)$/,
+      label: 'commit status',
-      cache((data, match, sendBadge, request) => {
+    }
-        const [, user, repo, branch, commit, format] = match
+  }
-        const apiUrl = `/repos/${user}/${repo}/compare/${branch}...${commit}`
+
-        const badgeData = getBadgeData('commit status', data)
+  static render({ isInBranch, branch }) {
-        githubApiProvider.request(request, apiUrl, {}, (err, res, buffer) => {
+    if (isInBranch) {
-          if (
+      return {
-            githubCheckErrorResponse(
+        message: `in ${branch}`,
-              badgeData,
+        color: 'brightgreen',
-              err,
+      }
-              res,
+    } else {
-              'commit or branch not found'
+      // status: ahead or diverged
-            )
+      return {
-          ) {
+        message: `not in ${branch}`,
-            if (res && res.statusCode === 404) {
+        color: 'yellow',
-              try {
+      }
-                if (
+    }
-                  JSON.parse(buffer).message.startsWith(
+  }
-                    'No common ancestor between'
+
-                  )
+  async handle({ user, repo, branch, commit }) {
-                ) {
+    let status
-                  badgeData.text[1] = 'no common ancestor'
+    try {
-                  badgeData.colorscheme = 'lightgrey'
+      ;({ status } = await this._requestJson({
-                }
+        url: `/repos/${user}/${repo}/compare/${branch}...${commit}`,
-              } catch (e) {
+        errorMessages: errorMessagesFor('commit or branch not found'),
-                badgeData.text[1] = 'invalid'
+        schema,
-                badgeData.colorscheme = 'lightgrey'
+      }))
-              }
+    } catch (e) {
-            }
+      if (e instanceof NotFound) {
-            sendBadge(format, badgeData)
+        const { message } = this._parseJson(e.buffer)
-            return
+        if (message.startsWith('No common ancestor between')) {
-          }
+          throw new InvalidParameter({ prettyMessage: 'no common ancestor' })
-          try {
+        }
-            const parsedData = JSON.parse(buffer)
+      }
-            const isInBranch =
+      throw e
-              parsedData.status === 'identical' ||
+    }
-              parsedData.status === 'behind'
+
-            if (isInBranch) {
+    const isInBranch = status === 'identical' || status === 'behind'
-              badgeData.text[1] = `in ${branch}`
+    return this.constructor.render({ isInBranch, branch })
-              badgeData.colorscheme = 'brightgreen'
-            } else {
-              // status: ahead or diverged
-              badgeData.text[1] = `not in ${branch}`
-              badgeData.colorscheme = 'yellow'
-            }
-            sendBadge(format, badgeData)
-          } catch (e) {
-            badgeData.text[1] = 'invalid'
-            sendBadge(format, badgeData)
-          }
-        })
-      })
-    )
   }
 }

services/github/github-commit-status.tester.js

@@ -39,7 +39,7 @@ t.create('commit status - commit not in branch')
   .expectBadge({
     label: 'commit status',
     message: 'commit or branch not found',
-    color: 'lightgrey',
+    color: 'red',
   })
 
 t.create('commit status - unknown commit id')
@@ -59,7 +59,7 @@ t.create('commit status - unknown branch')
   .expectBadge({
     label: 'commit status',
     message: 'commit or branch not found',
-    color: 'lightgrey',
+    color: 'red',
   })
 
 t.create('commit status - no common ancestor between commit and branch')
@@ -69,71 +69,11 @@ t.create('commit status - no common ancestor between commit and branch')
   .expectBadge({
     label: 'commit status',
     message: 'no common ancestor',
-    color: 'lightgrey',
-  })
-
-t.create('commit status - invalid JSON')
-  .get(
-    '/badges/shields/master/5d4ab86b1b5ddfb3c4a70a70bd19932c52603b8c.json?style=_shields_test'
-  )
-  .intercept(nock =>
-    nock('https://api.github.com')
-      .get(
-        '/repos/badges/shields/compare/master...5d4ab86b1b5ddfb3c4a70a70bd19932c52603b8c'
-      )
-      .reply(invalidJSON)
-  )
-  .expectBadge({
-    label: 'commit status',
-    message: 'invalid',
-    color: 'lightgrey',
-  })
-
-t.create('commit status - network error')
-  .get(
-    '/badges/shields/master/5d4ab86b1b5ddfb3c4a70a70bd19932c52603b8c.json?style=_shields_test'
-  )
-  .networkOff()
-  .expectBadge({
-    label: 'commit status',
-    message: 'inaccessible',
     color: 'red',
   })
 
-t.create('commit status - github server error')
+// Since the service is responsible for parsing its error response, this tests
-  .get(
+// the service, not BaseJsonService.
-    '/badges/shields/master/5d4ab86b1b5ddfb3c4a70a70bd19932c52603b8c.json?style=_shields_test'
-  )
-  .intercept(nock =>
-    nock('https://api.github.com')
-      .get(
-        '/repos/badges/shields/compare/master...5d4ab86b1b5ddfb3c4a70a70bd19932c52603b8c'
-      )
-      .reply(500)
-  )
-  .expectBadge({
-    label: 'commit status',
-    message: 'invalid',
-    color: 'lightgrey',
-  })
-
-t.create('commit status - 404 with empty JSON form github')
-  .get(
-    '/badges/shields/master/5d4ab86b1b5ddfb3c4a70a70bd19932c52603b8c.json?style=_shields_test'
-  )
-  .intercept(nock =>
-    nock('https://api.github.com')
-      .get(
-        '/repos/badges/shields/compare/master...5d4ab86b1b5ddfb3c4a70a70bd19932c52603b8c'
-      )
-      .reply(404, {})
-  )
-  .expectBadge({
-    label: 'commit status',
-    message: 'invalid',
-    color: 'lightgrey',
-  })
-
 t.create('commit status - 404 with invalid JSON form github')
   .get(
     '/badges/shields/master/5d4ab86b1b5ddfb3c4a70a70bd19932c52603b8c.json?style=_shields_test'
@@ -147,6 +87,6 @@ t.create('commit status - 404 with invalid JSON form github')
   )
   .expectBadge({
     label: 'commit status',
-    message: 'invalid',
+    message: 'unparseable json response',
     color: 'lightgrey',
   })