-
-
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
feat: add /convert path #26
feat: add /convert path #26
Conversation
@BOLT04 Currently and also if you put spec as YAMl you will have converted spec in YAML, this same situation with JSON -> JSON (check |
Hello, @magicmatatjahu! 👋🏼 I'm Genie from the magic lamp. Looks like somebody needs a hand! 🆘 At the moment the following comments are supported in pull requests:
|
@BOLT04 Do you need help here? :) |
@magicmatatjahu I haven't looked at this branch for a while, I've been short on time. As soon as I'm able I'll update the openapi.yml file and the rest of the tasks 👍 |
Hi @magicmatatjahu, I tested the errors thrown by So I think currently we can do some error handling like the try {
const asyncapi = fs.readFileSync(asyncapiFile, 'utf-8');
console.log(converter.convert(asyncapi, version, {
id: program.id,
}));
} catch (e) {
showErrorAndExit(e);
} For the validations below maybe we should not handle them in if (fromVersion === -1 || toVersion === -1) {
console.error(`Cannot convert from ${parsed.asyncapi} to ${version}.`);
return;
}
if (fromVersion > toVersion) {
console.error(`Cannot downgrade from ${parsed.asyncapi} to ${version}.`);
return;
}
if (fromVersion === toVersion) {
console.error(`Cannot convert to the same version.`);
return;
} |
Hello, @BOLT04! 👋🏼 I'm Genie from the magic lamp. Looks like somebody needs a hand! 🆘 At the moment the following comments are supported in pull requests:
|
@magicmatatjahu and @smoya the PR is now ready for review 👍 |
@BOLT04 Hello! Sorry for delay! I felt bad in previous week and in Thursday and Friday I didn't work. Sorry for delay!
I am 100% for it! Could you create PR on |
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.
Awesome! However, we should clarify some topics, please refer to my comments, and sorry for delay again! :)
src/services/convert.service.ts
Outdated
const asyncapiSpec = typeof spec === 'object' ? JSON.stringify(spec) : spec; | ||
const convertedSpec = convert(asyncapiSpec, version); | ||
|
||
return language === 'json' | ||
? this.convertToJSON(convertedSpec) | ||
: convertedSpec; |
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 some problem with that code.
typeof spec === 'object'
is ok but we should treat AsyncAPIDocument
type as normal object, because it is a instance of the AsyncAPIDocument
class, so JSON.stringify(spec)
won't work properly. We should change type for spec
argument to the object | string
(object
as any JSON).
Also language === 'json'
. As I remember I wrote in this PR that convert-js
under the hood can handle YAML and JSON as well, so we don't need to convert to JSON manually (you can check this line https://github.com/asyncapi/converter-js/blob/master/lib/index.js#L56) If user pass JSON will have JSON, if YAML, will have YAML. Of course maybe you wanna add option to convert YAML -> JSON and vice versa. Please elaborate on that :)
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.
Of course maybe you wanna add option to convert YAML -> JSON and vice versa
yes I thought the language
parameter was for this, so the user could send the payload as JSON, but ask for it to be converted to YAML. At least on the studio
or cli
I think that's how it's implemented. Should we not support this?
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 like that option, we can leave it :)
Of course here we have a "little" problem. When someone pass json
spec as string (stringified JSON) then converter-js
will return JSON object (not stringified JSON, you can check it here https://github.com/asyncapi/converter-js/blob/master/lib/index.js#L60). So we can have cases:
- someone will send
yaml
and want leave it - no problem with current implementation - someone will send
json
and want to leave it - no problem with current implementation - someone will send
json
and want to convert toyaml
- we have problem becauseconverter-js
will return JSON and we need to transform it to the YAML. We should improve above lines and alsoconvertToJSON
function to support that case. I hope that you understand :)
src/services/convert.service.ts
Outdated
? this.convertToJSON(convertedSpec) | ||
: convertedSpec; | ||
} catch (err) { | ||
logger.error('[ConvertService] An error has occurred while converting spec to version: {0}. Error: {1}', version, err); |
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 that we should throw error here without info about ConvertService
. ATM we don't use that annotations (I mean info about services in the error) so we should make everything as in other places :) Of course I like that idea, so we can go with solutions:
- start logging but also throw similar error
- throw only error without logging it
Please elaborate on it :)
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.
sorry I didn't understand your suggestion, should we make a new Error (defined on the exceptions folder) and throw that instead of err
? Or is your comment on the fact that we log the error?
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, so could we create a ConvertExcetion
and throw a new error of that type. I think the log adds a little bit of value in knowing where the error happened, but we can also remove the catch
if you want, wdyt?
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 🤔 I can create PR in converter-js
to throw errors here https://github.com/asyncapi/converter-js/blob/master/lib/index.js#L37 and we can go with solution like in other controllers, with returning ProblemException
and with proper type like .../problems/converter-...
. What do you think?
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.
Sorry for that late response! Really sorry, I have a hundreads of notifications 😅
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.
and we can go with solution like in other controllers, with returning ProblemException and with proper type like .../problems/converter-.... What do you think?
@magicmatatjahu should we change the type
field to point towards the README of the converter
repo, and add something like this: https://github.com/asyncapi/parser-js#error-types?
Currently we would point to https://api.asyncapi.com/problem/converter
or https://api.asyncapi.com/problem/converter-json
, but we don't handle those routes on the API... should we? I think it might be better to handle this on GitHub rather than render HTML or markdown in this API for Problem details, wdyt?
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.
@BOLT04 Yes, you're right. Atm we don't handle such an endpoint (I mean problem/...
) in our API, but we should. I created few days ago GSOC proposal to handle errors in our all packages using that problem
interface - asyncapi/community#266 One of the action point (but nice to have, not mandatory) is integration registry of all problem types in our organization. Pointing to the Readme
of project is nice, but better option will be point to api.server.com
. I know that errors throw by parser-js point to the Readme, but we should consolidate it.
Having api which will return details of problem we can then easy integrate with Studio and render info about problemo in nice way :) So I also see benefits to have one place where we will have described problems.
In the problem
interface we can also add additional fields (other than type
, details
etc) so we can add project
field with url
which points to the project:
{
url: 'https://github.com/asyncapi/converter',
problemUrl: ...
}
What do you think? Do you need some help with your code? I could help you, because I'm remorseful 😅
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.
Also I think that we should describe all problems throw by package in the Readme as we do in the parser-js
:)
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.
Also I think that we should describe all problems throw by package in the Readme as we do in the
parser-js
:)
completely agreed 👍
Having api which will return details of problem we can then easy integrate with Studio and render info about problemo in nice way :) So I also see benefits to have one place where we will have described problems.
@magicmatatjahu Yes that would be awesome! Then having more control about the result of the endpoint inside type
is better, instead of just pointing to a section in a README
. But these endpoints will be on this API and that @asyncapi/problem
package will handle all the logic for the endpoint? Could you specify this part a bit more 🙂 ?
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.
@BOLT04 ❤️
The @asyncapi/problem
package itself should be the library with which we should create the problem instance itself. It should not have any logic related to ServerAPI. The implementation of this endpoint itself should be here. I don't know how registry of problems should look like so that ServerAPI will be up-to-date with all the problems we have in the organization. This is a topic for discussion :)
await parse(asyncapi, prepareParserConfig(req)); | ||
const convertedSpec = await this.convertService.convertSpec( | ||
asyncapi, | ||
language, | ||
version.toString(), | ||
); | ||
|
||
if (!convertedSpec) { | ||
return next(new ProblemException({ | ||
type: 'invalid-json', | ||
title: 'Bad Request', | ||
status: 400, | ||
detail: 'Couldn\'t convert the spec to the requested version.' | ||
})); | ||
} | ||
const convertedSpecObject = YAML.load(convertedSpec); | ||
res.json({ | ||
asyncapi: convertedSpecObject | ||
}); |
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 know that it's a secret knowledge, because we don't have a deep dive docs for our ParserJS, but ParserJS throws error when you pass to it the old version (1.0.0, 1.1.0, 1.2.0 and all 2.0.0 rcs) - here is a line https://github.com/asyncapi/parser-js/blob/master/lib/parser.js#L79
but converter should allow to make operation on that versions (convert it to newest), so I think that we have problem here. We should "filter" old version and even if parse
will throw error, we should perform ahead actions. I hope that you understand.
And last thing:
const convertedSpecObject = YAML.load(convertedSpec);
as I described in the previous comments, we have such a logic with converting to the YAML/JSON in the converter-js side, but we can elaborate on that topic :)
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.
@BOLT04 Sorry for that late response, really sorry!
I added some suggestions. First we should improve converter-js
and then improve error handling in that PR. I will create PR in the converter-js
but we have to wait for merge it.
If you feel tired of this PR, I can always help you and contribute to this PR. Let me know if you agree :) I know you are waiting a long time with the merging of this PR, sorry, but we have problems related to the converter-js
. Sorry again!
src/services/convert.service.ts
Outdated
const asyncapiSpec = typeof spec === 'object' ? JSON.stringify(spec) : spec; | ||
const convertedSpec = convert(asyncapiSpec, version); |
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.
When we will merge that PR asyncapi/converter-js#96 (we have bug in the converter-js
, sorry!) we will be able to do only one operation:
const convertedSpec = convert(spec, version);
rest will be handled inside converter-js
package :)
src/services/convert.service.ts
Outdated
const asyncapiSpec = typeof spec === 'object' ? JSON.stringify(spec) : spec; | ||
const convertedSpec = convert(asyncapiSpec, version); | ||
|
||
return language === 'json' | ||
? this.convertToJSON(convertedSpec) | ||
: convertedSpec; |
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 like that option, we can leave it :)
Of course here we have a "little" problem. When someone pass json
spec as string (stringified JSON) then converter-js
will return JSON object (not stringified JSON, you can check it here https://github.com/asyncapi/converter-js/blob/master/lib/index.js#L60). So we can have cases:
- someone will send
yaml
and want leave it - no problem with current implementation - someone will send
json
and want to leave it - no problem with current implementation - someone will send
json
and want to convert toyaml
- we have problem becauseconverter-js
will return JSON and we need to transform it to the YAML. We should improve above lines and alsoconvertToJSON
function to support that case. I hope that you understand :)
src/services/convert.service.ts
Outdated
? this.convertToJSON(convertedSpec) | ||
: convertedSpec; | ||
} catch (err) { | ||
logger.error('[ConvertService] An error has occurred while converting spec to version: {0}. Error: {1}', version, err); |
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 🤔 I can create PR in converter-js
to throw errors here https://github.com/asyncapi/converter-js/blob/master/lib/index.js#L37 and we can go with solution like in other controllers, with returning ProblemException
and with proper type like .../problems/converter-...
. What do you think?
src/services/convert.service.ts
Outdated
? this.convertToJSON(convertedSpec) | ||
: convertedSpec; | ||
} catch (err) { | ||
logger.error('[ConvertService] An error has occurred while converting spec to version: {0}. Error: {1}', version, err); |
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.
Sorry for that late response! Really sorry, I have a hundreads of notifications 😅
src/services/convert.service.ts
Outdated
// JS Object -> pretty JSON string | ||
return JSON.stringify(jsonContent, null, 2); | ||
} catch (err) { | ||
logger.error('[ConvertService.convertToJSON] Error: {0}', err); |
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 go with solution described in the other comment (with ProblemException
error) :)
return next(new ProblemException({ | ||
type: 'invalid-json', | ||
title: 'Bad Request', | ||
status: 400, | ||
detail: 'Couldn\'t convert the spec to the requested version.' | ||
})); |
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.
When converter-js
will throw errors, we can improve that error :)
no problem at all @magicmatatjahu, it doesn't matter how long it takes, I'm patient 🙂. I'm just happy to be contributing and collaborating with the community 👍 . |
@magicmatatjahu could you help me figure out what is wrong with the PR testing action that fails? I know it's because installing npm dependencies fails with this: |
@BOLT04 Yeah, I see that error, I will check that :) |
@BOLT04 I had to reinstall whole project (remove |
@BOLT04 When you have time (don't rush) let me know if you accept these changes :) I will then add tests for service and we can merge. |
@magicmatatjahu yup all the changes look fine 👍, thanks for all the help 🙂 |
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
converter-js supports a JSON string being passed to it. There is no need to have the "yaml" package.
This validation is done by the OpenAPI spec, so it's not necessary to be on the controller
This is necessary to leverage the changes made by: asyncapi/converter-js#99
59a2ace
to
d5741e3
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
/rtm |
🎉 This PR is included in version 0.6.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Description
parser-js
openapi.yml
, so an error is thrown if the spec is not followed. Theconverter-js
also shouldn't reach the part:throw new Error('AsyncAPI document must be a valid JSON or YAML document.')
. This is because we callparse
before in order to validate the AsyncAPI document 1st.converter-js
, but @magicmatatjahu let me know if there are any test cases you want to cover 👍Note: Also I changed one validate controller test to include
/v1
, but it was weird since without this the test still works 😅Related issue(s)
Resolves #13