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

fix: clarify pointers for channel.servers and operation.channel fields #996

Conversation

smoya
Copy link
Member

@smoya smoya commented Nov 28, 2023

Fixes #991

Please help me writing proper sentences. Feel free to suggest changes.

spec/asyncapi.md Outdated
@@ -634,7 +634,7 @@ Field Name | Type | Description
<a name="channelObjectTitle"></a>title | `string` | A human-friendly title for the channel.
<a name="channelObjectSummary"></a>summary | `string` | A short summary of the channel.
<a name="channelObjectDescription"></a>description | `string` | An optional description of this channel. [CommonMark syntax](https://spec.commonmark.org/) can be used for rich text representation.
<a name="channelObjectServers"></a>servers | [[Reference Object](#referenceObject)] | An array of `$ref` pointers to the definition of the servers in which this channel is available. If `servers` is absent or empty, this channel MUST be available on all the servers defined in the [Servers Object](#serversObject). Please note the `servers` property value MUST be an array of [Reference Objects](#referenceObject) and, therefore, MUST NOT contain an array of [Server Objects](#serverObject). However, it is RECOMMENDED that parsers (or other software) dereference this property for a better development experience.
<a name="channelObjectServers"></a>servers | [[Reference Object](#referenceObject)] | An array of `$ref` pointers to the definition of the servers in which this channel is available. It MUST point to a subset of server definitions located in the [Servers Object](#serversObject) and MUST NOT point to a subset of server definitions located in the [Components Object](#componentsObject). If `servers` is absent or empty, this channel MUST be available on all the servers defined in the [Servers Object](#serversObject). Please note the `servers` property value MUST be an array of [Reference Objects](#referenceObject) and, therefore, MUST NOT contain an array of [Server Objects](#serverObject). However, it is RECOMMENDED that parsers (or other software) dereference this property for a better development experience.
Copy link
Member

Choose a reason for hiding this comment

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

Why not making it more explicit?

Suggested change
<a name="channelObjectServers"></a>servers | [[Reference Object](#referenceObject)] | An array of `$ref` pointers to the definition of the servers in which this channel is available. It MUST point to a subset of server definitions located in the [Servers Object](#serversObject) and MUST NOT point to a subset of server definitions located in the [Components Object](#componentsObject). If `servers` is absent or empty, this channel MUST be available on all the servers defined in the [Servers Object](#serversObject). Please note the `servers` property value MUST be an array of [Reference Objects](#referenceObject) and, therefore, MUST NOT contain an array of [Server Objects](#serverObject). However, it is RECOMMENDED that parsers (or other software) dereference this property for a better development experience.
<a name="channelObjectServers"></a>servers | [[Reference Object](#referenceObject)] | An array of `$ref` pointers to the definition of the servers in which this channel is available. This value MUST start with `#/servers/`, meaning only pointing to the [root servers object](#serversObject) is allowed. If `servers` is absent or empty, this channel MUST be available on all the servers defined in the [Servers Object](#serversObject). Please note the `servers` property value MUST be an array of [Reference Objects](#referenceObject) and, therefore, MUST NOT contain an array of [Server Objects](#serverObject). However, it is RECOMMENDED that parsers (or other software) dereference this property for a better development experience.

Copy link
Member

Choose a reason for hiding this comment

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

you cannot be explicit, you can have channel defined in a separate file and then the $ref to server would not start with #/servers

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. Technically it should contain the string #/servers but not necessary in the beginning of the pointer. In fact that's what the Spectral rule checks https://github.com/asyncapi/parser-js/blob/next-major-spec/src/ruleset/v3/ruleset.ts#L56 but you @fmvilas already know because you reviewed my PR and, in fact, you suggested it iirc.

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. So then the parser can't implement it this way either, right? @smoya

Copy link
Member Author

@smoya smoya Nov 29, 2023

Choose a reason for hiding this comment

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

You're right. So then the parser can't implement it this way either, right? @smoya

But the rule only applies to objects (Channels, Operations...) defined in the root. See https://github.com/asyncapi/parser-js/blob/next-major-spec/src/ruleset/v3/ruleset.ts#L51

If in components, "all" is allowed.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm 🤔 I still don't think it's correct. Unless we avoid the parser to resolve these references.

Copy link
Member Author

Choose a reason for hiding this comment

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

The Spectral rule applies before resolving the references. Not sure if this "answers" your question.

spec/asyncapi.md Outdated
@@ -830,7 +830,7 @@ Describes a specific operation.
Field Name | Type | Description
---|:---:|---
<a name="operationObjectAction"></a>action | `"send"` &#124; `"receive"` | **Required**. Use `send` when it's expected that the application will send a message to the given [`channel`](#operationObjectChannel), and `receive` when the application should expect receiving messages from the given [`channel`](#operationObjectChannel).
<a name="operationObjectChannel"></a>channel | [Reference Object](#referenceObject) | **Required**. A `$ref` pointer to the definition of the channel in which this operation is performed. Please note the `channel` property value MUST be a [Reference Object](#referenceObject) and, therefore, MUST NOT contain a [Channel Object](#channelObject). However, it is RECOMMENDED that parsers (or other software) dereference this property for a better development experience.
<a name="operationObjectChannel"></a>channel | [Reference Object](#referenceObject) | **Required**. A `$ref` pointer to the definition of the channel in which this operation is performed. Please note the `channel` property value MUST be a [Reference Object](#referenceObject) and, therefore, MUST NOT contain a [Channel Object](#channelObject). However, it is RECOMMENDED that parsers (or other software) dereference this property for a better development experience. It MUST point to a channel definition located in the [Channels Object](#channelsObject) and MUST NOT point to a channel definition located in the [Components Object](#componentsObject).
Copy link
Member

Choose a reason for hiding this comment

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

Same as with servers.

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.

you also need to update definition of channel reference in Operation Reply Object

and yeah, same with message -> #991 (comment)

spec/asyncapi.md Outdated Show resolved Hide resolved
spec/asyncapi.md Outdated
@@ -634,7 +634,7 @@ Field Name | Type | Description
<a name="channelObjectTitle"></a>title | `string` | A human-friendly title for the channel.
<a name="channelObjectSummary"></a>summary | `string` | A short summary of the channel.
<a name="channelObjectDescription"></a>description | `string` | An optional description of this channel. [CommonMark syntax](https://spec.commonmark.org/) can be used for rich text representation.
<a name="channelObjectServers"></a>servers | [[Reference Object](#referenceObject)] | An array of `$ref` pointers to the definition of the servers in which this channel is available. If `servers` is absent or empty, this channel MUST be available on all the servers defined in the [Servers Object](#serversObject). Please note the `servers` property value MUST be an array of [Reference Objects](#referenceObject) and, therefore, MUST NOT contain an array of [Server Objects](#serverObject). However, it is RECOMMENDED that parsers (or other software) dereference this property for a better development experience.
<a name="channelObjectServers"></a>servers | [[Reference Object](#referenceObject)] | An array of `$ref` pointers to the definition of the servers in which this channel is available. It MUST point to a subset of server definitions located in the [Servers Object](#serversObject) and MUST NOT point to a subset of server definitions located in the [Components Object](#componentsObject). If `servers` is absent or empty, this channel MUST be available on all the servers defined in the [Servers Object](#serversObject). Please note the `servers` property value MUST be an array of [Reference Objects](#referenceObject) and, therefore, MUST NOT contain an array of [Server Objects](#serverObject). However, it is RECOMMENDED that parsers (or other software) dereference this property for a better development experience.
Copy link
Member

Choose a reason for hiding this comment

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

you cannot be explicit, you can have channel defined in a separate file and then the $ref to server would not start with #/servers

@derberg
Copy link
Member

derberg commented Nov 28, 2023

examples fix #997

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.

I have some use cases I want to run by you and you let me know which rules are applied.

  • Use case 1: A root channel is defined as a reference, and the operation references that root channel. How do you define the message reference? To the root channel or the channel in the components section?
{
    asyncapi: 3.0.0,
    channels: {
        test: {
            $ref: '#/components/channels/test'
        }
    },
    operations: {
        operationTest: {
            channel: {
                $ref: '#/channels/test'
            },
            messages: [
                {
                    $ref: '#/channels/test/messages/test'
                }
            ]
        }
    },
    components: {
        channels: {
            test: {
                messages: {
                    test: {},
                    test2: {}
                }
            }
        }
    }
}
  • Use case 2: You use AsyncAPI to define a menu of channels and operations. Are operations allowed to reference channels in components?
{
    asyncapi: 3.0.0,
    components: {
        channels: {
            test: { ... }
        },
        operations: {
            operationTest: {
                channel: {
                    $ref: '#/components/channels/test'
                }
            }
        }
    }
}

@smoya
Copy link
Member Author

smoya commented Nov 29, 2023

I have some use cases I want to run by you and you let me know which rules are applied.

  • Use case 1: A root channel is defined as a reference, and the operation references that root channel. How do you define the message reference? To the root channel or the channel in the components section?

Technically, your example is compliant.
User friendly? Might sound a bit confusing indeed. In addition, I believe confusion increases due to the usage we are giving to JSON Reference in these cases (channels.servers, operations.channel, operations.messages, operationReply.channel, operationReply.messages), where they represent just "an ID" of the referenced object VS the rest of the document which are indeed JSON References that are expected to be dereferenced. In fact, and for example, the current description in v3 for operation.channel field says:

A $ref pointer to the definition of the channel in which this operation is performed. Please note the channel property value MUST be a Reference Object and, therefore, MUST NOT contain a Channel Object.

I would not allow, in this case, a ref to the message in components even though it is the same referenced in the linked channel because once everything gets dereferenced, those channels.servers, operations.channel, operations.messages, operationReply.channel, operationReply.messages will still be presented as references.
We could, however, improve our Spectral rule to detect these cases and, instead of just throwing the current error, be more user friendly specifying that even though they both point to the same message, they need to point to the root object. Not strong opinion and not very willing to do that but 🤷 .

  • Use case 2: You use AsyncAPI to define a menu of channels and operations. Are operations allowed to reference channels in components?

Operations inside components can reference to either channel or messages from components or from root.

@smoya
Copy link
Member Author

smoya commented Nov 29, 2023

@jonaslagoni I think I managed to clarify a bit the use cases you showed in your comment.

Also I improved the wording and added clarifications in operation reply object as well. Please all of you take a look 🙏 @fmvilas @derberg

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 Outdated Show resolved Hide resolved
smoya and others added 5 commits November 29, 2023 13:37
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Copy link

sonarcloud bot commented Nov 29, 2023

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

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.

I don't think it can be done clearer 👏🏼

@fmvilas
Copy link
Member

fmvilas commented Nov 29, 2023

I don't think it can be done clearer 👏🏼

Indeed!

@fmvilas
Copy link
Member

fmvilas commented Nov 29, 2023

/rtm

@asyncapi-bot asyncapi-bot merged commit 5fb88a0 into asyncapi:next-major-spec Nov 29, 2023
23 of 24 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 📦🚀

@smoya
Copy link
Member Author

smoya commented Nov 29, 2023

Even though this has been merged now, can you @jonaslagoni confirm your concerns about thos edge cases are now clarified with my changes added in the spec? thank you! 🙏

@jonaslagoni
Copy link
Member

Gonna give it another read when I am doing the converter update

@smoya smoya deleted the fix/clarifyChannelsAndMessagesPointers branch November 29, 2023 22:34
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