Repository: creachadair/jrpc2
Pull request #6 (2020-02-20 20:46:41)

Allow header framed messages to omit Content-Type.
Rework the Header framing, moving the existing implementation to StrictHeader. The latter still reports an error for content-type mismatches, but does so in a way that permits the caller to detect it.

The Header framing is now a light wrapper for StrictHeader that filters out these mismatches. The LSP framing now permits the sender to omit the content-type field without reporting an error.

Addresses #4.

creachadair: That's a good point. Are you OK with allowing no Content-Type to be set at all, but if it's set it must match? Or do you think it should be an error for the first case as well?
radeksimko: > Are you OK with allowing no Content-Type to be set at all, but if it's set it must match? Or do you think it should be an error for the first case as well?

From what I have seen in the wild (implementations of existing LSP clients) it's either valid Content-Type or no Content-Type header at all with the latter being overwhelmingly more common. So missing content-type should _not_ be treated as error in this context.

Unfortunately I cannot tell if there is any other context outside of LSP which requires implementation that ignores both incorrect content-type _and_ missing one and still sends correct one in outgoing requests.
creachadair: > From what I have seen in the wild (implementations of existing LSP clients) it's either valid Content-Type or no Content-Type header at all with the latter being overwhelmingly more common. So missing content-type should _not_ be treated as error in this context.

That makes sense to me, done.

> Unfortunately I cannot tell if there is any other context outside of LSP which requires framing that ignores both incorrect content-type _and_ missing one and still sends correct one in outgoing requests.

You are the first person other than myself to report using this package, so I think this approach is fine for now: If it causes other problems later, we can revisit the issue. 🙂

Patch:
diff --git a/channel/hdr.go b/channel/hdr.go
index 7d0e573..97b3579 100644
--- a/channel/hdr.go
+++ b/channel/hdr.go
@@ -4,13 +4,14 @@ import (
 	"bufio"
 	"bytes"
 	"errors"
+	"fmt"
 	"io"
 	"strconv"
 	"strings"
 )
 
-// Header defines a framing that transmits and receives messages using a header
-// prefix similar to HTTP, in which mimeType describes the content type.
+// StrictHeader defines a framing that transmits and receives messages using a
+// header prefix similar to HTTP, in which mimeType describes the content type.
 //
 // Specifically, each message is sent in the format:
 //
