Skip to content

Conversation

@diogosilva95
Copy link

@diogosilva95 diogosilva95 commented Nov 3, 2024

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

Checklist

@Fdawgs
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
@diogosilva95
Copy link
Author

diogosilva95 commented Nov 3, 2024

@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

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.

@diogosilva95
Copy link
Author

diogosilva95 commented Nov 5, 2024

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

@climba03003
Copy link
Member

climba03003 commented Nov 5, 2024

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

@mcollina
Copy link
Member

Isn't addSchema encapsulation aware?

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 👍🏼

@climba03003
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.

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Does not support $ref properties

5 participants