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

Schema errors since Fastify 4 #4028

Closed
2 tasks done
qqilihq opened this issue Jun 16, 2022 · 18 comments
Closed
2 tasks done

Schema errors since Fastify 4 #4028

qqilihq opened this issue Jun 16, 2022 · 18 comments
Labels
bug Confirmed bug

Comments

@qqilihq
Copy link

qqilihq commented Jun 16, 2022

Prerequisites

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

Fastify version

4.0.3

Plugin version

No response

Node.js version

16.13.0

Operating system

macOS

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

11.6.6

Description

First: Thank you for your work on Fastify 4. The changes look definitely promising (especially looking forward to the type providers which should make our code much cleaner) and I have just upgraded an existing project to Fastify 4.

During upgrading, I encountered the following errors related to the schema which caused me quite some head banging:

  • schema with key or id "…" already exists
  • "…" resolves to more than one schema

Steps to Reproduce

While I cannot give our entire schema here, I try to show the (as I think) relevant parts which contribute to this issue. We define our schema using typebox, and after upgrading our dependencies I had to apply these suggestions. Also we use @fastify/swagger.

export const File = Type.Object(
  {
    _id: Type.Ref(ObjectId),
    creator: Type.Ref(ObjectId),
    owner: Type.Ref(ObjectId),
    // .. further irrelevant properties omitted
  },
  { $id: 'File' }
);

export type File = Static<typeof File>;

export const Article = Type.Object(
  {
    _id: Type.Ref(ObjectId),
    imageBig: Type.Union([Type.Ref(File), Type.Null()]),
    imageSmall: Type.Union([Type.Ref(File), Type.Null()]),
    // .. further irrelevant properties omitted
  },
  { $id: 'Article' }
);

export type Article = Static<typeof Article>;

export const ObjectId = Type.String({
  pattern: '[0-9a-f]{24}',
  $id: 'ObjectId',
});

export type ObjectId = Static<typeof ObjectId>;

I load these through a plugin as follows:

import fastifyPlugin from 'fastify-plugin';
// .. imports

export default fastifyPlugin((fastify) => {
  fastify.addSchema(ObjectId);
  fastify.addSchema(File);
  fastify.addSchema(Article);
  return Promise.resolve();
});

The crucial parts:

  • File has more than one $ref to ObjectId
  • Article has more than one $ref to File

When starting the server, I would either see schema with key or id "ObjectId" already exists or "File" resolves to more than one schema. The behavior was unfortunately not really systematic or deterministic, and depended on the presence or absence of other (seemingly unrelated) routes.

