Skip to content
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

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

derberg
Copy link
Member

@derberg derberg commented Nov 7, 2023

in v3 address is what in v2 we know as channel key/name

in 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

Lukasz Gornicki: 
  in spec v2 channel name was both, name and address and documentation specified: Query parameters and fragments SHALL NOT be used, instead use bindings to define them.
  in spec v3 channel name is something different, and we have dedicated address property, and it’s description no longer says that query params should not be used.

  cc Fran Méndez. Do you remember if it was intentionally removed or lost in translation? I don’t remember what was the reason in v2 to not allow query params in the name :thinking_face: was it purely technical reason, to not have weird names? :thinking_face:


Fran Méndez:
  I think it was lost in translation. The reason for not having query params in channel addresses is that query params are purely an HTTP thing and that's why it was separated.

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this instead?

Suggested change
<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.

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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.

@derberg
Copy link
Member Author

derberg commented Nov 7, 2023

@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

@derberg derberg changed the title fix: add constraint about channel address to not use query params and… docs: add constraint about channel address to not use query params and… Nov 7, 2023
@jonaslagoni
Copy link
Member

@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 👍

Copy link
Collaborator

@char0n char0n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@derberg
Copy link
Member Author

derberg commented Nov 13, 2023

/rtm

@asyncapi-bot asyncapi-bot merged commit e76206a into asyncapi:next-major-spec Nov 13, 2023
16 of 17 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.0.0-next-major-spec.17 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants