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

"messageId" must be unique across all the messages. #793

Closed
wiegell opened this issue Jun 22, 2023 · 16 comments
Closed

"messageId" must be unique across all the messages. #793

wiegell opened this issue Jun 22, 2023 · 16 comments
Labels

Comments

@wiegell
Copy link

wiegell commented Jun 22, 2023

Describe the bug

One message cannot be reused in publish and subscribe in the same channel, if a messageId is defined on the message

To Reproduce

Steps to reproduce the behavior:

  1. Assume someMessage has messageId defined.
  2. Create a document with a channel like so:
channels:
  someChannel:
    publish:
      message:
        $ref: "#/components/messages/someMessage"
    subscribe:
      message:
        $ref: "#/components/messages/someMessage"
    bindings:
      eventstore:
        # This is not autogenerated to go code unfortunately
        streamName: "someStream"
  1. Try to asyncapi generate models golang
  2. See error error asyncapi2-message-messageId-uniqueness "messageId" must be unique across all the messages.

Expected behavior

This should be possible. My usecase is that i want the same service to publish and subscribe to the same eventstore channel, that is used to persist data

@github-actions
Copy link

Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

Copy link
Member

derberg commented Jun 27, 2023

@wiegell messageId is exactly for specifying unique messages, so later it can be used in validation. If in your case you do not need such unique id, why do you use messageId? it is not mandatory. Message namesomeMessage can be your identifier.

@wiegell
Copy link
Author

wiegell commented Jun 27, 2023

@derberg I use it because the validator gives a warning if it's not specified. Since it is the exact same type of message i'm publishing and subscribing to, it seems reasonable to me that the id could be the same.

@derberg
Copy link
Member

derberg commented Jun 27, 2023

I guess you're referring to https://github.com/WaleedAshraf/asyncapi-validator ? you do not have to use validateByMessageId, they still maintain API that allows you to specify that validation should be based on name, like {msgIdentifier: 'name'}

@wiegell
Copy link
Author

wiegell commented Jun 27, 2023

I mean the asyncapi cli validate command.
I know i don't have to specify it, but why not have the option when it seems to be best practice?
It just seems to me that the id's are evaluated for duplicates in an unfit place, when my initial example is not possible?

@derberg
Copy link
Member

derberg commented Jun 27, 2023

ah, you mean the warnings that show up after AsyncAPI document is validated.

actually I would question that missing messageId should be a warning.

I'll transfer this issue to parser repo as in the end it is parser related, not spec itself

@derberg derberg transferred this issue from asyncapi/spec Jun 27, 2023
@github-actions
Copy link

Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@derberg
Copy link
Member

derberg commented Jun 27, 2023

@smoya @jonaslagoni

  • any idea why missing messageId is popping up as warning? it is optional field
  • I think the point @wiegell brought makes sense. The thing is that we probably use spectral validation after dereferencing, so spectral sees duplicated inlined message. That should be changed, right? same about operationId I guess, when people will start reusing operations 🤔

@jonaslagoni
Copy link
Member

any idea why missing messageId is popping up as warning? it is optional field

Because spectral rules have been set up to do so 🤷 If you need more information I gotta tag @magicmatatjahu in 😄

I think the point @wiegell brought makes sense. The thing is that we probably use spectral validation after dereferencing, so spectral sees duplicated inlined message. That should be changed, right? same about operationId I guess, when people will start reusing operations 🤔

Yep, it's a bug.

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Jun 27, 2023

Yes, missed messageId is treated as warning because it's a good design pattern to define it, in CLI (as I remember) you can disallow all warnings. And for the second thing, it's a bug. Spectral rule should check the messageId by the message references, not by inline value.

@smoya
Copy link
Member

smoya commented Jun 28, 2023

Spectral rule should check the messageId by the message references, not by inline value.

But that's not possible with the current dereferencing strategy, isn't it? Spectral resolves references and then validate.

IIRC, we use our own reference Resolver, which is a wrapper of the @stoplight/spectral-ref-resolver. Would it make sense if we add a new internal field to the MessageObject after dereferencing, where we store a variation of the messageId like adding a suffix so we can later validate uniqueness based on that?

Using the previous example, the resolver will resolve #/components/messages/someMessage, and inject a new field called _IdToCompare (we need to choose a proper name), which can be ID + random suffix and then use that, in the case Is present, during validation?

WDYT @magicmatatjahu? Maybe there is a simpler approach but it scapes from my knowledge then.

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Jun 28, 2023

There should be a possibility to check the references (I mean the dereferenced messages) by the reference in JS memory, so comparison of two messages which are referenced from this same reference should be equal true - so you can create the storage mechanism to check equality. json-schema-ref-parser works in that way, I don't remember if Spectral resolver also.

EDIT: if references won't work, there is still possibility to use the documentInventory from Spectral context to check the "path" of references, so then we can make the comparison of $ref value.

@smoya
Copy link
Member

smoya commented Jun 28, 2023

@wiegell would you like to work on the fix considering the suggested solutions @magicmatatjahu posted? We can bring you assistance if you need it.

@wiegell
Copy link
Author

wiegell commented Jun 28, 2023

Thank you for the confidence, but i don't have the time atm. Also the messageId is not vital to my current project. Out of curiosity: does any of the code generators transpile the messageId into one of the destination languages? In go i only get the schema structs.

@Gilles-qbmt
Copy link

Gilles-qbmt commented Oct 2, 2023

I had the same issue when trying to generate code with the async-api generator.

I used a file template: $$message$$.java and the code uses the messageId. If the messages have a messageId it errors with not unique and when not specified the generator always outputs a single undefned.java file.

As a workaround I use "x-parser-message-name" on the messages since this is a fallback for the id field.

Copy link

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

@github-actions github-actions bot added the stale label Jan 31, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants