search

Nexmo / oas_parser

An open source Open API Spec 3 Definition Parser
MIT License
51 stars 16 forks source link

Use x-webhooks if no webhooks are defined #53

Closed lornajane closed 3 years ago

lornajane commented 4 years ago

Bridging the gap between the RC1 "implementer's draft" of OAS 3.1 and the main release, this PR will accept x-webhooks as a top level element formatted as the incoming webhooks will be formatted (bringing it in line with other tools with experimental support such as redoc). It parses it as webhooks, and as such it will be handled by renderers and other tools in the same way.

hummusonrails commented 4 years ago

@fabianrbz I pushed up a fix for my review comment, but since I am now also a contributor, I'd feel more comfortable if another set of eyes reviewed this who didn't also contribute.

mheap commented 3 years ago

We should be cautious here as x-webhooks was previously an OasParser::Callback instance, not an OasParser::Webhook

Merging this would cause the renderer to fail as x-webhooks would be expected in our current, pre-3.1 incarnation and also be parsed as a 3.1 compatible webhook. It is impossible for x-webhooks to satisfy both of these formats simultaneously.

See https://github.com/Nexmo/nexmo-oas-renderer/blob/ed891d7a5b4e8993fbd4628aceef937ca7de1638/lib/nexmo/oas/renderer/views/open_api/_webhooks.erb#L19-L30 for the relevant code

See https://github.com/Nexmo/api-specification/blob/master/definitions/sms.yml#L67-L69 for an example of pre-3.1 webhooks which would cause the renderer to fail if it used this version of oas_parser

lornajane commented 3 years ago

Yes, it would need to be hand in hand with a change to the SMS API description which is the only place it's currently used. I'm holding all other webhook additions to the API description until I can get this OpenAPI 3.0-friendly change made to our renderer - then we can get cracking on adding webhooks to all APIs and the renderer will cope when we upgrade them from 3.0 and x-webhooks to 3.1 and webhooks.

mheap commented 3 years ago

I'm happy to merge this, but if we do it needs to go out as a new major version and the OAS renderer will also need a major release

lornajane commented 3 years ago

Sounds perfectly sensible, and the renderer upgrade needs to land in ADP alongside the SMS API description update. What's the best way to co-ordinate this? @fabianrbz I think you should be at least informed if not in charge!

lornajane commented 3 years ago

Probably no longer useful, 3.1 is quite imminent and this would need attention to merge at the same time as the API description change. Tidying it up.