-
-
Notifications
You must be signed in to change notification settings - Fork 280
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
docs: add constraint about channel address to not use query params and… #986
docs: add constraint about channel address to not use query params and… #986
Conversation
@@ -629,7 +629,7 @@ Describes a shared communication channel. | |||
|
|||
Field Name | Type | Description | |||
---|:---:|--- | |||
<a name="channelObjectAddress"></a>address | `string` \| `null` | An optional string representation of this channel's address. The address is typically the "topic name", "routing key", "event type", or "path". When `null` or absent, it MUST be interpreted as unknown. This is useful when the address is generated dynamically at runtime or can't be known upfront. It MAY contain [Channel Address Expressions](#channelAddressExpressions). | |||
<a name="channelObjectAddress"></a>address | `string` \| `null` | An optional string representation of this channel's address. The address is typically the "topic name", "routing key", "event type", or "path". When `null` or absent, it MUST be interpreted as unknown. This is useful when the address is generated dynamically at runtime or can't be known upfront. It MAY contain [Channel Address Expressions](#channelAddressExpressions). Query parameters and fragments SHALL NOT be used, instead use [bindings](#channelBindingsObject) to define them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this instead?
<a name="channelObjectAddress"></a>address | `string` \| `null` | An optional string representation of this channel's address. The address is typically the "topic name", "routing key", "event type", or "path". When `null` or absent, it MUST be interpreted as unknown. This is useful when the address is generated dynamically at runtime or can't be known upfront. It MAY contain [Channel Address Expressions](#channelAddressExpressions). Query parameters and fragments SHALL NOT be used, instead use [bindings](#channelBindingsObject) to define them. | |
<a name="channelObjectAddress"></a>address | `string` \| `null` | An optional string representation of this channel's address. The address is typically the "topic name", "routing key", "event type", or "path". When `null` or absent, it MUST be interpreted as unknown. This is useful when the address is generated dynamically at runtime or can't be known upfront. It MAY contain [Channel Address Expressions](#channelAddressExpressions). HTTP query parameters and fragments SHALL NOT be used, instead use the [HTTP binding](https://github.com/asyncapi/bindings/tree/master/http#operation-binding-object) in the operations to define them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied the same sentence with had in v2
thing is that it is not only about HTTP, Websocket also have query params, and you never know what else might pop up in future, so I don't think we should be that specific as you propose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WebSocket doesn't have query params. It's the HTTP endpoint you have to reach for protocol upgrade. That's why I'm being specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you establish connection over HTTP, yes, but in AsyncAPI document you do not define separate server with HTTP protocol and binding. You specify server with websocket protocol and websocket binding https://github.com/asyncapi/bindings/tree/next-major-spec/websockets#channel-binding-object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right 👍 Ok, so let's not be that specific then. Even though, WS binding properties are about HTTP but that's fine.
@jonaslagoni fyi, I don't think it is changing anything from any tooling perspective, release date or stuff like that. Also nothing for release notes and migration guide |
Yep, agree 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Kudos, SonarCloud Quality Gate passed! |
/rtm |
e76206a
into
asyncapi:next-major-spec
🎉 This PR is included in version 3.0.0-next-major-spec.17 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
in v3
address
is what in v2 we know as channel key/namein v2 we had clear info about not using query params and fragments: https://github.com/asyncapi/spec/blob/master/spec/asyncapi.md#user-content-channels-object
during works on v3 this was lost
Confirmed in Slack it was lost by mistake, not intentionally, thus bringing it back