Merge pull request #1047 from github-community-projects/decyjphr-incorporate-pr-1018-nop-results

fix: full-sync NOP results without check run + idempotent ruleset create

Author: decyjphr

lib/plugins/rulesets.js

@@ -291,7 +291,7 @@ module.exports = class Rulesets extends Diffable {
         this.log.debug(`Ruleset created successfully ${JSON.stringify(res.url)}`)
         return res
       }).catch(e => {
-        return this.handleError(e)
+        return this.handleDuplicateOrError(e, attrs)
       })
     } else {
       if (this.nop) {
@@ -305,11 +305,45 @@ module.exports = class Rulesets extends Diffable {
         this.log.debug(`Ruleset created successfully ${JSON.stringify(res.url)}`)
         return res
       }).catch(e => {
-        return this.handleError(e)
+        return this.handleDuplicateOrError(e, attrs)
       })
     }
   }
 
+  // A ruleset create (POST) is not idempotent. Octokit's retry / auth-app
+  // layers can re-send a POST that already succeeded, and a repo can also be
+  // processed by two overlapping syncs (e.g. a full sync racing with the
+  // repository.created webhook). In both cases the second create hits
+  // "Name must be unique" (422) even though the ruleset now exists. Instead of
+  // failing the whole run, reconcile by looking the ruleset up by name and
+  // updating it in place so the create effectively becomes idempotent.
+  isDuplicateNameError (e) {
+    if (!e || e.status !== 422) return false
+    const errors = e.response && e.response.data && e.response.data.errors
+    const list = Array.isArray(errors) ? errors : []
+    return list.some(err => {
+      const msg = typeof err === 'string' ? err : (err && err.message)
+      return typeof msg === 'string' && /name must be unique/i.test(msg)
+    })
+  }
+
+  handleDuplicateOrError (e, attrs) {
+    if (!this.isDuplicateNameError(e)) {
+      return this.handleError(e)
+    }
+    this.log.debug(`Ruleset '${attrs && attrs.name}' already exists (concurrent or retried create); reconciling by update`)
+    return this.find().then(existing => {
+      const match = Array.isArray(existing)
+        ? existing.find(record => this.comparator(record, attrs))
+        : undefined
+      if (!match) {
+        return this.handleError(e)
+      }
+      const { id: _ignoredId, ...attrsWithoutId } = attrs || {}
+      return this.update(match, attrsWithoutId)
+    }).catch(err => this.handleError(err))
+  }
+
   remove (existing) {
     const parms = this.wrapAttrs(Object.assign({ id: existing.id }))
     if (this.scope === 'org') {

lib/settings.js

@@ -1161,6 +1161,18 @@ class Settings {
       })
     }
 
+    // Full-sync NOP runs do not have the webhook fields needed to report to a
+    // check run. Keep potentially sensitive diff values at debug level and log
+    // only a value-free summary at info level.
+    if (!payload?.check_run || !payload?.repository) {
+      this.log.debug({ results: this.results }, 'Dry-run results')
+      const summary = this.results
+        .map(res => `${res.type} ${res.plugin} ${res.repo}: ${res.action?.msg ?? ''}`)
+        .join('\n')
+      this.log.info(`Dry-run finished with ${this.results.length} planned change(s); the full diff is logged at debug level.\n${summary}`)
+      return
+    }
+
     let error = false
     const stats = {
       reposProcessed: {},

test/unit/lib/plugins/rulesets.test.js

@@ -179,6 +179,84 @@ describe('Rulesets', () => {
     })
   })
 
