From 695873132e4f30d5a1a780198f663c4fd3482013 Mon Sep 17 00:00:00 2001 From: "weekwith.me" <63915557+0417taehyun@users.noreply.github.com> Date: Mon, 18 Nov 2024 23:08:07 +0900 Subject: [PATCH] fix: Remove `idNumberMiddleware` and change to use `parameters` in `validPath` method instead (#8734) ## About the changes - Remove `idNumberMiddleware` method and change to use `parameters` field in `openApiService.validPath` method for the flexibility. - Remove unnecessary `Number` type converting method and change them to use `<{id: number}>` to specify the type. ### Reference The changed response looks like the one below. ```JSON { "id":"8174a692-7427-4d35-b7b9-6543b9d3db6e", "name":"BadDataError", "message":"Request validation failed: your request body or params contain invalid data. Refer to the `details` list for more information.", "details":[ { "message":"The `/params/id` property must be integer. You sent undefined.", "path":"/params/id" } ] } ``` I think it might be better to customize the error response, especially `"You sent undefined."`, on another pull request if this one is accepted. I prefer to separate jobs to divide the context and believe that it helps reviewer easier to understand. --- .../segment/admin-segment.e2e.test.ts | 10 +-- .../features/segment/segment-controller.ts | 63 ++++++++++++---- .../middleware/id-number-middleware.test.ts | 32 --------- src/lib/middleware/id-number-middleware.ts | 16 ----- src/lib/routes/admin-api/user-admin.ts | 72 +++++++++++++++---- src/lib/routes/admin-api/user/pat.ts | 13 +++- 6 files changed, 123 insertions(+), 83 deletions(-) delete mode 100644 src/lib/middleware/id-number-middleware.test.ts delete mode 100644 src/lib/middleware/id-number-middleware.ts diff --git a/src/lib/features/segment/admin-segment.e2e.test.ts b/src/lib/features/segment/admin-segment.e2e.test.ts index 019a822da6..0331b0ac1c 100644 --- a/src/lib/features/segment/admin-segment.e2e.test.ts +++ b/src/lib/features/segment/admin-segment.e2e.test.ts @@ -204,14 +204,8 @@ test('should create segments', async () => { expect(segments.map((s) => s.name)).toEqual(['a', 'b', 'c']); }); -test('should return 400 for non numeeric segment ID', async () => { - const { body } = await app.request - .get(`${SEGMENTS_BASE_PATH}/stringName`) - .expect(400); - expect(body).toMatchObject({ - message: - 'Request validation failed: your request body or params contain invalid data: ID should be an integer', - }); +test('should return 400 for non numeric segment ID', async () => { + await app.request.get(`${SEGMENTS_BASE_PATH}/stringName`).expect(400); }); test('should update segments', async () => { diff --git a/src/lib/features/segment/segment-controller.ts b/src/lib/features/segment/segment-controller.ts index 5ee69b8f59..d18fff901f 100644 --- a/src/lib/features/segment/segment-controller.ts +++ b/src/lib/features/segment/segment-controller.ts @@ -41,7 +41,6 @@ import { import { anonymiseKeys, extractUserIdFromUser } from '../../util'; import { BadDataError } from '../../error'; -import idNumberMiddleware from '../../middleware/id-number-middleware'; type IUpdateFeatureStrategySegmentsRequest = IAuthRequest< {}, @@ -156,11 +155,21 @@ export class SegmentsController extends Controller { summary: 'Get strategies that reference segment', description: 'Retrieve all strategies that reference the specified segment.', + parameters: [ + { + name: 'id', + in: 'path', + required: true, + schema: { + type: 'integer', + }, + description: 'a segment id', + }, + ], responses: { 200: createResponseSchema('segmentStrategiesSchema'), }, }), - idNumberMiddleware(), ], }); @@ -175,6 +184,17 @@ export class SegmentsController extends Controller { summary: 'Deletes a segment by id', description: 'Deletes a segment by its id, if not found returns a 409 error', + parameters: [ + { + name: 'id', + in: 'path', + required: true, + schema: { + type: 'integer', + }, + description: 'a segment id', + }, + ], tags: ['Segments'], operationId: 'removeSegment', responses: { @@ -182,7 +202,6 @@ export class SegmentsController extends Controller { ...getStandardResponses(401, 403, 409), }, }), - idNumberMiddleware(), ], }); @@ -198,13 +217,23 @@ export class SegmentsController extends Controller { 'Updates the content of the segment with the provided payload. Requires `name` and `constraints` to be present. If `project` is not present, it will be set to `null`. Any other fields not specified will be left untouched.', tags: ['Segments'], operationId: 'updateSegment', + parameters: [ + { + name: 'id', + in: 'path', + required: true, + schema: { + type: 'integer', + }, + description: 'a segment id', + }, + ], requestBody: createRequestSchema('upsertSegmentSchema'), responses: { 204: emptyResponse, ...getStandardResponses(400, 401, 403, 409, 415), }, }), - idNumberMiddleware(), ], }); @@ -219,12 +248,22 @@ export class SegmentsController extends Controller { description: 'Retrieves a segment based on its ID.', tags: ['Segments'], operationId: 'getSegment', + parameters: [ + { + name: 'id', + in: 'path', + required: true, + schema: { + type: 'integer', + }, + description: 'a segment id', + }, + ], responses: { 200: createResponseSchema('adminSegmentSchema'), ...getStandardResponses(404), }, }), - idNumberMiddleware(), ], }); @@ -353,7 +392,7 @@ export class SegmentsController extends Controller { req: IAuthRequest<{ id: number }>, res: Response, ): Promise { - const id = Number(req.params.id); + const id = req.params.id; const { user } = req; const strategies = await this.segmentService.getVisibleStrategies( id, @@ -386,10 +425,10 @@ export class SegmentsController extends Controller { } async removeSegment( - req: IAuthRequest<{ id: string }>, + req: IAuthRequest<{ id: number }>, res: Response, ): Promise { - const id = Number(req.params.id); + const id = req.params.id; let segmentIsInUse = false; segmentIsInUse = await this.segmentService.isInUse(id); @@ -403,10 +442,10 @@ export class SegmentsController extends Controller { } async updateSegment( - req: IAuthRequest<{ id: string }>, + req: IAuthRequest<{ id: number }>, res: Response, ): Promise { - const id = Number(req.params.id); + const id = req.params.id; const updateRequest: UpsertSegmentSchema = { name: req.body.name, description: req.body.description, @@ -423,10 +462,10 @@ export class SegmentsController extends Controller { } async getSegment( - req: Request<{ id: string }>, + req: Request<{ id: number }>, res: Response, ): Promise { - const id = Number(req.params.id); + const id = req.params.id; const segment = await this.segmentService.get(id); if (this.flagResolver.isEnabled('anonymiseEventLog')) { res.json(anonymiseKeys(segment, ['createdBy'])); diff --git a/src/lib/middleware/id-number-middleware.test.ts b/src/lib/middleware/id-number-middleware.test.ts deleted file mode 100644 index f3310799c7..0000000000 --- a/src/lib/middleware/id-number-middleware.test.ts +++ /dev/null @@ -1,32 +0,0 @@ -import express from 'express'; -import supertest from 'supertest'; -import idNumberMiddleware from './id-number-middleware'; - -describe('idNumberMiddleware', () => { - it('should pass when id is a valid integer', async () => { - const app = express(); - app.use('/:id', idNumberMiddleware()); - app.get('/:id', (req, res) => { - res.status(200).send('Valid ID'); - }); - - await supertest(app) - .get('/123') - .expect(200) - .expect((res) => { - expect(res.text).toBe('Valid ID'); - }); - }); - it('should throw BadDataError when id is not a valid integer', async () => { - const app = express(); - app.use('/:id', idNumberMiddleware()); - app.get('/:id', (req, res) => { - res.status(200).send('This should not be executed'); - }); - - const { body } = await supertest(app).get('/abc').expect(400); - expect(body).toMatchObject({ - details: [{ message: 'ID should be an integer' }], - }); - }); -}); diff --git a/src/lib/middleware/id-number-middleware.ts b/src/lib/middleware/id-number-middleware.ts deleted file mode 100644 index c5200e0792..0000000000 --- a/src/lib/middleware/id-number-middleware.ts +++ /dev/null @@ -1,16 +0,0 @@ -import { BadDataError } from '../error'; - -const idNumberMiddleware = (): any => { - return async (req, res, next) => { - const { id } = req.params; - if (!Number.isInteger(Number(id))) { - res.status(400).send( - new BadDataError('ID should be an integer').toJSON(), - ); - return; - } - next(); - }; -}; - -export default idNumberMiddleware; diff --git a/src/lib/routes/admin-api/user-admin.ts b/src/lib/routes/admin-api/user-admin.ts index ac0025d158..344dc09e69 100644 --- a/src/lib/routes/admin-api/user-admin.ts +++ b/src/lib/routes/admin-api/user-admin.ts @@ -55,7 +55,6 @@ import { type CreateUserResponseSchema, } from '../../openapi/spec/create-user-response-schema'; import type { IRoleWithPermissions } from '../../types/stores/access-store'; -import idNumberMiddleware from '../../middleware/id-number-middleware'; export default class UserAdminController extends Controller { private flagResolver: IFlagResolver; @@ -144,6 +143,17 @@ export default class UserAdminController extends Controller { operationId: 'changeUserPassword', summary: 'Change password for a user', description: 'Change password for a user as an admin', + parameters: [ + { + name: 'id', + in: 'path', + required: true, + schema: { + type: 'integer', + }, + description: 'a user id', + }, + ], requestBody: createRequestSchema('passwordSchema'), responses: { 200: emptyResponse, @@ -256,6 +266,15 @@ export default class UserAdminController extends Controller { description: 'Gets a list of permissions for a user, additional project and environment can be specified.', parameters: [ + { + name: 'id', + in: 'path', + required: true, + schema: { + type: 'integer', + }, + description: 'a user id', + }, { name: 'project', in: 'query', @@ -341,12 +360,22 @@ export default class UserAdminController extends Controller { operationId: 'getUser', summary: 'Get user', description: 'Will return a single user by id', + parameters: [ + { + name: 'id', + in: 'path', + required: true, + schema: { + type: 'integer', + }, + description: 'a user id', + }, + ], responses: { 200: createResponseSchema('userSchema'), ...getStandardResponses(400, 401, 404), }, }), - idNumberMiddleware(), ], }); @@ -367,9 +396,11 @@ export default class UserAdminController extends Controller { { name: 'id', in: 'path', + required: true, schema: { type: 'integer', }, + description: 'a user id', }, ], responses: { @@ -377,7 +408,6 @@ export default class UserAdminController extends Controller { ...getStandardResponses(400, 401, 403, 404), }, }), - idNumberMiddleware(), ], }); @@ -393,12 +423,22 @@ export default class UserAdminController extends Controller { operationId: 'deleteUser', summary: 'Delete a user', description: 'Deletes the user with the given userId', + parameters: [ + { + name: 'id', + in: 'path', + required: true, + schema: { + type: 'integer', + }, + description: 'a user id', + }, + ], responses: { 200: emptyResponse, ...getStandardResponses(401, 403, 404), }, }), - idNumberMiddleware(), ], }); } @@ -507,9 +547,12 @@ export default class UserAdminController extends Controller { ); } - async getUser(req: Request, res: Response): Promise { + async getUser( + req: Request<{ id: number }>, + res: Response, + ): Promise { const { id } = req.params; - const user = await this.userService.getUser(Number(id)); + const user = await this.userService.getUser(id); this.openApiService.respondWithValidation( 200, @@ -568,7 +611,7 @@ export default class UserAdminController extends Controller { async updateUser( req: IAuthRequest< - { id: string }, + { id: number }, CreateUserResponseSchema, UpdateUserSchema >, @@ -578,14 +621,14 @@ export default class UserAdminController extends Controller { const { id } = params; const { name, email, rootRole } = body; - await this.throwIfScimUser({ id: Number(id) }); + await this.throwIfScimUser({ id }); const normalizedRootRole = Number.isInteger(Number(rootRole)) ? Number(rootRole) : (rootRole as RoleName); const updateUser = await this.userService.updateUser( { - id: Number(id), + id, name, email, rootRole: normalizedRootRole, @@ -604,11 +647,14 @@ export default class UserAdminController extends Controller { ); } - async deleteUser(req: IAuthRequest, res: Response): Promise { + async deleteUser( + req: IAuthRequest<{ id: number }>, + res: Response, + ): Promise { const { user, params } = req; const { id } = params; - await this.throwIfScimUser({ id: Number(id) }); + await this.throwIfScimUser({ id }); await this.userService.deleteUser(+id, req.audit); res.status(200).send(); @@ -625,13 +671,13 @@ export default class UserAdminController extends Controller { } async changeUserPassword( - req: IAuthRequest<{ id: string }, unknown, PasswordSchema>, + req: IAuthRequest<{ id: number }, unknown, PasswordSchema>, res: Response, ): Promise { const { id } = req.params; const { password } = req.body; - await this.throwIfScimUser({ id: Number(id) }); + await this.throwIfScimUser({ id }); await this.userService.changePassword(+id, password); res.status(200).send(); diff --git a/src/lib/routes/admin-api/user/pat.ts b/src/lib/routes/admin-api/user/pat.ts index 1f9bdddb70..edec5f013b 100644 --- a/src/lib/routes/admin-api/user/pat.ts +++ b/src/lib/routes/admin-api/user/pat.ts @@ -28,7 +28,6 @@ import { createPatSchema, } from '../../../openapi/spec/create-pat-schema'; import { ForbiddenError, NotFoundError } from '../../../error'; -import idNumberMiddleware from '../../../middleware/id-number-middleware'; export default class PatController extends Controller { private patService: PatService; @@ -107,12 +106,22 @@ export default class PatController extends Controller { 'Delete a personal access token (PAT) for the current user.', description: 'Deletes a [personal access token](https://docs.getunleash.io/how-to/how-to-create-personal-access-tokens) (PAT) belonging to the current user.', + parameters: [ + { + name: 'id', + in: 'path', + required: true, + schema: { + type: 'integer', + }, + description: 'a personal access token id', + }, + ], responses: { 200: emptyResponse, ...getStandardResponses(401, 403, 404), }, }), - idNumberMiddleware(), ], }); }