Repository: creachadair/jrpc2
Pull request #12 (2020-03-16 12:19:10)

Report what extra fields are passed in error
I was implementing slightly more complex I/O logic (partially to make server mock-able) and I managed to mistakenly mix up client and server writers and readers and it wasn't clear to me immediately that the server was processing its own responses as requests until I dug deeper and exposed some more details in the error message.

I feel this could save some other people valuable debugging time.

creachadair: Let me start by saying I think this change (or some variation of it) is worthwhile. and we should do it. That said, I also have a question:

> I was implementing slightly more complex I/O logic (partially to make server mock-able)

Can you talk a bit about why you would like to _mock_ the server instead of just using the real thing? It's pretty easy to set up a real server for testing purposes, and it's not expensive to do:

```go
loc := server.NewLocal(assigner, opts)
loc.Client.CallResult(ctx, "Blah", args, &rsp)
loc.Server.Stop()
err := loc.Server.Wait()
```

This works over a direct channel so it should be fast and lightweight.

> I feel this could save some other people valuable debugging time.

Agreed. I went to the trouble to capture the extra fields, but never made up my mind what to do with them. We should do _something_ better than dropping them on the floor. 😀 
creachadair: I implemented (2) to play with it: https://github.com/creachadair/jrpc2/pull/13

Options I considered but did not implement:

- A `Closed` field to explicitly distinguish channel close events. This would be easy to add, but it seems redundant with `Stopped` since they are mutually exclusive.
- Capturing the "original" error as well as the filtered one. I couldn't see a good use case for needing to get the specific error value.

Neither would be hard to add, though.
radeksimko: > Can you talk a bit about why you would like to mock the server instead of just using the real thing?

I think the short answer is because I haven't fully assessed that as an option. 😄 

The longer answer requires also answering what am I testing and what should I be testing. For example I recently added a test during which client sends EOF (essentially calls `Close` on the writer) to simulate scenario which I observed in the wild, which in turn resulted in opening https://github.com/creachadair/jrpc2/pull/11

Perhaps though after the EOF problem is resolved I may be able to simplify the implementation a bit and convince myself the need for testing this scenario is not as strong. I want to revisit these tests anyway and find out how to address the asynchronous nature of request processing which can makes these tests flaky.

Patch:
diff --git a/base.go b/base.go
index 8ac4d7a..9b9f19d 100644
--- a/base.go
+++ b/base.go
@@ -294,7 +294,7 @@ func (j *jrequest) parseJSON(data []byte) error {
 
 	// Report an error for extraneous fields.
 	if len(extra) != 0 {
-		j.fail(code.InvalidRequest, "extra fields in request")
+		j.err = DataErrorf(code.InvalidRequest, extra, "extra fields in request")
 	}
 	return nil
 }


Test patch:
diff --git a/jrpc2_test.go b/jrpc2_test.go
index 9edb96f..00cc484 100644
--- a/jrpc2_test.go
+++ b/jrpc2_test.go
@@ -561,7 +561,7 @@ func TestOtherClient(t *testing.T) {
 
 		// Extra fields on an otherwise-correct request.
 		{`{"jsonrpc":"2.0","id": 7, "method": "Z", "params":[], "bogus":true}`,
-			`{"jsonrpc":"2.0","id":7,"error":{"code":-32600,"message":"extra fields in request"}}`},
+			`{"jsonrpc":"2.0","id":7,"error":{"code":-32600,"message":"extra fields in request","data":["bogus"]}}`},
 
 		// An empty batch request should report a single error object.
 		{`[]`, `{"jsonrpc":"2.0","id":null,"error":{"code":-32600,"message":"empty request batch"}}`},