Closed wehowski closed 4 years ago
1ma
commented
4 years ago Hi Till, thank you for your kind words.
I wasn't aware of that specification, but this looks useful and at first glance seems like it could be implemented without causing any breaking changes. I'll read it carefully and get back to you on this.
1ma
commented
4 years ago Alright, so there seems to be a major no-go reason for supporting this OpenRPC spec in the library. Despite it claims to be 100% compatible with JSON-RPC 2.0 it is actually not.
From section 5, legal JSON-RPC 2.0 responses are JSON objects containing the jsonrpc and id parameters in all cases, then result on successful responses or error on failed ones. On the other hand OpenRPC mandates openrpc, info, methods and several other optional parameters, so there is not even a bit of overlap.
There seems to be another service discovery specification floating around (Service Mapping Description), but it has the same problem. Even worse, it burdens itself with the responsibility of documenting things like HTTP methods and URLs, which are completely out of scope of JSON-RPC 2.0 (the same JSON-RPC 2.0 server can run behind a WebSocket, a raw TCP socket or even a command line application).
So as it stands the Success value object from my library is totally incompatible with OpenRPC, and supporting OpenRPC would mean adding and ad-hoc execution path in Server that would bypass the rest of the codebase and omit the JSON-RPC 2.0 rules whenever the method was rpc.discover, and I won't do that (though you are free to fork the library).
How-e-ver, if OpenRPC is not a hard requirement you can still implement a discovery endpoint within the means of the current library, it would go along these lines:
// services.json
{
"methods": [
{
"name": "rpc.discover",
"description": "Subtracts two integer numbers",
"parameters": null,
"result": null
},
{
"name": "subtract",
"description": "Subtracts two integer numbers",
"parameters": {
"minuend": "The first number",
"subtrahend": "The second number"
},
"result": "https://path/to/a/json/schema.json"
},
{
"name": "adder",
"description": "Adds an array of integers together",
"parameters": "An array of integers",
"result": "https://path/to/another/json/schema.json"
}
]
}I just made this format up. The point is that you would document your API however you see fit.
// ServiceDiscovery.php
namespace Some\Project;
use UMA\JsonRpc;
class ServiceDiscovery implements JsonRpc\Procedure
{
private const SERVICES_DEFINITION_PATH = __DIR__ . '/path/to/services.json';
/**
* @var stdClass
*/
private $serviceDiscoveryDocument;
public function __construct()
{
$this->discoveryDocument = json_decode(file_get_contents(self::SERVICES_DEFINITION_PATH), true);
}
public function __invoke(JsonRpc\Request $request): JsonRpc\Response
{
return new JsonRpc\Success($request->id(), $this->discoveryDocument);
}
public function getSpec(): ?stdClass
{
return null;
}
}Then you would simply register the procedure in the dependency injection container and map it
to the rpc.discover method using the regular Server::set() method. The above JSON document would be rendered inside the result key of the responses. Would that work for your use case?
wehowski
commented
4 years ago Alright, so there seems to be a major no-go reason for supporting this OpenRPC spec in the library. >Despite it claims to be 100% compatible with JSON-RPC 2.0 it is actually not.
From section 5, legal JSON-RPC 2.0 responses are JSON objects containing the jsonrpc and id >parameters in all cases, then result on successful responses or error on failed ones. On the other >hand OpenRPC mandates openrpc, info, methods and several other optional parameters, so there is >not even a bit of overlap.
Mh, could it be that there is a misunderstanding by you ore me?
The OpenRPC schema spec (how I do understand it) is NOT the schema of a JSON-Rpc RESPONSE, it is the schema of THE result MEMBER OF A RESPONSE in this case the result member of the result object of the rpc.discover method.
So as it stands the Success value object from my library is totally incompatible with OpenRPC No it is not I believe: How you said the OpenRPC spec is
out of the scopeof the JSON-RPC 2.0 spec. In no way, I would like to break or extend the json-rpc specification. It is just ANOTHER specification you CAN use to provide additional metadata to clients using your API.
I just implemented frdl/json-rpc-server-dicoverable using your library and it works!
You can try it out at my endpoint:
Request-Header: Authorization: digest, username="false", uri="https://domainundhomepagespeicher.webfan.de/software-center/modules-api/rpc/0.0.2/", response="52e66a027b15ad92d87d8dacda2fcee5", nc=00000001, cnonce="5e18f9ba-8a-32268611-NaN", nonce="1", qop="1" Content-Type: text/plain;charset=UTF-8 Origin: https://webfan.de User-Agent: Mozilla/5.0 (iPhone; CPU iPhone OS 11_0 like Mac OS X) AppleWebKit/604.1.38 (KHTML, like Gecko) Version/11.0 Mobile/15A372 Safari/604.1 X-Authorization: digest, username="false", uri="https://domainundhomepagespeicher.webfan.de/software-center/modules-api/rpc/0.0.2/", response="52e66a027b15ad92d87d8dacda2fcee5", nc=00000001, cnonce="5e18f9ba-8a-32268611-NaN", nonce="1", qop="1" X-Requested-With: XMLHttpRequest X-Webfan-Rpc-Auth-Cnonce: 5e18f9ba-8a-32268611-NaN X-Webfan-Rpc-Auth-Token: 52e66a027b15ad92d87d8dacda2fcee5 X-Webfan-Rpc-Auth-User: false
Request-Body: {jsonrpc: "2.0", id: 1578695098151, method: "rpc.discover"}
Test in the browser:
require(['Webfan.hps.rpc'], function(rpc){
rpc.client.call('rpc.discover').then(function(r){
require.main.frdl.alert.success(JSON.stringify(r), 0);
});
});though you are free to fork the library I do not like to fork it, because I think your library is fine how it is and does what it should and I respect authorship. Instead I would like to require it in my composer.json and extend the classes, and that is what I did already.
But I run into the issue that I cannot extend the Server class properly because you are using private properties!!! That is the main reason causing this issue here.
In the best case the Server class and the Procedure must not know of each other!
Would that work for your use case? Well, some of my condition are:
- The metadata should be a well known standard
- I want to automate documentation and metadata from source code or object instances,
I do not want to maintain additonal services.json documents.
Finally, I believe to single changes to the Sever class would help a lot for my issue:
mfg P.S.: Thank you for the link to Service Mapping, I will take a look... The OpenRPC specification for the params would be easier if it where simply a schema-7 like in your library.
1ma
commented
4 years ago Mh, could it be that there is a misunderstanding by you ore me? The OpenRPC schema spec (how I do understand it) is NOT the schema of a JSON-Rpc RESPONSE, it is the schema of THE result MEMBER OF A RESPONSE in this case the result member of the result object of the rpc.discover method.
Ah could be, though I didn't get that impression from the specification, it doesn't mention it. For instance, it says:
It is RECOMMENDED that the OpenRPC document be named: openrpc.json. Tooling that requires an OpenRPC document as input MAY assume the default document location to be ./openrpc.json, where the ./ represents the current working directory.
Furthermore the samples from their repository aren't embedded into JSON-RPC 2.0 responses either. But I haven't seen any other live OpenRPC APIs than yours so I'm not 100% sure.
Anyway, I could declare the Server properties as protected. Are you sure you'd still need a public getMethods() method then?
wehowski
commented
4 years ago If the methods are protected one could add the getMethods function in an extended class easy.
1ma
commented
4 years ago I've released v2.1.2 with those changes. Thanks for your feedback.
Hello, first of all thank you for the great work!
I was looking for a dicover protocol and found https://spec.open-rpc.org/ Maybe you like to implement it in your library? I did a first try: https://github.com/frdl/json-rpc-server-dicoverable