From fe12ffeb0215b223d68340ea875c51b1048b5da9 Mon Sep 17 00:00:00 2001 From: William Phetsinorath Date: Tue, 1 Sep 2026 14:08:52 +0200 Subject: [PATCH] fix(server-nestjs): make SonarQube user creation idempotent on create race Refs: #2633 Signed-off-by: William Phetsinorath Change-Id: Ib4be9a86e1f36e06b70daf1975411f0f6a6a6964 --- .../sonarqube-client.service.spec.ts | 29 +++++++++ .../sonarqube/sonarqube-client.service.ts | 16 ++++- .../modules/sonarqube/sonarqube.utils.spec.ts | 64 +++++++++++++++++++ .../src/modules/sonarqube/sonarqube.utils.ts | 38 +++++++++++ 4 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 apps/server-nestjs/src/modules/sonarqube/sonarqube.utils.spec.ts diff --git a/apps/server-nestjs/src/modules/sonarqube/sonarqube-client.service.spec.ts b/apps/server-nestjs/src/modules/sonarqube/sonarqube-client.service.spec.ts index 1041a52646..07b46925ad 100644 --- a/apps/server-nestjs/src/modules/sonarqube/sonarqube-client.service.spec.ts +++ b/apps/server-nestjs/src/modules/sonarqube/sonarqube-client.service.spec.ts @@ -107,6 +107,35 @@ describe('sonarqubeClientService', () => { ) await service.createUser(user) }) + + it('should re-fetch the existing user on a create race instead of throwing', async () => { + const login = faker.internet.username() + const existing = makeSonarqubeUser({ login }) + let createCalls = 0 + server.use( + http.post(`${sonarUrl}/api/users/create`, () => { + createCalls += 1 + return HttpResponse.json({ errors: [{ msg: `User '${login}' already exists` }] }, { status: 400 }) + }), + http.get(`${sonarUrl}/api/users/search`, ({ request }) => { + expect(new URL(request.url).searchParams.get('q')).toBe(login) + return HttpResponse.json({ users: [existing], paging: { pageIndex: 1, pageSize: 10, total: 1 } }) + }), + ) + + await expect(service.createUser({ email: `${login}@example.com`, local: 'true', login, name: login, password: faker.internet.password() })).resolves.toMatchObject({ login }) + + expect(createCalls).toBe(1) + }) + + it('should rethrow when the create fails for a non-collision reason', async () => { + const login = faker.internet.username() + server.use( + http.post(`${sonarUrl}/api/users/create`, () => HttpResponse.json({ errors: [{ msg: 'forbidden' }] }, { status: 403 })), + ) + + await expect(service.createUser({ email: `${login}@example.com`, local: 'true', login, name: login, password: faker.internet.password() })).rejects.toThrow() + }) }) describe('usersDeactivate', () => { diff --git a/apps/server-nestjs/src/modules/sonarqube/sonarqube-client.service.ts b/apps/server-nestjs/src/modules/sonarqube/sonarqube-client.service.ts index b2e0015048..e7da1b330f 100644 --- a/apps/server-nestjs/src/modules/sonarqube/sonarqube-client.service.ts +++ b/apps/server-nestjs/src/modules/sonarqube/sonarqube-client.service.ts @@ -13,6 +13,7 @@ import { Inject, Injectable, Logger } from '@nestjs/common' import { StartActiveSpan } from '../infrastructure/telemetry/telemetry.decorator' import { SonarqubeHttpClientService } from './sonarqube-http-client.service' import { SONARQUBE_MAX_PAGES, SONARQUBE_PAGE_SIZE } from './sonarqube.constants' +import { ensure } from './sonarqube.utils' export interface SonarqubePaging { pageIndex: number @@ -288,8 +289,19 @@ export class SonarqubeClientService { } @StartActiveSpan() - async createUser(params: CreateUserParams) { - await this.http.fetch('users/create', { method: 'POST', query: params }) + async createUser(params: CreateUserParams): Promise { + return ensure({ + create: async () => { + await this.http.fetch('users/create', { method: 'POST', query: params }) + return undefined + }, + reload: async () => { + for await (const user of this.searchUsers({ q: params.login })) { + if (user.login === params.login) return user + } + return undefined + }, + }) } @StartActiveSpan() diff --git a/apps/server-nestjs/src/modules/sonarqube/sonarqube.utils.spec.ts b/apps/server-nestjs/src/modules/sonarqube/sonarqube.utils.spec.ts new file mode 100644 index 0000000000..0fb47da470 --- /dev/null +++ b/apps/server-nestjs/src/modules/sonarqube/sonarqube.utils.spec.ts @@ -0,0 +1,64 @@ +import { HttpStatus } from '@nestjs/common' +import { describe, expect, it, vi } from 'vitest' +import { SonarqubeError } from './sonarqube-http-client.service' +import { ensure, isSonarqubeAlreadyExists, sonarProjectPropertiesFile } from './sonarqube.utils' + +describe('sonarProjectPropertiesFile', () => { + it('targets the project key with a quality-gate wait', () => { + expect(sonarProjectPropertiesFile('my-key')).toEqual([ + 'sonar.projectKey=my-key', + 'sonar.qualitygate.wait=true', + ]) + }) +}) + +describe('isSonarqubeAlreadyExists', () => { + it('matches a 409 or an already/exists message', () => { + expect(isSonarqubeAlreadyExists(new SonarqubeError('ClientError', 'conflict', { status: HttpStatus.CONFLICT }))).toBe(true) + expect(isSonarqubeAlreadyExists(new SonarqubeError('ClientError', `User 'bob' already exists`, { status: 400 }))).toBe(true) + }) + + it('rejects other errors', () => { + expect(isSonarqubeAlreadyExists(new SonarqubeError('ClientError', 'forbidden', { status: 403 }))).toBe(false) + expect(isSonarqubeAlreadyExists(new Error('already exists'))).toBe(false) + expect(isSonarqubeAlreadyExists(null)).toBe(false) + }) +}) + +describe('ensure', () => { + it('returns the created value when create succeeds', async () => { + const reload = vi.fn() + + await expect(ensure({ create: async () => 'created', reload })).resolves.toBe('created') + + expect(reload).not.toHaveBeenCalled() + }) + + it('reloads once on a collision and never retries create', async () => { + const error = new SonarqubeError('ClientError', 'already exists', { status: 409 }) + const create = vi.fn(async () => { throw error }) + const onCollision = vi.fn() + const reload = vi.fn(async () => 'existing') + + await expect(ensure({ create, reload, onCollision })).resolves.toBe('existing') + + expect(create).toHaveBeenCalledOnce() + expect(onCollision).toHaveBeenCalledWith(error) + expect(reload).toHaveBeenCalledOnce() + }) + + it('rethrows the original error when a collision finds nothing on reload', async () => { + const error = new SonarqubeError('ClientError', 'already exists', { status: 409 }) + + await expect(ensure({ create: async () => { throw error }, reload: async () => undefined })).rejects.toBe(error) + }) + + it('rethrows non-collision errors without reloading', async () => { + const error = new SonarqubeError('ClientError', 'forbidden', { status: 403 }) + const reload = vi.fn() + + await expect(ensure({ create: async () => { throw error }, reload })).rejects.toBe(error) + + expect(reload).not.toHaveBeenCalled() + }) +}) diff --git a/apps/server-nestjs/src/modules/sonarqube/sonarqube.utils.ts b/apps/server-nestjs/src/modules/sonarqube/sonarqube.utils.ts index 8b8fc8adff..f3046fbc65 100644 --- a/apps/server-nestjs/src/modules/sonarqube/sonarqube.utils.ts +++ b/apps/server-nestjs/src/modules/sonarqube/sonarqube.utils.ts @@ -1,6 +1,44 @@ +import { HttpStatus } from '@nestjs/common' +import { SonarqubeError } from './sonarqube-http-client.service' + export function sonarProjectPropertiesFile(projectKey: string) { return [ `sonar.projectKey=${projectKey}`, 'sonar.qualitygate.wait=true', ] } + +// Whether a SonarQube error signals an entity already existing (race +// collision): a 409, or a 4xx whose message mentions "already"/"exists" +// (SonarQube reports some collisions as a generic Bad Request). +export function isSonarqubeAlreadyExists(error: unknown): error is SonarqubeError { + if (!(error instanceof SonarqubeError)) return false + if (error.status === HttpStatus.CONFLICT) return true + return error.status !== undefined && error.status >= 400 && error.status < 500 && /already|exists/i.test(error.message) +} + +// Runs an idempotent write: tries `create`, and on a SonarQube race collision +// reloads via `reload` and returns the existing entity instead of failing. +// `onCollision` is invoked once when a collision is detected. If the reload +// finds nothing, the original error is rethrown so genuine failures are not +// swallowed. +export async function ensure({ + create, + reload, + onCollision, +}: { + create: () => Promise + reload: () => Promise + onCollision?: (error: unknown) => void +}): Promise { + try { + return await create() + } catch (error) { + if (isSonarqubeAlreadyExists(error)) { + onCollision?.(error) + const existing = await reload() + if (existing) return existing + } + throw error + } +}