Implement JSONRPC as wire protocol

pmlopes commented 4 years ago

Currently the wire protocol is a LV string, using the readme as protocol description. In order to make this bridge more efficient, it has been discussed in the past that it should adopt a standard format.

JSON-RPC is the closest we can get to this. Some work has already been started as a prototype but it is far from finished.

https://github.com/vert-x3/vertx-tcp-eventbus-bridge/tree/experimental/jsonrpc

This issue should track the implementation of this new wire protocol.

gracebish commented 3 years ago

@pmlopes, I am an intern with outreachy and would like to get started. I have labored to read the documentation and would like to claim an issue for work. Kindly guide me on how to get started.

pmlopes commented 3 years ago

Hi @gracebish welcome!

There are a couple of tasks you can already start to get acquainted with the project.

As you can see from this github repository you will need some basics:

Setup a working Java Development Environment. I'd recommend you to pick the right one for your Operation System here: https://adoptium.net/
You will need a working IDE, personally I prefer: https://www.jetbrains.com/idea/download/ (community edition is fine) but if you're used Visual Studio code before and would rather choose it, that is also fine: https://code.visualstudio.com/
You will need to install Apache Maven (our build tool): http://maven.apache.org/ If you use IntelliJ IDEA as IDE, I believe it already comes with it so you may optionally skip this step if you want to fully rely on the IDE

Once the setup is complete, I'd recommend you to clone this repository locally and try to build it. If you have installed maven you only need to run from the command line:

mvn verify

Congratulations, you just completed the first build of this project. Please have no hesitation to look at the code, try to identify any area you would like to know more, or have questions.

Ok, the second step is more interesting. In the README you can see all about the current protocol, although it's not complex, we would like to have it changed to JSON-RPC. To know more about JSON-RPC you should read a bit about it here: https://www.jsonrpc.org/specification we only care for 2.0.

If you require supplemental information, please don't hesitate to ping me. And we can continue with the explorations of the project.

Meanwhile, don't forget to join the discord server: https://discord.gg/KzEMwP2 where you can chat with the whole community!

gracebish commented 3 years ago

@pmlopes Thank you for the guidance. I however have got some set back while running 'mvn verify'. It seems to pull dependencies from another repo from the previous open source organization. When i visited my .m2 file, there is a repository from which it tries to fetch the dependencies. Should i delete that particular repo? Kindly advise me on how to avoid this.

nbrendah commented 3 years ago

Hello @pmlopes

I am an Outreachy Applicant for the December 2021 interneship round and i have interest in working on the project, Implement an event-bus bridge using JSON-RPC2 as wire protocol I have opened up a Pull Request on this feature. https://github.com/vert-x3/vertx-tcp-eventbus-bridge/pull/63 I kindly request you to review. I am warmly waiting for suggestions, recommendations,

Thank you so much.

pmlopes commented 3 years ago

@gracebish, @nbrendah

Here's a full (I think I've covered all the calls) translation of the messages.

A non-goal is to externalize this protocol into a package of java classes, so the same protocol can be used over:

vertx-web (websocket, for example)

I've written the examples using the vscode conventions:

A method name is composed as: {address|$}[/targetMethod]

Where address or "$" (reserved for system calls) is always present and optionally a "/targetMethod" is provided.

The protocol changes are as follows:

Basic messaging

send

// current protocol
{type: "send", address: "eventbusAddress", body: {}, headers: {}, replyAddress: "uuid"}
// with jsonrpc
{method: "eventbusAddress", params: {}, id: "uuid"}

publish

// current protocol
{type: "publish", address: "eventbusAddress", body: {}, headers: {}}
// after
{method: "eventbusAddress", params: {}}

Basic actions

register

// current protocol
{type: "register", address: "eventbusAddress", headers: {}}
// after
{method: "$/register", params: [eventbusAddress], id: "uuid"}
// id is required in order to receive a ack that the method succeeded

unregister

// current protocol
{type: "unregister", address: "eventbusAddress", headers: {}}
// after
{method: "$/unregister", params: [eventbusAddress], id: "uuid"}
// id is required in order to receive a ack that the method succeeded

ping

// current protocol
{type: "ping"}
// after
{method: "$/ping", id: "uuid"}
// id is required in order to receive a ack that the method succeeded

Proxies

invoke

// current protocol
{type: "send", address: "eventbusAddress", body: {}, headers: {action: "methodName"}, replyAddress: "uuid"}
// after
{method: "eventbusAddress/methodName", params: {}, id: "uuid"}

Well known replies

pong

// current protocol
{type: "pong"}
// after
{result: "pong"}

err

// current protocol
{type: "err", message: "access_denied|address_required|unknown_address|unknown_type"}
// after
{error: {code: -32601, message: "access_denied|address_required|unknown_address|unknown_type"}, id: "uuid"}
// where id is used to correlate the source request, order is not relevant in jsonrpc

Custom methods over the bridge

message

// current protocol
{type: "message", address: "destinationAddress", body: {}, headers: {}, replyAddress: "uuid", send: true}
// after (if send == true)
{method: "destinationAddress", params: {}, id: "uuid"}
// after (if send == false)
{method: "destinationAddress", params: {}}

This is the closest you can get to map between vertx eventbus messages and jsonrpc it will introduce some challenges regarding message formatting and transformation at the bridge. As the message format is different this can introduce some complexities with a message converter.

A second option is to, instead of having a mapping of jsonrpc method to an eventbus address, use the eventbus api directly:

For example:

ping

{ method:"ping", id:"uuid" }

send

{ method:"send", params{address: "eventBusAddress", params:{...}},id:"uuid"}

pmlopes commented 3 years ago

It is important to notice that there is no need to use a jsonrpc library, in the branch mentioned above, there is already a parser and encoder capable to handle the same messages that Visual Studio Code library does.

I think you should have a look (if you haven't yet) at:

https://github.com/vert-x3/vertx-examples/tree/4.x/core-examples/src/main/java/io/vertx/example/core/eventbus

Here you can experiment already with a few examples and see what the eventbus can do. Then the next step is to adapt those examples to use the current bridge and probably try to connect to it using either java, or javascript:

https://github.com/vert-x3/vertx-eventbus-bridge-clients

When you're familiar with the code, my suggestion is to create a large integration test where in each test you can test each kind of message. This will help you later to start changing the messages and have something to verify if everything is working together ok.

nbrendah commented 3 years ago

Thank you @pmlopes for this great explanation. For sure this explains everything and it makes things clear about how this whole requirements. I have accessed a number of resources where JSON-RPC was implemented. I was puzzled with the perceiving functionalities using existing TPC implementations.

I thought there were other initiation tasks, that we weren't going to dive straight into working on the real required product, but, however, for a greater picture of the journey before us, going this way is far a better option.

I am pusing to my first PR on this on Saturday Evening or Sunday early morning. The next time i tag you, i will have a PR on how i understood all this

@pmlopes How i wish i had a word other-than Thank you to bring out how much am grateful for your support and good work. Thank you de trop @pmlopes

pmlopes commented 3 years ago

Hi @nbrendah and @gracebish , maybe it wasn't clear for all of us (it's my 1st outreachy too). In this period what I'd like you to do is to get a good picture of the project ahead. The implementation should come later.

I'd like you to understand the requirements and notice that there are some details we need to plan how they are going to be solved.

For example, vertx event bus messages may include headers. Json RPC doesn't, but, vs code streaming JSON does support it like they exist in http.

The branch I've provided kinda handles headers like vs code. The thing is, for strict JSON RPC libraries this will not work. However headers are only used in some cases, can we make a decision on make them optional?

Or should we just use headers in the message body directly as part of the method call?

What I'm trying to say is that I'd like to have you participate in the decisions, not just write code!

gracebish commented 3 years ago

@pmlopes, thank you for your enlightenment. The headers should be optional.

On Fri, Oct 22, 2021 at 6:45 PM Paulo Lopes @.***> wrote:

