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(types): add lenient feature #1446

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

birktj
Copy link

@birktj birktj commented Aug 26, 2024

The lenient feature allows for parsing Requests and Notifications that doesn't have jsonrpc fields. This is useful for interacting with some implementations that are not completely standards-conforming.

I need this in order to communicate with Shelly devices: https://shelly-api-docs.shelly.cloud/gen2/General/RPCProtocol They are supposed to communicate with Jsonrpc 2.0, but don't supply jsonrpc fields.

This is of course very similar to #1385 except that it is behind a feature flag so that users have to opt-in to this behavior.

The `lenient` feature allows for parsing `Request`s and
`Notification`s that doesn't have `jsonrpc` fields. This is useful
for interacting with some implementations that are not completely
standards-conforming.
@birktj birktj requested a review from a team as a code owner August 26, 2024 15:39
@niklasad1
Copy link
Member

niklasad1 commented Aug 27, 2024

Hey, sounds like a reasonable approach to me with such a flag but it won't be able to omit the "jsonrpc field" when making notifications and requests which might break compatibility with some implementations.

So, the only thingy with lenient we assert that it's jsonrpc v2 request/notif but it could be in a jsonrpc v1 request/notif (there are some incompatibility stuff such as notifications is request with "id":null, result/error must be co-exist and could be more things that I don't remember). In addition the jsonrpc v1 specification doesn't specify the "jsonrpc" field so could be quite likely that calls without this field is a jsonrpc v1.

Sorry for long answer, but the conclusion is that if it's documented on the feature flag then that we assert that the call is jsonrpc v2 if the "jsonrpc":"2.0" field is missing then I'm okay to expose this feature.

/cc @jsdw @lexnv any other opinions?

@birktj
Copy link
Author

birktj commented Aug 27, 2024

Ah yes, I was thinking of adding a disclaimer like that, but totally forgot about it. What about something like:

The lenient feature might seem like it would support communicating with JSON-RPC 1.0 client/servers, but because of other subtle differences between the protocol versions this cannot be expected to work.

@jsdw
Copy link
Collaborator

jsdw commented Aug 27, 2024

The only thing with a feature flag is that if jsonrpsee is used elsewhere in the dependency tree, the feature flag will be cumulatively applied there too, making any other uses more "lenient" as well.

While I'm not against the approach (as long as this feature flag stuff is documented too), I'd also be in favour of adding some builder option like ".optional_jsonrpc_field()" or something so that it could be enabled for specific uses only.

@niklasad1
Copy link
Member

Yeah, I like it better remove this feature and add a build option to the clients .optional_jsonrpc_field API.
Then we need to make TwoPointZero -> Option<TwoPointZero> but I think that's better approach,

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.

3 participants