Skip to content

Commit

Permalink
fix: Remove idNumberMiddleware and change to use parameters in `v…
Browse files Browse the repository at this point in the history
…alidPath` 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.
  • Loading branch information
0417taehyun authored Nov 18, 2024
1 parent 18591dd commit 6958731
Show file tree
Hide file tree
Showing 6 changed files with 123 additions and 83 deletions.
10 changes: 2 additions & 8 deletions src/lib/features/segment/admin-segment.e2e.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
63 changes: 51 additions & 12 deletions src/lib/features/segment/segment-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ import {

import { anonymiseKeys, extractUserIdFromUser } from '../../util';
import { BadDataError } from '../../error';
import idNumberMiddleware from '../../middleware/id-number-middleware';

type IUpdateFeatureStrategySegmentsRequest = IAuthRequest<
{},
Expand Down Expand Up @@ -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(),
],
});

Expand All @@ -175,14 +184,24 @@ 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: {
204: emptyResponse,
...getStandardResponses(401, 403, 409),
},
}),
idNumberMiddleware(),
],
});

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

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

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -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,
Expand All @@ -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']));
Expand Down
32 changes: 0 additions & 32 deletions src/lib/middleware/id-number-middleware.test.ts

This file was deleted.

16 changes: 0 additions & 16 deletions src/lib/middleware/id-number-middleware.ts

This file was deleted.

Loading

0 comments on commit 6958731

Please sign in to comment.