PR-2044 (one schema for multiple routes) introduce breaking change behavior on routes schema resolve.

[SkeLLLa]

💥 Regression Report

PR #2044 breaks https://github.com/SkeLLLa/fastify-oas and probably related to fastify/fastify-swagger#224

Consider you have fastify onRoute hook and save the routes like

const myRouteData = []

fastify.addHook('onRoute', (route) => {
  myRouteData.push(route);
});

Earlier after .ready() method is finished his job, e.g. await app.ready(), in schema in route object was resolved to actual schema objects.

E.g. if you referenced schemas by id

{
  query: 'mySchema#'
}

after app is ready you could expect an object in route.schema.querystring.

However now it's not resolved and still is a string.

Last working version

Worked up to version: 2.11.0

Stopped working in version: 2.12.0

To Reproduce

Steps to reproduce the behaviour:

1. Add schema with fastify.addSchema
2. Reference that schema in route

Paste your code here:

const routes = []
fastify.addHook('onRoute', (route) => {
    // here route.schema should be an object
  routes.push(route)
});
await fastify.ready()
// iterate over routes and check if `body`, `params`, etc. are not strings.

Expected behavior

It expects that route.schema.[body,querystring,params,headers] will be an resolved object after app is ready. However after that PR it is a string (when schema ids are used)

Your Environment

• node version: 12
• fastify version: >=2.12.0
• os: Linux
• any other relevant information

[mcollina]

@SkeLLLa what would be your recommendation? Thanks.

[SkeLLLa]

@mcollina that's depends actually on that PR and if it could be done in different way.

During that Object.assign https://github.com/fastify/fastify/pull/2044/files#diff-bcf1db07025c10e7fdb2d2a49682c010R59 if we have string references to schema they will be "cloned", while earlier they were replaced later in original object.

And after that there's no easy way of getting resolved schemes. So as it was done in initial fastify-swagger PR you should use getSchemes. But if you have "scoped" schemes, then you should collect all schemes from fastify "child" scopes and then perform lookup by id.

According to this snippet #2024 (comment) I'd rather recommend just to clone schema object (shallow or deep - doesn't matter). Because in most cases there will be no need for having the same schemes for two different routes.
Another way to "workaround" issue described in link - is to have named scheme, instead of putting the same object into different routes. You can add scheme via fastify.addScheme and use id reference.

So I think it's easier to add documentation about using same scheme object in different routes and advice to use named schemes in this situation, rather applying fixes to several modules.

Or probably we can add some method to get all routes with resolved schemes, but I don't know if it's really good idea, cause it will need to walk through all fastify child scopes tree. This will help to solve fastify-swagger and fastify-oas issues without lot's of changes, but I'm not sure about how other people may used that functionality.

TLDR:

1. Find some other solution for that "workaround" (e.g. somewhere in https://github.com/fastify/fastify/pull/2044/files#diff-bcf1db07025c10e7fdb2d2a49682c010R89 we could check if schema was actually a string, and if it was, then we need to replace it in routeSchemas with an object)
2. Revert the change, consider it's breaking and land in 3.0
3. Revert workaround and document duplication issue with advice to use named schemes in this case
4. Expose method that returns an array of all routes with resolved schemes and update all plugins that rely on onRoute hook and schemes.

PS. May be @snacqs have some ideas about how to improve that workaround?

[SkeLLLa]

I'm actually for option 3, cause Original issue was about sharing single schema between two routes. And fastify already has documentation related to shared schemes https://github.com/fastify/fastify/blob/master/docs/Validation-and-Serialization.md#adding-a-shared-schema.

Btw shared schemes were mentioned in original issue #2024 (comment).

[Eomm]

I took master and reverted the commit and the issue is still there.
Also check out the v2.11 the route.schema is { query: 'mySchema#' } in the hooks

Moreover, the onRoute hook is run before compiling the schemas:

fastify/lib/route.js

Lines 193 to 194 in 9b53f9a

	// run 'onRoute' hooks
	for (const hook of this[kGlobalHooks].onRoute) {

So I think the issue is something else

Here the test I tried:

test('onRoute should have compiled schemas - replace-way', t => {
  t.plan(3)
  const fastify = Fastify()

  fastify.addHook('onRoute', function (route) {
    t.deepEqual(route.schema.query, {
      type: 'object',
      properties: {
        host: { type: 'number' }
      }
    })
    t.deepEqual(route.schema.querystring, {
      type: 'object',
      properties: {
        host: { type: 'number' }
      }
    })
  })

  fastify.addSchema({
    $id: 'mySchema',
    type: 'object',
    properties: {
      host: { type: 'number' }
    }
  })

  fastify.get('/', {
    schema: { query: 'mySchema#' }
  }, (req, reply) => {
    reply.send('hello')
  })

  fastify.inject({
    method: 'GET',
    url: '/'
  }, (err, res) => {
    t.error(err)
    t.strictEqual(res.statusCode, 200)
  })
})

[SkeLLLa]

@Eomm

Moreover, the onRoute hook is run before compiling the schemas.

Yes, it does. But afterwards (when app calls ready) those route objects are mutated and if you store them (by reference) somewhere, they will be updated. Actually that's why fastify-swagger's initializer is called after ready fired:

fastify.ready(err => {
  if (err) throw err
  fastify.swagger()
})

I definitely sure that this regression was introduced in 2.12, but I'll doublecheck now if it's was done in that PR.

[Eomm]

I definitely sure that this regression was introduced in 2.11, but I'll doublecheck now if it's was done in that PR.

I'm not arguing with that, I think we should triage more this issue 👍

If you have also a little test of your module to share, that was ok with 2.11 and fails with 2.12 it would be helpful (I was unable to find it here )

[SkeLLLa]

@Eomm I don't have those tests yet. But probably the same issue fastify/fastify-swagger#226 could help you.

Basically to reproduce the issue:

1. Add on route hook and store routes in some array
2. Add shared schema (for query, for example)
3. Add route that uses that shared schema
4. Here in onRoute you'll receive string in query.
5. Call fastify's ready function and await it. That's important step.
6. Check array created in step 1. Now route should contain an object in query, not string.

I think you could modify test's you've wrote by adding array for storing routes, fastify.ready call and checking array for resolved schemes.

[SkeLLLa]

That's what I think about what's happening here.

When you add a route onRoute hook is triggered and and you receive object with a route and unresolved schemes.

After that when fastify.ready is called, fastify calls https://github.com/fastify/fastify/blob/master/lib/schemas.js#L45.

And there traverse metod is called https://github.com/fastify/fastify/blob/master/lib/schemas.js#L69 and it replaced strings with resolved objects in https://github.com/fastify/fastify/blob/master/lib/schemas.js#L107

Before that PR that traverse was called on the same object, that was in onRoute hook. But now it's called on cachedSchema that's a shallow copy of original route object https://github.com/fastify/fastify/blob/master/lib/schemas.js#L59.

As it's a shallow copy keys like query, querystring, body, etc. that contain strings are cloned and they will not be updated by reference as it was before.

[mcollina]

@SkeLLLa would you like to send a PR? I'm a bit lost on what is the proposed fix for this.

[SkeLLLa]

@mcollina we haven't decided yet :). I'm on reverting that #2044 cause for sharing schemes between routes fastify already has shared schemes. But I'm not sure if everyone else agree.

[Eomm]

Now I understand, the test I posted above #2104 (comment) is wrong because the check should be done after the ready, not in the hook.

I would like to give it another check to understand if we can solve both the issues (this and the #2044)