fix: apply traits to standalone messages from components section #214

c-pius · 2020-12-10T17:20:38Z

Description

@derberg I tried to give it a shot but I am not really sure if the provided function is at the right place and also how the validateAndConvertMessage function works. I included the "x-parser-schema-id":"<anonymous-schema-1>" properties in the output schemas as far as I understood them. Unfortunately, they are only generated for messages that are used in a channel and not the standalone messages. I assume you would know how to fix those problems?

Besided, npm run test-browser was not working on my machine (OSX) from the beginning I cloned the repo. I am on node v12.14.1.

Related issue(s)
Resolves #210

github-actions

Welcome to AsyncAPI. Thanks a lot for creating your first pull request.

Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

magicmatatjahu

One thing to fix :) Thanks for contribution!

lib/parser.js

magicmatatjahu · 2020-12-10T18:57:29Z

I included the "x-parser-schema-id":"" properties in the output schemas as far as I understood them. Unfortunately, they are only generated for messages that are used in a channel and not the standalone messages. I assume you would know how to fix those problems?

The problem is here

parser-js/lib/models/asyncapi.js

Line 78 in d35d59a

constructor(...args) {

As I see we don't traverse in schemas inside the messages in components. We only traverse schemas in channels.message, components.parameters and component.schemas. @derberg should we fix it in this PR or leave it as a followup?

derberg

I tried to give it a shot but I am not really sure if the provided function is at the right place and also how the validateAndConvertMessage function works.

It looks good, thanks a bunch for jumping into it 🙇🏼
@magicmatatjahu explained well what is the purpose of this function. I'm only afraid that with the current implementation we will have messages (the ones from components.messages that are referred to channels) that are validated and converted twice, like this one -> https://github.com/asyncapi/parser-js/pull/214/files#diff-b335089706650932f2429d8b648707a16777501ad4e410fcbd35be14900707d8R14 which is a performance issue in my opinion because in large componentized specs with channels the validation will simply be doubled 🤔 I'm opened for suggestions 😄

I included the "x-parser-schema-id":"" properties in the output schemas as far as I understood them. Unfortunately, they are only generated for messages that are used in a channel and not the standalone messages. I assume you would know how to fix those problems?

a simple fix would bring the same issue as in my previous comment, I think. Anonymous schema id goes only to messages in channels. That would be too much for this PR for you to solve, and anyway it is a different topic that requires separate PR. Could you please create a separate issue, please 🙏🏼

I made code change suggestions that you can apply for now.

Besides, npm run test-browser was not working on my machine (OSX) from the beginning I cloned the repo. I am on node v12.14.1.

it was not working because of the missing condition @magicmatatjahu mentioned in one of his comments. Spec used in the browser test is as simple as

asyncapi: 2.0.0
info:
  title: My API
  version: 1.0.0
channels:
  "/test/tester":
    subscribe:
      message: {}

so it covers exactly the same case that @magicmatatjahu described, a spec without messages in components, and in fact, no components object at all. It would be awesome if you could add a test case for such a document to parse_test 🙏🏼

As a side note, I will create a PR to console log errors in the browser test, so nobody has to disable the headless mode of the browser and dig every issue on his/her own

last but not least -> you can take this PR out from draft to enable tests as you are pretty advanced with this PR and we anyway jumped into the review

test/parse_test.js

Co-authored-by: Lukasz Gornicki <[email protected]>

c-pius · 2020-12-13T16:03:22Z

added the guard that components.messages is defined and an additional test case for when there is no components object.

[duplicate application of traits] is a performance issue in my opinion because in large componentized specs with channels the validation will simply be doubled 🤔 I'm opened for suggestions 😄

I tried to give it a thought but I guess that would require to somehow apply the traits in conjunction with resolving the $ref properties? Maybe also remember the original $ref in a parser extension attribute and if there, replace the message in the operation with the referenced message object where traits have been applied brefore?

…to-standalone-messages

derberg · 2020-12-14T18:49:05Z

@c-pius trying to combine it with resolving of $ref-s would be quite a refactor and anyway this part is already complicated because of circular refs.

I like your idea about simple remembering that messages were processed already. Just remember it is not just applyTraits that is called twice on messages but validateAndConvertMessage too.

What if we revert the order? like process Messages from components first, and then Channels. Then in the case of applyTraits we do not have to even remember anything, we don't really have a performance issue 🤔 if I'm wrong, blame Monday 😄 but looking at 👇🏼 we are safe because after processing traits, traits prop is removed, so there is no risk it will be processed again? right?

function applyTraits(js) {
  if (Array.isArray(js.traits)) {
    for (const trait of js.traits) {
      for (const key in trait) {
        js[String(key)] = mergePatch(js[String(key)], trait[String(key)]);
      }
    }

    js['x-parser-original-traits'] = js.traits;
    delete js.traits;
  }
}

The only real concern is validateAndConvertMessage. We could add another prop there like x-validated=true and then just check if it is there, and then just not run the validation again. I'm only not sure this is a good prop name, maybe x-origin=components 🤔

But in general, it makes sense right? to process messages from components first, and then just process those unprocessed that are directly under channels?

c-pius · 2020-12-14T21:19:48Z

Actually I had already tried this using a check if the x-parser-message-name property is already there but only now noticed that this is added at a later point 🤦🏻 So yeah, your suggestion makes sense and would work 👍🏻

Regarding the name of the marker property I don't really mind any. I guess x-validated would be a bit more straightforward and cleaner to handle completely within validateAndConvertMessage. If we use x-origin we would actually need to either set it as part of customMessagesOperations func (therefore spread the logic over two functions, or pass another param to the validateAndConvertMessage indicating the component origin (maybe also possible using the existing pathToPayload param but I think not ideal as well).

I assume you intended it to be x-parser-validated, right? x-validated would have a high probability to collide with user defined extensions on a message I guess.

derberg · 2020-12-15T08:12:27Z

@c-pius yeap, x-parser- prefix is a must, sorry

derberg · 2020-12-15T08:13:37Z

I'm just afraid about validated as it will kinda imply that other messages from channels are not validated 😄
unless you remove it at the end from the document?

c-pius · 2020-12-15T09:58:45Z

I'm just afraid about validated as it will kinda imply that other messages from channels are not validated 😄
unless you remove it at the end from the document?

actually I would have put checking and setting the x-parser-validated field both into the validateAndConvertMessage function so that no matter if it is defined in channels or messages, if it has been validated once the flag is set. Probably something like:

async function validateAndConvertMessage(msg, originalAsyncAPIDocument, fileFormat, parsedAsyncAPIDocument, pathToPayload) {
  if (xParserValidated in msg && msg[xParserValidated] === true) {
    //only validate the message if it has not been done before
    return
  }

  //... function logic

  msg[xParserValidated] = true
}

or do you prefer it to be only added for components/messages and removed again afterwards?

derberg · 2020-12-15T10:20:45Z

you are right, good approach with having the logic in validateAndConvertMessage

I'm not 100% sure if x-parser-validated should stay or not because I'm not sure what it could be used for if it stays there in the document. I just prefer to leave something that might be useful.

what about x-parser-original-schema-format? every custom parser adds it:

and default parser should do it too -> https://github.com/asyncapi/parser-js/blob/master/lib/asyncapiSchemaFormatParser.js. And it would just have to be clearly described in the readme that every custom parser must add those fields to the document

Thoughts?

c-pius · 2020-12-15T20:57:55Z

Like the idea. Also similar to the x parser original trait approach then. Updated the PR already. Does that reflect what you meant?

And it would just have to be clearly described in the readme that every custom parser must add those fields to the document

Not sure how to handle this one though? From reading the existing doc I would have assumed to put it in the comment for the getMimeTypes() func but not sure if this would count as "clearly described" 😄

derberg · 2020-12-16T07:58:59Z

@c-pius I think adding a comment to parse function in 1st point, between function signature and jsdoc should be sufficient, just a simple explanation of what extra properties must be added to each message by the parser

derberg · 2020-12-16T08:12:45Z

lib/parser.js

@@ -190,6 +195,7 @@ async function validateAndConvertMessage(msg, originalAsyncAPIDocument, fileForm
  });

  msg.schemaFormat = DEFAULT_SCHEMA_FORMAT;
