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

feat: add new channels object #827

Merged
merged 11 commits into from
Sep 21, 2022

Conversation

fmvilas
Copy link
Member

@fmvilas fmvilas commented Aug 27, 2022

Related issue(s):
#663
#682
#618

@fmvilas
Copy link
Member Author

fmvilas commented Aug 28, 2022

@GreenRover this PR should unblock your request/reply work.

spec/asyncapi.md Outdated Show resolved Hide resolved
@fmvilas
Copy link
Member Author

fmvilas commented Sep 6, 2022

/progress 60

New channels shape is defined but it's still pending review by code owners.

@fmvilas
Copy link
Member Author

fmvilas commented Sep 7, 2022

@derberg @dalelane @smoya would you mind having a look?

@@ -162,7 +162,7 @@ Field Name | Type | Description
<a name="A2SInfo"></a>info | [Info Object](#infoObject) | **Required.** Provides metadata about the API. The metadata can be used by the clients if needed.
<a name="A2SServers"></a>servers | [Servers Object](#serversObject) | Provides connection details of servers.
<a name="A2SDefaultContentType"></a>defaultContentType | [Default Content Type](#defaultContentTypeString) | Default content type to use when encoding/decoding a message's payload.
<a name="A2SChannels"></a>channels | [Channels Object](#channelsObject) | **Required** The available channels and messages for the API.
<a name="A2SChannels"></a>channels | [Channels Object](#channelsObject) | The channels used by this [application](#definitionsApplication).
Copy link
Member Author

Choose a reason for hiding this comment

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

Notice channels is not required anymore.

spec/asyncapi.md Outdated Show resolved Hide resolved
<a name="channelItemObjectPublish"></a>publish | [Operation Object](#operationObject) | A definition of the PUBLISH operation, which defines the messages consumed by the application from the channel.
<a name="channelItemObjectParameters"></a>parameters | [Parameters Object](#parametersObject) | A map of the parameters included in the channel name. It SHOULD be present only when using channels with expressions (as defined by [RFC 6570 section 2.2](https://tools.ietf.org/html/rfc6570#section-2.2)).
<a name="channelItemObjectBindings"></a>bindings | [Channel Bindings Object](#channelBindingsObject) \| [Reference Object](#referenceObject) | A map where the keys describe the name of the protocol and the values describe protocol-specific definitions for the channel.
<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).
Copy link
Member Author

Choose a reason for hiding this comment

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

Address is no longer a URL template and, instead, it's just a plain string that supports Channel Address Expressions.

spec/asyncapi.md Outdated Show resolved Hide resolved
spec/asyncapi.md Show resolved Hide resolved


Using explicit by-name references to the servers on which the channel is available:
#### <a name="channelAddressExpressions"></a>Channel Address Expressions
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 went all-in defining our own templating rules here. I don't know what I was thinking when I defined the channel address as a URL template in version 2.0.0 😅 Have a look: https://www.rfc-editor.org/rfc/rfc6570#section-2.4.2.

spec/asyncapi.md Outdated

Expressions MUST be composed by a name enclosed in curly braces (`{` and `}`). E.g., `{userId}`.

When the channel address contains [delimiters](#channelObjectAddressDelimiter), the expressions MUST be surrounded by either the delimiters, the beginning of the string, or the end of the string. E.g., assuming a `.` (dot) delimiter, `users.{userId}` is correct but `users.id{userId}` is not.
Copy link
Member Author

Choose a reason for hiding this comment

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

This is covering most of the protocols supporting any form of wildcards: AMQP, MQTT, NATS, etc. It's still missing a Kafka feature that lets you subscribe to a regular expression. I tried to map it here but it's too complex and therefore left it out.

If you got any ideas on how to do it, I'm all ears. Otherwise, I'll open a follow-up issue and this can be included in future versions.


If you're curious, I came up with a solution like users_{userId:/[\w\d]{8}/i} but quickly got trapped in the differences between regular expression engines hole.

Kafka protocol doesn't define anything so it's hard for us to make a commitment here. Maybe go for the engine with fewer features, just to make sure everything is supported in every engine? Coming up with a subset ourselves? Should we just forget about it? Hot topic!

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

Left some comments

3.0 is materializing ❤️

spec/asyncapi.md Outdated Show resolved Hide resolved
spec/asyncapi.md Outdated Show resolved Hide resolved
spec/asyncapi.md Outdated Show resolved Hide resolved
spec/asyncapi.md Outdated Show resolved Hide resolved
spec/asyncapi.md Show resolved Hide resolved
@fmvilas
Copy link
Member Author

fmvilas commented Sep 9, 2022

@derberg I addressed all the feedback. Let me know what you think now.

@fmvilas fmvilas requested a review from jonaslagoni September 12, 2022 10:33
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

LGTM

again, info, once we merge, we release a release candidate 😉

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Just had a few remarks, otherwise looks good 👍


Describes the operations available on a single channel.
Describes a shared communication channel.
Copy link
Member

Choose a reason for hiding this comment

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

A shared communication channel with who? Am I the only one who feels like this sentence is incomplete in some way... 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't know in advance. We only know it's gonna be shared 🤷

spec/asyncapi.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@dalelane dalelane left a comment

Choose a reason for hiding this comment

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

LGTM - I particularly like moving the topic name out of the spec document key, and into an address field

Co-authored-by: Jonas Lagoni <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Sep 21, 2022

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
0.0% 0.0% Duplication

@fmvilas
Copy link
Member Author

fmvilas commented Sep 21, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 8805c88 into asyncapi:next-major-spec Sep 21, 2022
@fmvilas fmvilas deleted the new-channels-object branch September 21, 2022 12:43
@fmvilas
Copy link
Member Author

fmvilas commented Sep 21, 2022

😱 😄

@fmvilas
Copy link
Member Author

fmvilas commented Sep 21, 2022

@derberg It didn't create any release candidate. Is it expected?

@derberg
Copy link
Member

derberg commented Sep 21, 2022

no, mainly because the fix I did in release pipeline did not go to next-major-spec branch as there are merge conflicts between it and master. It is good though, as PR title should be feat!: to get 3.0 RC and not 2.5 RC. So by accident we did not release a confusing release 😄

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.

7 participants