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

feat: create Anypoint MQ bindings #63

Merged

Conversation

GeraldLoeffler
Copy link
Collaborator

Description

Adding binding specification for Anypoint MQ as requested in asyncapi/spec#514 .
Contains JSON schemata as per the latest convention for binding specifications.

Related issue(s)

asyncapi/spec#514

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate more about the Server Binding Object. What is the point of having it really? in your binding example you just provide binding version, rest of the props just look like original Server object.

@fmvilas
Copy link
Member

fmvilas commented Jun 22, 2021

I'm curious. Why do we need a binding for this? I don't think AnypointMQ is a protocol. In the end, it's a regular HTTP call. Can't this be expressed just by using protocol: http on the Server Object and channels like /{orgId}/environments/{envId}/destinations/{destinationName}/messages/{messageId}? @GeraldLoeffler would be great to hear your thoughts. Are we missing something?

@GeraldLoeffler
Copy link
Collaborator Author

GeraldLoeffler commented Jun 22, 2021

I'm curious. Why do we need a binding for this? I don't think AnypointMQ is a protocol. In the end, it's a regular HTTP call. Can't this be expressed just by using protocol: http on the Server Object and channels like /{orgId}/environments/{envId}/destinations/{destinationName}/messages/{messageId}? @GeraldLoeffler would be great to hear your thoughts. Are we missing something?

Anypoint MQ is a message broker that has a RESTful API but is normally accessed via a "native" client that operates on a slightly different level of abstraction than the REST API. For example, the native client doesn't need the {orgId} and {envId} that you have quoted for the REST API URL, because it "knows" that these are implicitly given by the credentials used to access the broker. (I explain this in my binding spec.)

So if anypointmq were not treated as its own protocol, then that level of abstraction would have to be somehow unearthed from an Async API doc that apparently uses http but is known to describe the interaction with the Anypoint MQ broker. That would be very hard and undesirable.

(As an aside, i would have preferred that level of fundamental objection to this project when i started it, not after i've completed it.)

This is also the `bindingVersion` for all binding objects defined by this specification.
In any given binding object, `latest` MAY alternatively be used to refer to the currently latest published version of this bindings specification.

The version of the AsyncAPI specification to which these bindings apply is `2.1.0`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to have this here? Why couldn't this binding be applied to a potential 2.2.0 or 2.1.1 version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to have this here? Why couldn't this binding be applied to a potential 2.2.0 or 2.1.1 version?

i noticed that none of the other binding specs are tied to a spec version, but in writing the binding spec i definitely had in mind to close the gap between AsyncAPI 2.0.0 and the Anypoint MQ protocol. So i considered it prudent to state that. In truth the binding spec also applies to 2.1.0 as far as i can tell.

@fmvilas : If you are opposed to binding specs being explicitly tied to AsyncAPI spec versions, then i'll remove this. Let me know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed but I think we should align in the way we do it, so the rest of the bindings can do it in the same way. Would you mind opening a separate issue for that and removing this line for now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and removed the line from the binding spec.

@fmvilas
Copy link
Member

fmvilas commented Jun 24, 2021

but is normally accessed via a "native" client that operates on a slightly different level of abstraction than the REST API.

I see. That seems to be a thing of your client, still not really a protocol but probably better suited as an extension. A binding has to describe everything you need to communicate with a remote endpoint. If I wanted to publish or subscribe to AnypointMQ using the REST API with curl or my own client, that binding would not be sufficient. And if we improve the binding, then it becomes unnecessary as per the reasons I described above.

(As an aside, i would have preferred that level of fundamental objection to this project when i started it, not after i've completed it.)

I'm sorry, Gerald. We really don't want you to work for nothing but you have to understand that 1. our time is limited, and 2. our knowledge of the AnypointMQ is close to zero. It's been right now that I found time to investigate and realized it was just a REST API. In any case, your work is not going into the void. As I said, I think this can be an extension and thus the work still applies.

@GeraldLoeffler
Copy link
Collaborator Author

GeraldLoeffler commented Jun 25, 2021

I disagree: anypointmq is a "protocol" precisely because the anypointmq protocol bindings adapt the AsyncAPI notions of server, channel, message, etc. to the native messaging model of Anypoint MQ. A protocol in AsyncAPI is clearly not just a network protocol, but that adaptation mechanism that allows an AsyncAPI document to be mapped to the best matching messaging primitives (including API and/or network calls) of the message broker (when the server is a broker). That's why, for example,

  • jms is a protocol although JMS is just a Java API and says nothing about the network protocol at all
  • amqp bindings specify timestamp, for example, although this has nothing to do with the network protocol

Put differently, if a message broker where to expose a SOAP-based API and another one a JSON-RPC API, both of which are HTTP-based, would you reject AsyncAPI protocol definitions for these? Would you require every AsyncAPI doc for these brokers to define SOAP and JSON-RPC payload structures using the http protocol? These brokers could have entirely different messaging models - one supporting pub-sub and correlation IDs, the other supporting message grouping and strict ordering - and the best mechanism in AsyncAPI to capture these characteristics (once, in the bindings spec) is to define protocols and bindings for these protocols. The fact that both network protocols are HTTP-based is irrelevant to the need for bindings.

The anypointmq protocol and bindings spec i've submitted allow concise AsyncAPI documents to be written (using those bindings) such that a code generator can generate a program that interacts with the Anypoint MQ broker. The generated code can use either direct REST API invocations or the native Anypoint MQ client: that depends on the generator and its configuration. Both are possible because the level of abstraction of the bindings matches that of Anypoint MQ.

If there were no Anypoint MQ protocol and bindings, then all AsyncAPI documents that interact with Anypoint MQ would have to use http as the protocol. This would require the authors of these Async API docs to study the Anypoint MQ API and messaging model, and then encode their understanding in the Async API documents - over and over again. For example, they would need to understand and encode (how?) that the HTTP POST body for sending a message with the Anypoint MQ REST API contains not just the message payload as understood by AsyncAPI but also the protocol and application headers, combined in a JSON object. A code generator would then only see the http binding objects and would only be able to generate a program that calls the Anypoint MQ REST API (because that's the level of abstraction of these AsyncAPI docs). The code generator would have a very hard time reverse-engineering the information in the AsyncAPI docs to generate code that uses the native Anypoint MQ client.

I think this discussion highlights the urgent need for a very general clarification: are AsyncAPI protocols just network protocols? Or are they (as i argue above) the mechanism (via bindings) by which the AsyncAPI messaging model is mapped to and adapted to the native messaging model of a broker (server)? Such that, in the extreme - like for JMS - the AsyncAPI protocol (jms) does not specify the network protocol at all.

@fmvilas
Copy link
Member

fmvilas commented Jun 25, 2021

I think this discussion highlights the urgent need for a very general clarification

Indeed. This needs to be well defined.

I see your point, Gerald. Now we just need to figure out what's the best thing to do here. Should we define AsyncAPI protocols as "just network protocols" and thus removing jms, sns, sqs, and mercure`. Or should we leave it open to any kind of mechanism that helps communicating with a broker? What's your opinion?

@fmvilas
Copy link
Member

fmvilas commented Jun 25, 2021

In retrospect, I think we shouldn't be so strict with this. So you have all my support in this becoming a binding. Thanks for ellaborating on this, Gerald.

Now, that said, there are some comments pending to solve in this PR:

  1. Comment from @derberg about the Server object.
  2. A comment from me about restricting this binding only to v2.0.0.
  3. If this is implementation-independent, I think there should be a way to specify organization, environment, and other missing stuff.

With all of that missing, I think it's hard but not imposible to make it for 2.1.0 of the spec. However, since the release cycle of the bindings is independent of the spec, you probably want to create a placeholder binding definition (like the one for jms, mercure, and many others) and then calmly work on this PR.

How does that sound? In any case, September is not far so that's also an option.

@GeraldLoeffler
Copy link
Collaborator Author

Should we define AsyncAPI protocols as "just network protocols" and thus removing jms, sns, sqs, and mercure`. Or should we leave it open to any kind of mechanism that helps communicating with a broker? What's your opinion?

My opinion is that the latter is the more useful choice: an AsyncAPI protocol (and the associated bindings) should capture everything that's common for message producers/consumers interacting with servers that share that AsyncAPI protocol. There is a wide range of possibilities:

  1. When the "server" is a well-known, hosted, server-less messaging system that is accessed via a web API endpoint (think: SNS), then there is a lot in common between all the AsyncAPI docs that use that "server" and its protocol: the messaging model (pub/sub, p2p, supported encodings of message bodies, support for correlation ID, TTL, ...), the exact port, the web API, the application/network protocol (HTTP). All of that should be subsumed in the AsyncAPI protocol/bindings (such as sns).
  2. When the server is a classic message broker (ActiveMQ, IBM MQ, ...) then there is a bit less in common between all the AsyncAPI docs that use that server and protocol: the messaging model is the same; the port may have a default but be configurable; even the network protocol might be configurable (such as for ActiveMQ, which has pluggable network protocols). All of the commonality should be subsumed in the AsyncAPI protocol/bindings (such that the server binding object for activemq has a field for network protocol).
  3. When the server is accessed via a standardised native API (think: JMS) then there is still a lot in common between all the AsyncAPI docs that use that server and protocol: the messaging model is the same; a JMS provider library (jar) is always needed; but nothing about the network protocol (incl. ports) is known. All of the commonality should be subsumed in the AsyncAPI protocol/bindings.
  4. When the server is accessed via a standardised network protocol (think: AMQP) then there is still a lot in common between all the AsyncAPI docs that use servers with that protocol: the messaging model is the same; but there are different client library implementations (that all implement that messaging model and network protocol).
  5. Lastly, when the server is not a broker or messaging system but just an arbitrary web API endpoint at some URL, then there is nothing much in common with other servers using that protocol, other than HTTP being the underlying protocol. Although, i could imagine factoring-out the commonality between, say all JSON-RPC APIs into a jsonrpc protocol/bindings (which fixes the network protocol to HTTP), or all gRPC APIs into a grpc protocol/bindings (which fixes the network protocol to HTTP/2).

In summary, i think it's most practically useful to view AsyncAPI protocols and bindings as capturing all client-visible commonalities between AsyncAPI servers that share that AsyncAPI protocol. In all cases that includes the messaging model (such as the supported protocol headers, data types for application headers, ways of encoding message bodies (bytes, streams, text, ...), routing information, support for correlation IDs, message groups, etc.); in some cases that also includes the client library (such as for jms) or the network protocol (such as for http and amqp) or the fact that the network protocol can be chosen amongst a list of supported protocols (such as for ActiveMQ).

@GeraldLoeffler
Copy link
Collaborator Author

With all of that missing, I think it's hard but not imposible to make it for 2.1.0 of the spec. However, since the release cycle of the bindings is independent of the spec, you probably want to create a placeholder binding definition (like the one for jms, mercure, and many others) and then calmly work on this PR.

How does that sound? In any case, September is not far so that's also an option.

i'll go back to being on vacation now, so let's aim for the next slot (September).

@fmvilas
Copy link
Member

fmvilas commented Jun 25, 2021

Oh it's true. You were on vacations. Come on man! Don't want to hear from you until you're back 😄 Enjoy your holidays!

@GeraldLoeffler
Copy link
Collaborator Author

Can you elaborate more about the Server Binding Object. What is the point of having it really? in your binding example you just provide binding version, rest of the props just look like original Server object.

the idea was simply to

  1. state clearly that in this version of the binding spec (as potentially opposed to future versions) the binding spec does not define any properties
  2. to allow x- properties to be added to the server binding object, if they should be needed by an implementation. Without server binding object in the binding spec, there is no place to add these specifically for the Anypoint MQ binding.

@GeraldLoeffler
Copy link
Collaborator Author

  1. If this is implementation-independent, I think there should be a way to specify organization, environment, and other missing stuff.

it works as it is (without organization and environment) in an implementation-independent way, due simply to the inherent nature of authenication against the Anypoint MQ broker with a registered Anypoint MQ client app.

My proposal would be to publish the binding spec as it is, gain experience, and improve it, if needed, in a subsequent version, such as by adding organization and environment if found useful.

@fmvilas
Copy link
Member

fmvilas commented Jul 26, 2021

Other than my comment related to restricting to 2.0.0 version, it looks good to me. Happy to approve and merge once the change has been done.

@fmvilas
Copy link
Member

fmvilas commented Jul 26, 2021

@derberg I just realized you also have to approve. Can you have a look please?

As requested, removing mention of AsyncAPI version (`2.1.0`) this binding spec was developed against/for.
@GeraldLoeffler
Copy link
Collaborator Author

Other than my comment related to restricting to 2.0.0 version, it looks good to me. Happy to approve and merge once the change has been done.

removed that reference to the AsyncAPI version @fmvilas

over to you @derberg

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left few comments, I really think we should follow the approach of just putting This object MUST NOT contain any properties. Its name is reserved for future use. under the section where we do not have any binding object really. It is consistent with other bindings.

Also really confusing part is about Server Object, and really suggest IBM MQ binding approach.

Regarding organization and environment. I agree this can be added later. For me as not experienced user of Anypoint MQ it is super confusing that I do not have to provide them anywhere while all docs talk about it https://help.mulesoft.com/s/article/How-to-use-Anypoint-MQ-Stats-API but I'm a total noob here and fully rely on your expertise so let us leave it as it is

anypointmq/README.md Show resolved Hide resolved
anypointmq/README.md Show resolved Hide resolved
anypointmq/README.md Show resolved Hide resolved
@GeraldLoeffler
Copy link
Collaborator Author

commited the changes requested @derberg

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for your patience @GeraldLoeffler

@fmvilas I think you need to approve again and then we are ready to merge to master as we are anyway not doing releases in this repo yet

@derberg
Copy link
Member

derberg commented Sep 14, 2021

@GeraldLoeffler I want to merge but you need to update to latest master, I think you did not select option for maintainers to update the branch too

@GeraldLoeffler
Copy link
Collaborator Author

GeraldLoeffler commented Sep 15, 2021

@GeraldLoeffler I want to merge but you need to update to latest master, I think you did not select option for maintainers to update the branch too

i've now newly fetched from upstream (asyncapi:master)

@derberg derberg merged commit 061b32d into asyncapi:master Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants