Message referencing from operation needs to be more strict

[derberg]

Context

• Messages in operation
  

• Messages in operation reply
  

Key of the problem

It MUST contain a subset of the messages defined in the [channel referenced in this operation](https://github.com/asyncapi/spec/blob/next-major-spec/spec/asyncapi.md#operationObjectChannel)

Confusion

but does that mean that the reference must point to a message through channel location, or can be also components?

for me this should be enforced:

asyncapi: '3.0.0'
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
channels:
  UserSignedUp: 
    messages:
      UserSignedUp:
        $ref: '#/components/messages/UserSigned'
operations:
  user/signedup:
    action: send
    channel: 
      $ref: '#/channels/UserSignedUp'
    messages:
      - $ref: '#/channels/UserSignedUp/messages/UserSignedUp'
components:
  messages:
    UserSigned:
      payload:
        type: object
        properties:
          displayName:
            type: string
            description: Name of the user
          email:
            type: string
            format: email
            description: Email of the user

this part

channel: 
      $ref: '#/channels/UserSignedUp'
    messages:
      - $ref: '#/channels/UserSignedUp/messages/UserSignedUp'

and people should not be allowed to do

channel: 
      $ref: '#/channels/UserSignedUp'
    messages:
      - $ref: '#/components/messages/UserSigned'

otherwise we'll get into a trouble of mismatch, unless I'm wrong and messageId is not considered to be a part of message

take this v2 example

asyncapi: '2.6.0'
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
channels:
  user/signedup:
    subscribe:
      message:
        $ref: '#/components/messages/UserSignedUp'
components:
  messages:
    UserSignedUp:
      payload:
        type: object
        properties:
          displayName:
            type: string
            description: Name of the user
          email:
            type: string
            format: email
            description: Email of the user

converter will convert it to

asyncapi: 3.0.0
info:
  title: Account Service
  version: 1.0.0
  description: This service is in charge of processing user signups
channels:
  user/signedup:
    address: user/signedup
    messages:
      subscribe.message:
        $ref: '#/components/messages/UserSignedUp'
operations:
  user/signedup.subscribe:
    action: send
    channel:
      $ref: '#/channels/user~1signedup'
    messages:
      - $ref: '#/components/messages/UserSignedUp'
components:
  messages:
    UserSignedUp:
      payload:
        type: object
        properties:
          displayName:
            type: string
            description: Name of the user
          email:
            type: string
            format: email
            description: Email of the user

operation points to message UserSignedUp but on a channel level we have a message subscribe.message

imho the only proper way is to specify that $ref pointer must point to the channel - and this needs to be validated with parser of course.

Am I missing any possible errors that this could cause?

[jonaslagoni]

That makes sense to me yea 🤔

[fmvilas]

Yes, that makes sense to me as well.

[buehlefs]

How should this interact with operation traits that specify a message? Only allowing references to messages in a specific channel for them would limit their reusability.

[derberg]

@buehlefs could you visualize a problem with an example? message trait is used usually when messageTrait keeps entire message definition, without references, and also messageTrait is something you use on message level 🤔 you would anyway add it in channel.message right?

[derberg]

@fmvilas @jonaslagoni be aware that it can affect release date, unless we get help to make sure that is supported in parser-js and converter-js. Probably examples need a review. Nevertheless, better to add this limitation now, and relax with 3.1 or 3.2, cause other way around it will be a breaking change

[buehlefs]

@derberg I am talking about an operationTrait object that can "contain any property from the Operation Object, except the action, channel and traits" (as per current pre release spec). I can see this being used to specify a login (or haertbeat) operation with a login message that will be used in multiple locations with different channels. But this reuse would only be possible if the message is referenced from components and not from a specific channel. (but I may be missing something here)

[smoya]

@buehlefs The following sentence located in the description of the Operation Trait object is wrong (nice catch!):

This object MAY contain any property from the Operation Object, except the action, channel and traits ones.

Source: https://www.asyncapi.com/docs/reference/specification/v3.0.0-next-major-spec.16#operationTraitObject

If you take a look at the fields an Operation Trait object has, messages field is not included.

You can confirm this is by design and expected by checking its corresponding JSON Schema definition at https://github.com/asyncapi/spec-json-schemas/blob/next-major-spec/definitions/3.0.0/operationTrait.json.

We should fix that sentence. (I created the following issue so we work on it asap #994)

[smoya]

@derberg @fmvilas I'm gonna start working on the validation for this in Parser-JS. We would need to add an Spectral rule to check that.

Btw, we do not have particular rules for v3 implemented atm besides the standard ruleset.
In case any of you think other must-have rules are missing, please mention it in an issue so we can work on it.

[fmvilas]

Awesome! Thanks, @smoya!

I was wondering today if we shouldn't apply a similar rule to the channel field in an operation. It should only be allowed to point to a channel in the channels section and not to a channel in components/channels or in another file or URL. Does that make sense?

It should be applied to the spec, JSON schemas, and the parser.

[jonaslagoni]

Probably a good idea 🤔

[smoya]

Awesome! Thanks, @smoya!

I was wondering today if we shouldn't apply a similar rule to the channel field in an operation. It should only be allowed to point to a channel in the channels section and not to a channel in components/channels or in another file or URL. Does that make sense?

It should be applied to the spec, JSON schemas, and the parser.

Yup, good catch

[smoya]

I was wondering today if we shouldn't apply a similar rule to the channel field in an operation. It should only be allowed to point to a channel in the channels section and not to a channel in components/channels or in another file or URL. Does that make sense?

I gave this another thought and I think this shouldn't happen. What happens with channels and linked operations declared under components?

Consider the following components in an AsyncAPI v3 doc:

components:
  channels:
    aChannel: 
      messages:
        aMessage:
          $ref: '#/components/messages/aMessage'
  operations:
    anOperation:
      action: send
      channel: 
        $ref: '#/components/channels/aChannel'
      messages:
        - $ref: '#/components/channels/aChannel/messages/aMessage'

You can see the anOperation refers to the aChannel channel, also in components. Same with its message.
If we force those to always refer to channels from #/channels, the use case above won't be valid.

I believe the current description of the messages field of an Operation is already telling us what should be the enforcements:

It MUST contain a subset of the messages defined in the channel referenced in this operation

If the channel is located under #/channels or rather #/components/channels is not important here if we always keep that consistency in referenced messages under messages field.

cc @fmvilas @jonaslagoni @derberg

[smoya]

Clarifying the rules we want to add:

• If the operation is required (under root operations), the operation channel must only point to a required channel (under root channels).
• If the operation is optional (under components), the operation channel can point either to a required channel (under root operations) or to an optional channel (under components).
• The messages, no matter where they are located, have to point to messages from the channel specified in the field channel.

Are we aligned?

cc @fmvilas @derberg @jonaslagoni and anyone interested

[fmvilas]

Yes, that's exactly what I mean.

[smoya]

The messages, no matter where they are located, have to point to messages from the channel specified in the field channel.

1/3 rules created: asyncapi/parser-js#911

[smoya]

• If the operation is optional (under components), the operation channel can point either to a required channel (under root operations) or to an optional channel (under components).

Actually this won't be considered as a rule because there is nothing to validate here, right?

[smoya]

If the operation is required (under root operations), the operation channel must only point to a required channel (under root channels).

Second rule created: smoya/parser-js#5
However, it is located in my fork, because all needed scaffolding for enabling Spectral rules for v3 are added into the opened PR asyncapi/parser-js#911. Once is merged, I will close and open a new PR against AsyncAPI repo.
But feel free to review anyway please.

[derberg]

we have similar situation with channel.servers where servers is an array of $ref pointers. Such $ref should always point to the root server and not components/server

so in general, we need to add one new sentence to servers under channels, messages under operation and operation reply, channels under operation and operation reply

suggested edit, for channel:

#current text:
A $ref pointer to the definition of the channel in which this operation is performed

#suggested edit
A $ref pointer to the definition of the channel in which this operation is performed. It MUST point to channel definition located in [Channels Object](#channelsObject) and MUST NOT point to channel definition located in [Components Object](#componentsObject)

#I know first must make it clear that components are not allowed, but I think better to be explicit like this

above example can of course be easily modified and for messages will for example sound

#current   

A list of $ref pointers pointing to the supported [Message Objects](https://github.com/asyncapi/spec/blob/next-major-spec/spec/asyncapi.md#messageObject) that can be processed by this operation. It MUST contain a subset of the messages defined in the [channel referenced in this operation](https://github.com/asyncapi/spec/blob/next-major-spec/spec/asyncapi.md#operationObjectChannel).

#new

A list of $ref pointers pointing to the supported [Message Objects](#messageObject) that can be processed by this operation. It MUST contain a subset of the messages defined in the [channel referenced in this operation](#operationObjectChannel). Individual $ref pointer MUST point to message definition located in the [channel referenced in this operation](#operationObjectChannel) and MUST NOT point to message definition located in [Components Object](#componentsObject)

[smoya]

we have similar situation with channel.servers where servers is an array of $ref pointers. Such $ref should always point to the root server and not components/server

Good catch again. I will create the proper Spectral rule as well.

[smoya]

I will create the proper Spectral rule as well.

Done. Added into the same previous PR asyncapi/parser-js#913

[smoya]

All parser-js required Spectral rules are now merged and enabled in next-major-spec

[smoya]

above example can of course be easily modified and for messages will for example sound

I don't think we need to change that. It sounds pretty clear to me, specially now that we will update the channel operation field to be explicit about where should it point to.

It MUST contain a subset of the messages defined in the channel referenced in this operation

[derberg]

@smoya but current text is not clear that $ref pointer must point to the channel - it only says the message must be the same (but it is not explicit - even for us as even our converter do not convert properly)

[smoya]

Let's move this discussion into #996

[smoya]

We are missing the Spectral rules for OperationReply.channel and OperationReply.messages.
They should validate that:

• messages are pointing to a subset of messages of the OperationReply.channel
• channel should point to:
  • root channel if the OperationReply is under root Operation
  • either root channel or component channel if the OperationReply is under components Operation or under components Replies.

Does it make sense?

[fmvilas]

I think it does, yeah 👍

[github-actions]

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 ❤️

[Shimork04]

Hi everyone! 👋

I believe this issue is still important despite the inactivity. We're missing Spectral rules for OperationReply.channel and OperationReply.messages. These rules should validate that:

messages point to a subset of messages in the OperationReply.channel.
channel should reference:
The root channel if OperationReply is under a root operation.
The root or a component channel if under components Operation or Replies.
To move forward, let's finalize these rules and implement them in Spectral. I'm happy to help with the implementation or testing.

Looking forward to your thoughts!

Thanks! ❤️