feat: allow servers and channels to be fetched from components #430

smoya · 2021-12-16T16:58:39Z

Note that this PR is targeting asyncapi:2022-01-release branch, so merging it won't release a new version.

Description
This PR is part of asyncapi/spec#660.
It allows both servers and channels to be retrieved from components, meaning the following will be now valid:

servers:
  production:
    $ref: '#/components/servers/myserver'
channels:
  some/events:
    $ref: '#/components/channels/myChannel'
components:
  servers:
    myserver:
      url: "http://localhost:5000/ws"
      protocol: ws
  channels:
    myChannel:
      description: "mychannel"

Related issue(s)
asyncapi/spec#660

github-actions

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide 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.

smoya · 2021-12-16T17:20:26Z

lib/models/asyncapi.js

+
+    if (this.hasComponents()) {
+      Object.entries(this.components().channels()).forEach(([key, value]) => {
+        // TODO This means that if two channels have the same name (but located in different place), the first one will be overwritten.


Perhaps the assumption is wrong? Am I missing something?

This code is based on current allMessages() function code

WDYT about this?

Messages as saved with uid based on

parser-js/lib/models/message.js

Line 17 in 2094035

return this.name() || this.ext('x-parser-message-name') || Buffer.from(JSON.stringify(this._json)).toString('base64');

so you can end with overrides. Hmm 🤔 In the case of channels you should name it with url path, so you can end with duplication but I think that your spec will be invalid - I mean two url for that same event - so here we can accept your solution.

In the case of servers, we should collect them by urls, not by names, because in components and servers they may have different names but they will be the same servers. It would be best to add servers from the #/servers section first and then check the url of servers from #/components/servers to see if such a server already exists, if it does then don't add the new item to the Map :)

Having the following example doc:

asyncapi: 2.2.0 info: title: test version: 1.0.0 servers: test: url: 'test.asyncapi.com' protocol: ws channels: /: subscribe: message: $ref: '#/components/messages/testMessage' components: messages: testMessage: {} channels: /: publish: message: $ref: '#/components/messages/testMessage'

Don't you think we should keep both / channels instead of overwriting one of them? They can be totally different channels. As channels don't have any extra property such name or id (related asyncapi/spec#614) I don't think we should overwrite them.

And I would say the same issue happens with servers if we consider a same URL can be used for let's say different protocols, URL field is not enough for making a server unique.

maybe it should be delegated to the spec, and spec should require unique names for server and channel across document, as it is with correlationId?

@fmvilas thoughts? it is related to asyncapi/spec#660

Regarding implementation here... From what I see, two servers will be equal only if all their properties are equal. Using only URL field is not enough because you can have two servers with the same URL but different protocols.

Does it make sense to compare the whole object to determine if they should be merged? What should we do if they are not equal but share the same name? Shall we use some kinda prefix? cc @derberg @magicmatatjahu

Agree with Lukasz. I'd not add them for now. Let's make sure this is considered for v3 and then we can work back a solution for v2.x documents.

I think that since there is code already that uses asyncapi.allMessages() this is needed. See https://github.com/search?q=org%3Aasyncapi+asyncapi.allMessages%28%29&type=code. Maybe those templates don't need that method and they should change their call to asyncapi.Messages()? Not sure, maybe you @magicmatatjahu or @derberg or @fmvilas can answer that.

In fact, if we move forward, I realize now I would need to create a PR on those repositories changing all asyncapi.Channels() to asyncapi.AllChannels() and the same for servers then.

Can you rephrase your question? I did not get it.

allMessages is indeed needed, so this is why it was added and is used in templates. This should stay as it is. There is a use case for it, as some folks simply do not use channels, and have messages only under components.

What we mean here is that Servers and Channels are new. We know we need to access them using regular helpers. So technically speaking, we need to have Servers and Channels part of Components model as you did it in this PR, and we can drop allChannels and allServers implementation for now, as it is nice to have, and can be added later when we really need it.

I tried to answer your questions without clarification from your side, but I'm not 100% sure I did it? Thoughts?

Ok, I didn't think we could access those channels and servers without those new methods but indeed you can always get them via asyncapi.components(). Then I agree it is fine to drop those new methods.

I tried to answer your questions without clarification from your side, but I'm not 100% sure I did it? Thoughts?

Yes you did 👍

Ok @magicmatatjahu @fmvilas @derberg allChannels and allServers functions has been removed from my PR now.

derberg · 2021-12-28T16:12:39Z

/help

asyncapi-bot · 2021-12-28T16:12:52Z

Hello, @derberg! 👋🏼

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

derberg · 2022-01-03T12:23:48Z

2 more comments from my side, before you merge, do not forget:

we do not update bundle on PR, but it will be anyway overwritten during release so maybe we can ignore
remember to bump spec schema dependency to the release candidate before merging

smoya · 2022-01-05T13:08:54Z

remember to bump spec schema dependency to the release candidate before merging

Done:

"@asyncapi/specs": "asyncapi/spec-json-schemas#2022-01-release",

we do not update bundle on PR, but it will be anyway overwritten during release so maybe we can ignore

We can leave it here right? Or should I remove it?

fmvilas · 2022-01-07T17:38:59Z

We can leave it here right? Or should I remove it?

I'd remove it as it could be a source of git conflicts.

smoya · 2022-01-07T17:42:45Z

We can leave it here right? Or should I remove it?

I'd remove it as it could be a source of git conflicts.

It's done!

fmvilas

🚀

derberg · 2022-01-11T09:56:09Z

package.json

@@ -67,7 +67,7 @@
  },
  "dependencies": {
    "@apidevtools/json-schema-ref-parser": "^9.0.6",
-    "@asyncapi/specs": "^2.11.0",
+    "@asyncapi/specs": "asyncapi/spec-json-schemas#2022-01-release",


@fmvilas are you sure we should grab sources from github?
imho we should either pull a specific version from npm 2.13.0-2022-01-release.1 or just fix to the release candidate tag, so 2022-01-release

Question: If we use 2022-01-release as version, it will point to the same code as latest commit in that branch? So it is the same as I added but pointing to the npm package tag instead of github branch, right? If so, I can change to it as IMHO it also makes sense as you said.

technically if you point to 2022-01-release and you do npm install then @asyncapi/specs version 2.13.0-2022-01-release.1 will be installed, if 2 release candidate is available, then 2.13.0-2022-01-release.2 will be installed.

so yeah, might be that the best solution is to point to the 2022-01-release npm tag instead of a specific version each time

…rsChannelsAsComponents

derberg

/dnm

derberg · 2022-01-13T11:10:43Z

@smoya yo, https://github.com/asyncapi/spec-json-schemas/releases/tag/v2.13.0-2022-01-release.3 is out, so you either need to just regenerate package-json or in package.json also point directly to v2.13.0-2022-01-release.3, more intuitive and straightforward indicator that this needs to be updated with every release candidate

smoya · 2022-01-13T11:18:50Z

@smoya yo, https://github.com/asyncapi/spec-json-schemas/releases/tag/v2.13.0-2022-01-release.3 is out, so you either need to just regenerate package-json or in package.json also point directly to v2.13.0-2022-01-release.3, more intuitive and straightforward indicator that this needs to be updated with every release candidate

@asyncapi/specs updated to v2.13.0-2022-01-release.3 is now a thing 🚀

sonarqubecloud · 2022-01-13T11:19:15Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

derberg · 2022-01-13T11:20:27Z

/rtm

asyncapi-bot · 2022-01-31T16:49:52Z

🎉 This PR is included in version 1.14.0-2022-01-release.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

smoya changed the base branch from master to 2022-01-release December 16, 2021 16:58

github-actions bot reviewed Dec 16, 2021

View reviewed changes

smoya force-pushed the feat/serversChannelsAsComponents branch from 04309f5 to 2094035 Compare December 16, 2021 17:04

smoya marked this pull request as ready for review December 16, 2021 17:06

smoya mentioned this pull request Dec 16, 2021

Allow servers and channels to be defined inside components asyncapi/spec#660

Closed

smoya changed the title ~~feat: allow servers and channels as components~~ feat: allow servers and channels to be fetched from components Dec 16, 2021

smoya commented Dec 16, 2021

View reviewed changes

smoya added 2 commits December 28, 2021 12:29

feat: allow servers and channels to be defined as components

293129c

allServers() and allChannels() at doc level

5bae790

smoya force-pushed the feat/serversChannelsAsComponents branch from 2094035 to 5bae790 Compare December 28, 2021 11:29

smoya added 2 commits December 28, 2021 13:42

handle servers collision

93c4e67

handle channels collision

fbdcc83

smoya requested review from derberg and magicmatatjahu December 28, 2021 12:57

smoya added 2 commits January 5, 2022 13:58

drop allChannels and allServers

6e481bd

asyncapi/specs package points to 2022-02-release branch

0d8171a

smoya added 2 commits January 7, 2022 18:40

revert bundle.js

776c950

revert bundle.js

37686f4

fmvilas approved these changes Jan 7, 2022

View reviewed changes

derberg reviewed Jan 11, 2022

View reviewed changes

smoya added 2 commits January 11, 2022 14:14

asyncapi/specs package points to 2022-02-release tag

a6ca9ff

Merge remote-tracking branch 'origin/2022-01-release' into feat/serve…

c508732

…rsChannelsAsComponents

smoya requested review from derberg and fmvilas January 12, 2022 10:58

derberg approved these changes Jan 12, 2022

View reviewed changes

fmvilas approved these changes Jan 12, 2022

View reviewed changes

update @asyncapi/specs to v2.13.0-2022-01-release.3

68f9f2d

asyncapi-bot added the ready-to-merge label Jan 13, 2022

asyncapi-bot merged commit 2f465bb into asyncapi:2022-01-release Jan 13, 2022

smoya deleted the feat/serversChannelsAsComponents branch January 13, 2022 11:21

asyncapi-bot added the released on @2022-01-release label Jan 31, 2022

github-actions bot approved these changes Jan 31, 2022

View reviewed changes

asyncapi-bot added the autoapproved label Jan 31, 2022

github-actions bot approved these changes Jan 31, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow servers and channels to be fetched from components #430

feat: allow servers and channels to be fetched from components #430

smoya commented Dec 16, 2021

github-actions bot left a comment

smoya Dec 16, 2021 •

edited

Loading

magicmatatjahu Dec 17, 2021

smoya Dec 17, 2021

derberg Dec 21, 2021

smoya Dec 28, 2021 •

edited

Loading

fmvilas Jan 3, 2022

smoya Jan 4, 2022 •

edited

Loading

derberg Jan 5, 2022

smoya Jan 5, 2022

smoya Jan 5, 2022

derberg commented Dec 28, 2021

asyncapi-bot commented Dec 28, 2021

derberg commented Jan 3, 2022

smoya commented Jan 5, 2022

fmvilas commented Jan 7, 2022

smoya commented Jan 7, 2022

fmvilas left a comment

derberg Jan 11, 2022

smoya Jan 11, 2022

derberg Jan 11, 2022

smoya Jan 11, 2022

derberg left a comment

derberg commented Jan 13, 2022

smoya commented Jan 13, 2022 •

edited

Loading

sonarqubecloud bot commented Jan 13, 2022

derberg commented Jan 13, 2022

asyncapi-bot commented Jan 31, 2022

feat: allow servers and channels to be fetched from components #430

feat: allow servers and channels to be fetched from components #430

Conversation

smoya commented Dec 16, 2021

github-actions bot left a comment

Choose a reason for hiding this comment

smoya Dec 16, 2021 • edited Loading

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

smoya Dec 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoya Jan 4, 2022 • edited Loading

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 28, 2021

asyncapi-bot commented Dec 28, 2021

derberg commented Jan 3, 2022

smoya commented Jan 5, 2022

fmvilas commented Jan 7, 2022

smoya commented Jan 7, 2022

fmvilas 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

derberg left a comment

Choose a reason for hiding this comment

derberg commented Jan 13, 2022

smoya commented Jan 13, 2022 • edited Loading

sonarqubecloud bot commented Jan 13, 2022

derberg commented Jan 13, 2022

asyncapi-bot commented Jan 31, 2022

smoya Dec 16, 2021 •

edited

Loading

smoya Dec 28, 2021 •

edited

Loading

smoya Jan 4, 2022 •

edited

Loading

smoya commented Jan 13, 2022 •

edited

Loading