feat: add /convert path #26

BOLT04 · 2022-01-09T19:13:48Z

Description

validate spec with parser-js
error handling. The versions supported are specified in openapi.yml, so an error is thrown if the spec is not followed. The converter-js also shouldn't reach the part: throw new Error('AsyncAPI document must be a valid JSON or YAML document.'). This is because we call parse before in order to validate the AsyncAPI document 1st.
service tests and more controller tests. I didn't add more service tests since it doesn't add much more logic to 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

magicmatatjahu · 2022-01-11T12:35:17Z

@BOLT04 Currently converter-js package support YAML and JSON, please check that lines https://github.com/asyncapi/converter-js/blob/4ace33ef8e2cc59e1f41964eb1246818cdbad815/lib/helpers.js#L5

and also if you put spec as YAMl you will have converted spec in YAML, this same situation with JSON -> JSON (check isYaml value in the converter https://github.com/asyncapi/converter-js/blob/4ace33ef8e2cc59e1f41964eb1246818cdbad815/lib/index.js#L56), so you don't need to worry about that :)

asyncapi-bot · 2022-01-11T12:35:32Z

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:

/ready-to-merge or /rtm - This comment will trigger automerge of PR in case all required checks are green, approvals in place and do-not-merge label is not added
/do-not-merge or /dnm - This comment will block automerging even if all conditions are met and ready-to-merge label is added

magicmatatjahu · 2022-01-20T17:39:44Z

@BOLT04 Do you need help here? :)

BOLT04 · 2022-01-20T22:48:44Z

@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 👍

BOLT04 · 2022-01-25T22:18:51Z

Hi @magicmatatjahu, I tested the errors thrown by converter-js and I think the only known exception that is thrown is this one right?

https://github.com/asyncapi/converter-js/blob/66e7d120e1cc7fa7f3da41080430598595966099/lib/helpers.js#L18

So I think currently we can do some error handling like the cli.js:

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 server-api, but make an improvement on converter-js instead, to throw errors in these cases, wdyt @magicmatatjahu ?

 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;
  }

asyncapi-bot · 2022-01-25T22:19:06Z

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:

/ready-to-merge or /rtm - This comment will trigger automerge of PR in case all required checks are green, approvals in place and do-not-merge label is not added
/do-not-merge or /dnm - This comment will block automerging even if all conditions are met and ready-to-merge label is added

BOLT04 · 2022-01-29T16:40:21Z

@magicmatatjahu and @smoya the PR is now ready for review 👍

magicmatatjahu · 2022-01-31T10:04:19Z

@BOLT04 Hello! Sorry for delay! I felt bad in previous week and in Thursday and Friday I didn't work. Sorry for delay!

For the validations below maybe we should not handle them in server-api, but make an improvement on converter-js instead, to throw errors in these cases, wdyt @magicmatatjahu ?

I am 100% for it! Could you create PR on converter-js with fix? :)

magicmatatjahu

Awesome! However, we should clarify some topics, please refer to my comments, and sorry for delay again! :)

magicmatatjahu · 2022-01-31T10:40:49Z

src/services/convert.service.ts

+      const asyncapiSpec = typeof spec === 'object' ? JSON.stringify(spec) : spec;
+      const convertedSpec = convert(asyncapiSpec, version);
+
+      return language === 'json'
+        ? this.convertToJSON(convertedSpec)
+        : convertedSpec;


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 :)

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?

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 to yaml - we have problem because converter-js will return JSON and we need to transform it to the YAML. We should improve above lines and also convertToJSON function to support that case. I hope that you understand :)

magicmatatjahu · 2022-01-31T10:43:43Z

src/services/convert.service.ts

+        ? this.convertToJSON(convertedSpec)
+        : convertedSpec;
+    } catch (err) {
+      logger.error('[ConvertService] An error has occurred while converting spec to version: {0}. Error: {1}', version, err);


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 :)

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?

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?

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?

Sorry for that late response! Really sorry, I have a hundreads of notifications 😅

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?

@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 😅

Also I think that we should describe all problems throw by package in the Readme as we do in the parser-js :)

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 🙂 ?

@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 :)

src/interfaces.ts

magicmatatjahu · 2022-01-31T11:08:38Z

src/controllers/convert.controller.ts

+      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
+      });


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 :)

openapi.yaml

magicmatatjahu

@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!

openapi.yaml

src/interfaces.ts

magicmatatjahu · 2022-02-09T11:20:49Z

src/services/convert.service.ts

