creachadair / jrpc2

A Go implementation of a JSON-RPC 2.0 server and client.
BSD 3-Clause "New" or "Revised" License
62 stars 10 forks source link

Optional parameter support in arrays #113

Closed 2opremio closed 5 months ago

2opremio commented 8 months ago

We are using jrpc2 for Stellar's Soroban RPC server.

We incorporated a new parameter to a method and made it optional, with the hope of making the change backwards compatible. Specifically, we changed:

type SimulateTransactionRequest struct {
    Transaction    string                    `json:"transaction"`
}

to

type SimulateTransactionRequest struct {
    Transaction    string                    `json:"transaction"`
    ResourceConfig *preflight.ResourceConfig `json:"resourceConfig,omitempty"`
}

This worked fine when passing a JSON object:

{
  "jsonrpc": "2.0",
  "id": 0,
  "method": "simulateTransaction",
  "params": {"transaction":"AAAAAgAAAADRjwIQ/2zB8tzxMB+71MMO4RoHWCBoTUcd+J0PEBHqKAAAAGQAAAF4AAAAAgAAAAAAAAAAAAAAAQAAAAEAAAAA0Y8CEP9swfLc8TAfu9TDDuEaB1ggaE1HHfidDxAR6igAAAAYAAAAAgAAAnMAYXNtAQAAAAEUBGABfgF+YAJ/fgBgAn5+AX5gAAACDQIBaQEwAAABaQFfAAADBgUBAgMDAwUDAQAQBhkDfwFBgIDAAAt/AEGAgMAAC38AQYCAwAALBy8FBm1lbW9yeQIAA2FkZAADAV8ABgpfX2RhdGFfZW5kAwELX19oZWFwX2Jhc2UDAgqMAgVdAgF/AX4CQAJAIAGnQf8BcSICQcAARg0AAkAgAkEGRg0AQgEhA0KDkICAgAEhAQwCCyABQgiIIQFCACEDDAELQgAhAyABEICAgIAAIQELIAAgATcDCCAAIAM3AwALmQEBAX8jgICAgABBIGsiAiSAgICAACACQRBqIAAQgoCAgAACQAJAIAIoAhANACACKQMYIQAgAiABEIKAgIAAIAIpAwCnDQAgACACKQMIfCIBIABUDQECQAJAIAFC//////////8AVg0AIAFCCIZCBoQhAAwBCyABEIGAgIAAIQALIAJBIGokgICAgAAgAA8LAAALEISAgIAAAAsJABCFgICAAAALBAAAAAsCAAsASw5jb250cmFjdHNwZWN2MAAAAAAAAAAAAAAAA2FkZAAAAAACAAAAAAAAAAFhAAAAAAAABgAAAAAAAAABYgAAAAAAAAYAAAABAAAABgAeEWNvbnRyYWN0ZW52bWV0YXYwAAAAAAAAABQAAAAAAG8OY29udHJhY3RtZXRhdjAAAAAAAAAABXJzdmVyAAAAAAAABjEuNzQuMQAAAAAAAAAAAAhyc3Nka3ZlcgAAAC8yMC4wLjMjOTNiMDllNDJlNGVmYTg0MWNiZDAzNGMwYmZmMGRjMzYyNzY1MDg2YwAAAAAAAAAAAAAAAAAA"}
}

However, this didn't work when passing an array. The following request led to an error:

{
  "jsonrpc": "2.0",
  "id": 0,
  "method": "simulateTransaction",
  "params": ["AAAAAgAAAADRjwIQ/2zB8tzxMB+71MMO4RoHWCBoTUcd+J0PEBHqKAAAAGQAAAF4AAAAAwAAAAAAAAAAAAAAAQAAAAEAAAAA0Y8CEP9swfLc8TAfu9TDDuEaB1ggaE1HHfidDxAR6igAAAAYAAAAAgAAAnMAYXNtAQAAAAEUBGABfgF+YAJ/fgBgAn5+AX5gAAACDQIBaQEwAAABaQFfAAADBgUBAgMDAwUDAQAQBhkDfwFBgIDAAAt/AEGAgMAAC38AQYCAwAALBy8FBm1lbW9yeQIAA2FkZAADAV8ABgpfX2RhdGFfZW5kAwELX19oZWFwX2Jhc2UDAgqMAgVdAgF/AX4CQAJAIAGnQf8BcSICQcAARg0AAkAgAkEGRg0AQgEhA0KDkICAgAEhAQwCCyABQgiIIQFCACEDDAELQgAhAyABEICAgIAAIQELIAAgATcDCCAAIAM3AwALmQEBAX8jgICAgABBIGsiAiSAgICAACACQRBqIAAQgoCAgAACQAJAIAIoAhANACACKQMYIQAgAiABEIKAgIAAIAIpAwCnDQAgACACKQMIfCIBIABUDQECQAJAIAFC//////////8AVg0AIAFCCIZCBoQhAAwBCyABEIGAgIAAIQALIAJBIGokgICAgAAgAA8LAAALEISAgIAAAAsJABCFgICAAAALBAAAAAsCAAsASw5jb250cmFjdHNwZWN2MAAAAAAAAAAAAAAAA2FkZAAAAAACAAAAAAAAAAFhAAAAAAAABgAAAAAAAAABYgAAAAAAAAYAAAABAAAABgAeEWNvbnRyYWN0ZW52bWV0YXYwAAAAAAAAABQAAAAAAG8OY29udHJhY3RtZXRhdjAAAAAAAAAABXJzdmVyAAAAAAAABjEuNzQuMQAAAAAAAAAAAAhyc3Nka3ZlcgAAAC8yMC4wLjMjOTNiMDllNDJlNGVmYTg0MWNiZDAzNGMwYmZmMGRjMzYyNzY1MDg2YwAAAAAAAAAAAAAAAAAA"]
}
jsonrpc error: ErrorObject { code: InvalidParams, message: "invalid parameters", data: Some(RawValue("[-32602] got 1 parameters, want 2")) }

