-
-
Notifications
You must be signed in to change notification settings - Fork 200
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
feat: support local reference in schema, support definitions keyword #676
base: master
Are you sure you want to change the base?
Conversation
Feature/def support
swap definitions with properties at defsToComponents transformer
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.
Will it work for definitions in nested schemas?
@ivan-tymoshenko I faced some issues related to relative definitions, and will appreciate if you can take a look Right now when we will try to do something like this(even on master):
We will get the 'Token "properties" does not exist' exception from Swagger.validate. As i understand, the relative definitions is the correct behavior for json-schema(so for json-schema-resolver which is used for ref resolve in this lib), but not for a swagger/openapi one, so we should patch the relative definitions to absolute? Another problem is the definitions patching. We have 2 sources of schema's here, first one is original coming from addHook return value which is resolved by json-schema-resolver, before any swagger/openapi related funcs is called The second one is the any schema that we get from transformDefsToComponents helper. And there are cases when both of this schemas are used together, for example to resolve a path, https://github.com/fastify/fastify-swagger/blob/master/lib/spec/openapi/utils.js#L263 So if we mutate something like definitions in the second schema, this will break this interactions, because the first schema remains original. So we need to consider patching on addHook level, or....? And the last issue that I see currently is about transforming the references, right now its done in transformDefsToComponents we don't know any info except the reference itself in this method, so we can't really determine the difference between type of references(absolute/relative/cross-reference/etc) and can't correctly patch them to the absolute. This becomes a problem mostly due to the json-schema-resolver behavior in our case, where it adds '#definitions/' to any input that we have later in transformDefsToComponents. E.g original Order#/properties/OrderId -> #/definitions/Order/properties/OrderId and so on. |
According to json schema spec if you define subschema with a $id property the root inside this schema chages. {
$id: 'globalsSchema',
properties: {
a: { $ref: '#' }, // points to the 'globalSchema'
b: {
$id: 'localSchema',
type: 'object',
properties: {
c: { $ref: '#' } // points to the local schema
}
}
}
} Swagger doesn't work like that. As I understand when you merge all schemas to the swagger schema you get one big schema with a new root. So |
What we can also try to do is to save each schema to the separate file and use path in $ref instead of schema id. Then it will will resolve all local references correctly. But I don't know how much overhead it will have. |
Got it, ill try to see what i can do here in a few days, i think it will be better to rethink on how this process of schema mutation from json-schema->openapi works altogether, because right now its really difficult to extend it on need. |
@ivan-tymoshenko finally got some time to finish this PR Now local references are being transformed to absolute, and 'definitions' keyword is being merged with 'properties', with precedence to the last one |
I will check it deeply ASAP. But I have a question for now. I think that problem is in the json-schema-resolver. Shouldn't functions like |
@Eomm Can you help us here? |
Both functions are patching behaviour related only to openapi schema standart, not the json-schema one. In the first case, openapi schema don't support the local refs, in the second - no support for 'definitions' keyword |
Now tests should pass, replaceAll is not supported in node 14, so I replaced it |
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.
Blocking it as I'm reviewing it.
I think that problem is in the json-schema-resolver. Shouldn't functions like localSchemaRefToAbs and patchDefinitionsKeywordInSchema be there?
At first read of this PR: yes, I think the same.
@@ -282,3 +282,155 @@ test('uses examples if has property required in body', async (t) => { | |||
t.ok(schema.parameters) | |||
t.same(schema.parameters[0].in, 'query') | |||
}) | |||
|
|||
test('support absolute refs in schema', async (t) => { |
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.
These due tests:
support absolute refs in schema
support relative refs in schema
Are already supported by json-schema-resolver Eomm/json-schema-resolver#8
The openapi
conversion instead ( test named support definitions keyword in schema
) is provided by this module.
So the questions are:
- is this an issue with the conversion from swagger to openapi?
- is this an issue with
fast-json-stringify
? (as it resolves the schemas by its own)
So, right now, I think this fix does not solve the core problem
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.
Indeed, this tests were passing in the json-schema-resolver lib, and lib itself is perfectly fine. However, the core problem is in the difference between json-schema standard, and swagger/openapi one.
First, the 'definitions' keyword. It is supported by json-schema standard, but not supported in openapi/swagger. However, it can be provided by user in schema. Issues related to that problem: #639, #524, #612. Here is my comment on why this occurs - #639 (comment).
To fix that, patchDefinitionsKeywordInSchema
method was introduced. This method is changing any occurrences of 'definitions' keyword to 'properties' one, including refs. It also merges them with precedence to the existing properties.
Second one, the support of local $refs in openapi(and possibly swagger). This is explained in #676 (comment) by Ivan. In short, local $ref like #/definitions/something
in final openapi schema is not supported by openapi itself.
To fix that, we use localSchemaRefToAbs
method, which resolves any local refs to the absolute ones.
support relative refs in schema
and support absolute refs in schema
tests were made for the second issue, first one for the issue itself, and the second one to ensure that we don't break anything that worked before, when introducing the local->abs resolver.
Additional tests that were introduced in test/util.js can give a more clear picture on this I think.
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.
Thanks for the detailed information, I need to catch up on this 👍🏽
I want to explore a bit the openapi repo as it is source of best practices in these cases
schema[key] = schema[key].split('definitions').join('properties') | ||
} else if (schema[key] && typeof schema[key] === 'object') { | ||
if (key === 'definitions') { | ||
schema.properties = { |
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.
Why do you move schema from the definitions
to the properties
? You shouldn't create new properties.
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.
Why do you move schema from the
definitions
to theproperties
? You shouldn't create new properties.
This is a workaround because 'definitions' keyword itself is not allowed(and will trigger error on openapi schema validation), but we still need to support it(#676 (comment))
The only other way I can think of is somehow allowing additional keywords, adding something like 'x-definitions'(or even the 'definitions' keyword itself) and moving data there, but I don't see a straightforward way to implement it.
@Eomm any thoughts on this yet? |
To review this PR, I need ~4h and I need to find such time 😮💨 |
Hi @Eomm, did you find some time, yet? Would be great to get this PR merged. 🚀 |
This is now blocking for 1 year.. this is not good. Hopefully somebody else can review and merge this PR. |
@asc11cat can you refresh this PR? There are quite a few conflicts now. |
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.
Stepping back
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.
Finally I have some time to review this PR. As I understand it still tries to move json schema definitions
under some schema properties
. I don't think it's a clean solution and it probably won't work with a recurcive refs. If swagger/openapi doesn't support the definitions
keyword I think the best solution would be to move all definitions
under #/components/schemas
.
After 6 hours trying to understand what is going on here I have few conclusions:
|
I agree that this plugin needs a rewrite/refactor |
Hello, any news on this case ? Does this PR just dropped because too complex too heavy with the current state of the library ? Or can we expect a little fix to manage it ? Thanks :) EDIT: I'm actually using this PR fork branch, and it seems to be a pretty good workaround, at least for my current project. Could be a short-time fix |
Addressing #675 and #639
Checklist
npm run test
andnpm run benchmark
and the Code of conduct