diff --git a/packages/backend/src/controllers/api/v1/admin/apps/create-auth-client.ee.test.js b/packages/backend/src/controllers/api/v1/admin/apps/create-auth-client.ee.test.js index 6e449cac..18728650 100644 --- a/packages/backend/src/controllers/api/v1/admin/apps/create-auth-client.ee.test.js +++ b/packages/backend/src/controllers/api/v1/admin/apps/create-auth-client.ee.test.js @@ -23,7 +23,7 @@ describe('POST /api/v1/admin/apps/:appKey/auth-clients', () => { it('should return created response for valid app config', async () => { await createAppConfig({ - key: 'gitlab' + key: 'gitlab', }); const appAuthClient = { @@ -35,7 +35,7 @@ describe('POST /api/v1/admin/apps/:appKey/auth-clients', () => { clientSecret: 'sample client secret', instanceUrl: 'https://gitlab.com', oAuthRedirectUrl: 'http://localhost:3001/app/gitlab/connection/add', - } + }, }; const response = await request(app) @@ -58,7 +58,7 @@ describe('POST /api/v1/admin/apps/:appKey/auth-clients', () => { clientSecret: 'sample client secret', instanceUrl: 'https://gitlab.com', oAuthRedirectUrl: 'http://localhost:3001/app/gitlab/connection/add', - } + }, }; await request(app) @@ -70,7 +70,7 @@ describe('POST /api/v1/admin/apps/:appKey/auth-clients', () => { it('should return bad request response for missing required fields', async () => { await createAppConfig({ - key: 'gitlab' + key: 'gitlab', }); const appAuthClient = { @@ -86,7 +86,9 @@ describe('POST /api/v1/admin/apps/:appKey/auth-clients', () => { expect(response.body.meta.type).toEqual('ModelValidation'); expect(response.body.errors).toMatchObject({ name: ["must have required property 'name'"], - formattedAuthDefaults: ["must have required property 'formattedAuthDefaults'"] + formattedAuthDefaults: [ + "must have required property 'formattedAuthDefaults'", + ], }); }); }); diff --git a/packages/backend/src/controllers/api/v1/admin/apps/create-config.ee.js b/packages/backend/src/controllers/api/v1/admin/apps/create-config.ee.js index 5e5d374a..7d5021aa 100644 --- a/packages/backend/src/controllers/api/v1/admin/apps/create-config.ee.js +++ b/packages/backend/src/controllers/api/v1/admin/apps/create-config.ee.js @@ -1,19 +1,10 @@ -import { renderError, renderObject } from '../../../../../helpers/renderer.js'; +import { renderObject } from '../../../../../helpers/renderer.js'; import AppConfig from '../../../../../models/app-config.js'; export default async (request, response) => { - const appKey = request.params.appKey; - - const appConfig = await AppConfig.query() - .findOne({ key: appKey }); - - if (appConfig) { - return renderError(response, [{ general: ['App config already exists.'] }], 409); - } - - const createdAppConfig = await AppConfig - .query() - .insertAndFetch(appConfigParams(request)); + const createdAppConfig = await AppConfig.query().insertAndFetch( + appConfigParams(request) + ); renderObject(response, createdAppConfig, { status: 201 }); }; diff --git a/packages/backend/src/controllers/api/v1/admin/apps/create-config.ee.test.js b/packages/backend/src/controllers/api/v1/admin/apps/create-config.ee.test.js index 839f6fb2..6499e71b 100644 --- a/packages/backend/src/controllers/api/v1/admin/apps/create-config.ee.test.js +++ b/packages/backend/src/controllers/api/v1/admin/apps/create-config.ee.test.js @@ -23,7 +23,6 @@ describe('POST /api/v1/admin/apps/:appKey/config', () => { it('should return created app config', async () => { const appConfig = { - key: 'gitlab', allowCustomConnection: true, shared: true, disabled: false, @@ -35,11 +34,14 @@ describe('POST /api/v1/admin/apps/:appKey/config', () => { .send(appConfig) .expect(201); - const expectedPayload = createAppConfigMock(appConfig); + const expectedPayload = createAppConfigMock({ + ...appConfig, + key: 'gitlab', + }); expect(response.body).toMatchObject(expectedPayload); }); - it('should return HTTP 409 for already existing app config', async () => { + it('should return HTTP 422 for already existing app config', async () => { const appConfig = { key: 'gitlab', allowCustomConnection: true, @@ -49,10 +51,17 @@ describe('POST /api/v1/admin/apps/:appKey/config', () => { await createAppConfig(appConfig); - await request(app) + const response = await request(app) .post('/api/v1/admin/apps/gitlab/config') .set('Authorization', token) - .send(appConfig) - .expect(409); + .send({ + disabled: false, + }) + .expect(422); + + expect(response.body.meta.type).toEqual('UniqueViolationError'); + expect(response.body.errors).toMatchObject({ + key: ["'key' must be unique."], + }); }); }); diff --git a/packages/backend/src/helpers/error-handler.js b/packages/backend/src/helpers/error-handler.js index 9de3d1d0..f4880812 100644 --- a/packages/backend/src/helpers/error-handler.js +++ b/packages/backend/src/helpers/error-handler.js @@ -1,9 +1,13 @@ import logger from './logger.js'; import objection from 'objection'; import * as Sentry from './sentry.ee.js'; -const { NotFoundError, DataError, ValidationError } = objection; +const { NotFoundError, DataError, ValidationError, UniqueViolationError } = + objection; import HttpError from '../errors/http.js'; -import { renderObjectionError } from './renderer.js'; +import { + renderObjectionError, + renderUniqueViolationError, +} from './renderer.js'; // Do not remove `next` argument as the function signature will not fit for an error handler middleware // eslint-disable-next-line no-unused-vars @@ -20,6 +24,10 @@ const errorHandler = (error, request, response, next) => { renderObjectionError(response, error, 422); } + if (error instanceof UniqueViolationError) { + renderUniqueViolationError(response, error); + } + if (error instanceof DataError) { response.status(400).end(); } diff --git a/packages/backend/src/helpers/renderer.js b/packages/backend/src/helpers/renderer.js index a87ce710..e8d47852 100644 --- a/packages/backend/src/helpers/renderer.js +++ b/packages/backend/src/helpers/renderer.js @@ -64,16 +64,31 @@ const renderError = (response, errors, status, type) => { return response.status(errorStatus).send(payload); }; +const renderUniqueViolationError = (response, error) => { + const errors = error.columns.map((column) => ({ + [column]: [`'${column}' must be unique.`], + })); + + return renderError(response, errors, 422, 'UniqueViolationError'); +}; + const renderObjectionError = (response, error, status) => { const { statusCode, type, data = {} } = error; const computedStatusCode = status || statusCode; - const computedErrors = Object.entries(data).map(([fieldName, fieldErrors]) => ({ - [fieldName]: fieldErrors.map(({ message }) => message) - })); + const computedErrors = Object.entries(data).map( + ([fieldName, fieldErrors]) => ({ + [fieldName]: fieldErrors.map(({ message }) => message), + }) + ); return renderError(response, computedErrors, computedStatusCode, type); }; -export { renderObject, renderError, renderObjectionError }; +export { + renderObject, + renderError, + renderObjectionError, + renderUniqueViolationError, +};