-
-
Notifications
You must be signed in to change notification settings - Fork 33
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.
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
chore: initialize repository #2
Conversation
application/json: | ||
schema: | ||
$ref: '#/components/schemas/Template' | ||
'422': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is the right code if someone gives the wrong body shape - e.g. not allowed template
field or invalid AsyncAPI. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
422
is used whenever the syntaxis is correct and understandable by your server, meaning the JSON or YAML is valid, but semantically it isn't, like your example of not allowed template field
. So in this case, makes sense to me.
However, for the other example invalid AsyncAPI
, 400
status code fits better since it will mean the syntaxis is wrong.
So I think we can add both status codes so we can differentiate the source of the errors.
openapi.yaml
Outdated
url: https://www.apache.org/licenses/LICENSE-2.0.html | ||
|
||
servers: | ||
- url: http://api.asyncapi.io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should change it, but for what? (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh, I like it as it is. Just a little but important change:
- url: http://api.asyncapi.io | |
- url: https://api.asyncapi.io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, thanks! But I had in mind for what domain 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, thanks! But I had in mind for what domain 😅
Yeah, and for me api.asyncapi.io
looks ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use api.asyncapi.com
instead. We should start getting rid of the .io
domain. It was used for historical reasons.
template: | ||
type: string | ||
description: Template name to be generated. | ||
enum: | ||
- 'html' | ||
- 'markdown' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In first iteration we will only support these two templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just dropped few review comments.
Also please remove.DS_Store
file.
openapi.yaml
Outdated
url: https://www.apache.org/licenses/LICENSE-2.0.html | ||
|
||
servers: | ||
- url: http://api.asyncapi.io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh, I like it as it is. Just a little but important change:
- url: http://api.asyncapi.io | |
- url: https://api.asyncapi.io |
openapi.yaml
Outdated
- url: http://api.asyncapi.io | ||
|
||
paths: | ||
/generate: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two suggestions regarding this endpoint:
1. API Versioning
I would suggest we start right now doing API versioning through the URI. I would add it so we can always differentiate from v1
, v2
, etc. Examples:
https://api.asyncapi.io/v1/generate
https://api.asyncapi.io/v2/generate
See https://www.freecodecamp.org/news/how-to-version-a-rest-api/
2. Add more resource semantics to the URI.
I see /generate
says not a lot about what the endpoint does. It generates, but I don't know what are we generating if I don't read the documentation.
I would suggest we add the resource we are generating as part of the URI. Depending on how we want to approach the API in terms of design, some options could be:
/generate/template
- Would we generate anything else apart from templates? Would it make sense in the future to have also (just an example)
/generate/uml
?
- Would we generate anything else apart from templates? Would it make sense in the future to have also (just an example)
/template/generate
- Would we do more things with templates? For example
/template/convert
?
- Would we do more things with templates? For example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. This idea with generate/template
is good, then we can extend it by generate/models
and other things to split functionality to separate paths.
About versioning, we can go with alpha
in first version and then switch to v1, or go with v1
from beginning. To be honest I was writing backend servers just only for fun and don't know what the best practices are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to start with the v1
or 1
since the beginning. No matters as it is in development phase (alpha version here) you will turn it to the final version eventually, creating all breaking changes that are needed during that period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I changed to the /v1/generate/template
- OpenAPI doesn't still support versioning so we need to add version to every path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if makes sense to use different servers for this? Like one server with the suffix /v1
on the URL, etc.
But anyway, I think it is ok like you did now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then just use another model like /generate/:templateName
, /convert/220/to/300
or similar. Read a bit about RPC over HTTP. Not everything has to be REST or GraphQL 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/generate/:templateName
was discussed in this thread and we decide to go with /generator
and template will as one of the payload's fields.
For /convert/220/to/300
I think that having from
and to
versions in request body will be better 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, just a thought: if you go for /convert
with from
and to
as payload fields, you should go for /generate
and not /generator
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, missed that comment 😅 Yeah, right, but instead of describing resources as a verb we should describe as a noun (we discuss about it in this thread), so it would be /converter
rather than /convert
. Thanks for the observation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I saw the convo. I just don't think converter and generator are resources. What is POST /generator supposed to do? Create a generator? No, right? I think your design is more RPC than REST, which is fine, just don't stick to REST rules if it's not REST. My two cents.
application/json: | ||
schema: | ||
$ref: '#/components/schemas/Template' | ||
'422': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
422
is used whenever the syntaxis is correct and understandable by your server, meaning the JSON or YAML is valid, but semantically it isn't, like your example of not allowed template field
. So in this case, makes sense to me.
However, for the other example invalid AsyncAPI
, 400
status code fits better since it will mean the syntaxis is wrong.
So I think we can add both status codes so we can differentiate the source of the errors.
@BOLT04 Do you wanna look at PR? I know that you participated in the discussion, maybe you would like to give some advice? :) |
@smoya Thanks for review! I added your suggestion and answered for your comments :) Could you look again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @magicmatatjahu 🙂, thank you so much for the ping!!! I wasn't aware this repo was created and that this type of work was in progress.
I left some comments, I'd love to have the community's opinion and discuss some topics of this API design.
Thank you again for the ping 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super cool, amazing work @magicmatatjahu 👍 and great discussions everyone 😃
6cd67ac
to
a84a0ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had a few comments, in case they are relevant 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Well, it took a while, but finally I will merge it :) Thank you all for the review. If the specification is not correct, we can always change it. |
Great stuff, Maciej! Looking forward for this API 🙌 |
Description
Initialize repository:
openapi.yaml
fileRelated issue(s)
See asyncapi/studio#86