You can find the full context at the downstream issue: https://github.com/stellar/soroban-rpc/issues/13

We are aware that JSON-RPC doesn't formally support optional parameters in arrays (but, in the examples, it does mention optional parameters when passing an object).

However, I was thinking that we could simply assume they were passed in order (which basically assumes that once a parameter is considered optional so are the rest).

A special struct tag could be used for extra safety if you deem it necessary, e.g.

type SimulateTransactionRequest struct {
    Transaction    string                    `json:"transaction"`
    ResourceConfig *preflight.ResourceConfig `json:"resourceConfig,omitempty" jrpc2:"arrayopt"`
}

@creachadair what are your thoughts? I could work on a PR if you find this acceptable.

creachadair commented 8 months ago

I'm not convinced this is is something we should add to the library. Is there some reason the caller cannot use object notation when calling this API?

The main trouble with optional parameters in array notation is that optional elements can occur anywhere. Even in the example above where there are only two parameters, when the decoder sees, say:

["value1"]

it does not have any reliable way to know which parameter is omitted, e.g., it could be either of these:

["value1", null]
[null, "value1"]

The problem only gets worse with more parameters. Even explicit tags don't solve the problem, e.g., suppose I have parameters A, B, C, D where B and C are "marked" as optional. When the request contains ["x", "y", "z"] does that mean A="x", B="y", D="z" or A="x", C="y", D="z"? And so on.

One could take the approach that Python does, where optional positional parameters can only occur at the end of the list. Even there, however, users often confuse the order of optional parameters. At least in the REPL a user has an easy way to see what went wrong. In an RPC, it's much harder to debug something like this, especially when it's the end user who's likely to get the error rather than the developer.

I'm not completely opposed to relaxing the "exactly equal" constraint on arguments to "less than or equal", but there's already a lot of magic happening here, and I worry that would only make issues harder to debug.

creachadair commented 8 months ago

Also, I should note, for the simple case outlined in your original comment, there's a pretty straightforward workaround: https://go.dev/play/p/Pk5yG3WlatX

2opremio commented 8 months ago

I agree, there is no optimal solution I can currently think of.

I was advocating for the Python way, which comes with its own downsides as you describe.

The reality is, however, that without a way to pass optionals in arrays or disabling them completely (and I don’t think there is a way to do that today either), optionals cannot really be used reliably for backwards compatibility on a public API (where you don’t control the clients).

creachadair commented 8 months ago

The reality is, however, that without a way to pass optionals in arrays or disabling them completely (and I don’t think there is a way to do that today either), optionals cannot really be used reliably for backwards compatibility on a public API (where you don’t control the clients).

Yes, that's true, but in a public API with an array-based argument list, there is already no non-breaking way to add optional parameters. In which case, another option would be to add a new API that requires an object parameter, and let clients migrate to it at their own pace.

2opremio commented 8 months ago

Agreed. Would you be open to add a configuration option which disables array parameters?

2opremio commented 8 months ago

Also, it would be interesting to know how other libraries are dealing with this. They may have come with a better solution.

creachadair commented 8 months ago

Agreed. Would you be open to add a configuration option which disables array parameters?

Yes, that seems like a reasonable thing to add: https://github.com/creachadair/jrpc2/pull/114

Also, it would be interesting to know how other libraries are dealing with this. They may have come with a better solution.

That's a good question, and I don't know the answer. By far the most prevalent use of JSON-RPC I'm aware of is LSP, and as far as I know none of its implementations use array-formatted parameters. That said, I haven't done any kind of exhaustive survey.