+  describe('idempotent create when the ruleset already exists (retried/concurrent POST)', () => {
+    function duplicateNameError () {
+      const e = new Error('Validation Failed')
+      e.status = 422
+      e.response = { data: { errors: ['Name must be unique'] } }
+      return e
+    }
+
+    function wireRequest (routeResults) {
+      const calls = []
+      const request = jest.fn().mockImplementation((route, body) => {
+        calls.push({ route, body })
+        const handler = routeResults[route]
+        return handler ? handler() : Promise.resolve({ url: route })
+      })
+      request.endpoint = jest.fn().mockImplementation((route, body) => ({ url: route, body }))
+      request.endpoint.merge = jest.fn().mockImplementation((route, body) => ({ method: 'GET', url: route, ...body }))
+      github.request = request
+      return calls
+    }
+
+    it('reconciles a repo ruleset by updating the existing one on 422 "Name must be unique"', async () => {
+      const attrs = generateRequestRuleset(0, 'synk', repo_conditions, [])
+      delete attrs.id
+      const existing = generateResponseRuleset(42, 'synk', repo_conditions, [])
+      const calls = wireRequest({
+        'POST /repos/{owner}/{repo}/rulesets': () => Promise.reject(duplicateNameError())
+      })
+      github.paginate = jest.fn()
+        .mockResolvedValueOnce([{ id: 42, name: 'synk', source_type: 'Repository' }])
+        .mockResolvedValueOnce([existing])
+
+      const plugin = configure([attrs])
+      await plugin.add(attrs)
+
+      const put = calls.find(c => c.route === 'PUT /repos/{owner}/{repo}/rulesets/{id}')
+      expect(put).toBeDefined()
+      expect(put.body.id).toBe(42)
+    })
+
+    it('reconciles an org ruleset by updating the existing one on 422 "Name must be unique"', async () => {
+      const attrs = generateRequestRuleset(0, 'synk', org_conditions, [], true)
+      delete attrs.id
+      const existing = generateResponseRuleset(7, 'synk', org_conditions, [], true)
+      const calls = wireRequest({
+        'POST /orgs/{org}/rulesets': () => Promise.reject(duplicateNameError())
+      })
+      github.paginate = jest.fn()
+        .mockResolvedValueOnce([{ id: 7, name: 'synk', source_type: 'Organization' }])
+        .mockResolvedValueOnce([existing])
+
+      const plugin = configure([attrs], 'org')
+      await plugin.add(attrs)
+
+      const put = calls.find(c => c.route === 'PUT /orgs/{org}/rulesets/{id}')
+      expect(put).toBeDefined()
+      expect(put.body.id).toBe(7)
+    })
+
+    it('does not reconcile (surfaces the error) for a 422 that is not a name-uniqueness violation', async () => {
+      const attrs = generateRequestRuleset(0, 'synk', repo_conditions, [])
+      delete attrs.id
+      const other = new Error('Validation Failed')
+      other.status = 422
+      other.response = { data: { errors: ['Something else is invalid'] } }
+      const calls = wireRequest({
+        'POST /repos/{owner}/{repo}/rulesets': () => Promise.reject(other)
+      })
+      github.paginate = jest.fn()
+
+      const plugin = configure([attrs])
+      await plugin.add(attrs)
+
+      expect(github.paginate).not.toHaveBeenCalled()
+      expect(calls.some(c => c.route === 'PUT /repos/{owner}/{repo}/rulesets/{id}')).toBe(false)
+    })
+  })
+
   describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and no status checks exist in GitHub', () => {
     it('it initialises the status checks with an empty list', () => {
       // Mock the GitHub API response

test/unit/lib/settings.test.js

@@ -1057,6 +1057,35 @@ repository:
         expect(msgs.some(m => /teams/.test(m))).toBe(true)
       })
 
+      it.each([
+        ['without a check run', {}],
+        ['without a repository', { check_run: { id: 123 } }]
+      ])('28. full-sync dry run %s logs a value-free summary instead of updating a check run', async (_description, payload) => {
+        stubContext.payload = { installation: { id: 123 }, ...payload }
+        stubContext.octokit.checks = { update: jest.fn().mockResolvedValue({}) }
+
+        const settings = new Settings(true, stubContext, mockRepo, {}, mockRef)
+        settings.results = [{
+          type: 'INFO',
+          plugin: 'Variables',
+          repo: 'test/test-repo',
+          endpoint: '',
+          action: {
+            msg: 'Changes found',
+            additions: {},
+            modifications: { MY_VAR: { value: 'plain-value' } },
+            deletions: {}
+          }
+        }]
+
+        await settings.handleResults()
+
+        expect(stubContext.log.info).toHaveBeenCalledWith(expect.stringContaining('Changes found'))
+        expect(stubContext.log.info).not.toHaveBeenCalledWith(expect.stringContaining('plain-value'))
+        expect(stubContext.log.debug).toHaveBeenCalledWith({ results: settings.results }, 'Dry-run results')
+        expect(stubContext.octokit.checks.update).not.toHaveBeenCalled()
+      })
+
       it('28. base-config filtering preserves org-rulesets informational NopCommands', async () => {
         stubContext.payload.repository = { owner: { login: 'test' }, name: 'safe-settings' }
         stubContext.payload.check_run = { id: 123, check_suite: { pull_requests: [{ number: 456 }] } }