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
fix: remove getSchema from schema creation #4023
base: main
Are you sure you want to change the base?
fix: remove getSchema from schema creation #4023
Conversation
af10de3
to
ee5ae7b
Compare
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.
The test is valid, you cannot avoid this usage from public and we should validate the stored schema can be used directly through getSchema
.
ee5ae7b
to
a9b6fbf
Compare
@climba03003 The problem is that when we use getSchema we create a duplicate of one or more schemas that are stored in one global context. It creates inconsistency. |
I means the test ensure the
Yes, it may not be a good usage. But it is a valid usage. |
That's why this PR is drarf. I don't tell that avoiding this behavior is the only solution. But there is a problem. Do you have any suggestions on how this can be fixed? |
I wanted to show the simple test with inconsistency and found out even the basic reference is not working. test('inner reference', t => {
t.plan(1)
const fastify = Fastify()
fastify.get('/', {
handler: () => {},
schema: {
response: {
200: {
type: 'object',
properties: {
p1: { $id: 'p1', type: 'string' },
p2: { $ref: 'p1' }
}
}
}
}
})
fastify.ready(err => t.error(err))
}) |
If it is inner reference of the schema, didn't it should be test('inner reference', t => {
t.plan(1)
const fastify = Fastify()
fastify.get('/', {
handler: () => {},
schema: {
response: {
200: {
type: 'object',
properties: {
p1: { $id: 'p1', type: 'string' },
p2: { $ref: '#/properties/p1' }
}
}
}
}
})
fastify.ready(err => t.error(err))
}) Specify the {
$id: '<hidden>', // here is the hidden uri-base, if it is the uri resources. It becomes http://example.com/schemas/xxx
type: 'object',
properties: {
p1: { $id: 'p1', type: 'string' }, // id here is relative uri to the schema root $id
p2: { $ref: 'p1' } // it is actually referencing to nothing
}
} |
I have double check with how I also see the point of fastify/fast-json-stringify#462. import FJS from "fast-json-stringify";
const stringify = FJS({
$id: "https://example.com/schemas/base",
type: "object",
properties: {
p1: { $id: "p1", type: "string" },
p2: { $ref: "p1" },
},
}); |
It's not just how ajv performs it, it's a part of the json schema draft 7 standard.
I didn't get about the replacement, but inside the schema, you should be able to define as many schemas as you want. |
I have made some investigation. Ajv checks for nested schemas with the same $ids. If these schemas have the same structure, it doesn't throw an error and processed them as one schema. The only case when it's not working is a top-level $ids, which we can handle. An important fact is that it is not ok. Using fastify.getSchema creates an incorrect schema and then it can be fixed by |
Can you clarify what behavior exactly? |
I totally agree with this statement and I understand the case. The scope is: the user wants the same schema it was put in using So, I wonder if marking the object returned by the I think this feature has an impact on fjs (as it should ignore these schemas). I will try to think other solutions |
@Eomm I thought about this solution. One note. Users can use getSchema to insert nested subschema. Example: fastify/test/schema-feature.test.js Line 618 in 00f4088
So, we will have to make a recursive check. |
Mmmm you are right but I don't like it: we should check how many recursive chek we are running to minimise these operations because I'm losing the count personally 😅 What if we check that the object pointer (using schema === schema) is the same? |
The getSchema method returns the original schema with its $id and all nested schemas with their $ids. In tests, the purpose of using the getSchema method is to create a reference to an already added schema, not to add a new one. The correct behavior is well described in the description of the addSchema method.
I marked this PR as a draft because I guess it can be a popular use case, so maybe there can be a more elegant solution. I would like to hear any comments about it.