saisilinus / node-express-mongoose-typescript-boilerplate

A boilerplate for making production-ready RESTful APIs using Node.js, TypeScript, Express, and Mongoose
MIT License
325 stars 93 forks source link

child route name getting fetched by objectId validation along with customerId #43

Closed tamirazrab closed 1 year ago

tamirazrab commented 1 year ago

Describe the bug A clear and concise description of what the bug is. router .route('/explore/:customerId') .get(validate(productValidation.exploreCrafts), productController.exploreCrafts)

Here's my route and here's what I'm getting in objectId validation value: https://i.imgur.com/aUvfj0u.png

Expected behavior Is there any way I can still use route as it is and only params gets picked up?

saisilinus commented 1 year ago

Hey. I can't view the image. Check your url and make sure it works

tamirazrab commented 1 year ago

Must've been private here's the URL https://imgur.com/a/nRe4JDP

saisilinus commented 1 year ago

Your customerId is not a valid mongo id

tamirazrab commented 1 year ago

If you see, I'm printing the value of objectId validator that I'm getting, and I can also see 'explore' there first, and then my customerId comes. My initial reason to create an issue was why 'explore' is getting there, and how can I prevent this. Removing 'explore' from the route worked.

saisilinus commented 1 year ago

It seems like this is a routing issue. Please check how Express handles dynamic routes. I don't have enough information to help you here. Why are you calling '/product/explore/:customerId' when you have defined the route as '/explore/:customerId'?

On Thu, May 4, 2023, 2:42 PM Tamir @.***> wrote:

If you see, I'm printing the value of objectId validator that I'm getting, and I can also see 'explore' there first, and then my customerId comes. My initial reason to create an issue was why 'explore' is getting there, and how can I prevent this. Removing 'explore' from the route worked.

— Reply to this email directly, view it on GitHub https://github.com/saisilinus/node-express-mongoose-typescript-boilerplate/issues/43#issuecomment-1534619128, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKCRT7FLQMQYDMQKSI5LR5TXEOIZLANCNFSM6AAAAAAXVRZGEA . You are receiving this because you commented.Message ID: <saisilinus/node-express-mongoose-typescript-boilerplate/issues/43/1534619128 @github.com>

tamirazrab commented 1 year ago

product is parent route defined in index.js of routes, explore is child route, I think it's because the way routes are designed here, If I want to use any child-route it's going to go in validation objectId function, can you provide example of how'd you handle the situation where you have sub routes that are accepting parameters.

{ path: '/product', route: productRoute, },

Then in productRoute I have:

router .route('/explore/:customerId') .get(validate(productValidation.exploreCrafts), productController.exploreCrafts)

I have couple of them like

router.put(/delete-file/:productId/`, validate(productValidation.deleteFile), authorize.isUserAuthorized, productController.deleteFiles)

router.put(/upload-product-file/:productId/:artisanId, validate(productValidation.uploadFile), authorize.isUserAuthorized, productController.uploadProductFiles)`

and am assuming all going to pass route as well to validator.

saisilinus commented 1 year ago

You don't have to add the /explore, /delete-file, or /upload-product-file parts. You can just have the main route as /product then have the child routes as /:customerId and differentiate them using the GET, PUT, and DELETE methods.

When you have a dynamic child route such as /:customerId then you add a sibling route as /explore/:customerId, Express will get confused as it is expecting a param at /:customerId but sometimes its getting /explore/:customerId and it doesn't parse the values consistently or at least like you would expect. Express sees /product/some-mongo-id and /product/explore/some-mongo-id as the same thing and parses /explore/some-mongo-id as /some-mongo-id.

If you need to add separate child routes with routes such as /explore and /delete-file you can't include dynamic routes so that Express is aware that you only have static routes and they all map to different paths.

Alternatively you can always create additional parent routes as there is no limit.

You can check out additional documentation here

tamirazrab commented 1 year ago

I see, didn't expected that behavior, well thanks for clarification I thought it had to do how route designed but seems like I got a lot of refactoring to do.