fix: non-nullable schema receiving null input does not coerce for default types on object and arrays #670

rhighs · 2023-12-27T21:18:55Z

This PR is related to issue #669. The changes applied bring an error being thrown with a somewhat descriptive message whenever a non-nullable schema of type 'object' validates against a null value.

Before this change, the generated code would ignore checks on the input value, trying to access properties on a null value which of course results in the application throwing cannot read <prop> of undefined. To address this we simply add a check on the function head and throw a meaningful error if needed.

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

ivan-tymoshenko

IMHO if user is not sure that his data matches the schema he should use a validator that will provide him a nice error. This change adds another validation check to the hot path that doesn't have anything to do with a serialization. I don't want turn this library to validator.

mcollina

I think we should be skipping the object if it's null or undefined.

rhighs · 2023-12-27T21:42:38Z

IMHO if user is not sure that his data matches the schema he should use a validator that will provide him a nice error. This change adds another validation check to the hot path that doesn't have anything to do with a serialization. I don't want turn this library to validator.

I really do get it, but it's the use of obj assuming it'll never be null that is just wrong IMO. A null check is very little cost for the value it brings. As far as i can see, there's a bunch of validation being done already, e.g. required fields.

rhighs · 2023-12-27T21:43:28Z

I think we should be skipping the object if it's null or undefined.

You mean just early exit the function?

ivan-tymoshenko · 2023-12-27T21:47:53Z

I really do get it, but it's the use of obj assuming it'll never be null that is just wrong IMO. A null check is very little cost for the value it brings. As far as i can see, there's a bunch of validation being done already, e.g. required fields.

The rule is simple. If your object can be null you use the nullable keyword, if not you don't use it. If you don't trust your data you validate it.

If you see some validation checks it doesn't mean you should bring more of them.

rhighs · 2023-12-27T21:54:42Z

I really do get it, but it's the use of obj assuming it'll never be null that is just wrong IMO. A null check is very little cost for the value it brings. As far as i can see, there's a bunch of validation being done already, e.g. required fields.

The rule is simple. If your object can be null you use the nullable keyword, if not you don't use it. If you don't trust your data you validate it.

If you see some validation checks it doesn't mean you should bring more of them.

Can't disagree, but It's pretty much just crashing as of now and it's hard to track down.

How about we return an empty object {}?

ivan-tymoshenko · 2023-12-27T22:02:22Z

Can't disagree, but It's pretty much just crashing as of now and it's hard to track down.

Which pretty much described in a documentation: https://github.com/fastify/fast-json-stringify#security-notice

Users are responsible for sending trusted data. fast-json-stringify guarantees that you will get a valid output only if your input matches the schema or can be coerced to the schema. If your input doesn't match the schema, you will get undefined behavior.

How about we return an empty object {}?

IMHO this looks even more dangerous to me, because if you don't notice that some of your code returns a null instead of a real object, you might have a problem if an empty object is also a valid response. From the application logic throwing an error is a correct behaviour. I just don't agree that this should be done in this library.

ivan-tymoshenko · 2023-12-27T22:07:28Z

From the other hand we have this paragraph (which I don't like either, but it doesn't matter) that describes how we coerce simple types if they are null, but schema is not nullable. And we support that. Maybe an empty object is a good idea.

rhighs · 2023-12-27T22:08:25Z

Can't disagree, but It's pretty much just crashing as of now and it's hard to track down.

Which pretty much described in a documentation: https://github.com/fastify/fast-json-stringify#security-notice

Users are responsible for sending trusted data. fast-json-stringify guarantees that you will get a valid output only if your input matches the schema or can be coerced to the schema. If your input doesn't match the schema, you will get undefined behavior.

How about we return an empty object {}?

IMHO this looks even more dangerous to me, because if you don't notice that some of your code returns a null instead of a real object, you might have a problem if an empty object is also a valid response. From the application logic throwing an error is a correct behaviour. I just don't agree that this should be done in this library.

Well, again I really do agreee but we could argue the same about this why is it bothering about nullable if we're only trying to serialize stuff?

Can't the nullable field just be ignored and serialize to null anything that happens to be null? It's not really clear to me why is it trying to behave differently on whether that field is set or not, doesn't the validator handle nulls itself?

mcollina

lgtm

mcollina · 2023-12-27T22:15:53Z

An obscure error from generated code is not useful. Either we throw or we consider null like undefined, but something we should change for the sake of DX.

ivan-tymoshenko · 2023-12-27T22:16:43Z

@rhighs please check this: #670 (comment)

rhighs · 2023-12-28T02:44:11Z

@ivan-tymoshenko Coercing into an empty object kinda makes sense but what's the added value? we'd still have to check for null. Raising an error is more appropriate IMO.

In case we wanna choose the first route function input could be defaulted to be an empty object here

const obj = (${toJSON('input')}) || {}

ivan-tymoshenko · 2023-12-28T18:14:56Z

It would be consistent having that we already coerce null to empty string, zero etc. Why logic for object should be different?

mcollina · 2023-12-28T19:45:09Z

I agree.

test/toJSON.test.js

mcollina

lgtm

can you update the PR title?

ivan-tymoshenko

Can you update the doc please? Add the object example here: https://github.com/fastify/fast-json-stringify#nullable-object.
Can you check/update the same behaviour for array. It should return an empty array for the null value. If not I will create another PR later.
Can you run the benchmarks please, as it affects the hot path?

index.js

rhighs · 2023-12-30T21:42:20Z

Can you update the doc please? Add the object example here: https://github.com/fastify/fast-json-stringify#nullable-object.

Can you check/update the same behaviour for array. It should return an empty array for the null value. If not I will create another PR later.

Can you run the benchmarks please, as it affects the hot path?

On a Ryzen 7 5800X Locked at 4.2 GHz, 16GB RAM, 8 cores and 16 threads:

FJS creation x 7,679 ops/sec ±1.16% (90 runs sampled)
CJS creation x 201,144 ops/sec ±0.40% (97 runs sampled)
AJV Serialize creation x 117,714,399 ops/sec ±0.67% (94 runs sampled)
JSON.stringify array x 4,068 ops/sec ±0.92% (97 runs sampled)
fast-json-stringify array default x 10,746 ops/sec ±0.54% (93 runs sampled)
fast-json-stringify array json-stringify x 11,102 ops/sec ±0.93% (93 runs sampled)
compile-json-stringify array x 10,656 ops/sec ±0.79% (95 runs sampled)
AJV Serialize array x 10,362 ops/sec ±1.37% (90 runs sampled)
JSON.stringify large array x 198 ops/sec ±0.77% (84 runs sampled)
fast-json-stringify large array default x 143 ops/sec ±1.40% (81 runs sampled)
fast-json-stringify large array json-stringify x 199 ops/sec ±0.44% (85 runs sampled)
compile-json-stringify large array x 522 ops/sec ±0.39% (94 runs sampled)
AJV Serialize large array x 162 ops/sec ±0.97% (82 runs sampled)
JSON.stringify long string x 12,593 ops/sec ±0.39% (96 runs sampled)
fast-json-stringify long string x 12,505 ops/sec ±0.32% (94 runs sampled)
compile-json-stringify long string x 12,557 ops/sec ±0.78% (94 runs sampled)
AJV Serialize long string x 31,406 ops/sec ±0.58% (88 runs sampled)
JSON.stringify short string x 14,173,359 ops/sec ±0.68% (98 runs sampled)
fast-json-stringify short string x 52,433,410 ops/sec ±0.34% (94 runs sampled)
compile-json-stringify short string x 50,096,908 ops/sec ±0.94% (98 runs sampled)
AJV Serialize short string x 48,903,497 ops/sec ±3.37% (93 runs sampled)
JSON.stringify obj x 2,655,636 ops/sec ±0.46% (94 runs sampled)
fast-json-stringify obj x 11,119,848 ops/sec ±0.64% (95 runs sampled)
compile-json-stringify obj x 26,579,854 ops/sec ±1.57% (93 runs sampled)
AJV Serialize obj x 14,695,781 ops/sec ±0.71% (94 runs sampled)
JSON stringify date x 642,000 ops/sec ±1.35% (94 runs sampled)
fast-json-stringify date format x 914,400 ops/sec ±0.63% (94 runs sampled)
compile-json-stringify date format x 651,345 ops/sec ±0.68% (93 runs sampled)

Note: I have a couple of services in the background running, which are taking their portion of memory...

rhighs · 2023-12-30T22:24:29Z

Can you update the doc please? Add the object example here: https://github.com/fastify/fast-json-stringify#nullable-object.

Can you check/update the same behaviour for array. It should return an empty array for the null value. If not I will create another PR later.

Can you run the benchmarks please, as it affects the hot path?

As per the array part it's quite easy as the code is pretty much the same. Previously it would throw an error if null was passed as it's check was done with Array.isArray contrary to objects where no check was done.

array -> [] object -> []

ivan-tymoshenko · 2023-12-30T23:21:05Z

Sorry my bad. I need to explain it better. There is no need to update the benchmarks in the readme. Just post the benchmarks before your change and after, so we can see if there is a performance drop.

rhighs · 2023-12-31T01:12:29Z

Before:

FJS creation x 7,300 ops/sec ±1.48% (91 runs sampled)
CJS creation x 194,723 ops/sec ±0.54% (93 runs sampled)
AJV Serialize creation x 110,981,583 ops/sec ±0.54% (95 runs sampled)
JSON.stringify array x 4,692 ops/sec ±0.43% (95 runs sampled)
fast-json-stringify array default x 10,860 ops/sec ±0.89% (96 runs sampled)
fast-json-stringify array json-stringify x 10,819 ops/sec ±0.99% (96 runs sampled)
compile-json-stringify array x 10,952 ops/sec ±1.01% (91 runs sampled)
AJV Serialize array x 10,760 ops/sec ±0.73% (90 runs sampled)
JSON.stringify large array x 227 ops/sec ±1.01% (88 runs sampled)
fast-json-stringify large array default x 143 ops/sec ±1.81% (80 runs sampled)
fast-json-stringify large array json-stringify x 235 ops/sec ±0.53% (86 runs sampled)
compile-json-stringify large array x 500 ops/sec ±0.93% (88 runs sampled)
AJV Serialize large array x 171 ops/sec ±1.17% (86 runs sampled)
JSON.stringify long string x 12,649 ops/sec ±0.38% (94 runs sampled)
fast-json-stringify long string x 12,687 ops/sec ±0.29% (99 runs sampled)
compile-json-stringify long string x 12,718 ops/sec ±0.19% (99 runs sampled)
AJV Serialize long string x 31,682 ops/sec ±0.55% (96 runs sampled)
JSON.stringify short string x 14,338,528 ops/sec ±0.42% (97 runs sampled)
fast-json-stringify short string x 52,349,008 ops/sec ±0.65% (95 runs sampled)
compile-json-stringify short string x 50,248,246 ops/sec ±0.93% (98 runs sampled)
AJV Serialize short string x 51,674,211 ops/sec ±0.71% (97 runs sampled)
JSON.stringify obj x 2,844,796 ops/sec ±0.33% (98 runs sampled)
fast-json-stringify obj x 15,334,932 ops/sec ±0.44% (97 runs sampled)
compile-json-stringify obj x 28,390,084 ops/sec ±0.54% (96 runs sampled)
AJV Serialize obj x 15,874,877 ops/sec ±0.98% (92 runs sampled)
JSON stringify date x 645,984 ops/sec ±0.28% (96 runs sampled)
fast-json-stringify date format x 888,474 ops/sec ±0.59% (98 runs sampled)
compile-json-stringify date format x 649,691 ops/sec ±0.27% (98 runs sampled)

After:

FJS creation x 7,502 ops/sec ±0.97% (92 runs sampled)
CJS creation x 199,630 ops/sec ±0.45% (94 runs sampled)
AJV Serialize creation x 110,547,521 ops/sec ±0.65% (92 runs sampled)
JSON.stringify array x 4,683 ops/sec ±0.50% (96 runs sampled)
fast-json-stringify array default x 9,999 ops/sec ±0.76% (95 runs sampled)
fast-json-stringify array json-stringify x 10,925 ops/sec ±0.85% (95 runs sampled)
compile-json-stringify array x 10,630 ops/sec ±0.73% (92 runs sampled)
AJV Serialize array x 10,425 ops/sec ±1.33% (92 runs sampled)
JSON.stringify large array x 235 ops/sec ±0.25% (86 runs sampled)
fast-json-stringify large array default x 149 ops/sec ±0.75% (85 runs sampled)
fast-json-stringify large array json-stringify x 233 ops/sec ±0.42% (91 runs sampled)
compile-json-stringify large array x 468 ops/sec ±0.75% (90 runs sampled)
AJV Serialize large array x 174 ops/sec ±0.53% (85 runs sampled)
JSON.stringify long string x 12,488 ops/sec ±0.31% (99 runs sampled)
fast-json-stringify long string x 12,735 ops/sec ±0.43% (95 runs sampled)
compile-json-stringify long string x 12,661 ops/sec ±0.37% (99 runs sampled)
AJV Serialize long string x 31,534 ops/sec ±0.49% (94 runs sampled)
JSON.stringify short string x 14,262,892 ops/sec ±0.31% (94 runs sampled)
fast-json-stringify short string x 52,056,184 ops/sec ±0.56% (94 runs sampled)
compile-json-stringify short string x 49,724,227 ops/sec ±1.23% (97 runs sampled)
AJV Serialize short string x 51,144,398 ops/sec ±0.70% (92 runs sampled)
JSON.stringify obj x 2,824,685 ops/sec ±0.42% (96 runs sampled)
fast-json-stringify obj x 12,678,859 ops/sec ±0.57% (94 runs sampled)
compile-json-stringify obj x 27,943,798 ops/sec ±0.66% (94 runs sampled)
AJV Serialize obj x 16,137,187 ops/sec ±0.66% (96 runs sampled)
JSON stringify date x 649,665 ops/sec ±0.70% (97 runs sampled)
fast-json-stringify date format x 910,714 ops/sec ±0.39% (97 runs sampled)
compile-json-stringify date format x 641,014 ops/sec ±0.81% (96 runs sampled)

mcollina · 2023-12-31T09:45:44Z

it seems we are losing quite a lot of performance :(.

rhighs · 2024-01-03T12:43:58Z

it seems we are losing quite a lot of performance :(.

How's it even possible adding a check + reassignment causes a 3mln ops/sec drop? 🤨

mcollina · 2024-01-04T08:38:24Z

That is a hot path ;), and it's probably allocating a huge amount of objects that needs to be collected thereafter.

Try not allocating, but rather skipping the object rendering altogether.

rhighs · 2024-01-04T15:41:38Z

That is a hot path ;), and it's probably allocating a huge amount of objects that needs to be collected thereafter.

Try not allocating, but rather skipping the object rendering altogether.

Ok this makes a lot of sense! However, the rendering part includes some bits of validation e.g. checking for required fields. Do you think we can ignore this in case of coercion from null to {}?

mcollina · 2024-01-04T15:46:54Z

Yes, it's better than crashing.

This helps avoiding useless allocations. It also causes required fields checks to be skipped entirely as those are done at rendering. see: fastify#670

rhighs · 2024-01-04T16:02:43Z

With new changes applied:

