asyncapi / spec-json-schemas

AsyncAPI schema versions
Apache License 2.0
56 stars 54 forks source link

feat: add amqp binding 0.4.0 #555

Open timonback opened 3 months ago

timonback commented 3 months ago

Description

In Java Spring applications, it is possible to define an application that connects to an amqp broker, defines exchanges, queues and listens to incoming message on these queues.

My current understanding of amqp:

  1. A publisher connects to a broker
  2. The publisher sends a message with a routing key to an exchange
  3. The broker matches the message's routing key with the routing patterns of the defined exchanges and forwards the message to those queues.
  4. (Bindings are used to connect an exchange with a queue, which do have a n-to-m mapping)
  5. The consumer receives message from the queue

In AsyncAPI, the amqp binding supports two types of channels: routingKey (which is an exchange) and queue. Unfortunately, there is no link between the two.

Proposal: Add (optional) fields:

  1. name to specify the actual routing pattern used (in the (topic) exchange or amqp binding)
  2. channel.$ref to connect the is=routingKey channel type to the is=queue channel type.

I am looking forward to your thoughts. Please let me know, if there are also tests that need to be updated and/or the amqp v0.4.0 needs to be mentioned somewhere to get bundled.

Example:

asyncapi: 3.0.0
info:
  title: Springwolf example project - AMQP
  version: 1.0.0
  description: Springwolf example project to demonstrate springwolfs abilities
defaultContentType: application/json
servers:
  amqp-server:
    host: amqp:5672
    protocol: amqp
channels:
  queue-update-id_#_CRUD-topic-exchange-1-id:
    address: CRUD-topic-exchange-1
    messages:
      java.lang.String:
        $ref: "#/components/messages/java.lang.String"
    bindings:
      amqp:
        is: routingKey
        name: queue-update-id
        channel:
          $ref: #/components/channels/queue-update_id
        exchange:
          name: CRUD-topic-exchange-1
          type: topic
          durable: true
          autoDelete: false
          vhost: /
        bindingVersion: 0.3.0
  queue-update_id:
    address: queue-update
    bindings:
      amqp:
        is: queue
        queue:
          name: queue-update
          durable: false
          exclusive: false
          autoDelete: false
          vhost: /
        bindingVersion: 0.3.0
components:
  schemas:
    SpringRabbitListenerDefaultHeaders:
      type: object
      properties: {}
      examples:
        - {}
    java.lang.String:
      title: String
      type: string
      examples:
        - '"string"'
  messages:
    java.lang.String:
      headers:
        $ref: "#/components/schemas/SpringRabbitListenerDefaultHeaders"
      payload:
        schemaFormat: application/vnd.aai.asyncapi+json;version=3.0.0
        schema:
          $ref: "#/components/schemas/java.lang.String"
      name: java.lang.String
      title: String
      bindings:
        amqp:
          bindingVersion: 0.3.0
operations:
  queue-update-id_#_CRUD-topic-exchange-1-id_receive_bindingsUpdate:
    action: receive
    channel:
      $ref: "#/channels/queue-update-id_#_CRUD-topic-exchange-1-id"
    bindings:
      amqp:
        expiration: 0
        bindingVersion: 0.3.0
    messages:
      - $ref: "#/channels/queue-update-id_#_CRUD-topic-exchange-1-id/messages/java.lang.String"

(Original PR: https://github.com/asyncapi/bindings/pull/259)

Pakisan commented 3 months ago

Reference:

RabbitMQ

channel.queue_bind(exchange=exchange_name,
                   queue=queue_name,
                   routing_key='black')

What do you think about next format:

Proposed change is:

{
  "bindings": {
    "amqp": {
      "is": "routingKey",
      "name": "black",
      "channel": {
        "$ref": "#/components/channels/queue-update"
      },
      "exchange": {
        "name": "CRUD-topic-exchange-1",
        "type": "topic",
        "durable": true,
        "autoDelete": false,
        "vhost": "/"
      },
      "bindingVersion": "0.4.0"
    }
  }
}

add channel property, which is reference to Channel

timonback commented 2 months ago

What are the next steps? (Do I need to ping someone/post somewhere for a review)

The channel binding contains the two new fields ˋnameˋ and ˋchannelˋ, which @Pakisan mentions in the above comment.

Pakisan commented 2 months ago

@timonback I'll approve it tomorrow, after test implementation

timonback commented 2 months ago

@Pakisan Is there something I can do to support?

Pakisan commented 2 months ago

@timonback Merge latest master, where I have enabled json schemas test

I'll attach patch with test to add and it's done

Sorry for delay. I forget to write you back about changes to pull

Did you check it in SpringWolf? Everything is working like expected?

timonback commented 2 months ago

Great. I just rebased the branch, tests are still green - however amqp 0.4.0 is not under test. I assume that is what you will patch @Pakisan?

Yes, the implementation on the Springwolf side is done. One example of a amqp exchange, that accepts all messages (routingKey = '#') and will forward it to the queue-read queue:

  CRUD-topic-exchange-2_id:
    address: CRUD-topic-exchange-2
    messages:
      io.github.springwolf.examples.amqp.dtos.ExamplePayloadDto:
        $ref: "#/components/messages/io.github.springwolf.examples.amqp.dtos.ExamplePayloadDto"
    bindings:
      amqp:
        channel:
          $ref: "#/channels/queue-read-id"
        is: routingKey
        name: "#"
        exchange:
          name: CRUD-topic-exchange-2
          type: topic
          durable: true
          autoDelete: false
          vhost: /
        bindingVersion: 0.4.0

https://github.com/springwolf/springwolf-core/pull/886/files#diff-678a62a10ef6fa6d072912da05e54720cf9c958e85f114ec5aafad5580384f43R40-R57 (Since we validate the spec using asyncapi/parser-js in tests, we will wait till that dependency was updated as well before merging on the Springwolf side)

Pakisan commented 2 months ago

tests__add_amqp_binding_0_4_0.patch

@timonback

sonarcloud[bot] commented 2 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

timonback commented 2 months ago

Thanks for the help, adding tests for the bindings has paid off.

@Pakisan I updated the PR. Does the schemas/3.0.0.json file need to be updated as well (via bundler)? I can find the 0.3.0 amqp binding only in there.

Pakisan commented 1 month ago

Thanks for the help, adding tests for the bindings has paid off.

@Pakisan I updated the PR. Does the schemas/3.0.0.json file need to be updated as well (via bundler)? I can find the 0.3.0 amqp binding only in there.

It will be updated on project build

npm run build

After that new schema will be assembled. With and without references

jonaslagoni commented 1 month ago

Once https://github.com/asyncapi/bindings/pull/259 is merged we can merge this 🙂

timonback commented 1 month ago

@jonaslagoni My understanding based on @Pakisan comment is, that this is the correct change. Comment: https://github.com/asyncapi/bindings/pull/259#issuecomment-2314601080

The other PR is obsolete, I am happy to close it.

jonaslagoni commented 1 month ago

Bindings is like the spec repository just for bindings.

The specification must change before the JSON Schema files.

So the markdown files still need to change as they set the "standard" 😊

timonback commented 1 month ago

@jonaslagoni Sorry, I am not familiar with the process. How to proceed from here?

Merge https://github.com/asyncapi/bindings/pull/259 first, then this PR?

Who can approve/merge either of these PRs?

Pakisan commented 1 month ago

@jonaslagoni let's allow to merge

@timonback is proposing great feature, for their and our community - extend ways do describe interaction with AMQP

Lack of documentation it's not blocker for me, because here is already ready PR, which wasn't approved

jonaslagoni commented 1 month ago

As there is no specific codeowners for AMQP, I assume its the core owners: https://github.com/asyncapi/bindings/blob/8ca568958be9316a13514110a4db502bc8d390b8/CODEOWNERS#L9

You need their approval for any merge to happen 🙂 Best way is to spread the word on slack and ping a few codeowners IMO.

It's not the documentation thats important @Pakisan, without the bindings merged, there is no feature as it does not exist in the standard. These JSON Schema documents are just tooling.

But I agree it should be moved forward ASAP 🙂

Pakisan commented 1 month ago

fyi: @derberg @jonaslagoni i'll merge it at this evening. Everything what's left, you can assign to me and I'll finish it as soon as it possible

I need this stuff to present our specification in upcoming talk about Event Mesh, AsyncAPI, Event Clouds and Spring Wolf

@timonback how fast new build will be available?

timonback commented 1 month ago

@Pakisan I have updated the original PR (https://github.com/asyncapi/bindings/pull/259), which is an incremental improvement, while the amqp binding will probably have further questions along the way.

Proposed change on the Springwolf side: https://github.com/springwolf/springwolf-core/pull/1033 Springwolf has a test to verify that the generated spec is conform to parser-js validation, so we need an updated parser-js package first.

In case you need something for your presentation, you can checkout the Springwolf PR and run the gradle publishToMavenLocal task to create a SNAPSHOT version locally.