feat: conversion of openapi '3.0' to asyncapi '3.0' #269
Conversation
Gmin2
requested review from
fmvilas,
magicmatatjahu,
asyncapi-bot-eve and
jonaslagoni
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
There was a problem hiding this comment.
Your code is way too tangled together at the moment. There should be clear separations between converting AsyncAPI between versions and converting OpenAPI to AsyncAPI 🙂
Its completely fine to have 2 functions, one called convert for converting AsyncAPI between versions and then another called ? to convert OpenAPI to AsyncAPI 🤙
Also make sure you add more test, especially edge cases for it to convert 🙂
But this is a good first PR to merge, good first initial feature 👍
|
dont merge @jonaslagoni need to add some more test cases |
|
@asyncapi/bounty_team |
|
@Gmin2 ping me if you need anything |
|
@jonaslagoni can we merge this PR, i will make another PR with more testcases and refining the conversion of path objects |
|
Since you already included everything in one PR and it is not a fully working change by it self, we cannot unfortunately... |
|
@jonaslagoni completed all the work, please review |
|
@Gmin2 let me know when you are ready for a final review. |
There was a problem hiding this comment.
Looking good, a bit more is needed before we can merge this ✌️
A few things are needed:
- Make sure you test the perspective option
- Make sure that new functions have jsdocs associated that explains abstracted what it does
- Make sure that the converted operation object also assigns the
.replysection where needed.
|
@Gmin2 also make sure you update the readme file to introduce the new feature |
There was a problem hiding this comment.
Forgot to mention (actually thought I did in the last review) remember to change the main readme file in the repo so everyone can find this new functionality 🙂
Ahh, wrote it here: #269 (comment)
Done, need a final look @jonaslagoni |
There was a problem hiding this comment.
I know this last efforts always are the toughest to perfect but makes all the difference for the users 🤙
I see some edge cases that are not covered so far by the tests and will make complex OpenAPI documents fail. I dont think we can do anything about External -> internal, except make a notice in the readme about it being something not supported. However, External schema objects and Nested schema objects is something that should be supported. Same with parameters of course.
- External schema objects
openapi: 3.0.0
info:
title: test
version: 1.0.0
description: Test
paths:
/test:
get:
responses:
'200':
description: Successful response
content:
application/json:
schema:
$ref: './external.json'
- Nested schema objects
openapi: 3.0.0
info:
title: test
version: 1.0.0
description: Test
paths:
/test:
get:
responses:
'200':
description: Successful response
content:
application/json:
schema:
$ref: '#/components/schemas/test'
components:
schemas:
test:
type: object
properties:
message:
$ref: '#/components/schemas/test2'
test2:
type: object
properties:
message:
type: string
Here, you cannot convert both schemas to the multi format schema, because referencing test2 as a multi format schema inside test is not allowed.
- External -> internal
openapi: 3.0.0
info:
title: test
version: 1.0.0
description: Test
paths:
/test:
get:
responses:
'200':
description: Successful response
content:
application/json:
schema:
$ref: './external.json'
components:
schemas:
test:
type: object
properties:
message:
type: string
# external.json
type: object
properties:
message:
$ref: './asyncapi.json#/components/schemas/test'Same as above, referencing test as a multi format schema inside external is not allowed.
- Parameters are not being converted in the AsyncAPI document, even though we have explicit test document for it 🤔
src/openapi.ts
Outdated
| if (isRefObject(schema)) { | ||
| // Check if it's an external reference | ||
| if (schema.$ref.startsWith('./') || schema.$ref.startsWith('http')) { | ||
| return schema; | ||
| } | ||
| return schema; | ||
| } |
There was a problem hiding this comment.
This is not entirely correct we still need to support External schema objects case, as we still need to convert the schema to multi schema object even if external. We just dont touch the external file and do any changes there. That is the restriction.
Make sure to add a test case for this.
There was a problem hiding this comment.
if (schema.$ref.startsWith('./') || schema.$ref.startsWith('http')) {
// Convert external references to multi-format schema objects
return {
schemaFormat: 'application/vnd.oai.openapi;version=3.0.0',
schema: schema
};
}
return schema;pushed this
|
|
/rtm |
|
🎉 This PR is included in version 1.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
@all-contributors please add @Gmin2 for code, doc, example, test |
|
I've put up a pull request to add @Gmin2! 🎉 |
Description
openapi,info,servers,tags,externalDocs,andsecurity.pathkeywordcomponents.Related issue(s)
Fixes #233