+  msg[String(xParserOriginalSchemaFormat)] = schemaFormat;


code from https://github.com/asyncapi/parser-js/pull/214/files#diff-2f45270c7389933f71b380538ff52e106ee5cb66a0b573d9967467dfdbc11c02R187 runs a message parser for a given mime type, so if in your AsyncAPI file you have a message that has schema provided in avro, then this code runs -> https://github.com/asyncapi/parser-js/pull/214/files#diff-2f45270c7389933f71b380538ff52e106ee5cb66a0b573d9967467dfdbc11c02R187

1st the avro parser puts avro mime type under x-parser-original-schema-format

2nd you override it with default schema format, asyncapi one, so we loose information about the original schema format

This is why instead of putting this line here, you need to have it in this parser of default asyncapi schema -> https://github.com/asyncapi/parser-js/blob/master/lib/asyncapiSchemaFormatParser.js

Add support for https://github.com/asyncapi/avro-schema-parser/blob/master/index.js#L7 too and put info about it in the readme as well please 🙏🏼

derberg · 2020-12-16T08:17:00Z

I also wonder if this knowledge about concerns to performance, and the risk of invoking validation twice is not going to be just our secret here in PR. Not sure though if there is a possible test case that could be added to check this, like check if our default parser was invoked only once? 🤔

…to-standalone-messages

derberg · 2020-12-23T13:01:43Z

lib/asyncapiSchemaFormatParser.js

  const payload = message.payload;
  if (!payload) return;

+  // considered save here because default parser handles JSON Schema payloads
+  message[String(xParserOriginalPayload)] = JSON.parse(JSON.stringify(payload));


what about 👇🏼

Suggested change

message[String(xParserOriginalPayload)] = JSON.parse(JSON.stringify(payload));

message[String(xParserOriginalPayload)] = Object.assign({}, payload);

Object.assign creates a new object but the copied properties are still shallow copies. so the payload object itself will not get a x-parser-schema-id applied, but for instance a "name" property within the payload will get it applied.

you are right.. 🤔
we cannot leave JSON.stringify as it won't work with circular references in schemas

🤔

Unfortunately I am not very deep into the node world (I guess you have noticed already 😄 ). So I would need to let you make the call on what to do best.

Considering that lodash.clonedeep would be available as individual dependency and looking behind the curtains on what would need to be done in an own helper function, I am not sure if manually implementing it is a good idea. On the other hand, I also cannot judge the impact of adding a new dependency to this lib.

lodash is just 3.3kb gziped which is fine, I'm more concerned about performance again. We add something that is not that much needed but will add performance issues to large files again 🤔

I have doubts if this is the way to go. If we should not fall back to the initial idea about adding an additional field that we can use to check if the message was processed or not. Sorry, I just did not take into consideration this issue with copying objects

no worries, got you. and yeah, since the payload is not actually transformed, the x-parser-original-payload would always include the same structure except the added x-parser attributes. not sure if there is any use for this except consistency with custom parsers regarding the x-parser-original-payload being there?

yeah, the use case if for those that have different formats of schemas, like avro for example, so after parsing the document they can still access the content before conversion in case they want to include it in docs or prefer to read original schema, for example in runtime validations.

so, again back to what could be the name of the extension that holds information if the message was already processed or not 😄

derberg · 2020-12-23T13:02:39Z

@c-pius I added a suggestion. Lodash is always an option but for parser let us try to keep it as slim as possible

c-pius · 2020-12-24T14:30:19Z

so, again back to what could be the name of the extension that holds information if the message was already processed or not 😄

the hardest question of all 😄 as far as I can tell the open options are:

we still go for x-parser-original-schema-format
- pro: reuse of existing property
- con: we rely on the custom parsers to actually set the property to avoid applying the parsing twice
new property which is set and checked in validateAndConvertMessage()
- pro: independent from parsers, concise within one function
- con: additional x-parser-... property (e.g. x-parser-message-validated, x-parser-message-parsed, x-parser-message-converted)

I am still wondering actually if we could also set the x-parser-original-schema-format ourselves in validateAndConvertMessage()? Isn't the used parser actually selected by the original schema format || default here:

parser-js/lib/parser.js

Line 182 in b32899e

await PARSERS[String(schemaFormat)]({

If so, we would be independent from the custom parsers and wouldn't need an additional property

derberg · 2021-01-04T17:06:30Z

@c-pius hey, sorry mate but I and others had a longer break here. 2020 was crazy busy and we needed a few days without asyncapi.

Let us have a new property -> x-parser-message-parsed

This reverts commit 6b4d8b8.

This reverts commit 207c80a.

This reverts commit 9c71814.

c-pius · 2021-01-05T08:43:46Z

no worries, I also enjoyed some time off 😉 and happy new year!

I reverted the x-parser-original-schema and x-parser-original-payload content and added the check using x-parser-message-parsed flag in the validateAndConvertMessage() 👍🏻

derberg

lgtm, I just made suggestions to be clear about new functionality, that it is only for messages from components.

@magicmatatjahu anything from your side? can you approve?

lib/parser.js

magicmatatjahu · 2021-01-05T13:23:50Z

@derberg 8 new lines (without tests etc) with 42 comments? 😄 I'll check, give me some time to read whole thread.

derberg · 2021-01-05T13:27:19Z

@magicmatatjahu no worries mate, I don't think you have to read it through, we had a long discussion here as we were just looking for the best possible solution on how to run operations on messages just once) in case the message is in components and also ref-ed from the channel. But we are back to the beginning I would say, just introduced a new extension x-parser-message-parsed and that is it, so I would say just look at the code to make sure I did not overlook something because of our back and forth

magicmatatjahu

LGTM with new label :)

magicmatatjahu · 2021-01-05T13:42:04Z

lib/parser.js

  await customChannelsOperations(parsedJSON, asyncapiYAMLorJSON, initialFormat, options);
 }

 async function validateAndConvertMessage(msg, originalAsyncAPIDocument, fileFormat, parsedAsyncAPIDocument, pathToPayload) {
+  //check if the message has been parsed before
+  if (xParserMessageParsed in msg && msg[String(xParserMessageParsed)] === true) return;


It's only a question. (xParserMessageParsed in msg) === true will work?

in general yes, if (xParserMessageParsed in msg) would work as well as of now. the second check is to check whether the value of x-parser-message-parsed is also set to boolean value of true. if we omit this we would check the sheer presence of the flag and skip validation. would be okay for me as well. just thought the check for a boolean true value would make it more resilient

Co-authored-by: Lukasz Gornicki <[email protected]>

sonarqubecloud · 2021-01-05T14:02:24Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

c-pius · 2021-01-05T14:06:35Z

@derberg 8 new lines (without tests etc) with 42 comments? 😄 I'll check, give me some time to read whole thread.

well, sorry 😆 just some serious thinking going on here 🤓

asyncapi-bot · 2021-01-07T12:04:35Z

🎉 This PR is included in version 1.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

derberg · 2021-01-07T12:05:43Z

Finally released. Thanks @c-pius for this PR and patience in the discussion 😄

c-pius · 2021-01-07T13:53:51Z

awesome 🥳

thank you for guiding me and for the great work on this project in general 😄

* master: (24 commits) chore: Refactored code location for iterators (asyncapi#225) chore(release): 1.3.1 (asyncapi#222) fix: apply traits to standalone messages from components section (asyncapi#214) test: improve feedback loop from browser tests (asyncapi#216) ci: fix the space-issue in bump workflow (asyncapi#218) ci: update global workflows (asyncapi#217) chore(deps): bump ini from 1.3.5 to 1.3.7 (asyncapi#215) ci: rename pr testing job name and test node 14 (asyncapi#213) ci: bump workflow to start on push instead of release (asyncapi#212) chore(release): 1.3.0 (asyncapi#211) feat: add a traverse schema function (asyncapi#198) ci: add workflow that bumps parser in other asyncapi repos (asyncapi#208) ci: fix release workflow step that is responsible for handling twitter (asyncapi#209) ci: update global workflows (asyncapi#206) ci: disable any testing on draft PR (asyncapi#204) chore(release): 1.2.0 (asyncapi#202) feat: extend the components and asyncapi model with has-like functions (asyncapi#192) chore(deps-dev): bump semantic-release from 17.0.6 to 17.2.3 (asyncapi#199) chore(release): 1.1.1 (asyncapi#197) fix: channels with name '/' fail on validation (asyncapi#196) ...

c-pius added 2 commits December 10, 2020 17:53

fix: NOT YET WORKING apply traits to standalone messages

df0c326

applied linting rules

f4a59f5

github-actions bot reviewed Dec 10, 2020

View reviewed changes

magicmatatjahu requested changes Dec 10, 2020

View reviewed changes

lib/parser.js Show resolved Hide resolved

lib/parser.js Show resolved Hide resolved

derberg requested changes Dec 11, 2020

View reviewed changes

test/parse_test.js Outdated Show resolved Hide resolved

test/parse_test.js Outdated Show resolved Hide resolved

derberg mentioned this pull request Dec 11, 2020

test: improve feedback loop from browser tests #216

Merged

c-pius and others added 4 commits December 13, 2020 15:09

updated output json 1

7769b6e

Co-authored-by: Lukasz Gornicki <[email protected]>

updated output json 2

7ebd02d

Co-authored-by: Lukasz Gornicki <[email protected]>

guard on components and components.messages

c5e5ede

test for asyncapi without components object

f190db1

c-pius changed the title ~~fix: [WIP] apply traits to standalone messages~~ fix: apply traits to standalone messages Dec 13, 2020

c-pius marked this pull request as ready for review December 13, 2020 15:56

c-pius mentioned this pull request Dec 13, 2020

anonymous schema ids for standalone messages #219

Closed

c-pius added 2 commits December 13, 2020 17:26

Merge remote-tracking branch 'upstream/master' into fix/apply-traits-…

148a2a4

…to-standalone-messages

fixed test to include anonymous schema ids in schema objects

7cb782e

c-pius added 2 commits December 15, 2020 21:47

adding check for not validating messages twice

fe01eb7

convert to string instead of disabling eslint

88011c2

derberg reviewed Dec 16, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix/apply-traits-…

069a0bf

…to-standalone-messages

derberg reviewed Dec 23, 2020

View reviewed changes

c-pius added 4 commits January 5, 2021 09:00

Revert "default parser adding x-parser-original-payload"

341aa20

This reverts commit 6b4d8b8.

Revert "added comment on x-parser-original-schema-format"

5d191b0

This reverts commit 207c80a.

Revert "applying x-parser-original-schema-format in default parser"

159e574

This reverts commit 9c71814.

added x-parser-message-parsed flag

8bb2263

derberg requested changes Jan 5, 2021

View reviewed changes

lib/parser.js Outdated Show resolved Hide resolved

lib/parser.js Outdated Show resolved Hide resolved

lib/parser.js Outdated Show resolved Hide resolved

derberg changed the title ~~fix: apply traits to standalone messages~~ fix: apply traits to standalone messages from components section Jan 5, 2021

magicmatatjahu previously approved these changes Jan 5, 2021

View reviewed changes

magicmatatjahu reviewed Jan 5, 2021

View reviewed changes

Update lib/parser.js

ec9710c

Co-authored-by: Lukasz Gornicki <[email protected]>

c-pius dismissed magicmatatjahu’s stale review via ec9710c January 5, 2021 14:01

c-pius and others added 2 commits January 5, 2021 15:01

Update lib/parser.js

e6acb00

Co-authored-by: Lukasz Gornicki <[email protected]>

Update lib/parser.js

7b8db16

Co-authored-by: Lukasz Gornicki <[email protected]>

derberg approved these changes Jan 5, 2021

View reviewed changes

magicmatatjahu approved these changes Jan 7, 2021

View reviewed changes

derberg merged commit bb13b9d into asyncapi:master Jan 7, 2021

asyncapi-bot added the released label Jan 7, 2021

derberg mentioned this pull request Feb 3, 2021

x-parser-original-payload isn't the original payload with asyncapi schema parser #245

Closed

	message[String(xParserOriginalPayload)] = JSON.parse(JSON.stringify(payload));
	message[String(xParserOriginalPayload)] = Object.assign({}, payload);

fix: apply traits to standalone messages from components section #214

fix: apply traits to standalone messages from components section #214

Conversation

c-pius commented Dec 10, 2020

github-actions bot left a comment

Choose a reason for hiding this comment

magicmatatjahu left a comment

Choose a reason for hiding this comment

magicmatatjahu commented Dec 10, 2020

derberg left a comment

Choose a reason for hiding this comment

c-pius commented Dec 13, 2020

derberg commented Dec 14, 2020

c-pius commented Dec 14, 2020

derberg commented Dec 15, 2020

derberg commented Dec 15, 2020

c-pius commented Dec 15, 2020

derberg commented Dec 15, 2020

c-pius commented Dec 15, 2020

derberg commented Dec 16, 2020

Choose a reason for hiding this comment

derberg commented Dec 16, 2020

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

derberg commented Dec 23, 2020

c-pius commented Dec 24, 2020

derberg commented Jan 4, 2021

c-pius commented Jan 5, 2021

derberg left a comment

Choose a reason for hiding this comment

magicmatatjahu commented Jan 5, 2021

derberg commented Jan 5, 2021

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

sonarqubecloud bot commented Jan 5, 2021

c-pius commented Jan 5, 2021

asyncapi-bot commented Jan 7, 2021

derberg commented Jan 7, 2021

c-pius commented Jan 7, 2021