Improve performance for const schemas by DanieleFedeli · Pull Request #511 · fastify/fast-json-stringify

DanieleFedeli · 2022-08-25T17:06:46Z

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 the Developer's Certification of Origin
and the Code of conduct

As per issue #502, when using a const schema, the performance degrades. This is the continuation of #505.

Benchmark

Pre PR

Post PR

This PR supports nullable values:

test('schema with const string and no input', t => {
  t.plan(2)

  const schema = {
    type: 'object',
    properties: {
      foo: { const: 'bar' }
    }
  }

  const validate = validator(schema)
  const stringify = build(schema)
  const output = stringify({})

  t.equal(output, '{}')
  t.ok(validate(JSON.parse(output)), 'valid schema')
})

Or it coerces the value to the const property if it is required

test('schema with const string and no input but required property', t => {
  t.plan(2)

  const schema = {
    type: 'object',
    properties: {
      foo: { const: 'bar' }
    },
    required: ['foo']
  }

  const validate = validator(schema)
  const stringify = build(schema)
  const output = stringify({})

  t.equal(output, '{"foo":"bar"}')
  t.ok(validate(JSON.parse(output)), 'valid schema')
})

index.js

mcollina

lgtm

index.js

ivan-tymoshenko · 2022-08-25T19:32:57Z