+      const asyncapiSpec = typeof spec === 'object' ? JSON.stringify(spec) : spec;
+      const convertedSpec = convert(asyncapiSpec, version);


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 :)

magicmatatjahu · 2022-02-09T11:39:09Z

src/services/convert.service.ts

+      const asyncapiSpec = typeof spec === 'object' ? JSON.stringify(spec) : spec;
+      const convertedSpec = convert(asyncapiSpec, version);
+
+      return language === 'json'
+        ? this.convertToJSON(convertedSpec)
+        : convertedSpec;


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 to yaml - we have problem because converter-js will return JSON and we need to transform it to the YAML. We should improve above lines and also convertToJSON function to support that case. I hope that you understand :)

magicmatatjahu · 2022-02-09T11:47:09Z

src/services/convert.service.ts

+        ? this.convertToJSON(convertedSpec)
+        : convertedSpec;
+    } catch (err) {
+      logger.error('[ConvertService] An error has occurred while converting spec to version: {0}. Error: {1}', version, err);


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?

magicmatatjahu · 2022-02-09T11:48:07Z

src/services/convert.service.ts

+        ? this.convertToJSON(convertedSpec)
+        : convertedSpec;
+    } catch (err) {
+      logger.error('[ConvertService] An error has occurred while converting spec to version: {0}. Error: {1}', version, err);


Sorry for that late response! Really sorry, I have a hundreads of notifications 😅

magicmatatjahu · 2022-02-09T11:48:56Z

src/services/convert.service.ts

+      // JS Object -> pretty JSON string
+      return JSON.stringify(jsonContent, null, 2);
+    } catch (err) {
+      logger.error('[ConvertService.convertToJSON] Error: {0}', err);


We should go with solution described in the other comment (with ProblemException error) :)

magicmatatjahu · 2022-02-09T11:49:53Z

src/controllers/convert.controller.ts

+        return next(new ProblemException({
+          type: 'invalid-json',
+          title: 'Bad Request',
+          status: 400,
+          detail: 'Couldn\'t convert the spec to the requested version.'
+        }));


When converter-js will throw errors, we can improve that error :)

BOLT04 · 2022-02-12T14:20:59Z

@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!

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 👍 .
I'd like to collaborate with you on this PR yes 👍 , I can also help on the PR for converter-js so that we then use those errors in here.

BOLT04 · 2022-03-09T19:13:00Z

@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: npm verb stack Error: Unsupported platform for fsevents@2.3.2: wanted {"name":"fsevents","version":"2.3.2","engines":{"node":"^8.16.0 || ^10.6.0 || >=11.0.0"},"os":["darwin"],"dependencies":{},"optionalDependencies":{},"peerDependenciesMeta":{},"devDependencies":{"node-gyp":"^6.1.0"},"bundleDependencies":false,"peerDependencies":{},"deprecated":false,"_resolved":"https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz","_integrity":"sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==","_shasum":"8a526f78b8fdf4623b709e0b975c52c24c02fd1a","_shrinkwrap":null,"_id":"fsevents@2.3.2","_from":"fsevents@~2.3.2","_requested":{"type":"range","registry":true,"raw":"fsevents@~2.3.2","name":"fsevents","escapedName":"fsevents","rawSpec":"~2.3.2","saveSpec":null,"fetchSpec":"~2.3.2"},"_spec":"fsevents@~2.3.2","_where":"/home/runner/work/server-api/server-api/node_modules/rollup"} (current: {"os":"linux","cpu":"x64"}). But I'm not sure why this is happening, and what change caused it 😢

magicmatatjahu · 2022-03-14T17:10:06Z

@BOLT04 Yeah, I see that error, I will check that :)

magicmatatjahu · 2022-03-14T18:38:37Z

@BOLT04 I had to reinstall whole project (remove node_modules with package-lock.json) and it seems work. Also I pushed my propositions to the code. I fixed handling of converting between JSON <-> YAML and also improve errors :) Sorry for that delay. Please let me know what do you think. Except unit tests for ConverterService I think that everything is ready to merge - of course you can see problems in implementation so please let me know! :)

magicmatatjahu · 2022-03-15T09:58:41Z

@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.

BOLT04 · 2022-03-19T12:31:54Z

@magicmatatjahu yup all the changes look fine 👍, thanks for all the help 🙂

asyncapi#13

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

sonarqubecloud · 2022-03-28T08:49:36Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

magicmatatjahu · 2022-03-28T08:54:54Z

