feat(openapi): add support for callbacks #804
Conversation
|
sorry to disturb @ivan-tymoshenko , is it possible to have this PR merged? |
| function resolveCallbacks (schema, ref) { | ||
| const callbacksContainer = {} | ||
|
|
||
| Object.entries(schema).forEach(([event, eventValue]) => { | ||
| callbacksContainer[event] = {} | ||
|
|
||
| Object.entries(eventValue).forEach(([callbackUrl, callbackUrlValue]) => { | ||
| callbacksContainer[event][callbackUrl] = {} | ||
|
|
||
| Object.entries(callbackUrlValue).forEach(([httpMethod, httpMethodValue]) => { | ||
| callbacksContainer[event][callbackUrl][httpMethod] = {} | ||
|
|
||
| if (httpMethodValue.requestBody) { | ||
| callbacksContainer[event][callbackUrl][httpMethod].requestBody = convertJsonSchemaToOpenapi3(ref.resolve(httpMethodValue.requestBody)) | ||
| } | ||
|
|
||
| // at least one callback response is required | ||
| callbacksContainer[event][callbackUrl][httpMethod].responses = httpMethodValue.responses | ||
| ? convertJsonSchemaToOpenapi3(ref.resolve(httpMethodValue.responses)) | ||
| : { '2XX': { description: 'Default Response' } } | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| return callbacksContainer | ||
| } | ||
|
|
There was a problem hiding this comment.
My brain hurts readin gthis. Why are there so many Object.entries and forEach calls? That could have negative performance impact.
So basically you are traversing the provided schema. Cant we do it more elegant? cant we use normal for loops?
If I check your example than you make all this fuss, to only change the schema value?
There was a problem hiding this comment.
Seems like the PR is dead since this comment. Can you suggest your version? Otherwise, any way to move on so we can have callbacks support?
There was a problem hiding this comment.
@ThomasFlorelli
I have nothing prepared. But you can propose a new PR. We have a no-cookie-licking policy.
I need some easy understandable code to be able to approve it.
There was a problem hiding this comment.
Here is a flatter version: #829. There was indeed no need to loop over callback urls and http methods since we only expect one of these per event based on Swagger docs.
I'm not sure what your conventions are so let me know if some things feel wrong.
I've added some checks to avoid throwing errors when callback schemas are badly formatted. That's to avoid making this PR a breaking change for people currently using fastify-swagger with callbacks in their schemas that might be wrong.
If that's not what you expect, I can remove these checks.
There was a problem hiding this comment.
Hey, jumping back in here, I lost track of this after the initial PR and things got busy. I suppose I could have used:
for (const [event, eventValue] of Object.entries(schema)) {
instead, but that doesn't feel materially different. To be honest, I prefer the nested loops to keeping track of all the various assignments and continues in the alternate PR, but to each their own.
More importantly, it's not even true that we only have a single callback URL and HTTP method. The link provided has an example of multiple URLs for a single callback event https://swagger.io/docs/specification/v3_0/callbacks/#multiple-callbacks
Putting multiple verbs into a single event/url is also valid:
"{$request.body#/anotherUrl}": # The callback URL,
# Refers to the passed URL
post:
requestBody: # Contents of the callback message
required: true
content:
application/json:
schema:
type: object
properties:
message:
type: string
example: Some event happened
required:
- message
responses: # Expected responses to the callback message
"200":
description: Your server returns this code if it accepts the callback
put:
requestBody: # Contents of the callback message
required: true
content:
application/json:
schema:
type: object
properties:
message:
type: string
example: Some event happened
required:
- message
responses: # Expected responses to the callback message
"200":
description: Your server returns this code if it accepts the callback
I understand the nested loops can be tough to parse, but I tried to space things out and use reasonable naming conventions to keep this flexible for any use case.
There was a problem hiding this comment.
My bad on reading the doc wrong 🙏 With the PR that got merged we will ignore all urls and methods after the first one.
The main issue I see with the current PR is the risk to create badly formatted objects by partially filling them before checking for validity. I'm not sure what the consequences of it would be.
I reused the test cases you provided to make sure to support every scenario, plus some checks to cover above-mentioned risks.
The positive part is we made a step forward. It's not that big of a leap to add support for several callback urls & methods, thought I don't currently see many ways to have this without nested loops (which I'm fine with honestly, I just wanted things to move forward a tad faster).
Not sure what's the emergency on this is but I'd be happy to find some time in the coming days to add what's missing along with unsubscribe which is not yet supported.
There was a problem hiding this comment.
Yeah, the extra checks to ensure properly formatted callbacks are definitely a great addition.
I struggled with the decision to have that many nested loops, but the spec is so flexible that it is tough to break it out in other ways. I suppose we could have several sub-functions that are called in a chain, but jumping around in code seems harder to track than having the loops all together. Additional comments before each new loop would make it easier to parse and walk through what each step is doing.
Add support for callbacks in OpenAPI v3 solving #727
Checklist
npm run testandnpm run benchmarkand the Code of conduct