Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: create json schema ref resolver #507

Merged
merged 2 commits into from Aug 29, 2022

Conversation

ivan-tymoshenko
Copy link
Member

Fix #506

I want to return a custom JSON schema reference resolver with some changes. It will only resolve JSON schema references only at serializer compilation time. All refs used for validation still will be resolved by Ajv. It's a proof of concept. I need to read the JSON schema doc carefully before merging it.

@ivan-tymoshenko ivan-tymoshenko force-pushed the create-json-schema-reference-resolver branch from aef1674 to 17bd2d8 Compare August 21, 2022 17:39
@ivan-tymoshenko ivan-tymoshenko marked this pull request as ready for review August 21, 2022 17:39
@ivan-tymoshenko
Copy link
Member Author

@adrai Can you confirm that these changes fix your case?

@adrai
Copy link
Member

adrai commented Aug 21, 2022

Yes, seems much better, but still slower than before.

v4.1.0 156ms
v4.2.0 687ms
v4.2.0 with this fix 241ms

@adrai
Copy link
Member

adrai commented Aug 21, 2022

@ivan-tymoshenko
Copy link
Member Author

Yes, seems much better, but still slower than before.

Another optimization that I want to do is to add external schemas to the RefResolver only once instead of adding them for each route. But it would be a part of another PR. I want to move the fast-json-stringify-compiler to the fast-json-stringify.

I guess the problem is not in refResolver.getSchema. It should work as fast as refFinder worked. @adrai Can you check that, please?

@adrai
Copy link
Member

adrai commented Aug 21, 2022

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

@@ -114,8 +110,9 @@ function build (schema, options) {
schemaKey = key + externalSchema.$id // relative URI
}

if (ajvInstance.getSchema(schemaKey) === undefined) {
if (refResolver.getSchema(schemaKey) === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

I would try to check the ajv's readonly properties.
This does not trigger the compiling and we could avoid the deep/equal logic

(x.schemas[schemaKey] || x.refs[schemaKey]) === undefined
const Ajv = require('ajv')

const x = new Ajv()

const externalSchema = {
  $id: 'root',
  definitions: {
    subschema: {
      $id: 'subschema',
      definitions: {
        anchorSchema: {
          $id: '#anchor',
          type: 'string'
        }
      }
    }
  }
}

x.addSchema(externalSchema)

{
  const id = 'subschema'
  console.log(x.schemas[id] || x.refs[id])
}

{
  const id = 'root'
  console.log(x.schemas[id] || x.refs[id])
}

{
  const id = 'subschema#anchor'
  console.log(x.schemas[id] || x.refs[id])
}

Copy link
Member Author

Choose a reason for hiding this comment

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

The only concern I have is that refs and schemas is not a part of public API and can be changed without major release.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it public?
https://github.com/ajv-validator/ajv/blob/master/lib/core.ts#L281

They would have declared as private instead

Comment on lines +117 to +120
if (
ajvInstance.refs[schemaKey] === undefined &&
ajvInstance.schemas[schemaKey] === undefined
) {
Copy link
Member

Choose a reason for hiding this comment

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

I meant that this check should be enough to replace the new RefResolver holder as both (ajv and refresolver) are storing the same values

I will try it locally

Copy link
Member Author

Choose a reason for hiding this comment

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

It won't work for nested schemas like 'subschema' and 'subschema#anchor'. I will work if we want to know if Ajv has a schema, but we can't get it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see. Ajv resolves references in that cases.

Another reason why I want to have a separate abstraction for this is because Ajv does two things: validates schemas at runtime and resolves references at compile time. It's not good. So in #504 I will have to create two Ajv instances (for schema validation and reference resolving), because there would be two different syntaxes.

And since we own this code it's easy to optimize it. For example in case if we want to reuse external schemas.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Eomm I rebased #504 to this branch. You can see there that RefResolver/Validator have different responsibilities and different reasons to be changed.

@mcollina
Copy link
Member

@Eomm is this good to land?

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

Lgtm

@mcollina mcollina merged commit b0bb1a6 into master Aug 29, 2022
@Uzlopak Uzlopak deleted the create-json-schema-reference-resolver branch August 29, 2022 16:20
@adrai
Copy link
Member

adrai commented Aug 30, 2022

Will this land in the next fastify version?

@mcollina
Copy link
Member

Ideally yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

possible performance impact when updating from v4.2.0 to v5.0.0
4 participants