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.

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

Add implementation for /convert path #13

Closed
magicmatatjahu opened this issue Dec 3, 2021 · 8 comments · Fixed by #26
Closed

Add implementation for /convert path #13

magicmatatjahu opened this issue Dec 3, 2021 · 8 comments · Fixed by #26
Assignees
Labels
enhancement New feature or request released

Comments

@magicmatatjahu
Copy link
Member

AC:

  • add new path to the OpenAPI document and document it
  • create controller for /convert path
  • create needed services or reuse existing
  • handle errors
  • write tests
@magicmatatjahu magicmatatjahu added the enhancement New feature or request label Dec 3, 2021
@magicmatatjahu magicmatatjahu changed the title Add implementation for /convert Add implementation for /convert path Dec 3, 2021
@BOLT04
Copy link
Member

BOLT04 commented Dec 23, 2021

Hi @magicmatatjahu 🙂, about this path is it related to the functionality of the converter-js?
I found this issue asyncapi/cli#41 so I thought it might have something to do with it, am I correct?

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Jan 3, 2022

@BOLT04 Hi! Sorry for that late response but I had vacation in Christmas to the New Year :)

Hi @magicmatatjahu 🙂, about this path is it related to the functionality of the converter-js?
I found this issue asyncapi/cli#41 so I thought it might have something to do with it, am I correct?

Yes, it's related to the converter-js tool. We try to have that same functionality in the server-api as in cli. I think that we should first discuss what we need in the request body and response for that path and then we can implement it. Do you wanna handle that?

@BOLT04
Copy link
Member

BOLT04 commented Jan 3, 2022

@magicmatatjahu no problem, I hope you had a good time on your vacation 😃.
For the request I was thinking something like this:

// Note: Spec versions are defined in @asyncapi/specs
export enum SpecsEnum {
  '1.0.0',
  '1.1.0',
  '1.2.0',
  '2.0.0-rc1',
  '2.0.0-rc2',
  '2.0.0',
  '2.1.0',
  '2.2.0',
  'latest'
}

export interface ConvertRequestDto {
  version?: SpecsEnum;
  language: 'json' | '',
  asyncapi: AsyncAPIDocument
}

Basically the request, ConvertRequestDto, would receive the parameters of convert-js plus the language field to know on the document should be returned.
To support YAML I don't think the asyncapi field can be a string. At least on Postman to test this API we would need to add some pre-request script, to set a variable with the YAML string. Because in JS we can have a multi-line string, but in JSON we can't have that in a string field.
So in order to support YAML, we could implement the controller to support application/x-www-form-urlencoded. The YAML would be a file and the rest of the fields would be in the form data as well.

The response could just be an object with the field asyncapi, that has the converted spec. Then we could add more fields to the response if we want to extend. If the converted spec was requested to be in YAML, I think we could also have application/x-www-form-urlencoded on the response... but it seems a bit weird 🤔.
What do you think @magicmatatjahu ?

Also, these TS types could be used on the project to have type-safety on the req.body, or some IntelliSense if we have controllers that take a lot of fields. Let me know if you think this is a good idea 👍

@magicmatatjahu
Copy link
Member Author

@BOLT04 I think that in the ConvertRequestDto the language field should be also optional, if someone will send yaml then it should be sent as yaml (this same with json).

About YAML: Tbh I'm not expert in backend and that all staff with REST etc. but why we need that logic related to the YAML? As you wrote JSON doesn't support multiline but YAML can be sent with newline characters (\n) and everything will work. If someone will want to send YAML by (e.g.) cURL should properly parse YAML and we can treat asyncapi field as normal string - in that same way works /generate path, it supports JSON and YAML as well without any additional Content-Type. We should support handling files but not at the moment, we should only focus on sending raw data in request body. Server-API at the beginning will be only used in the AsyncAPI Studio and we will handle YAML in proper way in the Studio, don't worry. Of course that solution with file should be handled sooner or later. Could you create an issue?

The response could just be an object with the field asyncapi, that has the converted spec. Then we could add more fields to the response if we want to extend.

Good idea :)

If the converted spec was requested to be in YAML, I think we could also have application/x-www-form-urlencoded on the response... but it seems a bit weird 🤔.

Converted YAML should have proper indentation by \n and blank spaces so even if someone will use it in the editor/ save to the file then will have a correct YAML, so that Content-Type isn't needed.

Also, these TS types could be used on the project to have type-safety on the req.body, or some IntelliSense if we have controllers that take a lot of fields. Let me know if you think this is a good idea 👍

👌🏼

Please have in mind that the converter-js throws proper errors when someone will use not supported versions etc. Also very important: if someone will send AsyncAPI it should be first validate by parser-js to check that given spec is correct - converter doesn't check that so it must have the correct spec on the input. Have in mind also that parser-js doesn't support < 2.0.0 versions (including rcs for 2.0.0), so you will have to write some logic with error handling. If you will have problems please write :)

@smoya Do you wanna add something?

@BOLT04
Copy link
Member

BOLT04 commented Jan 4, 2022

ok awesome, thanks for all the input 👍
Since the spec should be validated first before going to converter-js, I'll focus on finishing up the /validate PR and then make a draft for this one. Once that PR gets merged the ParserService could maybe be used by this convert controller to implement those validations and error handling 🙂

@BOLT04
Copy link
Member

BOLT04 commented Jan 7, 2022

Please have in mind that the converter-js throws proper errors when someone will use not supported versions etc.

@magicmatatjahu could you help find where these errors are being thrown in convert-js, I only see console.error and an empty return with undefined 😅 ?

BOLT04 added a commit to BOLT04/server-api that referenced this issue Jan 8, 2022
BOLT04 added a commit to BOLT04/server-api that referenced this issue Jan 9, 2022
Added the "yaml" package to convert JSON to YAML, since convert-js only seems to support YAML specs.
If we don't want to support a JSON spec on the HTTP request, we can delete this dependency.

If we do want that capability, then perhaps we should choose just one YAML package (yaml or js-yaml) that can do these conversions.

asyncapi#13
@BOLT04 BOLT04 mentioned this issue Jan 9, 2022
3 tasks
@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Jan 10, 2022

@BOLT04 Heh 😅 , I had in mind parser-js not converter-js.

I only see console.error and an empty return with undefined 😅 ?

You're right but it's a cli.js, you should see at this https://github.com/asyncapi/converter-js/blob/master/lib/index.js#L28, so when you will convert things you should try catch it, and return proper error (if it will exist) :)

magicmatatjahu pushed a commit to BOLT04/server-api that referenced this issue Mar 24, 2022
magicmatatjahu pushed a commit to BOLT04/server-api that referenced this issue Mar 24, 2022
Added the "yaml" package to convert JSON to YAML, since convert-js only seems to support YAML specs.
If we don't want to support a JSON spec on the HTTP request, we can delete this dependency.

If we do want that capability, then perhaps we should choose just one YAML package (yaml or js-yaml) that can do these conversions.

asyncapi#13
@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 0.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants