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

refactor: add RAML Schema Parser #577

Merged
merged 3 commits into from
Aug 26, 2022

Conversation

smoya
Copy link
Member

@smoya smoya commented Aug 17, 2022

Description

This PR does the following:

Note
Schema validation is done through https:/raml-org/webapi-parser. It does not provide a real JSON Path value for validation errors or something that can be really transformed in all cases. By now, path will point to the message payload but we can keep investigating other solutions.

Related issue(s)
#480

@smoya smoya changed the base branch from master to next-major August 17, 2022 14:59
@smoya smoya changed the title Feat/raml schema parser refactor: add RAML Schema Parser Aug 17, 2022
const jsonModel = await r2j.dt2js(payload, 'tmpType', { draft: '06' });
const convertedType = jsonModel.definitions.tmpType;

message['x-parser-original-schema-format'] = input.schemaFormat || input.defaultSchemaFormat;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as #576 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote comment about that in PR for AVRO :) We will handle that in one place.

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. I left two comments, please refer to them and we can merge that PR :)

const jsonModel = await r2j.dt2js(payload, 'tmpType', { draft: '06' });
const convertedType = jsonModel.definitions.tmpType;

message['x-parser-original-schema-format'] = input.schemaFormat || input.defaultSchemaFormat;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote comment about that in PR for AVRO :) We will handle that in one place.

report.results.forEach(result => {
validateResult.push({
message: result.message,
path: input.path, // Info provided by the RAML parser doesn't provide a real path to the error.
Copy link
Member

@magicmatatjahu magicmatatjahu Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the custom parsers path of single error should point to from perspective of given payload - in other words, payload should be treated as root of error, we will concatenate error path in one place, concatenate path of schema + path of error, so here you should return empty array.

Suggested change
path: input.path, // Info provided by the RAML parser doesn't provide a real path to the error.
path: [] // Info provided by the RAML parser doesn't provide a real path to the error.

Copy link
Member Author

@smoya smoya Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say input.path value is: ["channels", "myChannel", "publish", "message", "payload"], the behaviour will be different that https:/asyncapi/parser-js/pull/578/files#r952869403

I think I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there you operate on error results, in mentioned comment you create mock data for testing. .path of input is location on which data you operate some logic, error.path is path of the error, but from .path point of view - it's a local path that we will merge after validation 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed!

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing, rest is awesome! :)

message.payload = convertedType;
delete message.schemaFormat;
} catch (e) {
console.error(e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I don't think so that we need that console.error. We will try catch this inside customOperations :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed!

@sonarcloud
Copy link

sonarcloud bot commented Aug 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀 Should we wait for review of parser's maintainers?

@smoya
Copy link
Member Author

smoya commented Aug 26, 2022

We can merge now as we have one approval from a code owner.

@smoya
Copy link
Member Author

smoya commented Aug 26, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 354df12 into asyncapi:next-major Aug 26, 2022
@smoya smoya deleted the feat/ramlSchemaParser branch August 26, 2022 07:44
magicmatatjahu pushed a commit to magicmatatjahu/parser-js that referenced this pull request Oct 3, 2022
derberg pushed a commit that referenced this pull request Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants