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

Support jrpcv1 clients #44

Closed
roytman opened this issue Mar 13, 2021 · 4 comments
Closed

Support jrpcv1 clients #44

roytman opened this issue Mar 13, 2021 · 4 comments

Comments

@roytman
Copy link

roytman commented Mar 13, 2021

Hello, and thanks for awesome package.

I'd like to ask about a small extension.

If the server is started with the AllowV1 flag, it accepts jrpcv1 requests (requests without the jsonrpc version string), however, its responses include the version string, and as a result, v1 clients don't accept the responses.

I have an extension that allows sending responses according to corresponding requests. If AllowV1 is true and a request doesn't include the version string, its response will not include it either.

Server-initiated messages (Callbacks and Notifications) require a slightly different approach. One of the possible implementations is to store the client version in the Server struct.

What do you think about this extension?

@creachadair
Copy link
Owner

What you're describing is possible, but I'm not convinced it's a good fit for this module. I added the version tolerance settings based on the advice of https://www.jsonrpc.org/specification#compatibility, but there are several other differences between the two protocols beyond just the version marker.

Making this module support JSON-RPC 1.0 properly would need several other changes—the 1.0 standard models both ends of a connection as full peers, for example. Also, a strict 1.0 implementation requires object formats that 2.0 forbids, e.g., a successful response in 1.0 looks like {"id":1, "result":…, "error":null}, where as 2.0 does not permit both "result" and "error" to be set. I point this out not to defend strictness—but simply to say that if an existing client is already strict enough to reject responses because of the "jsonrpc":"2.0" field in the reply, it is likely to have other issues even if that is removed.

If the eventual goal is to swap an existing service from 1.0 to 2.0, it's probably better to set up separate endpoints to allow clients to be rolled over incrementally. Having said that, however, I'm interested to hear more about what you have in mind.

@roytman
Copy link
Author

roytman commented Mar 14, 2021

Thanks for your response.
I went again over JSON-RPC specs, and there is an additional difference between the versions: the error response format.
jrpcv1 doesn't define its structure, where jrpcv2 has a strict definition.
Even current implementation doesn't correctly parse jrpcv1 errors.

All mentioned differences do not require significant design changes, maybe you can consider adding them.

I try to reimplement jrpcv1 server, which should be able to send notifications to clients, which is not supported by the standard Golang libraries. I cannot add new endpoints to clients, because they are legacy code and out of my control.

@creachadair
Copy link
Owner

I went again over JSON-RPC specs, and there is an additional difference between the versions: the error response format.

Yes, definitely—my list of example differences wasn't comprehensive.

I try to reimplement jrpcv1 server, which should be able to send notifications to clients, which is not supported by the standard Golang libraries. I cannot add new endpoints to clients, because they are legacy code and out of my control.

I see—and from your comment above, it sounds like these legacy clients are sensitive about the format of request and response messages from the server.

Based on this discussion, I don't think 1.0 support is a good fit for this module. Having to plumb version sensitivity through the request flow is not technically that difficult, but it's a lot of little changes and I don't see a clear benefit. Indeed, perhaps even the existing AllowV1 switches might have been a mistake, since they don't go far enough to meaningfully work.

For your needs, I think you might be better off with a fork to make an entirely JSON-RPC 1.0 implementation. That would not require any extra plumbing—just drop the version markers from the wire format, and update the encoding/decoding logic to be more forgiving about errors. It wouldn't be 2.0 compliant anymore, but it sounds like for your needs that isn't important. The main caveat is that the server isn't a true "peer" in the 1.0 sense.

@roytman
Copy link
Author

roytman commented Mar 14, 2021

indeed meantime I work with a fork.
Thanks, I'll close this issue.

@roytman roytman closed this as completed Mar 14, 2021
creachadair added a commit that referenced this issue Nov 26, 2021
The intent of these options was to tolerate calls to/from JSON-RPC 1.0
implementations. That alone isn't enough for interoperability, however.

For example:

- Server replies still contain the v2 version marker, which (some) v1 clients
  do not accept.

- The v2 Error object has a stricter structure than v1, and the client only
  accepts v2.

Since this library implements v2 specifically, I do not think it's worthwhile
to add extra plumbing for those cases (e.g., tracking v1 shape through the
handler, tolerating arbitrary Error geometry). On that basis, the tolerance
settings are not carrying their weight.

For context see #44.

Updates #46
creachadair added a commit that referenced this issue Nov 26, 2021
The intent of these options was to tolerate calls to/from JSON-RPC 1.0
implementations. That alone isn't enough for interoperability, however.

For example:

- Server replies still contain the v2 version marker, which (some) v1 clients
  do not accept.

- The v2 Error object has a stricter structure than v1, and the client only
  accepts v2.

Since this library implements v2 specifically, I do not think it's worthwhile
to add extra plumbing for those cases (e.g., tracking v1 shape through the
handler, tolerating arbitrary Error geometry). On that basis, the tolerance
settings are not carrying their weight.

For context see #44.

Updates #46
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

No branches or pull requests

2 participants