Skip to content

refactor: create separate schema for Ajv validation#504

Merged
mcollina merged 2 commits intomasterfrom
create-separate-validation-schema
Aug 29, 2022
Merged

refactor: create separate schema for Ajv validation#504
mcollina merged 2 commits intomasterfrom
create-separate-validation-schema

Conversation

@ivan-tymoshenko
Copy link
Member

@ivan-tymoshenko ivan-tymoshenko commented Aug 14, 2022 •
edited
Loading

How it's working now:
It takes a user-defined schema, adds necessary adjustments for Ajv (now it's only for date type), and then works with modified schema.

How it will work:
It will clone a user-defined schema, modify it and pass it to the Ajv. Serialization will work with the original schema. The only requirement we must follow is that the schema must not change its structure during the modification. In other words, the JSON pointers must match in two schemas, but the schemas themselves (type, format, keywords, ... etc.) may differ.

Motivation:
It's too complicated to maintain one syntax for serialization and validation rules. Our code depends on Ajv validation syntax.

Depends on #507

@ivan-tymoshenko ivan-tymoshenko marked this pull request as draft August 14, 2022 12:54
@ivan-tymoshenko ivan-tymoshenko marked this pull request as ready for review August 14, 2022 13:22
@Fdawgs Fdawgs added the benchmark Label to run benchmark against PR and main branch label Aug 14, 2022
@github-actions
Copy link

github-actions bot commented Aug 14, 2022

Node: 14
PR:

FJS creation x 3,026 ops/sec ±1.52% (88 runs sampled)
CJS creation x 128,979 ops/sec ±0.58% (93 runs sampled)
AJV Serialize creation x 24,197,870 ops/sec ±0.20% (95 runs sampled)
JSON.stringify array x 3,606 ops/sec ±0.12% (97 runs sampled)
fast-json-stringify array default x 5,415 ops/sec ±0.32% (98 runs sampled)
fast-json-stringify array json-stringify x 5,316 ops/sec ±0.24% (98 runs sampled)
compile-json-stringify array x 5,244 ops/sec ±0.70% (91 runs sampled)
AJV Serialize array x 5,402 ops/sec ±0.20% (93 runs sampled)
JSON.stringify large array x 178 ops/sec ±0.32% (82 runs sampled)
fast-json-stringify large array default x 82.42 ops/sec ±0.22% (49 runs sampled)
fast-json-stringify large array json-stringify x 178 ops/sec ±0.11% (83 runs sampled)
compile-json-stringify large array x 236 ops/sec ±0.19% (91 runs sampled)
AJV Serialize large array x 84.48 ops/sec ±0.17% (72 runs sampled)
JSON.stringify long string x 11,254 ops/sec ±0.35% (98 runs sampled)
fast-json-stringify long string x 11,259 ops/sec ±0.09% (98 runs sampled)
compile-json-stringify long string x 11,254 ops/sec ±0.10% (98 runs sampled)
AJV Serialize long string x 14,703 ops/sec ±0.47% (90 runs sampled)
JSON.stringify short string x 8,891,428 ops/sec ±0.14% (95 runs sampled)
fast-json-stringify short string x 24,701,216 ops/sec ±0.09% (99 runs sampled)
compile-json-stringify short string x 24,216,046 ops/sec ±0.40% (96 runs sampled)
AJV Serialize short string x 22,894,010 ops/sec ±0.21% (97 runs sampled)
JSON.stringify obj x 2,170,861 ops/sec ±0.51% (89 runs sampled)
fast-json-stringify obj x 5,533,620 ops/sec ±0.12% (94 runs sampled)
compile-json-stringify obj x 12,122,110 ops/sec ±0.11% (94 runs sampled)
AJV Serialize obj x 6,715,022 ops/sec ±0.64% (94 runs sampled)
JSON stringify date x 717,098 ops/sec ±0.12% (91 runs sampled)
fast-json-stringify date format x 1,536,148 ops/sec ±0.24% (97 runs sampled)
compile-json-stringify date format x 718,086 ops/sec ±0.45% (94 runs sampled)

MAIN:

Node: 16
PR:

FJS creation x 2,573 ops/sec ±2.32% (84 runs sampled)
CJS creation x 110,393 ops/sec ±0.30% (92 runs sampled)
AJV Serialize creation x 19,284,303 ops/sec ±0.33% (92 runs sampled)
JSON.stringify array x 2,929 ops/sec ±0.85% (94 runs sampled)
fast-json-stringify array default x 4,313 ops/sec ±0.42% (94 runs sampled)
fast-json-stringify array json-stringify x 4,235 ops/sec ±0.26% (94 runs sampled)
compile-json-stringify array x 4,375 ops/sec ±1.00% (89 runs sampled)
AJV Serialize array x 4,524 ops/sec ±0.28% (92 runs sampled)
JSON.stringify large array x 148 ops/sec ±0.29% (83 runs sampled)
fast-json-stringify large array default x 61.74 ops/sec ±0.89% (63 runs sampled)
fast-json-stringify large array json-stringify x 147 ops/sec ±0.47% (82 runs sampled)
compile-json-stringify large array x 200 ops/sec ±0.40% (84 runs sampled)
AJV Serialize large array x 64.61 ops/sec ±0.81% (66 runs sampled)
JSON.stringify long string x 9,760 ops/sec ±0.25% (94 runs sampled)
fast-json-stringify long string x 9,731 ops/sec ±0.69% (94 runs sampled)
compile-json-stringify long string x 9,741 ops/sec ±0.36% (93 runs sampled)
AJV Serialize long string x 10,633 ops/sec ±0.25% (94 runs sampled)
JSON.stringify short string x 7,155,146 ops/sec ±0.79% (94 runs sampled)
fast-json-stringify short string x 21,694,278 ops/sec ±0.31% (93 runs sampled)
compile-json-stringify short string x 19,624,640 ops/sec ±0.32% (92 runs sampled)
AJV Serialize short string x 18,391,672 ops/sec ±0.34% (94 runs sampled)
JSON.stringify obj x 1,797,196 ops/sec ±0.20% (95 runs sampled)
fast-json-stringify obj x 4,506,962 ops/sec ±0.25% (96 runs sampled)
compile-json-stringify obj x 10,106,748 ops/sec ±0.84% (95 runs sampled)
AJV Serialize obj x 5,531,539 ops/sec ±0.22% (93 runs sampled)
JSON stringify date x 621,792 ops/sec ±0.28% (95 runs sampled)
fast-json-stringify date format x 1,215,679 ops/sec ±0.77% (93 runs sampled)
compile-json-stringify date format x 626,112 ops/sec ±0.32% (94 runs sampled)

MAIN:

Node: 18
PR:

FJS creation x 2,631 ops/sec ±1.26% (88 runs sampled)
CJS creation x 128,782 ops/sec ±0.54% (92 runs sampled)
AJV Serialize creation x 21,786,096 ops/sec ±0.53% (92 runs sampled)
JSON.stringify array x 3,568 ops/sec ±0.09% (99 runs sampled)
fast-json-stringify array default x 4,842 ops/sec ±0.15% (98 runs sampled)
fast-json-stringify array json-stringify x 4,766 ops/sec ±0.80% (95 runs sampled)
compile-json-stringify array x 4,926 ops/sec ±0.25% (96 runs sampled)
AJV Serialize array x 5,002 ops/sec ±0.16% (96 runs sampled)
JSON.stringify large array x 173 ops/sec ±0.53% (88 runs sampled)
fast-json-stringify large array default x 78.04 ops/sec ±0.51% (43 runs sampled)
fast-json-stringify large array json-stringify x 173 ops/sec ±0.13% (88 runs sampled)
compile-json-stringify large array x 227 ops/sec ±0.12% (82 runs sampled)
AJV Serialize large array x 80.98 ops/sec ±0.53% (69 runs sampled)
JSON.stringify long string x 11,291 ops/sec ±0.08% (98 runs sampled)
fast-json-stringify long string x 11,280 ops/sec ±0.11% (96 runs sampled)
compile-json-stringify long string x 11,262 ops/sec ±0.08% (96 runs sampled)
AJV Serialize long string x 14,849 ops/sec ±0.09% (95 runs sampled)
JSON.stringify short string x 8,829,040 ops/sec ±0.09% (93 runs sampled)
fast-json-stringify short string x 25,291,792 ops/sec ±0.15% (96 runs sampled)
compile-json-stringify short string x 21,687,192 ops/sec ±0.24% (94 runs sampled)
AJV Serialize short string x 20,091,776 ops/sec ±1.93% (89 runs sampled)
JSON.stringify obj x 2,138,563 ops/sec ±0.46% (97 runs sampled)
fast-json-stringify obj x 5,177,527 ops/sec ±0.08% (96 runs sampled)
compile-json-stringify obj x 11,200,558 ops/sec ±0.15% (96 runs sampled)
AJV Serialize obj x 6,654,567 ops/sec ±0.54% (95 runs sampled)
JSON stringify date x 736,735 ops/sec ±0.10% (99 runs sampled)
fast-json-stringify date format x 1,605,446 ops/sec ±0.09% (94 runs sampled)
compile-json-stringify date format x 730,329 ops/sec ±0.48% (97 runs sampled)

MAIN:

@github-actions github-actions bot removed the benchmark Label to run benchmark against PR and main branch label Aug 14, 2022
@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 15, 2022

It makes sense to me.

mcollina
mcollina approved these changes Aug 15, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ivan-tymoshenko ivan-tymoshenko marked this pull request as draft August 16, 2022 07:15
@ivan-tymoshenko ivan-tymoshenko mentioned this pull request Aug 23, 2022
Merged
@ivan-tymoshenko ivan-tymoshenko force-pushed the create-separate-validation-schema branch 2 times, most recently from 787dd9a to b2d55a0 Compare August 24, 2022 18:46
@ivan-tymoshenko ivan-tymoshenko force-pushed the create-separate-validation-schema branch from b2d55a0 to e6fcbe9 Compare August 29, 2022 16:29
@ivan-tymoshenko
Copy link
Member Author

ivan-tymoshenko commented Aug 29, 2022 •
edited
Loading

There are two updates.

  1. I moved all Ajv logic to the Validator class. The Validator is a class Adapter. It has two public methods addSchema and validate. It should validate schema according to our rules. It encapsulates all Ajv validation logic in one place.
  2. There is a small change in API. If you call the build method in the debug mode, one of the return parameters would be the Validator instance instead of the Ajv instance.

@ivan-tymoshenko ivan-tymoshenko marked this pull request as ready for review August 29, 2022 16:51
climba03003
climba03003 approved these changes Aug 29, 2022
View reviewed changes
Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code itself look good to me and the changes on debug output is breaking.

@ivan-tymoshenko
You are the one focusing on this package. Can I ask how many breaking changes are you expected in the near future? I am considering should we group a bunch of them before another major.

ref-resolver.js
}
this.insertSchemaBySchemaId(schema, schemaId)
this.insertSchemaSubschemas(schema, schemaId)
if (this.getSchema(schemaId) === undefined) {
Copy link
Member

@climba03003 climba03003 Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this changes itself self contained?
Do we need a test case for it?

Copy link
Member Author

@ivan-tymoshenko ivan-tymoshenko Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't add this check. I just moved it inside the class.

fast-json-stringify/index.js

Line 113 in b0bb1a6

if (refResolver.getSchema(schemaKey) === undefined) {

@ivan-tymoshenko
Copy link
Member Author

ivan-tymoshenko commented Aug 29, 2022

@climba03003 I can leave the Ajv instance and remove it in the next major release.

@mcollina mcollina merged commit 8997bf1 into master Aug 29, 2022
@Uzlopak Uzlopak deleted the create-separate-validation-schema branch August 29, 2022 19:51
@ivan-tymoshenko ivan-tymoshenko mentioned this pull request Aug 30, 2022
Merged
@Uzlopak Uzlopak mentioned this pull request Aug 31, 2022
Closed
2 tasks
@ivan-tymoshenko ivan-tymoshenko mentioned this pull request Aug 31, 2022
Closed
2 tasks
@climba03003 climba03003 mentioned this pull request Sep 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@climba03003 climba03003 climba03003 approved these changes

@Eomm Eomm Awaiting requested review from Eomm

@Uzlopak Uzlopak Awaiting requested review from Uzlopak

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ivan-tymoshenko @Uzlopak @mcollina @climba03003 @Fdawgs