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

Shouldn't we remove messageId in v3? #978

Closed
derberg opened this issue Oct 10, 2023 · 13 comments
Closed

Shouldn't we remove messageId in v3? #978

derberg opened this issue Oct 10, 2023 · 13 comments
Labels
❔ Question A question about the spec or processes

Comments

@derberg
Copy link
Member

derberg commented Oct 10, 2023

So in v3 under channel you need to provide a list of messages ad s key/value, where key is unique messageId

What is the use case for still having messageId in a Message Object then?

From current description, messageId on Message Object level is to Unique string used to identify the message so like it was before v3. But when I'm sharing messages across documents, I think better to treat key of the message under channel messages as unique identifier.

wdyt?

cc @dalelane that reviewed #827 with me
cc @fmvilas as contributor
cc @jonaslagoni as release coordinator

@derberg derberg added the ❔ Question A question about the spec or processes label Oct 10, 2023
@jonaslagoni
Copy link
Member

I stand pretty firm not to alter v3 at all.

@fmvilas
Copy link
Member

fmvilas commented Oct 27, 2023

Let's take the opportunity and remove it now. Otherwise, we would have to deprecate it and carry it until v4. I say not to complicate our lives in v3 with messageId. If we see a use case for it afterward, we add it back (maybe with a different name to avoid confusion). Adding is easy.

@derberg
Copy link
Member Author

derberg commented Oct 30, 2023

@jonaslagoni @smoya can you folks confirm how complicated will that be (removal of messageId now) from parser perspective. In latest parser, what is considered to be messageId?

Spec says below, and it is in case of Messages under a Channel.
Screenshot 2023-10-30 at 14 29 29

There is one problem if we remove messageId from Message object. That if someone uses AsyncAPI as a collection of messages, without channels, they do not have a way to assign a unique messageId

@smoya
Copy link
Member

smoya commented Oct 30, 2023

What is the use case for still having messageId in a Message Object then?

Considering the following doc:

asyncapi: '3.0.0'
info:
  title: Test
  version: 1.0.0
channels:
  userSignedUp:
    address: 'user/signedup'
    messages:
      one:
        $ref: '#/components/messages/userSignedUp'
      two:
        $ref: '#/components/messages/userSignedUpWithExpandedData'
components:
  messages:
    userSignedUp:
      messageId: userSignedUp
      payload:
        type: object
        properties:
          username:
            type: string
    userSignedUpWithExpandedData:
      messageId: detailedUserSignedUp
      payload:
        type: object
        properties:
          username:
            type: string
          address:
            type: string
          phone:
            type: number

Prior to removing messageId, the id for those messages will be the one on that field, fallbacking to other options (See next point I'm commenting).
However, what would be the expected message id for the messages under the channel if we remove the messageId field?
one or two will be, even though they are pointing to #/components/messages/{userSignedUp|userSignedUpWithExpandedData}.
Would you then consider channel one is different from userSignedUp one just because the key on that messages map is different?

@jonaslagoni @smoya can you folks confirm how complicated will that be (removal of messageId now) from parser perspective. In latest parser, what is considered to be messageId?

I don't think it is hard. However, this is the current logic:

id(): string {
    return this.messageId() || this._meta.id || this.extensions().get(xParserMessageName)?.value<string>() as string;
}

Source: https://github.com/asyncapi/parser-js/blob/3582d31d61fde2c04d79b168102965fd405f1069/src/models/v3/message-trait.ts#L17-L19

Note that this._meta.id is the key on the key/val map if exists (I think always in the case of v3).

@jonaslagoni
Copy link
Member

Just another breaking changes, which of course will cause break stuff here and there 😄

@Tenischev
Copy link
Member

This double messageId identifier actually makes a bit of mess, but I would support @smoya point about potentially different messageId in channel object and in message object or even different messageId for the same message object in different channel objects.
For me as template maintainer and from programming point of view, it's more logical to rely on messageId in the message object to name the object (by means of programming language). Why I should name the object different (by messageId from channel object) if it's the same object underneath?
So, I would create an object (by means of programming language) with name from messageId in the message object and MAY BE create an empty extensions objects (by means of programming language) which will simply inherit original object with names from messageId in channel object.

Also, the point I would like to highlight, that Operation Object doesn't have a map and simply use the list of message objects which is nice and do not make any disruption.

@fmvilas
Copy link
Member

fmvilas commented Nov 2, 2023

Following Sergio's example:

asyncapi: '3.0.0'
info:
  title: Test
  version: 1.0.0
channels:
  userSignedUp:
    address: 'user/signedup'
    messages:
      one:
        $ref: '#/components/messages/userSignedUp'
      two:
        $ref: '#/components/messages/userSignedUpWithExpandedData'
components:
  messages:
    userSignedUp:
      messageId: userSignedUp
      payload:
        type: object
        properties:
          username:
            type: string
    userSignedUpWithExpandedData:
      messageId: detailedUserSignedUp
      payload:
        type: object
        properties:
          username:
            type: string
          address:
            type: string
          phone:
            type: number

Just want to remark that being able to set the message ID in place by one or two can also be a great feature. It lets you add IDs that are custom to your document and make sure it doesn't change when the referenced message changes, which may be on a external file/url.

And also would love to remind that for code generation purposes, we have the name property already.

So yeah, I'm all in for removing messageId.

@fmvilas
Copy link
Member

fmvilas commented Nov 3, 2023

BTW, I'm picking up this issue and I commit to have it done by next week.

@fmvilas
Copy link
Member

fmvilas commented Nov 3, 2023

PR for the spec: #984. Will follow up with PRs for parser-js and JSON Schemas.

@fmvilas
Copy link
Member

fmvilas commented Nov 6, 2023

Added the PR to fix the converter to the list above.

@fmvilas
Copy link
Member

fmvilas commented Nov 7, 2023

I'm closing this issue as it's been addressed already. Thanks for the fast reviews and support 🙏

@fmvilas fmvilas closed this as completed Nov 7, 2023
@WaleedAshraf
Copy link
Contributor

WaleedAshraf commented Jul 20, 2024

Eh, I missed this conversation.

messageId was really useful for tooling. It was added here: asyncapi/parser-js#414

The main difference between messageId and name was the messageId was to be machine-readable and unique across the complete document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❔ Question A question about the spec or processes
Projects
None yet
Development

No branches or pull requests

6 participants