Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validation and serialization #2023

Merged
merged 27 commits into from Mar 14, 2020
Merged

Validation and serialization #2023

merged 27 commits into from Mar 14, 2020

Conversation

Eomm
Copy link
Member

@Eomm Eomm commented Jan 4, 2020

This PR includes:

Changes

  • ♻ the schemaCompiler has been renamed to validatorCompiler for clarity: this property is a function that must return a new function to validate the request parts
  • ♻ the validatorCompiler function has a new signature: (method, url, httpPart, schema) to improve the tools that we can build around it (like store the generated functions without recompiling them on start)

Addition

  • 🆕 a new serializerCompiler method has been added to open to customization of the default serializer (default: fast-json-stringify)
  • 🆕 easier debug: the schema generation error say if it is a validation (ajv problem) or serialization (fast-json-stringify issue) problem

Breaking changes

  • 🔴 removed the JSON shared schema replace-way) due it is not standard**
  • 🔴removed the schemaResolver feature: it is not needed anymore without the shared schema "replace-way" since we are not resolving the $ref keyword anymore

** I have tested a lot of configurations to don't let the users suffer the upgrade, and if we wrote this:

Migration guide

removed the JSON shared schema [replace-way]

You wrote this:

schema: {
  body: 'schemaId#'
}

Now you can write this:

schema: {
  body: { $ref: 'schemaId#' }
}

removed the schemaResolver feature

You wrote this:

  const fastify = Fastify()
  const ajv = new AJV()
  ajv.addSchema(fastClone(schemaParent))
  ajv.addSchema(fastClone(schemaUsed))

  fastify.setSchemaCompiler(schema => ajv.compile(schema))
  fastify.setSchemaResolver((ref) => {
    return ajv.getSchema(ref).schema
  })

Now you can write this:

  const fastify = Fastify()
  const ajv = new AJV()
  ajv.addSchema(schemaA)
  ajv.addSchema(schemaBRefToA)

  fastify.setValidatorCompiler((method, url, httpPart, schema) => {
    return ajv.compile(schema)
  })

How it works (default)

When a route with a schema for validation (query, params, path, body) is found, an AJV instance is built. The instance will be the same for all the children plugins unless the user customizes the validatorCompiler per context or per route.
The same happens for the serialization.

Additional optimization:
When two different contexts need to build an AJV instance. If the ajv custom options and the shared schema (added with .addSchema) are the same, these two contexts will use the same AJV instance.

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

@Eomm Eomm added semver-major Issue or PR that should land as semver major v3.x Issue or pr related to Fastify v3 labels Jan 4, 2020
@Eomm Eomm changed the base branch from master to next January 4, 2020 15:54
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this was implemented was because fast-json-stringify did not support external schemas. Are you sure it will do the job for us?

@jsumners This feature was also something you needed for Joi support. What do you think?

@jsumners
Copy link
Member

jsumners commented Jan 5, 2020

I haven't looked through all of this to determine how it affects Joi support, but I can say that I no longer have need of that accursed library.

I think we definitely need to improve schema support in v3. We advertise having full support for JSON Schema, but as soon as you start trying to use references the whole thing falls apart in various ways. I fully support moving the schema container out of Fastify itself and merely accepting one being supplied at boot time.

@Eomm
Copy link
Member Author

Eomm commented Jan 6, 2020

The reason this was implemented was because fast-json-stringify did not support external schemas.

From 1.15 it supports it at 3 levels of nesting

Are you sure it will do the job for us?

Yes, because those people are ourself 😆
I could invest more time on that module

This feature was also something you needed for Joi support.

I will test all the past use cases and will write a "how to migrate" guide to include it in the release message for v3

@delvedor delvedor added this to the v3.0.0 milestone Jan 7, 2020
@Eomm
Copy link
Member Author

Eomm commented Jan 22, 2020

Still 45 failing test before finish + docs

The backbone code is complete I think.
Any feedback or ideas for improvement of the code are appreciated

The logic right now is:

  • there is a validator item that generates the validation's functions
  • there is a serializator item that does the serialization's functions for the response
  • a fastify child instance use the validator and serializator of the parent if any

The default code flow is:

  • fastify start without any validator and serializator
  • the first route with a schema option will build the validator and serializator. This mean that an ajv instance and a fast-json-stringify instance will be created
  • the schemas are compiled by the items. The code in validation.js is now split between validation e serialization
  • the schemas.js file is now only a container of schemas and each fastify instance will have one of it for encapsulation. the cache is shared, instead and no more at route level

@mcollina
Copy link
Member

This should significantly improve our cold start time on serverless, definitely +1.

