fastify-autoload icon indicating copy to clipboard operation
fastify-autoload copied to clipboard

Params validation

Open Zack-Heisnberg opened this issue 1 year ago • 7 comments

Prerequisites

  • [X] I have written a descriptive issue title
  • [X] I have searched existing issues to ensure the bug has not already been reported

Fastify version

^4.0.0

Plugin version

No response

Node.js version

19

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

20.04

Description

the required for param validation doesn't work and allow empty strings

Steps to Reproduce

            fastify.get<{
              Params: {
                model: string;
              },
              Querystring: {
                sort: string;
                asc: number;
              }
            }>('/listAll/:model/', {
              schema: {
                params: {
                  type: 'object',
                  properties: {
                    model: { type: 'string' },
                  },
                  required: ['model'],
                },
                querystring: {
                  type: 'object',
                  properties: {
                    sort: { type: 'string' },
                    asc: { type: 'number' },
                  },
                  required: ['sort'],
                },
              },
            }, async function (request, reply) {
              return 'db endpoint for => ' + request.params.model;
            });

i undestand this can be fixed with

           params: {
                  type: 'object',
                  properties: {
                    model: { type: 'string' },
                    minLength: 1, 
                  },
                  required: ['model'],
                },

but i think the required on a string should do it for params

Current Behavior

CURL /listAll/t?sort=p => this is ok

CURL /listAll/t=> validation works , sort is required

CURL /listAll//?sort=p => get ok 200 , validation doesn't works , and allow the model to be empty

Expected Behavior

CURL /listAll/t?sort=p => this is ok

CURL /listAll/t=> validation works , sort is required

CURL /listAll//?sort=p => validation works , and doesn't allow the model to be empty

Zack-Heisnberg avatar Jun 21 '23 08:06 Zack-Heisnberg

I don't think this could be considered an issue, it seems like an acceptable behaviour to me: empty string is still a string, different from undefined

giuliowaitforitdavide avatar Jun 21 '23 15:06 giuliowaitforitdavide

I would check here: https://stackoverflow.com/questions/45888524/empty-values-validation-in-json-schema-using-ajv

I would not expect that behaviour with that schema since typeof "" === 'string' is true

In any case, it is an AJV thing and it is not related to fastify:

  • https://github.com/ajv-validator/ajv/issues/885

but i think the required on a string should do it for params

Adding this custom logic could be a harmful as it would be not a standard JSON schema anymore - you can implement it on your app using the onRoute hook to add such logic tho

Eomm avatar Jun 21 '23 16:06 Eomm

I don't think this could be considered an issue, it seems like an acceptable behaviour to me: empty string is still a string, different from undefined

I would check here: https://stackoverflow.com/questions/45888524/empty-values-validation-in-json-schema-using-ajv

I would not expect that behaviour with that schema since typeof "" === 'string' is true

In any case, it is an AJV thing and it is not related to fastify:

but i think the required on a string should do it for params

Adding this custom logic could be a harmful as it would be not a standard JSON schema anymore - you can implement it on your app using the onRoute hook to add such logic tho

i am not speaking about the validation of the string ,

i am speaking about the

     required: ['model'],

for a querystring

                      querystring: {
                        type: 'object',
                        properties: {
                          sort: { type: 'string' },
                          asc: { type: 'number' },
                        },
                        required: ['sort'],
                      },

even without the minlength it's work as the request when we don't set "sort" it's undefined

but for params

           params: {
                          type: 'object',
                          properties: {
                            model: { type: 'string' },
                          },
                          required: ['model'],
                        },
                        
                        

the required is useless as it is , because it's replaced by the route no matter what we define it

and the problem is when we access req.params.model , it's shows as undefined and not an empty string , so why the required is not working

Zack-Heisnberg avatar Jun 23 '23 01:06 Zack-Heisnberg

and the problem is when we access req.params.model , it's shows as undefined and not an empty string , so why the required is not working

Can't reproduce

const app = require('fastify')({ logger: true })
app.get('/listAll/:model/', {
  schema: {
    params: {
      type: 'object',
      properties: {
        model: { type: 'string' }
      },
      required: ['model']
    },
    querystring: {
      type: 'object',
      properties: {
        sort: { type: 'string' },
        asc: { type: 'number' }
      },
      required: ['sort']
    }
  }
}, async (request, reply) => {
  return { hello: request.params.model }
  // prints {"hello":""}
})

app.listen({ port: 8080 })

Could you add a Minimal, Reproducible Example?

Without it, we are unable to help you.

Eomm avatar Jun 23 '23 09:06 Eomm

interesting the issue is with using the fastify/autoload

how to reproduce

1 - install fastify-cli npm install fastify-cli --global

2 - genereate a new server and run npm install fastify generate . --esm

3 - change the root.js ( or any route point inside routes ) add the exact same code

                fastify.get('/listAll/:model/', {
                      schema: {
                        params: {
                          type: 'object',
                          properties: {
                            model: { type: 'string' }
                          },
                          required: ['model']
                        },
                        querystring: {
                          type: 'object',
                          properties: {
                            sort: { type: 'string' },
                            asc: { type: 'number' }
                          },
                          required: ['sort']
                        }
                      }
                    }, async (request, reply) => {
                      return { hello: request.params.model }
                      // prints {"hello":""}
                    })

4 - now this endpoint http://localhost:3000/admin/listAll//?sort=dsa you see how the :model is empty you will get {"hello":""}

and if you change route to

              fastify.get('/listAll/:model',

using this http://localhost:3000/admin/listAll/?sort=dsa we get {"hello":""}

without using the @fastify/autoload this issue doesn't happen :/ instead you get 404 error the issue happen with nested folders as well

so should i move this issue there ?

Zack-Heisnberg avatar Jun 26 '23 09:06 Zack-Heisnberg

It worth investigating on it, I will move the issue 👍🏼

Eomm avatar Jun 27 '23 12:06 Eomm

@Zack-Heisnberg could you check if https://github.com/fastify/fastify/pull/4903 solved?

Eomm avatar Jul 19 '23 16:07 Eomm