Hi @nbrendah https://github.com/nbrendah and @gracebish https://github.com/gracebish , maybe it wasn't clear for all of us (it's my 1st outreachy too). In this period what I'd like you to do is to get a good picture of the project ahead. The implementation should come later.

I'd like you to understand the requirements and notice that there are some details we need to plan how they are going to be solved.

For example, vertx event bus messages may include headers. Json RPC doesn't, but, vs code streaming JSON does support it like they exist in http.

The branch I've provided kinda handles headers like vs code. The thing is, for strict JSON RPC libraries this will not work. However headers are only used in some cases, can we make a decision on make them optional?

Or should we just use headers in the message body directly as part of the method call?

What I'm trying to say is that I'd like to have you participate in the decisions, not just write code!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-tcp-eventbus-bridge/issues/56#issuecomment-949748424, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOPD5MERVJGB6BMBIFT5APLUIGBIXANCNFSM4R7HVPUQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

pmlopes commented 3 years ago

The question about headers is how to handle security. For example a common pattern is to use headers like in http:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization

If we make them optional we need to specify the message format to clearly identify them or have a way to signal clients that support headers.

nbrendah commented 3 years ago

Hey @pmlopes, it has taken me quite some time to kinder figure out this project. Actually, each time i looked into the code base, i got new aspect.
I cloned the experimental/jsonrcp branch and took a look at its current state of the code. I did some tweaks to make maven build work while ignoring existing tests.

I released that implementations have been done for register, unregister, send, publish, ping and some response mapping like err. I also came across some to-do in the source code. But, however, all JsonRPCStreamEventBusBridgeTest test cases were failing.

As a requirement for outreachy interneship, an applicant must make at-least one contribution. Yes, i made https://github.com/vert-x3/vertx-tcp-eventbus-bridge/pull/63 but i wouldn't wish to submit it as the only contribution because its even off topic.

I am of a view, in the remaining few days, how about i make at-least only one test method pass, and then submit that as my contribution. Might it be too much to handle in the remaining days?

In outreachy applications, we are required to create a good timeline for our project. As i am filled with some knowledge about what we are going to create, i am uncomfortable with task assessment for the whole project. Maybe, if you got some free time, you can help us with task assessment in terms of priority and time. If this is our requirement like in some other internships, I will try to be as realistic as possible.

If i manage making the failing tests finally work, should i push on the branch experimental/jsonrcp or just like normal, (creating a new branch reflecting the issue number)

I had promised a PR but i guess next time.

Thank you @pmlopes for using this time to make us understand the project, otherwise it would be more hard for us in the near future.

pmlopes commented 3 years ago

@gracebish @nbrendah

The core work for the project can be defined as:

Fork the repository and switch to the branch "experimental/jsonrpc"
Create a new Integration test (or tests) based on:

JsonRPCStreamEventBusBridgeTest

Define all the message types:

client send message not expecting a response
client sends a message expecting a response
client subscribes to a channel, server sends a reply
client subscribes to a channel, server publishes a reply
client subscribes to a channel, server sends multiple replies
client unsubscribes a channel, server sends a message and it is not received on the client
client send ping and expect pong

Note that currently the test class I mentioned do not pass all tests, this is because the implementation may be broken.

You should disable it and implement your tests. As you start implementing the tests you may find that the implementation is broken, in that case I'd like you to discuss how to fix it to pass the test.

We had a discussion on the format of the messages, but let's start simple and improve later.

Initially lets assume that the bridge only handles 5 methods:

send
publish
register
unregister
ping

Send and publish commands should have a json object as params:

{
  address: String, 
  headers: JsonObject,
  body: any
}

We may consider using an array keeping the order as described in the object above.

Register/Unregister should have as params a single string, so the JsonArray should look like:

[
  address: String
]

Ping should not need any params.

nbrendah commented 3 years ago

Hello @pmlopes I have opened up a draft https://github.com/vert-x3/vertx-tcp-eventbus-bridge/pull/67 for the first test case client send message not expecting a response but it is still a WIP I am open for any kind of discussion, video calls, discord community interaction, etc

Thank you so much

vert-x3 / vertx-tcp-eventbus-bridge