Mirrors/unleash.unleash
1
0
Fork 0
mirror of https://github.com/Unleash/unleash.git synced 2024-12-22 19:07:54 +01:00
Code Issues Projects Releases Wiki Activity

fix: Remove idNumberMiddleware and change to use parameters in validPath method instead (#8734)

Browse Source 
## 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.
This commit is contained in:
weekwith.me 2024-11-18 23:08:07 +09:00 committed by GitHub
parent 18591dd017
commit 695873132e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 123 additions and 83 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
src/lib/features/segment/admin-segment.e2e.test.ts
View File

@ -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 () => {

63
src/lib/features/segment/segment-controller.ts
View File

@ -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<SegmentStrategiesSchema>,
): Promise<void> {
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<void> {
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<void> {
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<void> {
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']));

32
src/lib/middleware/id-number-middleware.test.ts
View File

@ -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' }],
});
});
});

16
src/lib/middleware/id-number-middleware.ts
View File

@ -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;

72
src/lib/routes/admin-api/user-admin.ts
View File

@ -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<UserSchema>): Promise<void> {
async getUser(
req: Request<{ id: number }>,
res: Response<UserSchema>,
): Promise<void> {
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<void> {
async deleteUser(
req: IAuthRequest<{ id: number }>,
res: Response,
): Promise<void> {
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<void> {
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();

13
src/lib/routes/admin-api/user/pat.ts
View File

@ -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(),
],
});
}