Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(openapi): add support for callbacks #804

Closed
wants to merge 1 commit into from
Closed

feat(openapi): add support for callbacks #804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions examples/dynamic-openapi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,59 @@ fastify.register(async function (fastify) {
}
}
}, (req, reply) => { reply.send({ hello: `Hello ${req.body.hello}` }) })

fastify.post('/subscribe', {
schema: {
description: 'subscribe for webhooks',
summary: 'webhook example',
security: [],
response: {
201: {
description: 'Succesful response'
}
},
body: {
type: 'object',
properties: {
callbackUrl: {
type: 'string',
examples: ['https://example.com']
}
}
},
callbacks: {
myEvent: {
'{$request.body#/callbackUrl}': {
post: {
requestBody: {
content: {
'application/json': {
schema: {
type: 'object',
properties: {
message: {
type: 'string',
example: 'Some event happened'
}
},
required: [
'message'
]
}
}
}
},
responses: {
200: {
description: 'Success'
}
}
}
}
}
}
}
})
})

fastify.listen({ port: 3000 }, err => {
Expand Down
28 changes: 28 additions & 0 deletions lib/spec/openapi/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,33 @@ function resolveResponse (fastifyResponseJson, produces, ref) {
return responsesContainer
}

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
}

Comment on lines +375 to +401
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ThomasFlorelli ThomasFlorelli Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@R-Campbell R-Campbell Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ThomasFlorelli ThomasFlorelli Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@R-Campbell R-Campbell Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

function prepareOpenapiMethod (schema, ref, openapiObject, url) {
const openapiMethod = {}
const parameters = []
Expand Down Expand Up @@ -416,6 +443,7 @@ function prepareOpenapiMethod (schema, ref, openapiObject, url) {
if (schema.deprecated) openapiMethod.deprecated = schema.deprecated
if (schema.security) openapiMethod.security = schema.security
if (schema.servers) openapiMethod.servers = schema.servers
if (schema.callbacks) openapiMethod.callbacks = resolveCallbacks(schema.callbacks, ref)
for (const key of Object.keys(schema)) {
if (key.startsWith('x-')) {
openapiMethod[key] = schema[key]
Expand Down
53 changes: 53 additions & 0 deletions test/spec/openapi/refs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,3 +355,56 @@ test('renders $ref schema with additional keywords', async (t) => {
t.match(res.statusCode, 400)
t.match(openapiObject.paths['/url1'].get.parameters[0].schema, cookie)
})

test('support $ref in callbacks', async (t) => {
const fastify = Fastify()

await fastify.register(fastifySwagger, openapiOption)
fastify.register(async (instance) => {
instance.addSchema({ $id: 'Subscription', type: 'object', properties: { callbackUrl: { type: 'string', examples: ['https://example.com'] } } })
instance.addSchema({ $id: 'Event', type: 'object', properties: { message: { type: 'string', examples: ['Some event happened'] } } })
instance.post('/subscribe', {
schema: {
body: {
$ref: 'Subscription#'
},
response: {
200: {
$ref: 'Subscription#'
}
},
callbacks: {
myEvent: {
'{$request.body#/callbackUrl}': {
post: {
requestBody: {
content: {
'application/json': {
schema: { $ref: 'Event#' }
}
}
},
responses: {
200: {
description: 'Success'
}
}
}
}
}
}
}
}, () => {})
})

await fastify.ready()

const openapiObject = fastify.swagger()

t.equal(typeof openapiObject, 'object')
t.match(Object.keys(openapiObject.components.schemas), ['Subscription', 'Event'])
t.equal(openapiObject.components.schemas.Subscription.properties.callbackUrl.example, 'https://example.com')
t.equal(openapiObject.components.schemas.Event.properties.message.example, 'Some event happened')

await Swagger.validate(openapiObject)
})
178 changes: 178 additions & 0 deletions test/spec/openapi/schema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1025,3 +1025,181 @@ test('avoid overwriting params when schema.params is provided', async t => {
}
})
})

test('support callbacks', async () => {
test('includes callbacks in openapiObject', async t => {
const fastify = Fastify()

await fastify.register(fastifySwagger, openapiOption)
fastify.register(async (instance) => {
instance.post('/subscribe', {
schema: {
body: {
$id: 'Subscription',
type: 'object',
properties: {
callbackUrl: {
type: 'string',
examples: ['https://example.com']
}
}
},
response: {
200: {
$id: 'Subscription',
type: 'object',
properties: {
callbackUrl: {
type: 'string',
examples: ['https://example.com']
}
}
}
},
callbacks: {
myEvent: {
'{$request.body#/callbackUrl}': {
post: {
requestBody: {
content: {
'application/json': {
schema: {
type: 'object',
properties: {
message: {
type: 'string',
example: 'Some event happened'
}
},
required: [
'message'
]
}
}
}
},
responses: {
200: {
description: 'Success'
}
}
}
}
},
myOtherEvent: {
'{$request.body#/callbackUrl}': {
post: {
responses: {
200: {
description: 'Success'
},
500: {
description: 'Error'
}
}
}
}
}
}
}
}, () => {})
})

await fastify.ready()

const openapiObject = fastify.swagger()

console.log(openapiObject)

t.equal(typeof openapiObject, 'object')
t.equal(typeof openapiObject.paths['/subscribe'].post.callbacks, 'object')

const definedPath = openapiObject.paths['/subscribe'].post.callbacks

t.strictSame(definedPath.myEvent['{$request.body#/callbackUrl}'].post.requestBody.content['application/json'].schema.properties, {
message: {
type: 'string',
example: 'Some event happened'
}
})

t.same(definedPath.myOtherEvent['{$request.body#/callbackUrl}'].post.requestBody, null)

await Swagger.validate(openapiObject)
})

test('sets callback response default if not included', async t => {
const fastify = Fastify()

await fastify.register(fastifySwagger, openapiOption)
fastify.register(async (instance) => {
instance.post('/subscribe', {
schema: {
body: {
$id: 'Subscription',
type: 'object',
properties: {
callbackUrl: {
type: 'string',
examples: ['https://example.com']
}
}
},
response: {
200: {
$id: 'Subscription',
type: 'object',
properties: {
callbackUrl: {
type: 'string',
examples: ['https://example.com']
}
}
}
},
callbacks: {
myEvent: {
'{$request.body#/callbackUrl}': {
post: {
requestBody: {
content: {
'application/json': {
schema: {
type: 'object',
properties: {
message: {
type: 'string',
example: 'Some event happened'
}
},
required: [
'message'
]
}
}
}
}
}
}
}
}
}
}, () => {})
})

await fastify.ready()

const openapiObject = fastify.swagger()

t.equal(typeof openapiObject, 'object')
t.equal(typeof openapiObject.paths['/subscribe'].post.callbacks, 'object')

const definedPath = openapiObject.paths['/subscribe'].post

console.log(definedPath.callbacks.myEvent['{$request.body#/callbackUrl}'].post.responses)

t.equal(definedPath.callbacks.myEvent['{$request.body#/callbackUrl}'].post.responses['2XX'].description, 'Default Response')

await Swagger.validate(openapiObject)
})
})