@@ -14,4 +14,4 @@ export interface FastifySchema {
/**
* Compiler for FastifySchema Type
*/
export type FastifySchemaCompiler = (schema: FastifySchema) => unknown
export type FastifySchemaCompiler = (method: string, url: string, httpPart: string, schema: FastifySchema) => unknown
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ethan-Arrowood I'm not sure, but the export interface FastifySchema could be wrong cause the user will receive as input a plain schema, not the complete schema configuration

fastify.post('/the/url', {
  body: bodyJsonSchema,
  querystring: queryStringJsonSchema,
  params: paramsJsonSchema,
  headers: headersJsonSchema
}, handler)

The user will get 4 calls with bodyJsonSchema, queryStringJsonSchema, paramsJsonSchema, headersJsonSchema each one

Could you help me with the TS test? I need only some suggestions 😇

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused -- i thought those properties fall under the schema property on the route options object. I noticed in your unit tests theres an reponse property now added to the schema so that should be added to the FastifySchema interface.

These schema properties are specified as type unknown because this allows the user to tell TypeScript what they are strictly. If we used something like { [k: string]: any } then we are making the type system non-strict by default and that does not lead to a great user experience. Check out the Schema section in my TS docs for more info.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought those properties fall under the schema property on the route options object

This is correct

in your unit tests theres an reponse property now added to the schema so that should be added to the FastifySchema interface.

This feature is already in place from master

My concern is that the FastifySchemaCompiler want a FastifySchema as input (schema parameter) but this parameter is a JSON Schema.
I think we need a different type here since FastifySchema works for route's configuration only.

I'll try to explain me by example:

// want a JSON schema input
  fastify.addSchema({
    $id: 'test',
    type: 'object',
    properties: {
      id: { type: 'number' }
    }
  })

  // want a `FastifySchema` type
  fastify.get('/:id', {
    handler: (req, reply) => {
      reply.send({ id: req.params.id * 2, ignore: 'it' })
    },
    schema: {
      params: { $ref: 'test#' },
      response: {
        200: { $ref: 'test#' }
      }
    }
  })

fastify.setValidatorCompiler((method, url, httpPart, schema) => {
 // looking the route above, the args are:
- httpPart = "params"
- schema = { $ref: 'test#' }
})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me now

@Eomm Eomm marked this pull request as ready for review January 26, 2020 21:55
@Eomm Eomm requested a review from a team January 26, 2020 21:58
@Eomm Eomm mentioned this pull request Jan 26, 2020
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

Can you please update the PR description to match the current status of this PR?
I'd also like to see a migration guide.

@Eomm
Copy link
Member Author

Eomm commented Feb 24, 2020

Can you please update the PR description to match the current status of this PR?

Done

@stale
Copy link

stale bot commented Mar 11, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue or pr with more than 15 days of inactivity. label Mar 11, 2020
@Eomm
Copy link
Member Author

Eomm commented Mar 11, 2020

Stalebot, I'm waiting for @Ethan-Arrowood 👍

@stale stale bot removed the stale Issue or pr with more than 15 days of inactivity. label Mar 11, 2020
Copy link
Member

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 😄 (sorry for missing this, im still trying to figure out github notifications lol)

@Eomm
Copy link
Member Author

Eomm commented Mar 13, 2020

@fastify/core someone would like to check this PR?

Here all the changes:
#2023 (comment)

I would like to merge it in next before merging master since there are a couple of fixes in v2 that need to be adapted

@jsumners
Copy link
Member

I really can't review it other than to say the code looks good. I'll need time to try it in an RC, and I don't even know when I'll have that time.

@Eomm
Copy link
Member Author

Eomm commented Mar 13, 2020

No problem!
I hope someone else of the team could give it a check: for sure this is a request 🙏 , not an order 😀

@mcollina mcollina requested a review from delvedor March 13, 2020 18:25
Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great work!!

@adrai
Copy link
Member

adrai commented Apr 29, 2020

Has the binary format been removed in v3?

@Eomm
Copy link
Member Author

Eomm commented Apr 29, 2020

Has the binary format been removed in v3?

I don't understand what you mean.
Could you add more info?

@adrai
Copy link
Member

adrai commented Apr 29, 2020

Sorry, I was not clear enough, (and did not even investigate)...
{ format: 'binary' } like here: fastify/fastify-swagger#47
I just tried the v3.0.0-rc.1 version and now it can't find the binary format anymore:
Failed building the validation schema for PUT: /file/organisations/:organisationId, due to error unknown format "binary" is used in schema at path "#"

@adrai
Copy link
Member

adrai commented Apr 29, 2020

Now I know why.... sorry my fault...
it's because of this change: schemaCompiler -> validatorCompiler
all green now

fralonra added a commit to fralonra/fastify that referenced this pull request Jun 14, 2020
- remove error `FST_ERR_SCH_BUILD`.

This error was removed in fastify#2023 , and doesn't exist in current master branch [error.js](https://github.com/fastify/fastify/blob/master/lib/errors.js).
These lines were re-added to the file in #13f51c06ccc4f117fd8fa1892d8edefc554746b2.
@fralonra fralonra mentioned this pull request Jun 14, 2020
4 tasks
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver-major Issue or PR that should land as semver major v3.x Issue or pr related to Fastify v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants