fix: properly instantiate nested schemas #86
Conversation
|
@mt35-rs I tried this out and while it generally makes sense and seems to work it does break the tests (I need to figure out why those don't run for this PR) and produces a bunch of weird changes for a complex production API of mine too, like: @@ -25214,7 +31876,7 @@
"content": {
"application/json": {
"schema": {
- "$ref": "#/components/schemas/GetPreviewStreamsResponse"
+ "$ref": "#/components/schemas/GetPreviewStreamsResponse2"
}
}
},
@@ -25609,7 +32271,7 @@
"content": {
"application/problem+json": {
"schema": {
- "$ref": "#/components/schemas/ErrorModel"
+ "$ref": "#/components/schemas/ErrorModel2"
}
}
},While seemingly innocent these have the potential to break generated SDKs and maybe other tools, so I'd like to get that fixed before merging. Otherwise this looks great! 👍 |
|
See here for some explanation of what is going on. |
|
That is my hypothesis at least. I haven't confirmed this. |
| } | ||
| // Add the generated schema to the list of schemas associatd | ||
| // with this component. | ||
| c.Schemas[k] = s |
There was a problem hiding this comment.
Found the fix. There is logic in addExistingSchema that wasn't being run, causing a later DeepEqual to fail and generating extra models. With this change the tests pass again for me.
| c.Schemas[k] = s | |
| c.addExistingSchema(s, k, generateSchemaField) |
There was a problem hiding this comment.
@mt35-rs let me know if you want to apply this, otherwise I can open a new PR with the fix.
There was a problem hiding this comment.
Probably better for you to apply this. You know the ins and outs better than I do.
fix: properly instantiate nested schemas, replaces #86
|
Fixed by #100. Note that you now need to opt-in to the new behavior via |
My previous fix for recursion prevented the crash but it wasn't a complete fix. The issue I uncovered is that the OpenAPI specification data was incomplete. You can see this by trying to access
/schemas/SomeSchema.jsonwhereSomeSchemarefers to a Go type that is not a response type but is nested inside a response type. Before this fix, that would result in an error. But just as bad, the OpenAPI spec generated (i.e., the contents of/openapi.json) were incomplete...schema definitions were missing there as well.The reason for this is that my fix for infinite recursion was to simply replace all struct fields with a
$refto the actual schema rather than elaborate the schema in place. As far as I can tell, this works fine. But the reason it is incomplete is that the wayhumaconstructs the OpenAPI components and their schemas is to simply generate a schema for the response type and publish that. It didn't publish the nested schemas. The reason is that previously it didn't need to because they were all inline and so they would get picked up anyway. But now they are not inline, they are simply referenced. But that$refpointer didn't resolve properly because it pointed to a schema definition that was never actually inserted into the OpenAPI specification document.This change fixes that. The fix is pretty simple and it arose from me not understanding how
humatraverses the resources to build the OpenAPI spec. With this fix in place, every time we skip over a nested schema$ref(to avoid recursion) while generate a schema, we make a note of this. This information is then available after the schema is generated. In the case of injecting the response schema into the OpenAPI components, we include a second pass where we also inject any nested schemas if they haven't already been injected.As a result, both the
/schemas/SomeType.jsonURLs resolve to an actual JSON Schema document but also the/openapi.jsonfile is complete (i.e., includes all definitions referenced via$ref).