fix: use ajv for schema ref resolving #462
Conversation
e08cc63 to
ac7fd4d
Compare
|
Before merging, it is needed to resolve fastify's failed tests. #454 (comment) |
|
Could you send a PR to Fastify to update this? Feel free to use a git-dependency to verify. |
|
@mcollina @climba03003 Am I right that Fastify allows adding global (external) schemas for serialization only if they have $id property? And if the schema does have $id property it always equals the schema's key? If so, can we consider that all external schemas received from Fastify have the $id property and it always equals key? |
|
I think the right person to ask is @Eomm. |
|
Yes, it would be better if you ask Eomm. Since, the function you pointing to is shared between |
@climba03003 If so, what behavior would you expect for this feature? |
There was a problem hiding this comment.
I think this PR simply a lot the code and fixes some tests that were wrong.
We must be aware that we are relying more on AJV to store&get the schema. For this reason, I think we may hit a performance drop at startup because ajv clones all the schemas added via ajv.addSchema.
Am I right that Fastify allows adding global (external) schemas for serialization only if they have $id property?
Yes, it is a good assumption, but I saw the JSON key fallback that I think is reasonable.
Before merging a draft PR against fastify should be linked to be sure all cases are covered. I expect some fixes on the fastify's tests 😄
|
I'm a bit confused. Should we ship this as minor or major? |
|
This will break some cases that don't follow the json schema draft 7 standard. IMHO these cases shouldn't be popular, but I don't have any statistics so IDK. This PR changes two wrong tests behavior. All Fastify tests will pass after merging this fastify/fast-json-stringify-compiler#14. Although @climba03003 makes sense here #464 (comment), that some people can have some wrong use cases that are not covered with our tests. On the other hand, it fixes so many basic cases, so I don't really want to wait a year for Fastify@5. Maybe it makes sense to release it as @climba03003 suggested #464 (comment), and if it turns out that many wrong cases do not work, revert the changes or patch some popular wrong cases until the next major release. @Eomm wdyt? |
|
I think this PR deserves to be shipped with fastify@4, but I think it needs some more checks about perf impact. Could @ivan-tymoshenko share the results? I will try the bench script too. |
|
Unfortunately, it has a significant performance drop on cold start. I will think if something can be done about it. |
|
I found the problem. I disabled ajv schema validation. Cold start benchmark results: Main branch
Feature branch
Benchmark: https://gist.github.com/ivan-tymoshenko/18abb40f5b4bdf27fb520eeb10b735f1 |
|
from my test, this PR does not impact this repo's benchmark script. Maybe it worth adding a bench test with some $refs. WDYT? |
|
@Eomm try this one. https://gist.github.com/ivan-tymoshenko/18abb40f5b4bdf27fb520eeb10b735f1
|
This PR shouldn't affect runtime. All changes are about stringify func compilation. |
|
I run this: https://github.com/fastify/fast-json-stringify/tree/master/benchmark Your test results: Looking if we could set something else to reduce this little drop |
Reopen #454
Fix #452 #417 #350
The idea behind this solution is to use ajv's addSchema and getSchema methods to resolve json schema references. To do this, as the serializer is built, it keeps track of and updates a reference to the current location. The reference consists of schema id + json pointer. When it needs to validate subschema (if/else, oneof, anyof), it uses the reference.
There are two tests that use an incorrect format of json schema id/ref. They will not work after this fix.
It's an external schema that uses a schema key (base URI) that starts with #. As it is considered a schema key if it can start with #. # should be used only as a local anchor inside the schema.
fast-json-stringify/test/ref.test.js
Line 981 in 310325b
It uses a reference combined with the other two references.
fast-json-stringify/test/ref.test.js
Line 870 in 310325b
And as we have a reference for the current location in the schema we can use it as a comment inside anonymous functions instead of building a custom version of json pointer (locationPath).