After spending several hours, I came up with the following fixes which got the back to running:

  1. In fastify's schema-controller.js copy the schemas in getSchemas:

    diff --git a/node_modules/fastify/lib/schema-controller.js b/node_modules/fastify/lib/schema-controller.js
    index 1484019..98aeafe 100644
    --- a/node_modules/fastify/lib/schema-controller.js
    +++ b/node_modules/fastify/lib/schema-controller.js
    @@ -59,7 +59,7 @@ class SchemaController {
       }
     
       getSchemas () {
    -    return this.schemaBucket.getSchemas()
    +    return JSON.parse(JSON.stringify(this.schemaBucket.getSchemas()))
       }
     
       // Schema Controller compilers holder
  2. In fast-json-stringify, $refs seem to get dereferenced to full schema objects, but when occuring several times the duplicate $id properties lead to the issue described above. My (very dirty) fix was to simply strip out all $id, but I'm sure that's not the right way to do it:

    diff --git a/node_modules/fast-json-stringify/index.js b/node_modules/fast-json-stringify/index.js
    index 1952905..484ceb7 100644
    --- a/node_modules/fast-json-stringify/index.js
    +++ b/node_modules/fast-json-stringify/index.js
    @@ -964,6 +964,21 @@ function buildValue (locationPath, input, location) {
               // Ajv does not support js date format. In order to properly validate objects containing a date,
               // it needs to replace all occurrences of the string date format with a custom keyword fjs_date_type.
               // (see https://github.com/fastify/fast-json-stringify/pull/441)
    +
    +          // fix for --
    +          // * schema with key or id "…" already exists
    +          // * reference "…" resolves to more than one schema
    +          function stripId(obj) {
    +            delete obj.$id;
    +            Object.entries(obj).forEach(([_key, value]) => {
    +              if (typeof value === 'object') {
    +                stripId(value);
    +              }
    +            });
    +            return obj;
    +          }
    +          stripId(location.schema);
    +
               const extendedSchema = clone(location.schema)
               extendDateTimeType(extendedSchema)

Various code comments and issues (such as this) suggest issues due to mutation of the schema, and I guess my issue is also caused by this.

I would love to provide a runnable example, however I was not really able to reproduce it within a manageable amount of code yet. If there's any more details I can provide, please let me know. On the other hand, I will be happy for any feedback, suggestions, etc.

Expected Behavior

No response

@jsumners
Copy link
Member

Are you actually trying to register the same schema (by $id) more than once?

@qqilihq
Copy link
Author

qqilihq commented Jun 16, 2022

@jsumners Actually no (at least not knowingly 🙂 ).

There's only the fastify.addSchema(ObjectId);, all other occurrences are Type.Ref to this one.

@jsumners
Copy link
Member

I don't use the schema container you are referencing. So I don't know what you're stating. It sounds like either:

  1. You have duplicate schema ids.
  2. You are registering the same schema multiple times under the assumption each will be registered in a different schema encapsulation context.

A reproduction repo would be most helpful.

@qqilihq
Copy link
Author

qqilihq commented Jun 16, 2022

I have distilled this down into a minimal example: https://github.com/qqilihq/fastify-issue-4028

During this, I figured out that this one will cause the issue:

image: Type.Union([Type.Ref(File), Type.Null()]),

@jsumners
Copy link
Member

@fastify/typescript

cc: @sinclairzx81

@climba03003
Copy link
Member

climba03003 commented Jun 16, 2022

It is not related to TypeScript and actually related to how fast-json-stringify treat the $ref key.
If you move the response to querystring, that means it used by ajv.
The server can be started successfully.

Minimal repro extracted from https://github.com/qqilihq/fastify-issue-4028

import Fastify from "fastify";

const fastify = Fastify();
fastify.addSchema({
  $id: "ObjectId",
  type: "string",
});
fastify.addSchema({
  $id: "File",
  type: "object",
  properties: {
    _id: { $ref: "ObjectId" },
    name: { type: "string" },
    owner: { $ref: "ObjectId" },
  },
});
fastify.addSchema({
  $id: "Article",
  type: "object",
  properties: {
    _id: {
      $ref: "ObjectId",
    },
    name: {
      type: "string",
    },
    image: {
      anyOf: [
        {
          $ref: "File",
        },
        {
          type: "null",
        },
      ],
    },
  },
});

fastify.get(
  "/",
  { schema: { response: { 200: { $ref: "Article" } } } },
  () => {}
);

await fastify.ready();

@climba03003 climba03003 added the bug Confirmed bug label Jun 16, 2022
@sinclairzx81
Copy link
Contributor

@jsumners I had a quick look at this, and wasn't able to reproduce the duplicate ID error through Fastify, so for example, the following is ok.

import { Type } from '@sinclair/typebox'
import Fastify from 'fastify'

const fastify = Fastify({
    ajv: {
        customOptions: {
            keywords: ['kind', 'modifier']
        }
    }
})

const File = Type.Object({
    name: Type.String(),
    size: Type.Integer()
}, { $id: 'File' })

fastify.addSchema(File)

fastify.post('/', {
    schema: {
        body: Type.Object({
            image: Type.Union([Type.Ref(File), Type.Null()]),
        })
    }
}, (req, res) => res.send('ok'))

fastify.listen({ port: 5000 })

But was able to get fast-json-stringify throwing duplicate ID errors if configuring both ajv and the schema options (which maybe indirectly configured somehow). I believe the preference is to configure schema only.

import { Type } from '@sinclair/typebox'
import Serialize from 'fast-json-stringify'

const File = Type.Object({
    name: Type.String(),
    size: Type.Integer()
}, { $id: 'File' })

const T = Type.Object({
    image: Type.Union([Type.Ref(File), Type.Null()]),
})

Serialize(T, { 
    schema: { File },
    // ajv: { schemas: { File } } // uncommenting this line produces the error
                                  //
                                  // Error: schema with key or id "File" already exists
})

Not sure if the second example might be a clue as to what error might be occurring, but for all intents and purposes, the reference external schema should be working ok (as per the users example).

Hope that helps
S

mcollina added a commit that referenced this issue Jun 21, 2022
As part of the error handling refactoring of #3261, we should
not be setting a custom status code for validation routes.
We should rely only on the error.statusCode property instead and
leave the user full customization capabilities. Unfortunately
a change was missed.

Fixes: #4028
Ref: #3261
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina
Copy link
Member

This was fixed by #4049

@mcollina
Copy link
Member

You can try main if you'd like. It'll likely go out next week.

@qqilihq
Copy link
Author

qqilihq commented Jun 23, 2022

First: Thank you all for your efforts!

Unfortunately, it still doesn't work for me. I have updated the repro repo above, and I still get:

FastifyError [Error]: Failed building the serialization schema for HEAD: /article, due to error reference "ObjectId" resolves to more than one schema
    at Boot.<anonymous> (/Users/pk/Desktop/fastify-schema-issue/node_modules/fastify/lib/route.js:318:21)
    at Object.onceWrapper (node:events:509:28)
    at Boot.emit (node:events:402:35)
    at /Users/pk/Desktop/fastify-schema-issue/node_modules/avvio/boot.js:160:12
    at /Users/pk/Desktop/fastify-schema-issue/node_modules/avvio/plugin.js:276:7
    at done (/Users/pk/Desktop/fastify-schema-issue/node_modules/avvio/plugin.js:201:5)
    at check (/Users/pk/Desktop/fastify-schema-issue/node_modules/avvio/plugin.js:225:9)
    at node:internal/process/task_queues:141:7
    at AsyncResource.runInAsyncScope (node:async_hooks:199:9)
    at AsyncResource.runMicrotask (node:internal/process/task_queues:138:8) {
  code: 'FST_ERR_SCH_SERIALIZATION_BUILD',
  statusCode: 500
}

@mcollina
Copy link
Member

@qqilihq I think the error is actually correct, you are reusing the same objectid twice.

@qqilihq
Copy link
Author

qqilihq commented Jun 23, 2022

@mcollina Thanks for the reply. Actually (knowingly) I define it only once end then use $ref (via Type.Ref(ObjectId)) to refer to it, e.g. here:

https://github.com/qqilihq/fastify-issue-4028/blob/main/src/schemas/file.schema.ts#L6

If there's something I'm misunderstanding fundamentally, I'll be happy for any hint :-)

@mcollina
Copy link
Member

How do I run that project? It doesn't seem to start or generate an error.

@qqilihq
Copy link
Author

qqilihq commented Jun 23, 2022

@mcollina

git clone https://github.com/qqilihq/fastify-issue-4028.git
cd fastify-issue-4028
yarn
yarn start

@mcollina mcollina reopened this Jun 23, 2022
@mcollina
Copy link
Member

The problem is caused by exposeHeadRoutes: true, I think we are not resetting something correctly.

@qqilihq
Copy link
Author

qqilihq commented Jun 23, 2022

@mcollina I can indeed confirm that this fixes the issue with the sample project posted above, thank you!

Unfortunately, I still get an error in my real, more complex project for a PUT route, even after adding exposeHeadRoutes: true 🤔 (Failed building the serialization schema for PUT). In this case, there's a GET, PUT, and DELETE for the same route path, so probably the same/similar issue?

@mcollina
Copy link
Member

fastify/fast-json-stringify#470 solves the problem. Update your deps, it work for me now!

@qqilihq
Copy link
Author

qqilihq commented Jun 23, 2022

Yeah -- this worked! Thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants