Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
diogosilva95 wants to merge 5 commits into fastify:main
base: main
Choose a base branch
Loading
from diogosilva95:master

Conversation

Copy link

@diogosilva95 diogosilva95 commented Nov 3, 2024
edited
Loading

Fixes the issue #113 by adding the schemas to the internal avj instance used by the plugin

Checklist

Copy link
Member

Fdawgs commented Nov 3, 2024

@diogosilva95 The CI is failing, can you add a unit test as well please?

@Fdawgs Fdawgs linked an issue Nov 3, 2024 that may be closed by this pull request
2 tasks
Copy link
Author

diogosilva95 commented Nov 3, 2024
edited
Loading

@Fdawgs added the test and fixed lint :)

@Fdawgs Fdawgs requested a review from a team November 4, 2024 20:03
Copy link
Member

@climba03003 climba03003 left a comment
edited
Loading

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.

Copy link
Author

diogosilva95 commented Nov 5, 2024
edited
Loading

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

Copy link
Member

climba03003 commented Nov 5, 2024
edited
Loading

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()

Copy link
Member

Isn't addSchema encapsulation aware?

Eomm reacted with thumbs up emoji

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

The ajv instance is encapsulated too.
I would just add another test to cover the cliba feedback 👍🏼

Copy link
Member

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.

gurgunday reacted with thumbs up emoji melroy89 reacted with confused emoji Eomm and mcollina reacted with rocket emoji

@Fdawgs Fdawgs requested a review from Copilot November 20, 2024 10:59
Copy link

@Copilot Copilot AI left a 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)) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Eomm Eomm Eomm left review comments

@climba03003 climba03003 climba03003 left review comments

Copilot code review Copilot Copilot left review comments

@Fdawgs Fdawgs Fdawgs approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Does not support $ref properties

AltStyle によって変換されたページ (->オリジナル) /