FJS creation x 9,839 ops/sec ±0.50% (95 runs sampled)
CJS creation x 245,423 ops/sec ±0.05% (99 runs sampled)
AJV Serialize creation x 111,028,596 ops/sec ±0.75% (95 runs sampled)
JSON.stringify array x 7,080 ops/sec ±0.39% (98 runs sampled)
fast-json-stringify array default x 10,813 ops/sec ±0.47% (96 runs sampled)
fast-json-stringify array json-stringify x 11,264 ops/sec ±0.59% (93 runs sampled)
compile-json-stringify array x 11,711 ops/sec ±0.65% (93 runs sampled)
AJV Serialize array x 10,902 ops/sec ±0.46% (96 runs sampled)
JSON.stringify large array x 353 ops/sec ±0.04% (95 runs sampled)
fast-json-stringify large array default x 179 ops/sec ±0.73% (83 runs sampled)
fast-json-stringify large array json-stringify x 352 ops/sec ±0.12% (95 runs sampled)
compile-json-stringify large array x 566 ops/sec ±0.31% (90 runs sampled)
AJV Serialize large array x 206 ops/sec ±0.21% (88 runs sampled)
JSON.stringify long string x 15,577 ops/sec ±0.21% (99 runs sampled)
fast-json-stringify long string x 15,584 ops/sec ±0.12% (95 runs sampled)
compile-json-stringify long string x 15,678 ops/sec ±0.06% (97 runs sampled)
AJV Serialize long string x 32,832 ops/sec ±0.75% (98 runs sampled)
JSON.stringify short string x 15,147,251 ops/sec ±1.31% (89 runs sampled)
fast-json-stringify short string x 52,854,210 ops/sec ±0.24% (95 runs sampled)
compile-json-stringify short string x 52,053,267 ops/sec ±0.16% (96 runs sampled)
AJV Serialize short string x 51,895,033 ops/sec ±0.25% (96 runs sampled)
JSON.stringify obj x 4,375,547 ops/sec ±0.21% (98 runs sampled)
fast-json-stringify obj x 15,414,734 ops/sec ±0.70% (95 runs sampled)
compile-json-stringify obj x 29,158,443 ops/sec ±0.10% (99 runs sampled)
AJV Serialize obj x 16,483,246 ops/sec ±0.77% (97 runs sampled)
JSON stringify date x 1,178,900 ops/sec ±0.08% (98 runs sampled)
fast-json-stringify date format x 1,679,058 ops/sec ±0.24% (100 runs sampled)
compile-json-stringify date format x 1,173,961 ops/sec ±0.20% (97 runs sampled)

mcollina

lgtm

mcollina · 2024-01-05T10:00:24Z

@ivan-tymoshenko PTAL

ivan-tymoshenko

Perf looks good now.

index.js

ivan-tymoshenko

See comment above ^^^

This helps avoiding useless allocations. It also causes required fields checks to be skipped entirely as those are done at rendering. see: fastify#670

ivan-tymoshenko

lgtm

throw an error on non-nullable schema receiving null input

14bcc09

ivan-tymoshenko requested changes Dec 27, 2023

View reviewed changes

mcollina requested changes Dec 27, 2023

View reviewed changes

mcollina approved these changes Dec 27, 2023

View reviewed changes

fix: coerce non-nullable schema null object to {}

6d50c95

rhighs commented Dec 30, 2023

View reviewed changes

test/toJSON.test.js Outdated Show resolved Hide resolved

mcollina approved these changes Dec 30, 2023

View reviewed changes

ivan-tymoshenko requested changes Dec 30, 2023

View reviewed changes

index.js Outdated Show resolved Hide resolved

rhighs changed the title ~~fix: throw an error on non-nullable schema receiving null input~~ fix: non-nullable schema receiving null input should coerce to {} Dec 30, 2023

fix: coerce to default type on strict null eq.

d4b9b06

array -> [] object -> []

rhighs changed the title ~~fix: non-nullable schema receiving null input should coerce to {}~~ fix: non-nullable schema receiving null input does not coerce for default types on object and arrays Dec 30, 2023

rhighs force-pushed the handle-null-issue-669 branch from 5458cc3 to d4b9b06 Compare December 30, 2023 23:56

mcollina approved these changes Jan 5, 2024

View reviewed changes

ivan-tymoshenko approved these changes Jan 5, 2024

View reviewed changes

index.js Show resolved Hide resolved

ivan-tymoshenko requested changes Jan 5, 2024

View reviewed changes

fix: coercion should skip rendering altogether

9552bb5

This helps avoiding useless allocations. It also causes required fields checks to be skipped entirely as those are done at rendering. see: fastify#670

rhighs force-pushed the handle-null-issue-669 branch from e49e80c to 9552bb5 Compare January 5, 2024 15:13

ivan-tymoshenko approved these changes Jan 5, 2024

View reviewed changes

mcollina merged commit 721c599 into fastify:master Jan 5, 2024
19 checks passed

rhighs mentioned this pull request Jan 6, 2024

Response schema compiler generated validation code does not check for null values on optional object fields #669

Closed

2 tasks

Eomm mentioned this pull request Jan 29, 2024

chore: sync generated code fastify/fastify#5295

Merged

NEO97online mentioned this pull request Apr 3, 2024

bump fast-json-stringify to v5.10.0 fastify/fastify#5383

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: non-nullable schema receiving null input does not coerce for default types on object and arrays #670

fix: non-nullable schema receiving null input does not coerce for default types on object and arrays #670

rhighs commented Dec 27, 2023

ivan-tymoshenko left a comment

mcollina left a comment

rhighs commented Dec 27, 2023

rhighs commented Dec 27, 2023

ivan-tymoshenko commented Dec 27, 2023

rhighs commented Dec 27, 2023 •

edited

Loading

ivan-tymoshenko commented Dec 27, 2023

ivan-tymoshenko commented Dec 27, 2023 •

edited

Loading

rhighs commented Dec 27, 2023 •

edited

Loading

mcollina left a comment

mcollina commented Dec 27, 2023

ivan-tymoshenko commented Dec 27, 2023

rhighs commented Dec 28, 2023 •

edited

Loading

ivan-tymoshenko commented Dec 28, 2023

mcollina commented Dec 28, 2023

mcollina left a comment

ivan-tymoshenko left a comment

rhighs commented Dec 30, 2023 •

edited

Loading

rhighs commented Dec 30, 2023

ivan-tymoshenko commented Dec 30, 2023

rhighs commented Dec 31, 2023

mcollina commented Dec 31, 2023

rhighs commented Jan 3, 2024

mcollina commented Jan 4, 2024

rhighs commented Jan 4, 2024

mcollina commented Jan 4, 2024

rhighs commented Jan 4, 2024

mcollina left a comment

mcollina commented Jan 5, 2024

ivan-tymoshenko left a comment •

edited

Loading

ivan-tymoshenko left a comment

ivan-tymoshenko left a comment

fix: non-nullable schema receiving null input does not coerce for default types on object and arrays #670

fix: non-nullable schema receiving null input does not coerce for default types on object and arrays #670

Conversation

rhighs commented Dec 27, 2023

Checklist

ivan-tymoshenko left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

rhighs commented Dec 27, 2023

rhighs commented Dec 27, 2023

ivan-tymoshenko commented Dec 27, 2023

rhighs commented Dec 27, 2023 • edited Loading

ivan-tymoshenko commented Dec 27, 2023

ivan-tymoshenko commented Dec 27, 2023 • edited Loading

rhighs commented Dec 27, 2023 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Dec 27, 2023

ivan-tymoshenko commented Dec 27, 2023

rhighs commented Dec 28, 2023 • edited Loading

ivan-tymoshenko commented Dec 28, 2023

mcollina commented Dec 28, 2023

mcollina left a comment

Choose a reason for hiding this comment

ivan-tymoshenko left a comment

Choose a reason for hiding this comment

rhighs commented Dec 30, 2023 • edited Loading

rhighs commented Dec 30, 2023

ivan-tymoshenko commented Dec 30, 2023

rhighs commented Dec 31, 2023

mcollina commented Dec 31, 2023

rhighs commented Jan 3, 2024

mcollina commented Jan 4, 2024

rhighs commented Jan 4, 2024

mcollina commented Jan 4, 2024

rhighs commented Jan 4, 2024

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Jan 5, 2024

ivan-tymoshenko left a comment • edited Loading

Choose a reason for hiding this comment

ivan-tymoshenko left a comment

Choose a reason for hiding this comment

ivan-tymoshenko left a comment

Choose a reason for hiding this comment

rhighs commented Dec 27, 2023 •

edited

Loading

ivan-tymoshenko commented Dec 27, 2023 •

edited

Loading

rhighs commented Dec 27, 2023 •

edited

Loading

rhighs commented Dec 28, 2023 •

edited

Loading

rhighs commented Dec 30, 2023 •

edited

Loading

ivan-tymoshenko left a comment •

edited

Loading