index.js

        `
-      } else if ('const' in schema) {
-        code += `
+        } else if ('const' in schema) {


This should be removed.

ivan-tymoshenko · 2022-08-25T19:39:42Z

index.js

-    const defaultValue = schema.properties[key].default
+    let defaultValue = schema.properties[key].default
+    if (isRequired) {
+      defaultValue = defaultValue !== undefined ? defaultValue : schema.properties[key].const


const shouldn't work as a default value. If you define a schema constant property, it must const value, whatever value the user passes.

- Removed switchTypeSchema - Use of variable isRequired where defined - Rebase to PR #510 - Remove use of default value for const

ivan-tymoshenko · 2022-08-26T15:15:02Z

index.js

+      )}
      `
-    } else if (required.includes(key)) {
+    } else if (isRequired && isConst) {


This should work in a different way.

You should handle value serialization only in one place otherwise you will have code duplication and can make an error. In your case, you miss the null check here. So please handle all schema serialization logic inside buildValue function.

We don't need to do this check for const

fast-json-stringify/index.js

Line 365 in 844eba5

if (obj[${sanitized}] !== undefined) {

.

Null checks you mean this?

const schema = { "type": "object", "properties": { "foo": { "const": "bar" } } } stringify({ "foo": "bar" }) // { "foo": "bar" } stringify({ "foo": "baz" }) // { "foo": "bar" } stringify({ "foo": 1 }) // { "foo": "bar" } stringify({}) // { }

To answer 2.
If I don't have that checks how can I handle this?

const schema = { "type": "object", "properties": { "foo": { "const": "bar" } } } stringify({}) // { }

I think that the const value shouldn't work as a default value. It shouldn't have any other additional behavior.

const schema = { "type": "object", "properties": { "foo": { "const": "bar" } }, "required": ['foo'] } stringify({}) // throw new Error('foo is required')

I am OK with that, but I remember that someone agrees on this comment.

Yes, you are right, it should. From my point of view. But let's wait for other opinions.

If you are waiting for me, I'm leaving this to you.

I would like to hear @climba03003 opinion as it was his suggestion #505 (comment).

When the schema said the property is required, and missing one from the input.
It just convenient to always put the value there since it is required.

Just like why we always put the const value regardless the input.

I do not block if it turns to throw.
It consistences with the current required handling for other properties.

Uzlopak · 2022-08-26T15:44:48Z

I read the code. Now it is like 80% reformatting. 19% tests and 1% change. And somehow i dont see any code which is responsible for comparing const with the actual value. I guess ajv does the validation? I cant follow, lol. How i read it, it is always serializing const no matter what the actual value is.

I am not that into the codebase so i guess i am out of here.

DanieleFedeli · 2022-08-26T15:47:14Z

@Uzlopak Yes my bad, I don't know why I have the format changes since I don't even have the formats on save options.

ivan-tymoshenko · 2022-08-26T15:51:35Z

@Uzlopak yes you are right. The main idea of #505 (review), #505 (comment) was to ignore the actual value and always insert the const value. I agree with this point of view because fjs is not a validator and if we can do something without validation we don't have to do validation. If you don't agree please share your thoughts.

ivan-tymoshenko · 2022-08-26T15:59:16Z

@DanieleFedeli Null check.

const schema = {
  "type": "object",
  "properties": {
    "foo": {
      "nullable": true
      "const": "bar"
    }
  }
}

stringify({foo:null}) // {foo:null}

const schema = {
  "type": "object",
  "properties": {
    "foo": {
      "type": [...whatever, 'null']
      "const": "bar"
    }
  }
}

stringify({foo:null}) // {foo:null}

ivan-tymoshenko · 2022-08-27T07:53:14Z

@DanieleFedeli, Can you revert the formatting changes? It would be easier to review for other people.

DanieleFedeli · 2022-08-27T07:58:21Z

I can try

mcollina

lgtm

mcollina · 2022-08-29T08:37:00Z

@climba03003 are you ok for this to land?

climba03003

Yes.
LGTM

DanieleFedeli added 6 commits August 16, 2022 11:03

Improved performance for const schema

68ff81c

Merge branch 'master' of github.com:DanieleFedeli/fast-json-stringify

dbd3395

Improve const object schema performance

5fa6e25

Improve string and object performance

ca55edf

Remove unwanted validation for 'const' properties

b2ce4d5

Restored example file

05c92cf

Uzlopak reviewed Aug 25, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

Changed findIndex with indexOf

f73da5a

ivan-tymoshenko reviewed Aug 25, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

ivan-tymoshenko reviewed Aug 25, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

Removed useless inference when schema is const

e2c3bd6

mcollina approved these changes Aug 25, 2022

View reviewed changes

Uzlopak reviewed Aug 25, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

ivan-tymoshenko requested changes Aug 25, 2022

View reviewed changes

DanieleFedeli added 3 commits August 26, 2022 16:50

Review requests

ca1357d

- Removed switchTypeSchema - Use of variable isRequired where defined - Rebase to PR #510 - Remove use of default value for const

Rebase to #510 (use hasOwnProperty from Object.prototype)

844eba5

Use isRequired when available

49301dc

ivan-tymoshenko reviewed Aug 26, 2022

View reviewed changes

DanieleFedeli added 3 commits August 27, 2022 17:14

Restored FJS format style

0eb362a

Restore FJS format style

b80bc4a

Added support for nullable / type: [..., 'null']

3879db9

ivan-tymoshenko approved these changes Aug 28, 2022

View reviewed changes

DanieleFedeli mentioned this pull request Aug 28, 2022

Infer type schema #512

Closed

4 tasks

mcollina approved these changes Aug 29, 2022

View reviewed changes

climba03003 approved these changes Aug 29, 2022

View reviewed changes

mcollina merged commit ddb1c48 into fastify:master Aug 29, 2022

This was referenced Aug 30, 2022

refactor: remove const validation branch #515

Merged

feat: drop nullable const support #520

Closed

fix: remove array items type checking #524

Merged

Uh oh!

Conversation

DanieleFedeli commented Aug 25, 2022

Checklist

Benchmark

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivan-tymoshenko Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanieleFedeli Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivan-tymoshenko Aug 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

climba03003 Aug 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Aug 26, 2022

Uh oh!

DanieleFedeli commented Aug 26, 2022

Uh oh!

ivan-tymoshenko commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivan-tymoshenko commented Aug 26, 2022

Uh oh!

ivan-tymoshenko commented Aug 27, 2022

Uh oh!

DanieleFedeli commented Aug 27, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Aug 29, 2022

Uh oh!

climba03003 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ivan-tymoshenko Aug 26, 2022 •

edited

Loading

DanieleFedeli Aug 26, 2022 •

edited

Loading

ivan-tymoshenko Aug 27, 2022 •

edited

Loading

climba03003 Aug 27, 2022 •

edited

Loading

ivan-tymoshenko commented Aug 26, 2022 •

edited

Loading