-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
RPC Discover endpoint with basic informations #18068
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
Conversation
|
Could be interested in this: @mweatherley and @purplg |
| let servers = match ( | ||
| world.get_resource::<crate::http::HostAddress>(), | ||
| world.get_resource::<crate::http::HostPort>(), | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very minor nit, but I guess it bothers me slightly that this isn't transparent to the transport used for BRP; I think ideally in the future (e.g. when we eventually include more detailed method information in the OpenRPC document) we'd have the server info be separately registered by the transport plugin in some kind of resource in the internals.
Just a thought!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I just wasn’t confident enough in any solution to include it.
|
@alice-i-cecile Something missing or is it ready for merge? |
|
This needs a second review: I would, but I've never worked with this flavor of thing before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall happy with this kind of functionality! 🥳 Helps both with discoverability and things like generating API clients.
Curious about the use of 1.0.0-rc1 spec instead of the latest spec.
The information included for the methods is pretty sparse currently. It would be nice (maybe in a follow up) to extend it with more stuff, possibly by extending RemoteMethods to hold more metadata like:
- Summary
- Description
- etc
| // /// Parameters for the RPC method | ||
| // #[serde(default)] | ||
| // pub params: Vec<Parameter>, | ||
| // /// The expected result of the method | ||
| // #[serde(skip_serializing_if = "Option::is_none")] | ||
| // pub result: Option<Parameter>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason these were left out?
params is required in OpenRPC 1.3.2.
resultis required in OpenRPC 1.0.0-rc1.
Maybe these could be included as $refs using some of the json_schema code? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it wasn't that easy: #16744 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, yeah it's a bit tricky that the methods are serde-based rather than bevy_reflect, but it also makes sense for usecases with non-reflectable data 🤔
There is https://github.com/GREsau/schemars which can generate JSON schemas for serde types. Not sure if we'd want to bring in another dep though.
I say include a (possibly empty) params either way though, just to be spec-compliant. There might be clients that rely on it being present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another route could be to add manual schemas for the BRP types, which we already kind of do in module docs: https://dev-docs.bevyengine.org/src/bevy_remote/lib.rs.html#99
Ideally those would not be duplicated though, so if there was a way to write them in one place and have them available both through OpenRPC and in bevy docs that would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argument about including params makes sense, done :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To have a full RPC Discover support the stuff you've described would be great. For the basic one I am aiming on atm what this PR provides is enough IMHO. But I would love to see it with proper params support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, this is a great start! Approving now 😄
Co-authored-by: Viktor Gustavsson <[email protected]>
|
@Leinnan ping me when this is passing CI and I'll get this merged for you. |
|
@alice-i-cecile ping ;) |
Objective
It does not resolves issue for full support for OpenRPC for
bevy_remote, but it is a first step in that direction. Connected to the #16744 issue.Solution
rpc.discoverendpoint to the bevy_remote which follows https://spec.open-rpc.org/#openrpc-document For now in methods array only the name, which is the endpoint address is populated.bevy_remote.Testing
Tested the commands by running the BRP sample( cargo run --example server --features="bevy_remote") and with these curl command:
The output is:
{ "jsonrpc": "2.0", "id": 1, "result": { "info": { "title": "Bevy Remote Protocol", "version": "0.16.0-dev" }, "methods": [ { "name": "bevy/mutate_component", "params": [] }, { "name": "bevy/insert", "params": [] }, { "name": "bevy/get", "params": [] }, { "name": "bevy/spawn", "params": [] }, { "name": "bevy/get+watch", "params": [] }, { "name": "bevy/destroy", "params": [] }, { "name": "bevy/list", "params": [] }, { "name": "bevy/mutate_resource", "params": [] }, { "name": "bevy/reparent", "params": [] }, { "name": "bevy/registry/schema", "params": [] }, { "name": "bevy/get_resource", "params": [] }, { "name": "bevy/query", "params": [] }, { "name": "bevy/remove_resource", "params": [] }, { "name": "rpc.discover", "params": [] }, { "name": "bevy/insert_resource", "params": [] }, { "name": "bevy/list_resources", "params": [] }, { "name": "bevy/remove", "params": [] }, { "name": "bevy/list+watch", "params": [] } ], "openrpc": "1.3.2", "servers": [ { "name": "Server", "url": "127.0.0.1:15702" } ] } }