/rtm

asyncapi-bot · 2022-03-28T09:02:30Z

🎉 This PR is included in version 0.6.0 🎉

The release is available on:

GitHub release
v0.6.0

Your semantic-release bot 📦🚀

BOLT04 requested review from smoya and magicmatatjahu January 9, 2022 19:13

BOLT04 self-assigned this Jan 9, 2022

BOLT04 marked this pull request as ready for review January 29, 2022 16:39

magicmatatjahu requested changes Jan 31, 2022

View reviewed changes

magicmatatjahu reviewed Feb 9, 2022

View reviewed changes

magicmatatjahu mentioned this pull request Feb 17, 2022

feat: throw errors, do not log them asyncapi/converter-js#99

Merged

BOLT04 mentioned this pull request Feb 28, 2022

An interface/project for describing errors/problems in tools in our organization. asyncapi/community#266

Closed

BOLT04 requested a review from asyncapi-bot-eve as a code owner March 14, 2022 18:10

BOLT04 and others added 7 commits March 24, 2022 17:17

chore(deps): add @asyncapi/converter and @asyncapi/specs

8517b38

asyncapi#13

fix: clean up code smell

02637f9

chore: update package-lock

f3db717

feat: add /convert to openapi.yml

0a685c7

refactor: remove yaml dep and fix convert service

dbc2248

converter-js supports a JSON string being passed to it. There is no need to have the "yaml" package.

fix: remove version validation in convert controller

f2849bc

This validation is done by the OpenAPI spec, so it's not necessary to be on the controller

BOLT04 and others added 14 commits March 24, 2022 17:18

test: fix validate path's test

dd5ec56

chore: clean up import

3eb8a7e

refactor: use specs package in interfaces

497eef5

chore: update package-lock.json

1a09e19

test: fix spec interface

b1efff8

refactor: add suggestions

b27152d

refactor: error handling for converter service

72d11ed

chore: update package-lock.json

08d6e6b

chore: update converter dependency

e1e698a

This is necessary to leverage the changes made by: asyncapi/converter-js#99

improve /convert endpoint

69f6c64

update deps

1f5ee6e

update deps

795969e

reinstall whole project

e70297a

add tests for convert service

d5741e3

magicmatatjahu force-pushed the feat/13-add-convert-path branch from 59a2ace to d5741e3 Compare March 24, 2022 16:18

magicmatatjahu added 2 commits March 25, 2022 13:14

add convert path to app

d70de20

add more unit tests

02323fd

magicmatatjahu previously approved these changes Mar 28, 2022

View reviewed changes

update deps

c4f7e21

magicmatatjahu dismissed their stale review via c4f7e21 March 28, 2022 08:49

magicmatatjahu approved these changes Mar 28, 2022

View reviewed changes

asyncapi-bot added the ready-to-merge label Mar 28, 2022

asyncapi-bot merged commit aa76e75 into asyncapi:master Mar 28, 2022

asyncapi-bot added the released label Mar 28, 2022

BOLT04 deleted the feat/13-add-convert-path branch March 29, 2022 19:44

		const asyncapiSpec = typeof spec === 'object' ? JSON.stringify(spec) : spec;
		const convertedSpec = convert(asyncapiSpec, version);

feat: add /convert path #26

feat: add /convert path #26

Conversation

BOLT04 commented Jan 9, 2022 • edited Loading

magicmatatjahu commented Jan 11, 2022

asyncapi-bot commented Jan 11, 2022

magicmatatjahu commented Jan 20, 2022

BOLT04 commented Jan 20, 2022

BOLT04 commented Jan 25, 2022

asyncapi-bot commented Jan 25, 2022

BOLT04 commented Jan 29, 2022

magicmatatjahu commented Jan 31, 2022

magicmatatjahu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BOLT04 Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicmatatjahu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BOLT04 commented Feb 12, 2022

BOLT04 commented Mar 9, 2022

magicmatatjahu commented Mar 14, 2022

magicmatatjahu commented Mar 14, 2022

magicmatatjahu commented Mar 15, 2022 • edited Loading

BOLT04 commented Mar 19, 2022

sonarqubecloud bot commented Mar 28, 2022

magicmatatjahu commented Mar 28, 2022

asyncapi-bot commented Mar 28, 2022

BOLT04 commented Jan 9, 2022 •

edited

Loading

BOLT04 Feb 28, 2022 •

edited

Loading

magicmatatjahu commented Mar 15, 2022 •

edited

Loading