asyncapi / parser-js

AsyncAPI parser for Javascript (browser-compatible too).
Apache License 2.0
119 stars 97 forks source link

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

Closed wiegell closed 5 months ago

wiegell commented 1 year ago

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"
  3. Try to asyncapi generate models golang
  4. 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[bot] commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

github-actions[bot] commented 1 year ago

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 commented 1 year ago

@smoya @jonaslagoni

jonaslagoni commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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.

github-actions[bot] commented 9 months ago

This issue has been automatically marked as stale because it has not had recent activity :sleeping:

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 :heart: