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

Worse performance with response schema when 'const' keyword is used #502

Closed
2 tasks done
DanieleFedeli opened this issue Aug 7, 2022 · 6 comments · Fixed by #505
Closed
2 tasks done

Worse performance with response schema when 'const' keyword is used #502

DanieleFedeli opened this issue Aug 7, 2022 · 6 comments · Fixed by #505
Labels
bug Confirmed bug help wanted Help the community by contributing to this issue

Comments

@DanieleFedeli
Copy link
Contributor

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.3.0

Plugin version

No response

Node.js version

16.13.0

Operating system

macOS

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

13.0

Description

When specifying the response schema with 'const' I noticed that the RPS decreased.

Steps to Reproduce

Options used:

{
  schema: {
    response: {
        200: {
          type: "object",
          properties: {
            msg: { type: "string" },
          }
      }
   }
}

Schema benchmark:
Screenshot 2022-08-07 alle 12 06 23

Options used:

{
  schema: {
    response: {
        200: {
          type: "object",
          properties: {
            msg: { const: "hello world" },
          }
      }
   }
}

Schema with const:
Screenshot 2022-08-07 alle 12 08 13

Repository with the code: https://github.com/DanieleFedeli/fastify-response-schema-issue

Expected Behavior

I expect at least the same performance when specifying const in the schema.

@mcollina
Copy link
Member

mcollina commented Aug 7, 2022

Very interesting! This looks like a bug in fast-json-stringify. I'll move the bug there.

@mcollina mcollina transferred this issue from fastify/fastify Aug 7, 2022
@mcollina mcollina added bug Confirmed bug help wanted Help the community by contributing to this issue labels Aug 7, 2022
@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 7, 2022

How is the performance if we also use type: "string" or if we use enum instead of const?

@DanieleFedeli
Copy link
Contributor Author

With type string

{
  type: "object",
  properties: {
    msg: { type: "string", const: "hello world" },
  }
}

Screenshot 2022-08-07 alle 16 18 25

With enum

{
  type: "object",
  properties: {
    msg: { enum: ["hello world"] },
  }
}

Screenshot 2022-08-07 alle 16 20 00

It improves drastically

@climba03003
Copy link
Member

climba03003 commented Aug 7, 2022

I would say it is expected.
When the type is not defined and const is exist, FJS need to guess whether the content is meet the schema or not.
So, it went through ajv for the guessing and it is expected to be slow in this case.

code += `
if(ajv.validate(${JSON.stringify(schema)}, ${input}))
json += '${JSON.stringify(schema.const)}'
else
throw new Error(\`Item $\{JSON.stringify(${input})} does not match schema definition.\`)
`

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 7, 2022

But i can determine the type of const before i use it in ajv. So do we have to improve the docs or should we write a function which traverses the entries for const and determines the corresponding type?

@RafaelGSS
Copy link
Member

+1 to mention it in the docs and open a new fresh issue to optimize it as we can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug help wanted Help the community by contributing to this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants