feat: add name and summary properties to message examples

[lbroudoux]

Description

• Provide formal type for message examples
• Complete message examples with name and summary attributes
• Tests have been updated/completed

Related issue(s)

This realizes to asyncapi/spec#329
New attributes are defined in asyncapi/spec#534
New schema implementation is in asyncapi/spec-json-schemas#54

[github-actions]

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a 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.

[derberg]

@lbroudoux hey, you created a PR against master but it should be the release branch. I changed it but I think you create a branch from master and now PR shows other files too. Can you just remove irrelevant parts from that PR?

[derberg]

Implementation is fine but the problem is that this PR introduces a breaking change, examples returns an array of MessageExample instead of an array of any so things like this will start failing. I don't think we can add MessageExample model 🤔 thoughts?

another pair of 👀 ? @fmvilas @jonaslagoni

Also would be great to have a test case for example with name or summary without payload, to test the new dependencies keyword from the json schema.

as for the bundle, it needs to stay in the PR, I tried to help here, and made sure it is unchanged but it is still showing up 🤷🏼

[fmvilas]

I don't get it. How is this failing if we're just adding summary and name?

[jonaslagoni]

I don't get it. How is this failing if we're just adding summary and name?

@fmvilas problem is we are changing the return type from any -> MessageExample meaning the check .find(e => e.payload) will fail since we no longer receive the examples as is JSON data, but a class wrapper with functions to access the data. i.e. it have to be changed to something like .find(e => e.payload()) or .find(e => e._json.payload).

[derberg]

@fmvilas sorry for picking wrong words, I actually assumed you will look into the code and read my entire comment 😛
Failing I mean that won't work as expected. Basically returned array from getPayloadExamples will have payload functions, so for example markdown and HTML templates will no longer display examples.

[jonaslagoni]

My short answer is I think we should trigger a major release because of this change.

[derberg]

@jonaslagoni this part I'm not sure, this is why I called you out. The example object already had 2 properties, headers and payload and we had no dedicated model for it. So adding name and summary does not mean we must add it. I think we could go with just adding tests and that is it 🤔 without adding specific model

[jonaslagoni]

@jonaslagoni this part I'm not sure, this is why I called you out. The example object already had 2 properties, headers and payload and we had no dedicated model for it. So adding name and summary does not mean we must add it. I think we could go with just adding tests and that is it 🤔 without adding specific model

Does not change the fact that we alter the structure of the return type, we change it from a property -> function. Which is a breaking change. If we want to leave out the model then sure, we could avoid it, but it would be a strange behavior compared to the rest of the implementation. Just to avoid major version change.

[fmvilas]

Still confused 🤔 I think this is a breaking change on the parser model not on the spec. The fact that payload is now a function instead of a property is a problem of the implementation. Can't we just continue using a plain JSON as before instead of a MessageExample model?

[magicmatatjahu]

We can treat it as normal JS object, as previous, not as new MessageExample instance and then everything should works :)

Basically returned array from getPayloadExamples will have payload functions, so for example markdown and HTML templates will no longer display examples.

@derberg Don't worry, I will fix it as fast as I can, when we will go with breaking change 😘

[derberg]

oh, where did I write it is a spec breaking change? It is a PR to parser, and I mean it is breaking change for the parser. Wanted your opinion to validate if I'm right that don't have to go with MessageExample

@jonaslagoni

Does not change the fact that we alter the structure of the return type, we change it from a property -> function

that's the point, we should not change return type

[jonaslagoni]

So you want to create a solution that we wouldn't normally do, just to avoid a major version change? Cause not doing this with models will create a weird behavior/structure.

[derberg]

@jonaslagoni

the current implementation is:

/**
   * @returns {any[]}
   */
  examples() {
    return this._json.examples;
  }

And examples are already a well-described array of objects, with payload and headers props.
We already should have a model for it, but we didn't.

So basically in my opinion this PR doesn't have to solve the old error and introduce a model. This PR is related to change in the spec that adds new props to examples. So the scope of this PR should only make sure that new fields are returned and properly validated (tests)

So you want to create a solution that we wouldn't normally do

so we are not creating anything new here. Lack of the model for examples can and should be done later in a separate Pr independent from the 2.1 release flow.

This is how I see it.

[jonaslagoni]

So basically in my opinion this PR doesn't have to solve the old error and introduce a model. This PR is related to change in the spec that adds new props to examples. So the scope of this PR should only make sure that new fields are returned and properly validated (tests)

Riiight, I see your point yea, then I agree, it should not introduce the model 🙂

[derberg]

@lbroudoux I think the conclusion from this discussion is clear. Please do not introduce MessageExample with this PR as it is going to be a breaking change. Would be great if you could create a followup issue about MessageExample and point to the commit where you have changes so someone doesn't have to write it in the future.

[lbroudoux]

@derberg I carefully followed the thread yesterday as I'm not a JS expert ;-)

I did not have the full history of the component nor the constraints you put on it. However I was wondering why we couldn't have a breaking change in parser implementation - even if this is not a breaking change in spec. Parser version is not indexed on spec version so we could have version bump here for supporting 2.1 of spec.

Anyway, as you suggested I'm gonna create an issue referring the commit for MessageExample implementation and will refine this PR to remove type information and just let validation / tests features.

[lbroudoux]

I may also rename this PR as it will no longer provide formal type but just ensure untyped object attributes are there.

[derberg]

However I was wondering why we couldn't have a breaking change in parser implementation - even if this is not a breaking change in spec. Parser version is not indexed on spec version so we could have version bump here for supporting 2.1 of spec.

not sure what perspective others have. My perspective is, we do not yet properly test all the generator templates, but on the other hand, we have a mechanism that automatically bumps Parser in all the packages that use it, so this major release would trigger releases of all AsyncAPI tools and next days after the release we would do the firefighting. So I'm just pragmatic here and prefer to do a controlled burn later once we are secured on all the edges 😄 So yeah, it is not that I'm afraid of breaking changes, I actually love to release regularly, no matter major or minor, it's just that I don't want to release if I'm sure it will cause failures 🤷🏼

[lbroudoux]

Yeah. Totally agree on the pragmatic approach to avoid firefighting 😉 What's why without having the full context in mind as you have I cannot take part of the decision process. I've already created the issue #321 to keep trace on this and will remove type from the PR asap.

[derberg]

@lbroudoux no worries mate.
I recommend we get asyncapi/spec-json-schemas#54 merged first and it will trigger the release of release candidate for the spec package with the latest JSON Schema. Then you can use it here to work on the tests I mentioned in one of my comments. Of course this is just recommendation, you can also just modify @asyncapi/spec entry in package.json to point to https://github.com/lbroudoux/asyncapi-node#message-examples to already work on tests

[lbroudoux]

I've added some tests locally using the spec from https://github.com/lbroudoux/asyncapi-node#message-examples and they're validation the required/optional rules from new schema. They're also validating invalid spec as well - no payload - but maybe I'll have update asyncapi/spec-json-schemas#54 since https://github.com/asyncapi/asyncapi-node/pull/54/files#r656823461 😉

[lbroudoux]

Tests have been completed to check the either / or requirement rule on payload and headers. I'll redo a check when release candidate will be available and update this PR just after.

[derberg]

@lbroudoux you know you didn't push anything right?

[lbroudoux]

Yes it was just a notice. I'll keep changes locally until the release candidate is ready as you suggested.

[derberg]

@lbroudoux RC is out https://www.npmjs.com/package/@asyncapi/specs/v/2.8.0-2021-06-release.3

[lbroudoux]

@derberg Now updated!

[derberg]

@lbroudoux code-wise - perfecto! Just please fix the failing linter, remove this single vars

[lbroudoux]

Dammit! I forgot a last npm lint...

[derberg]

@lbroudoux and don't forget to change the PR title, same as on other PRs in other repos would be best

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[lbroudoux]

Yep. Now done. All PRs now relates to adding name and summary properties to message examples.

[derberg]

LGTM 🚀

[magicmatatjahu]

LGTM! 🚀

[asyncapi-bot]

🎉 This PR is included in version 1.6.0-2021-06-release.1 🎉

The release is available on:

• npm package (@2021-06-release dist-tag)
• GitHub release

Your semantic-release bot 📦🚀