server: Introduce IgnoreUnknownFields option

[radeksimko]

As mentioned in the comment, according to the JSON-RPC specification:

The absence of expected names MAY result in an error being generated.

The option therefore allows the server to choose between a strict or tolerant behaviour.

---

To put this into the context of a real use case:

I'm trying to build a Language Server with this library while also using https://github.com/sourcegraph/go-lsp for the structs. These structs aren't always up to date though and so the client which technically implements the same protocol (likely using a different LSP library and even a different language) may be "more complete". I'd like the Language Server to be tolerant of this.

[creachadair]

I'm not opposed. In fact, I originally implemented it without that restriction. I'd like to think a bit more about whether we can do this without quite so much plumbing. I'll try to follow up when I've had a chance to play with it some more.

[creachadair]

After more thought, I don't think this approach is going to work. The module assumes JSON decoding is context-free, meaning it does not distinguish between "decoding for server X" and "decoding for server Y". Even if we add a flag to the server and plumb it down to the request, there are at least three other places where we decode parameters that do not have access to that flag:

1. The handler package generates reflective stubs for other functions.
2. The jrpc2.ParseRequests helper parses one or more requests from a JSON text.
3. The client handler for server push parses "requests" received from the server.

We could address (3) by also adding a client option too, but (1) and (2) are not reachable that way.

I can think of a few options to move forward. Not all of these are mutually exclusive:

1. Revert 78545e2, which enabled strict parsing.
2. Add a package-level constructor hook that can be overridden by the caller.
3. Document that handlers that want non-default parsing behaviour should implement json.Unmarshaler on their argument type(s).

Note that (1) and (3) are miscible. If the default is strict parsing, we can get non-strict parsing by writing:

type T struct { /* … */ }

func (t *T) UnmarshalJSON(bits []byte) error {
  type U T
  return json.Unmarshal(bits, (*U)(t))
}

Working example here: https://play.golang.org/p/dSP28YQAuHM. This is admittedly a little ugly, but would be easy to automate with a go generate rule for types that need it. The same trick works in the other direction, but the boilerplate is more complex:

func (t *T) UnmarshalJSON(bits []byte) error {
  dec := json.NewDecoder(bytes.NewReader(bits))
  dec.DisallowUnknownFields()
  type U T
  return dec.Decode((*U)(t))
}

We could hide some of the boilerplate, but I found myself doing this a bunch and I thought that strict should be the default. I don't have any good data to suggest which default is "better" other than my own experience using it.

I don't particularly like option (2) at all, but I included it for completeness. Global state has to do some work to justify is existence, and I'm not seeing it here. At minimum it would make the tests a lot more complicated.

[radeksimko]

Thank you for explaining your thoughts in such detail.

I took some time to think about this too and I agree with your arguments against global state.

I first thought that asking consumers of this library to make sure the types they use are either complete or implement custom unmarshaler is a lot to ask.

However I then came to the conclusion that it's probably a good thing anyway if the types are expected to be behind and like you said it's not too hard to achieve with go generate.

I don't have a strong opinion on what should be the default, but I'm leaning more towards the current state. Explicit (opting into unknown fields) is better than implicit.

I'm just not sure what's the best place to document this though.

[creachadair]

I'm just not sure what's the best place to document this though.

I made an attempt in #8, I'd appreciate your feedback if you have time.

[radeksimko]

Closing in favour of #8