-
-
Notifications
You must be signed in to change notification settings - Fork 276
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
Let channels be identified by an ID rather than their address. #663
Comments
I love the move to a property, whether What exactly is the reason we need an ID rather than making channels an array? Especially now that #665 is added. |
I guess the reason is that we might also allow people to reference channels not only from components but from |
Okay I retract my suggestion, you are right. Especially if you want to define two channels operating over the same address, with slightly different behavior, you would need to add a |
The following PR's have been created: |
Hi everybody, I was CCed in asyncapi/spec-json-schemas#171, so let me share my opinion as well here. IMHO I can see a couple of problems here:
This means that components:
channels:
channel1:
description: channel description
channels:
user/signup:
$ref: "#/components/channels/channel1" As you can clearly see, the The only place where this is problematic is in following fragment: channels:
user/logout:
$ref: "#/channels/user~1signedup"
user/signedup:
description: channel description But as we now have reusable |
Please look at this proposal from point of view person who need to write several AsyncAPI documents and use that same channel in few documents. Of course can make reference by: $ref: "#/components/channel1" but then needs to remember that (e.g.) "user/signup" channel address, yes? :) So that PR adds possibility to make more easier the "channels registry" and treats address as Please read #618 proposal to understand why we wanna make it :) Of course "channel registry" is not the only reason, but it is one of many, perhaps the best why it makes sense. |
@magicmatatjahu I really did that. Where I want to get is to have AsyncAPI spec version where the only required fields are reusable.yaml asyncapi: 2.3.0
info:
title: reusable document fragment
version: 1.0.0
components:
channels:
channel1:
description: channel description Then it will be consumed by other definitions: definition1.yaml asyncapi: 2.3.0
info:
title: definition 1
version: 1.0.0
channels:
path/to/channel:
$ref: "./reusable.yaml/#components/channels/channel1" definition2.yaml asyncapi: 2.3.0
info:
title: definition 2
version: 1.0.0
channels:
path/to/channel:
$ref: "./reusable.yaml/#components/channels/channel1" This is currently not possible as
#618 is super long ;] I'm trying to look at this change introduced in this issue in isolated way. People have full ability to create keys as they seem fit and can use a lot of imagination to come up with the key names that are sometimes exotic. This issue IMHO tries to resolve one particular issue which is resolvable by other means (small change in spec + recommended practices). |
Yeah, exactly, currently it's not possible. Changing it is easy (to make channels options) but we need for that Maybe I don't understand the second paragraph (sorry!), but a channel key and an address are two completely different things in my opinion. I understand that the keys may be different, but this should be just "information" for the developer, not for a user reading the documentation. It's a bit like a server url - it may have a different key but url identifies its "uniqueness" and so it is with the channel address.
And this is probably the problem, because it is better to solve this at the level of specification and not through recommended practices. There are things that should be recommended, but if there is an option to fix it in the specification itself, then let's go the second way. As I wrote, currently the problem are references and people then have to remember that a given channel should have a given key in the Of course, lets wait for others opinion, but I understand in this way the changes that @fmilas proposed and @smoya introduced :) |
Additionally, one may wonder how to overwrite such an address if we want to have something like a channel but without the address? We can create a |
Don't worry; It's definitely me (not being clear). What I tried to say was this: asyncapi: 2.3.0
info:
title: reusable document fragment
version: 1.0.0
components:
channels:
user/signup:
description: channel description
channels:
user/signup:
$ref: #/components/channels/user~1signup People can still use (and probably will) The last argument I have is about semantics: Given definition 1: channels:
userSignedUp:
address: user/signedup
description: ... And definition 2: channels:
userSignedUp:
$ref: "./definition1.yaml/#/channels/userSignedUp" When I change definition 1 to: channels:
userSignedUp:
address: user/loggedout
description: ... Then resolution/dereferencing/bundling will still work The current status quo of |
Hi guys, I was thinking about this some more during the night, and looking at this change through the lens of different persona - as a tooling author (not definition writer) I actually like this change. This changes makes And I want to thank you for giving me the opportunity to voice my opinions here (through two different personas - definitions author and tooling author) and sorry for hijacking this discussion a bit. |
Your opinion is super important on this (and also the opinion of anyone interested in the topic). I explicitly pinged you in the PR because I knew you had feedback to provide and most precisely, you were willing to provide it, which to be honest, in my mind, this is just awesome. So please keep providing feedback around any PR you see because I think this is what it really matters on this community. In my case, whenever I open a new RFC, I'm looking actively for "Noes" rather than "Yes"; for "This is not gonna work" rather than "oh cool LGTM", you know what I mean :). Thanks for your extremely valuable help here @char0n ! |
Feedback (with other point of view) is very important and there are no apologies for it :) The more "no" votes the better because then we know from additional use cases. Referring to the change itself: I wonder if when we have an asyncapi: 3.0.0
info: ...
channels:
- $ref: '#/components/channels/someChannel' Of course |
Why wouldn't just look like |
Currently Of course that suggestion has problem with referencing when you want to make ref to Benefits:
|
This would IMHO make referencing even more difficult as people often change their specification and reorder stuff to make more sense or to look better. With using list semantics the indexes will change by reordering the internals of the channels:
- address: path/to/channel1
- address: path/to/channel2 Pointer to channe1: JSON Pointer: #/channels/0 channels:
- address: path/to/channel2
- address: path/to/channel1 Pointer to channe1: JSON Pointer: #/channels/1 |
I agree @char0n on this. Also I want to emphasize the fact that channels need a identifier. This is key, and one of the reasons behind this change. Read my comment here #663 (comment) |
This is a great discussion and it sounds like a good idea. Any chance we could have a brief real-time discussion of this, either at a v3 meeting or a dedicated tech discussion? |
I would also be interested of the impact of this on #618 , especially the reference between operations and channels, which is proposed to not be a JSON ref. That seems suboptimal to me, from an understanding and tooling perspective.
|
I like this a lot, especially because it'll help reusability, i.e. breaking models down into reusable files. |
Good point! I think that we have "little" problem here, however proposal proposed in this issue does not contradict the above sentence. If you want to use the operation, you must have a channel defined in the operations:
sendUserSignedUpEvent:
channel:
$ref: #/channels/someChannel
action: send then after dereferencing we have the channel object and not its identifier. To check if a given channel is in the context of the described documentation (in the |
Of course, referencing can be a problem if we want to treat operations and channels in a registry fashion. Then we have to remember about those keys in the dictionary between files and I wonder if the operation and channel identifiers should be defined inside the object and not outside, but it's my little digression which we should discuss. cc @smoya @fmvilas |
In my brain, IDs should be unique across a document. So yes, it is a problem in the right moment two different objects share the same id. |
But they don't share the same ID. In your example: channels:
channelA:
$ref: '#/components/channels/aChannel' # --> ID here is channelA
channelB:
$ref: '#/components/channels/aChannel' # --> ID here is channelB
components:
channels:
aChannel:
# channel object # --> ID here is aChannel |
As per your comment in #663 (comment), the id will come from the JSON pointer. So it will be: channels:
channelA:
$ref: '#/components/channels/aChannel' # --> ID here is aChannel
channelB:
$ref: '#/components/channels/aChannel' # --> ID here is aChannel
components:
channels:
aChannel:
# channel object # --> no id unless... manually added? |
Yeah sorry, my fault. It should come from the context when possible, not from the JSON pointer. I think it makes sense to take it from the context, otherwise, why would we bother having |
If we make it mandatory to put channel definitions inside channels:
- channelA
- channelB
operations:
sendSomething:
action: send
channel: channelA
components:
channels:
channelA:
# channel object or $ref
channelB:
# channel object or $ref The channel ID comes from the context. In this case, from the key in Benefits
Drawbacks
If we use channels:
channelA:
$ref: '#/components/channels/aChannel'
channelB:
$ref: '#/components/channels/aChannel'
operations:
sendSomething:
action: send
channel:
$ref: '#/channels/channelA'
components:
channels:
aChannel:
# channel object or $ref Still, the channel ID comes from the context. In this case, from the key in Benefits
Drawbacks
|
For the record, I want to raise a problem with our spec that is currently a thing, and that might be possible to fix with the first suggestion @fmvilas made or actually any approach that moves definitions to live under components and be referred from an array of IDs (or JSON Pointers): servers:
c: ...
d: ...
components:
channels:
someChannel:
servers: [c, d] # do we refer to the servers located under components or rather in the root of the document?
servers:
c: ...
d: ... With Servers becoming an array of Ids, it gets fixed: servers:
- c
- d
components:
channels:
someChannel:
servers: [c, d] # c and d servers refer to servers defined always in components.
servers:
c: ...
d: ... Also with Servers becoming an array of JSON Pointers: servers:
- '#/components/servers/c'
- '#/components/servers/d'
components:
channels:
someChannel:
servers: [c, d] # c and d servers refer to servers defined always in components.
servers:
c: ...
d: ... However, we could also fix it by using JSON Pointers for all Ids instead, however you will see that the fact servers:
c: # ...
d: # ...
components:
channels:
someChannel:
servers:
- '#/components/servers/c'
- '#/components/servers/d'
servers:
c: ...
d: ... |
@smoya regarding your comment about problems with spec and
so nobody should expect that ids in an array of I have to admit that after holidays it is pretty difficult to catch up with discussion in this issue as I have a feeling there are 2 different but related topics. And this is why I think there is some confusion in the discussion too.
AD1
@smoya you are referring to a comment from @fmvilas but not related to
so, on regards to
AD2So looking again at
@fmvilas so we are thinking about some solution, that after parsing document and dereferencing, it looks like below, more or less:
and then we have a model function that can return if yes, then it doesn't sound as bad as I thought at the beginning. It would be up to parsers to implement their way of getting the channel Id. I think the most important rule that we have in our contribution guide is consistency. So approach with using am I missing something? |
@derberg no man, I think you're pretty much aligned with the discussion.
Well, yeah, I get what you say but also understand Sergio's point of view. Unless you read the whole spec (which is unlikely) you could probably expect these IDs are coming from components. But yeah, nothing really bad and I know we can't be fixing problems for those who don't read docs, right? Especially, when it's a spec. I think we can agree that referencing using |
We can use |
I created a separate discussion for that topic: #829. |
This issue has been automatically marked as stale because it has not had recent activity 😴 It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation. There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model. Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here. Thank you for your patience ❤️ |
Getting there dear bot. Be patient. |
This is already done 🙂 |
This issue has been automatically marked as stale because it has not had recent activity 😴 It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation. There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model. Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here. Thank you for your patience ❤️ |
But please bot do not close it yet until v3 is out 🙏 |
This issue has been automatically marked as stale because it has not had recent activity 😴 It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation. There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model. Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here. Thank you for your patience ❤️ |
Context
This is part of the RFC #618 (comment).
In an effort around reducing the scope of such RFC, it has been split into several proposals so they can move forward independently.
The problem
Channels are currently identified by their address. E.g.:
With the current approach, and once #660 happens, we will have few issues:
When referencing a channel, the channel identifier should match with the refereced one, otherwise the address will be different than in the referenced channel. E.g.:
That introduces a problem when we need to change the address of a shared channel. Let's say Kafka topic name changes. With the current approach, we need to change the address on all files that reference the
user/signedup
channel fromcommon.yaml
(as JSON Reference will fail as well).As you may noticed, the JSON Pointer for
user/signedup
channel isuser~1signedup
. It is unexpected and you have to have some JSON Pointer knowledge or find examples to find out such format. It is very error-prone.Suggestion
To add a new
address
field to the channel definition that represents the real address and not the channel identifier.With that change, the previous example will look like:
Fixes #739 as well
The text was updated successfully, but these errors were encountered: