-
-
Notifications
You must be signed in to change notification settings - Fork 17
fix: assign schemas to host ajv instance #126
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
Conversation
@diogosilva95 The CI is failing, can you add a unit test as well please?
@Fdawgs added the test and fixed lint :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it is a good idea, when encapsulation is exist, you can have same $id
with different content.
Note sure if it is a good idea, when encapsulation is exist, you can have same
$id
with different content.
would you provide an example please? when you add a schema to the ajv instance with an existing id it throws an error
when you add a schema to the ajv instance with an existing id it throws an error
That is the exact problem I mention, fastify
allows the same $id
of schema exists across different encapsulated context.
You can see the below foo
schema shared the same name but different properties.
import fastify from "fastify"; const app = fastify(); app.register(async (instance) => { instance.addSchema({ $id: 'foo', type: 'object', properties: { foo: { type: 'string' } } }) }) app.register(async (instance) => { instance.addSchema({ $id: 'foo', type: 'object', properties: { foo: { type: 'string' }, bar: { type: 'string' } } }) }) await app.ready()
Isn't addSchema
encapsulation aware?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The ajv
instance is encapsulated too.
I would just add another test to cover the cliba feedback 👍🏼
The ajv instance is encapsulated too.
The problem is similar to @fastify/swagger
. The instance of ajv
is global in the root context of registered level.
So, the problem still persist, unless it creates a new instance of ajv
on each new context creation.
@Copilot
Copilot
AI
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.
Comments skipped due to low confidence (2)
index.js:46
- [nitpick] The variable name 'schemas' is clear, but it could be more specific. Consider renaming it to 'registeredSchemas'.
const schemas = Object.values(fastify.getSchemas())
index.js:49
- The condition checks if the schema is already added to AJV, but it does not handle the case where 'schema.$id' is undefined. Consider adding a check for 'schema.$id'.
if (!ajv.getSchema(schema.$id)) {
Uh oh!
There was an error while loading. Please reload this page.
Fixes the issue #113 by adding the schemas to the internal avj instance used by the plugin
Checklist
npm run test
andnpm run benchmark
and the Code of conduct