@@ -27,10 +28,15 @@ import (
 //    \r\n
 //    123\n
 //
-// If mimeType == "", the Content-Type header is omitted. Note, however, that
-// the framing function returned by Header does not verify that the encoding of
-// messages matches the declared mimeType.
-func Header(mimeType string) Framing {
+// If mimeType == "", the Content-Type header is omitted when sending.
+//
+// If the content type of an otherwise-valid received message does not match
+// the expected value, Recv returns the decoded message along with an error of
+// concrete type *ContentTypeMismatchError.
+//
+// Note: The framing returned by StrictHeader does not verify the encoding of a
+// message matches the declared mimeType.
+func StrictHeader(mimeType string) Framing {
 	return func(r io.Reader, wc io.WriteCloser) Channel {
 		var ctype string
 		if mimeType != "" {
@@ -46,6 +52,17 @@ func Header(mimeType string) Framing {
 	}
 }
 
+// A ContentTypeMismatchError is reported by the Recv method of a Header
+// framing when the content type of the message does not match the type
+// expected by the channel.
+type ContentTypeMismatchError struct {
+	Got, Want string // the observed and expected content type values
+}
+
+func (c *ContentTypeMismatchError) Error() string {
+	return fmt.Sprintf("content type mismatch: got %q, want %q", c.Got, c.Want)
+}
+
 // An hdr implements Channel. Messages sent on a hdr channel are framed as a
 // header/body transaction, similar to HTTP.
 type hdr struct {
@@ -71,7 +88,10 @@ func (h *hdr) Send(msg []byte) error {
 	return err
 }
 
-// Recv implements part of the Channel interface.
+// Recv implements part of the Channel interface. If the content type of the
+// received message does not match the expected value, Recv returns the decoded
+// message along with an error of concrete type *ContentTypeMismatchError.  The
+// caller may choose to ignore this error by testing explicitly for this type.
 func (h *hdr) Recv() ([]byte, error) {
 	var contentType, contentLength string
 	for {
@@ -97,9 +117,11 @@ func (h *hdr) Recv() ([]byte, error) {
 		}
 	}
 
-	// Verify that the content-type matches what we expect.
+	// Verify that the content-type matches what we expect, but defer reporting
+	// it until the message has been fully decoded.
+	var contentErr error
 	if contentType != h.mtype {
-		return nil, errors.New("invalid content-type")
+		contentErr = &ContentTypeMismatchError{Got: contentType, Want: h.mtype}
 	}
 
 	// Parse out the required content-length field.
@@ -122,12 +144,34 @@ func (h *hdr) Recv() ([]byte, error) {
 	if _, err := io.ReadFull(h.rd, data[:size]); err != nil {
 		return nil, err
 	}
-	return data[:size], nil
+	return data[:size], contentErr
 }
 
 // Close implements part of the Channel interface.
 func (h *hdr) Close() error { return h.wc.Close() }
 
+// Header returns a framing that behaves as StrictHeader, but allows received
+// messages to omit the Content-Type header without error. An error will still
+// be reported if a content-type is set but does not match.
+func Header(mimeType string) Framing {
+	strict := StrictHeader(mimeType)
+	return func(r io.Reader, wc io.WriteCloser) Channel {
+		return opthdr{strict(r, wc).(*hdr)}
+	}
+}
+
+// An opthdr is a wrapper around hdr that filters out the error reported when
+// the inbound message does not specify a content-type.
+type opthdr struct{ *hdr }
+
+func (o opthdr) Recv() ([]byte, error) {
+	msg, err := o.hdr.Recv()
+	if v, ok := err.(*ContentTypeMismatchError); ok && v.Got == "" {
+		err = nil
+	}
+	return msg, err
+}
+
 // LSP is a header framing (see Header) that transmits and receives messages on
 // r and wc using the MIME type application/vscode-jsonrpc. This is the format
 // preferred by the Language Server Protocol (LSP), defined by


Test patch:
diff --git a/channel/channel_test.go b/channel/channel_test.go
index a23b01f..e6d96f0 100644
--- a/channel/channel_test.go
+++ b/channel/channel_test.go
@@ -83,6 +83,7 @@ var tests = []struct {
 	{"NoMIME", Header("")},
 	{"RS", Split('\x1e')},
 	{"RawJSON", RawJSON},
+	{"StrictHeader", StrictHeader("text/plain")},
 	{"Varint", Varint},
 }
 
@@ -152,6 +153,55 @@ func TestEmptyMessage(t *testing.T) {
 	}
 }
 
+func TestHeaderTypeMismatch(t *testing.T) {
+	cli, srv := newPipe(StrictHeader("text/plain"))
+	defer cli.Close()
+	defer srv.Close()
+
+	noError := func(err error) bool { return err == nil }
+	tests := []struct {
+		payload string
+		ok      func(error) bool
+	}{
+		// With a content type provided, no error is reported.
+		// Order of headers and extra headers should not affect this.
+		{"Content-Type: text/plain\r\nContent-Length: 3\r\n\r\nfoo", noError},
+		{"Extra: ok\r\nContent-Length: 4\r\nContent-Type: text/plain\r\n\r\nquux", noError},
+
+		// With a content type provided, report an error if it doesn't match.
+		{"Content-Length: 2\r\nContent-Type: application/json\r\n\r\nno", func(err error) bool {
+			v, ok := err.(*ContentTypeMismatchError)
+			return ok && v.Got == "application/json" && v.Want == "text/plain"
+		}},
+
+		// With a content type omitted, a sentinel error is reported.
+		{"Content-Length: 5\r\n\r\nabcde", func(err error) bool {
+			v, ok := err.(*ContentTypeMismatchError)
+			return ok && v.Got == "" && v.Want == "text/plain"
+		}},
+
+		// Other errors do not use this sentinel.
+		{"Nothing: nohow\r\n\r\nfailure\n", func(err error) bool {
+			_, isSentinel := err.(*ContentTypeMismatchError)
+			return err != nil && !isSentinel
+		}},
+	}
+	h := cli.(*hdr)
+	for _, test := range tests {
+		go func() {
+			if _, err := h.wc.Write([]byte(test.payload)); err != nil {
+				t.Errorf("Send %q failed: %v", test.payload, err)
+			}
+		}()
+		msg, err := srv.Recv()
+		if !test.ok(err) {
+			t.Errorf("Recv failed: %v\n >> %q", err, msg)
+		} else {
+			t.Logf("Recv OK: %q", msg)
+		}
+	}
+}
+
 func TestWithTrigger(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {