Improve performance for const schemas

[DanieleFedeli]

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.

Benchmark pre fix:

Benchmark post fix:

As you can see, object const schemas did not improve. I am working on that.

[mcollina]

wow, good numbers! Is there anything missing here (the PR is in draft)?

[DanieleFedeli]

I am trying to figure out if I can improve const object schemas as well

[Uzlopak]

@DanieleFedeli

What about

  if (schema.const) {
    if (typeof schema.const !== 'object') {
        return typeof schema.const
    } else if (schema.const === null) {
        return 'null'
    } else if (Array.isArray(schema.const)) {
        return 'array'
    } else {
        return 'object'
    }
  }

[DanieleFedeli]

@Uzlopak I tried but object schemas are not serialized correctly.
test/const.test.js fails

[DanieleFedeli]

Improve const strings and const objects 🚀

[mcollina]

lgtm

[Uzlopak]

I am not so convinced by this, as it is not deterministic and would break probably on circular references and bigint.

[Uzlopak]

So probably returns false if you set const: { a: 1, b: 2 } then it would return false on { b: 2, a: 1}

Maybe we need here rfdc instead of stringification for objects

Also what happens when we provide a Date.

E.g.

const: { }

and JSON.stringify on Date is also returning { }

So probably it would pass on Dates.

[Uzlopak]

sry, my mistake JSON.stringify(Date) returns the ISODateTime. But still a potential pitfall.

[Eomm]

Agree on rfdc to remove any __proto__

We know that schemas should be treated as code, but adding this const check gives to the user the possibility to inject bad code

[Uzlopak]

I would expect this to pass.

test('schema with const object', (t) => {
  t.plan(1)

  const schema = {
    type: 'object',
    properties: {
      foo: { const: { a: 1, b: 2 } }
    }
  }

  const stringify = build(schema)
  t.doesNotThrow(() => stringify({
    foo: { b: 2, a: 1 }
  }))
})

[Uzlopak]

tbh. I thought the first commit was good enough as it was improving the performance of primitives.

And I actually dont like the notation of passing an Object simply with const.

Because this works perfect and is the JSONSchema-way

test('schema with const object', (t) => {
  t.plan(2)

  const schema = {
    type: 'object',
    properties: {
      foo: {
        type: 'object',
        additionalProperties: false,
        required: ['a', 'b'],
        properties: {
          a: { const: 1 },
          b: { const: 2 }
        }
      }
    }
  }

  const stringify = build(schema)
  t.doesNotThrow(() => stringify({
    foo: { b: 2, a: 1 }
  }))
  t.doesNotThrow(() => stringify({
    foo: { a: 1, b: 2 }
  }))
})

[DanieleFedeli]

Ok then I will revert the last commit

[Uzlopak]

Please wait for the feedback of @ivan-tymoshenko. Maybe he has some idea.

[ivan-tymoshenko]

@Uzlopak @climba03003 Is there a reason we match data with a const before inserting the const? Can't we just insert the const? We have a vague explanation of when we validate data and when we don't, but it looks like we can avoid it in this case for performance purposes. WDYT?

[Eomm]

I want to start a discussion here: why not coerce the output value to the const value instead?
A json schema with const is telling me (standard quotes :

An instance validates successfully against this keyword if its value
is equal to the value of the keyword.

So, a serialization perspective should be "no matter what the input is, the output will be the const value."

I think fast-json-stringify is not a validator, it is an information filter (with rare exceptions such as anyOf and allOf)

[Eomm]

I would add an option to turn this feature on, or it will be a major version with a harder distribution across userland

[Eomm]

Agree on rfdc to remove any __proto__

We know that schemas should be treated as code, but adding this const check gives to the user the possibility to inject bad code

[Uzlopak]

i have no strong opinion on this. I just think that he first commit was a nice patch, which had only the issue, that it maybe could have resulted in some hickups like what if the const is a bigint or function. But it would have improved the performance significantly, if somebody would have provided some json schema without type information.

I assume that the following commits were under the impression that we want to improve the performance under any circumstances. Thats why I wrote, that I liked the first commit, as it was small and very effective. It does not even make fast-json-stringify to a validator as you mentioned @Eomm .

I think the first commit is a good base for a PR. But I would actually also not simply return typeof value.

more like

function getJSONSchemaType(value) {
  const type = typeof value
  if (type === 'string') {
    return type;
  } else if (type === 'boolean') {
    return type
  } else if (type === 'number') {
    // NaN and Infinity and -Infinity are not serializable
    if (isFinite(value)) {
      return type
    }
  }
}

[ivan-tymoshenko]

i have no strong opinion on this. I just think that he first commit was a nice patch, which had only the issue, that it maybe could have resulted in some hickups like what if the const is a bigint or function. But it would have improved the performance significantly, if somebody would have provided some json schema without type information.

I assume that the following commits were under the impression that we want to improve the performance under any circumstances. Thats why I wrote, that I liked the first commit, as it was small and very effective. It does not even make fast-json-stringify to a validator as you mentioned @Eomm .

I think the first commit is a good base for a PR. But I would actually also not simply return typeof value.

more like

function getJSONSchemaType(value) {
  const type = typeof value
  if (type === 'string') {
    return type;
  } else if (type === 'boolean') {
    return type
  } else if (type === 'number') {
    // NaN and Infinity and -Infinity are not serializable
    if (isFinite(value)) {
      return type
    }
  }
}

@Uzlopak, I think we've got a slight misunderstanding. What @Eomm and I mean is that FJS already validates the data when inserting a constant, which is unnecessary.

  if (schema.const) {
    return `json += JSON.stringify(schema.const)`
  }

Something like that.

[Eomm]

Let me make an example @Uzlopak

talking about the behaviour, I think this test should be always successful:

const x = stringify({
      foo: 2
    })
// x should be '{"foo":1}'

No matter what the input object is, the output should be always equals to const

[climba03003]

So, a serialization perspective should be "no matter what the input is, the output will be the const value."

Just one question, how do we handle array of types that include null.

{
  "type": ["string", "null"],
  "const": "foo"
}

It is a special case that user may expect the const to be null-able.

[Uzlopak]

That is kind of contradicting.

[climba03003]

That is kind of contradicting.

Yes, I thought of it. But, I can't really think of a way to optionally parse the const.

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

stringify({ "foo": "bar" }) // { "foo": "bar" }
stringify({ "foo": "baz" }) // { "foo": "bar" }
stringify({ "foo": 1 }) // { "foo": "bar" }
stringify({}) // { "foo": "bar" } ??? what if I want { } only

I think we should

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


stringify({ "foo": "bar" }) // { "foo": "bar" }
stringify({ "foo": "baz" }) // { "foo": "bar" }
stringify({ "foo": 1 }) // { "foo": "bar" }
stringify({}) // { }

const schema = {
  "type": "object",
  "properties": {
    "foo": {
      "const": "bar"
    }
  },
  "required": ['foo'] // here is important
}


stringify({ "foo": "bar" }) // { "foo": "bar" }
stringify({ "foo": "baz" }) // { "foo": "bar" }
stringify({ "foo": 1 }) // { "foo": "bar" }
stringify({}) // { "foo": "bar" }

[DanieleFedeli]

So what is the plan?
Should I revert to 68ff81c and then open an issue for objects?

[Eomm]

So what is the plan? Should I revert to 68ff81c and then open an issue for objects?

No, that commit is not a problem.
The discussion here is that the following line applies a validation processing that is unwanted:

throw new Error(\`Item $\{JSON.stringify(${input})} does not match schema definition.\`)`

I think that we should force the const value into the serialized object whatever the input object is.

The @climba03003 #505 (comment) is the perfect summary of how a serializer should use a const.
The type: null case could be added in a follow PR because it is an edge case

Moreover, this feature should be hidden by an option flag: coerceConst: Bool if you need to ship this feature in the current major version.

If someone does not agree with my opinion, please explain.

[DanieleFedeli]

@Eomm In this case I agree with you.
I added validation because I saw the way how const schema was handled:

case undefined:
.
.
.
.
else if ('const' in schema) {
 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.\`)`
}

But if it not needed I am happy to remove it

[ivan-tymoshenko]

@Eomm In this case I agree with you. I added validation because I saw the way how const schema was handled:

case undefined:
.
.
.
.
else if ('const' in schema) {
 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.\`)`
}

But if it not needed I am happy to remove it

Please don't forget about null case.