fix: Remove idNumberMiddleware and change to use parameters in validPath method instead

[0417taehyun]

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.

{
   "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.

Important files

Discussion points

1. Why I choose to remove the middleware

I saw #6860 pull request to add a middleware to validate /:id path parameter whether it is number or not. However, we can still validate the path parameters by using parameters field in openApiService.validPath method.

There are several more reasons that it would be better to use validPath method instead of custom middleware.

1.1. Flexibility

First, it will be hard to validate multiple ids because we should differentiate the name of ids or change the logic of middleware to embrace the array. For example, if we plan to build an API such as GET users/:id/tokens/:id to get a specific toke of a specific user, we can't validate both /:id values by using current middleware. Additionally, the situation will be worse if the type of ids is different, one is integer while the other is string.

Consequently, It would be better to differentiate the name of ids like GET users/:userId/tokens/:tokenId and validate them by using parameters like the one below. It is more precise and concise because we even don't need to convert the type by using Number method.

        this.route({
            method: 'get',
            path: '/:userId/tokens/:tokenId',
            middleware: [
                openApiService.validPath({
                    parameters: [
                        {
                            name: 'userId',
                            in: 'path',
                            required: true,
                            schema: {
                                type: 'integer',
                            },
                            description: 'a user id',
                        },
                        {
                            name: 'tokenId',
                            in: 'path',
                            required: true,
                            schema: {
                                type: 'integer',
                            },
                            description: 'a token id of the user',
                        },
                    ],
                }),
            ],
        }); 

1.2. Documentation

Moreover, I think Unleash uses openApiService method to generate Open API document and we can reuse this to describe the parameters on the document.

2. Why I use 'integer' instead of 'number' in parameters field

2.1. Boundary

Number field embraces both integer and float. You can check this on document

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
unleash-monorepo-frontend	⬜️ Ignored (Inspect)	Visit Preview		Nov 14, 2024 1:06pm

[vercel]

@0417taehyun is attempting to deploy a commit to the unleash-team Team on Vercel.

A member of the Team first needs to authorize it.

[0417taehyun]

I think it is enough to check whether it responds with status code 400 instead of checking both status code and error message.

[gastonfournier]

I think this is well aligned with what we proposed in #4471, I'll bring it to the team

[0417taehyun]

@gastonfournier , @Tymek

Thanks to consider my proposal and I also think this aligns with #4471 pull request.

I applied your suggestions with a6351c5 commit and I think there are some linting issues on main branch hence I also fix it with a5ddf7d commit by rebase to main branch. The linting issue isn't caused by changes of mine.

[0417taehyun]

@gastonfournier , @Tymek

Hi, are there any issues that I need to resolve?

[gastonfournier]

Thanks for the ping @0417taehyun I think it's good to go