From b854406a88386581ee9ca68ea7aa0b78e1d9c9f2 Mon Sep 17 00:00:00 2001 From: Ian Cottrell Date: Thu, 9 Apr 2020 23:00:00 -0400 Subject: [PATCH] internal/jsonrpc2: make the wire structures private The wire structures do not need to be public, and making them so might make it harder to keep the package correct without breaking changes if the protocol changes in the future. Change-Id: I03a5618c63c9f7691183d4285f88a177ccdd3b35 Reviewed-on: https://go-review.googlesource.com/c/tools/+/227838 Run-TryBot: Ian Cottrell TryBot-Result: Gobot Gobot Reviewed-by: Robert Findley --- gopls/integration/replay/main.go | 68 +++++++++++++++++++------------ internal/jsonrpc2/jsonrpc2.go | 39 +++++++----------- internal/jsonrpc2/wire.go | 67 ++++++++++++++++++------------ internal/jsonrpc2/wire_test.go | 4 +- internal/lsp/lsprpc/lsprpc.go | 4 +- internal/lsp/protocol/log.go | 34 +++++++++------- internal/lsp/protocol/protocol.go | 13 ++---- 7 files changed, 125 insertions(+), 104 deletions(-) diff --git a/gopls/integration/replay/main.go b/gopls/integration/replay/main.go index 1bd69ac046..0090eebdab 100644 --- a/gopls/integration/replay/main.go +++ b/gopls/integration/replay/main.go @@ -35,6 +35,29 @@ var ( svresp = make(map[string]*parse.Logmsg) ) +type wireRequest struct { + VersionTag string `json:"jsonrpc"` + Method string `json:"method"` + Params *json.RawMessage `json:"params,omitempty"` + ID *jsonrpc2.ID `json:"id,omitempty"` +} + +type wireResponse struct { + VersionTag string `json:"jsonrpc"` + Result *json.RawMessage `json:"result,omitempty"` + Error interface{} `json:"error,omitempty"` + ID *jsonrpc2.ID `json:"id,omitempty"` +} + +type wireCombined struct { + VersionTag interface{} `json:"jsonrpc"` + ID *jsonrpc2.ID `json:"id,omitempty"` + Method string `json:"method"` + Params *json.RawMessage `json:"params,omitempty"` + Result *json.RawMessage `json:"result,omitempty"` + Error interface{} `json:"error,omitempty"` +} + func main() { log.SetFlags(log.Lshortfile) flag.Usage = func() { @@ -139,7 +162,7 @@ func main() { } } -func msgType(c *p.Combined) parse.MsgType { +func msgType(c *wireCombined) parse.MsgType { // Method, Params, ID => request // Method, Params, no-ID => notification // Error => error response @@ -181,25 +204,28 @@ func send(ctx context.Context, l *parse.Logmsg, stream jsonrpc2.Stream, id *json if err != nil { n = 0 } - id = &jsonrpc2.ID{Number: int64(n)} + id = jsonrpc2.NewIntID(int64(n)) } var r interface{} switch l.Type { case parse.ClRequest: - r = jsonrpc2.WireRequest{ - ID: id, - Method: l.Method, - Params: &y, + r = wireRequest{ + VersionTag: "2.0", + ID: id, + Method: l.Method, + Params: &y, } case parse.SvResponse: - r = jsonrpc2.WireResponse{ - ID: id, - Result: &y, + r = wireResponse{ + VersionTag: "2.0", + ID: id, + Result: &y, } case parse.ToServer: - r = jsonrpc2.WireRequest{ - Method: l.Method, - Params: &y, + r = wireRequest{ + VersionTag: "2.0", + Method: l.Method, + Params: &y, } default: log.Fatalf("sending %s", l.Type) @@ -211,18 +237,10 @@ func send(ctx context.Context, l *parse.Logmsg, stream jsonrpc2.Stream, id *json stream.Write(ctx, data) } -func strID(x *jsonrpc2.ID) string { - if x.Name != "" { - log.Printf("strID returns %s", x.Name) - return x.Name - } - return strconv.Itoa(int(x.Number)) -} - -func respond(ctx context.Context, c *p.Combined, stream jsonrpc2.Stream) { +func respond(ctx context.Context, c *wireCombined, stream jsonrpc2.Stream) { // c is a server request // pick out the id, and look for the response in msgs - id := strID(c.ID) + id := fmt.Sprint(c.ID) for _, l := range msgs { if l.ID == id && l.Type == parse.SvResponse { // check that the methods match? @@ -280,7 +298,7 @@ func mimic(ctx context.Context) { log.Fatal(err) } stream := jsonrpc2.NewHeaderStream(fromServer, toServer) - rchan := make(chan *p.Combined, 10) // do we need buffering? + rchan := make(chan *wireCombined, 10) // do we need buffering? rdr := func() { for { buf, _, err := stream.Read(ctx) @@ -288,7 +306,7 @@ func mimic(ctx context.Context) { rchan <- nil // close it instead? return } - msg := &p.Combined{} + msg := &wireCombined{} if err := json.Unmarshal(buf, msg); err != nil { log.Fatal(err) } @@ -325,7 +343,7 @@ big: respond(ctx, x, stream) continue done // still waiting case parse.ClResponse, parse.ReportErr: - id := strID(x.ID) + id := fmt.Sprint(x.ID) seenids[id] = true if id == l.ID { break done diff --git a/internal/jsonrpc2/jsonrpc2.go b/internal/jsonrpc2/jsonrpc2.go index f483393aaa..cf50d44f92 100644 --- a/internal/jsonrpc2/jsonrpc2.go +++ b/internal/jsonrpc2/jsonrpc2.go @@ -33,7 +33,7 @@ type Conn struct { seq int64 // must only be accessed using atomic operations stream Stream pendingMu sync.Mutex // protects the pending map - pending map[ID]chan *WireResponse + pending map[ID]chan *wireResponse } // Request is sent to a server to represent a Call or Notify operaton. @@ -44,7 +44,7 @@ type Request struct { done []func() // The Wire values of the request. - WireRequest + wireRequest } type constError string @@ -56,7 +56,7 @@ func (e constError) Error() string { return string(e) } func NewConn(s Stream) *Conn { conn := &Conn{ stream: s, - pending: make(map[ID]chan *WireResponse), + pending: make(map[ID]chan *wireResponse), } return conn } @@ -69,7 +69,7 @@ func (c *Conn) Notify(ctx context.Context, method string, params interface{}) (e if err != nil { return fmt.Errorf("marshalling notify parameters: %v", err) } - request := &WireRequest{ + request := &wireRequest{ Method: method, Params: jsonParams, } @@ -98,12 +98,12 @@ func (c *Conn) Notify(ctx context.Context, method string, params interface{}) (e // result must be of a type you an pass to json.Unmarshal. func (c *Conn) Call(ctx context.Context, method string, params, result interface{}) (_ ID, err error) { // generate a new request identifier - id := ID{Number: atomic.AddInt64(&c.seq, 1)} + id := ID{number: atomic.AddInt64(&c.seq, 1)} jsonParams, err := marshalToRaw(params) if err != nil { return id, fmt.Errorf("marshalling call parameters: %v", err) } - request := &WireRequest{ + request := &wireRequest{ ID: &id, Method: method, Params: jsonParams, @@ -127,7 +127,7 @@ func (c *Conn) Call(ctx context.Context, method string, params, result interface // are racing the response. Also add a buffer to rchan, so that if we get a // wire response between the time this call is cancelled and id is deleted // from c.pending, the send to rchan will not block. - rchan := make(chan *WireResponse, 1) + rchan := make(chan *wireResponse, 1) c.pendingMu.Lock() c.pending[id] = rchan c.pendingMu.Unlock() @@ -196,16 +196,16 @@ func (r *Request) Reply(ctx context.Context, result interface{}, err error) erro if err == nil { raw, err = marshalToRaw(result) } - response := &WireResponse{ + response := &wireResponse{ Result: raw, ID: r.ID, } if err != nil { - if callErr, ok := err.(*Error); ok { + if callErr, ok := err.(*wireError); ok { response.Error = callErr } else { - response.Error = &Error{Message: err.Error()} - var wrapped *Error + response.Error = &wireError{Message: err.Error()} + var wrapped *wireError if errors.As(err, &wrapped) { // if we wrapped a wire error, keep the code from the wrapped error // but the message from the outer error @@ -239,17 +239,6 @@ func (r *Request) OnReply(do func()) { r.done = append(r.done, do) } -// combined has all the fields of both Request and Response. -// We can decode this and then work out which it is. -type combined struct { - VersionTag VersionTag `json:"jsonrpc"` - ID *ID `json:"id,omitempty"` - Method string `json:"method"` - Params *json.RawMessage `json:"params,omitempty"` - Result *json.RawMessage `json:"result,omitempty"` - Error *Error `json:"error,omitempty"` -} - // Run blocks until the connection is terminated, and returns any error that // caused the termination. // It must be called exactly once for each Conn. @@ -264,7 +253,7 @@ func (c *Conn) Run(runCtx context.Context, handler Handler) error { return err } // read a combined message - msg := &combined{} + msg := &wireCombined{} if err := json.Unmarshal(data, msg); err != nil { // a badly formed message arrived, log it and continue // we trust the stream to have isolated the error to just this message @@ -285,7 +274,7 @@ func (c *Conn) Run(runCtx context.Context, handler Handler) error { req := &Request{ conn: c, - WireRequest: WireRequest{ + wireRequest: wireRequest{ VersionTag: msg.VersionTag, Method: msg.Method, Params: msg.Params, @@ -305,7 +294,7 @@ func (c *Conn) Run(runCtx context.Context, handler Handler) error { rchan, ok := c.pending[*msg.ID] c.pendingMu.Unlock() if ok { - response := &WireResponse{ + response := &wireResponse{ Result: msg.Result, Error: msg.Error, ID: msg.ID, diff --git a/internal/jsonrpc2/wire.go b/internal/jsonrpc2/wire.go index 216e45cffb..632c53194c 100644 --- a/internal/jsonrpc2/wire.go +++ b/internal/jsonrpc2/wire.go @@ -34,10 +34,10 @@ var ( ErrServerOverloaded = NewError(-32000, "JSON RPC overloaded") ) -// WireRequest is sent to a server to represent a Call or Notify operaton. -type WireRequest struct { +// wireRequest is sent to a server to represent a Call or Notify operaton. +type wireRequest struct { // VersionTag is always encoded as the string "2.0" - VersionTag VersionTag `json:"jsonrpc"` + VersionTag wireVersionTag `json:"jsonrpc"` // Method is a string containing the method name to invoke. Method string `json:"method"` // Params is either a struct or an array with the parameters of the method. @@ -52,19 +52,30 @@ type WireRequest struct { // It will always have the ID field set to tie it back to a request, and will // have either the Result or Error fields set depending on whether it is a // success or failure response. -type WireResponse struct { +type wireResponse struct { // VersionTag is always encoded as the string "2.0" - VersionTag VersionTag `json:"jsonrpc"` + VersionTag wireVersionTag `json:"jsonrpc"` // Result is the response value, and is required on success. Result *json.RawMessage `json:"result,omitempty"` // Error is a structured error response if the call fails. - Error *Error `json:"error,omitempty"` + Error *wireError `json:"error,omitempty"` // ID must be set and is the identifier of the Request this is a response to. ID *ID `json:"id,omitempty"` } -// Error represents a structured error in a Response. -type Error struct { +// wireCombined has all the fields of both Request and Response. +// We can decode this and then work out which it is. +type wireCombined struct { + VersionTag wireVersionTag `json:"jsonrpc"` + ID *ID `json:"id,omitempty"` + Method string `json:"method"` + Params *json.RawMessage `json:"params,omitempty"` + Result *json.RawMessage `json:"result,omitempty"` + Error *wireError `json:"error,omitempty"` +} + +// wireError represents a structured error in a Response. +type wireError struct { // Code is an error code indicating the type of failure. Code int64 `json:"code"` // Message is a short description of the error. @@ -73,36 +84,34 @@ type Error struct { Data *json.RawMessage `json:"data"` } -// VersionTag is a special 0 sized struct that encodes as the jsonrpc version +// wireVersionTag is a special 0 sized struct that encodes as the jsonrpc version // tag. // It will fail during decode if it is not the correct version tag in the // stream. -type VersionTag struct{} +type wireVersionTag struct{} // ID is a Request identifier. -// Only one of either the Name or Number members will be set, using the -// number form if the Name is the empty string. type ID struct { - Name string - Number int64 + name string + number int64 } func NewError(code int64, message string) error { - return &Error{ + return &wireError{ Code: code, Message: message, } } -func (err *Error) Error() string { +func (err *wireError) Error() string { return err.Message } -func (VersionTag) MarshalJSON() ([]byte, error) { +func (wireVersionTag) MarshalJSON() ([]byte, error) { return json.Marshal("2.0") } -func (VersionTag) UnmarshalJSON(data []byte) error { +func (wireVersionTag) UnmarshalJSON(data []byte) error { version := "" if err := json.Unmarshal(data, &version); err != nil { return err @@ -115,6 +124,12 @@ func (VersionTag) UnmarshalJSON(data []byte) error { const invalidID int64 = math.MaxInt64 +// NewIntID returns a new numerical request ID. +func NewIntID(v int64) *ID { return &ID{number: v} } + +// NewStringID returns a new string request ID. +func NewStringID(v string) *ID { return &ID{name: v} } + // Format writes the ID to the formatter. // If the rune is q the representation is non ambiguous, // string forms are quoted, number forms are preceded by a # @@ -126,24 +141,24 @@ func (id *ID) Format(f fmt.State, r rune) { switch { case id == nil: fmt.Fprintf(f, numF, invalidID) - case id.Name != "": - fmt.Fprintf(f, strF, id.Name) + case id.name != "": + fmt.Fprintf(f, strF, id.name) default: - fmt.Fprintf(f, numF, id.Number) + fmt.Fprintf(f, numF, id.number) } } func (id *ID) MarshalJSON() ([]byte, error) { - if id.Name != "" { - return json.Marshal(id.Name) + if id.name != "" { + return json.Marshal(id.name) } - return json.Marshal(id.Number) + return json.Marshal(id.number) } func (id *ID) UnmarshalJSON(data []byte) error { *id = ID{} - if err := json.Unmarshal(data, &id.Number); err == nil { + if err := json.Unmarshal(data, &id.number); err == nil { return nil } - return json.Unmarshal(data, &id.Name) + return json.Unmarshal(data, &id.name) } diff --git a/internal/jsonrpc2/wire_test.go b/internal/jsonrpc2/wire_test.go index 12c45d2fcc..758b0bbaef 100644 --- a/internal/jsonrpc2/wire_test.go +++ b/internal/jsonrpc2/wire_test.go @@ -35,13 +35,13 @@ var wireIDTestData = []struct { quoted: `#0`, }, { name: `number`, - id: &jsonrpc2.ID{Number: 43}, + id: jsonrpc2.NewIntID(43), encoded: []byte(`43`), plain: `43`, quoted: `#43`, }, { name: `string`, - id: &jsonrpc2.ID{Name: "life"}, + id: jsonrpc2.NewStringID("life"), encoded: []byte(`"life"`), plain: `life`, quoted: `"life"`, diff --git a/internal/lsp/lsprpc/lsprpc.go b/internal/lsp/lsprpc/lsprpc.go index 8a05a77baa..6e10337681 100644 --- a/internal/lsp/lsprpc/lsprpc.go +++ b/internal/lsp/lsprpc/lsprpc.go @@ -533,9 +533,7 @@ func handshaker(client *debugClient, goplsPath string, handler jsonrpc2.Handler) } func sendError(ctx context.Context, req *jsonrpc2.Request, err error) { - if _, ok := err.(*jsonrpc2.Error); !ok { - err = fmt.Errorf("%w: %v", jsonrpc2.ErrParse, err) - } + err = fmt.Errorf("%w: %v", jsonrpc2.ErrParse, err) if err := req.Reply(ctx, nil, err); err != nil { event.Error(ctx, "", err) } diff --git a/internal/lsp/protocol/log.go b/internal/lsp/protocol/log.go index d073fd2ae6..f7cb076ce1 100644 --- a/internal/lsp/protocol/log.go +++ b/internal/lsp/protocol/log.go @@ -41,15 +41,21 @@ func (s *loggingStream) Write(ctx context.Context, data []byte) (int64, error) { return count, err } -// Combined has all the fields of both Request and Response. +// wireCombined has all the fields of both Request and Response. // We can decode this and then work out which it is. -type Combined struct { - VersionTag jsonrpc2.VersionTag `json:"jsonrpc"` - ID *jsonrpc2.ID `json:"id,omitempty"` - Method string `json:"method"` - Params *json.RawMessage `json:"params,omitempty"` - Result *json.RawMessage `json:"result,omitempty"` - Error *jsonrpc2.Error `json:"error,omitempty"` +type wireCombined struct { + VersionTag interface{} `json:"jsonrpc"` + ID *jsonrpc2.ID `json:"id,omitempty"` + Method string `json:"method"` + Params *json.RawMessage `json:"params,omitempty"` + Result *json.RawMessage `json:"result,omitempty"` + Error *wireError `json:"error,omitempty"` +} + +type wireError struct { + Code int64 `json:"code"` + Message string `json:"message"` + Data *json.RawMessage `json:"data"` } type req struct { @@ -106,11 +112,11 @@ func (m *mapped) setServer(id string, r req) { const eor = "\r\n\r\n\r\n" -func logCommon(outfd io.Writer, data []byte) (*Combined, time.Time, string) { +func logCommon(outfd io.Writer, data []byte) (*wireCombined, time.Time, string) { if outfd == nil { return nil, time.Time{}, "" } - var v Combined + var v wireCombined err := json.Unmarshal(data, &v) if err != nil { fmt.Fprintf(outfd, "Unmarshal %v\n", err) @@ -132,7 +138,7 @@ func logOut(outfd io.Writer, data []byte) { } id := fmt.Sprint(v.ID) if v.Error != nil { - fmt.Fprintf(outfd, "[Error - %s] Received #%s %s%s", tmfmt, id, v.Error, eor) + fmt.Fprintf(outfd, "[Error - %s] Received #%s %s%s", tmfmt, id, v.Error.Message, eor) return } buf := strings.Builder{} @@ -171,7 +177,7 @@ func logOut(outfd io.Writer, data []byte) { if v.Result != nil { r = string(*v.Result) } - fmt.Fprintf(&buf, "%s\n%s\n%s%s", p, r, v.Error, eor) + fmt.Fprintf(&buf, "%s\n%s\n%s%s", p, r, v.Error.Message, eor) } outfd.Write([]byte(buf.String())) } @@ -187,7 +193,7 @@ func logIn(outfd io.Writer, data []byte) { // ID !Method Result(might be null, but !Params) => Sending response (could we get an Error?) // !ID Method Params => Sending notification if v.Error != nil { // does this ever happen? - fmt.Fprintf(outfd, "[Error - %s] Sent #%s %s%s", tmfmt, id, v.Error, eor) + fmt.Fprintf(outfd, "[Error - %s] Sent #%s %s%s", tmfmt, id, v.Error.Message, eor) return } buf := strings.Builder{} @@ -230,7 +236,7 @@ func logIn(outfd io.Writer, data []byte) { if v.Result != nil { r = string(*v.Result) } - fmt.Fprintf(&buf, "%s\n%s\n%s%s", p, r, v.Error, eor) + fmt.Fprintf(&buf, "%s\n%s\n%s%s", p, r, v.Error.Message, eor) } outfd.Write([]byte(buf.String())) } diff --git a/internal/lsp/protocol/protocol.go b/internal/lsp/protocol/protocol.go index a5e6590ef3..cab85ae193 100644 --- a/internal/lsp/protocol/protocol.go +++ b/internal/lsp/protocol/protocol.go @@ -48,15 +48,13 @@ func CancelHandler(handler jsonrpc2.Handler) jsonrpc2.Handler { if err := json.Unmarshal(*req.Params, ¶ms); err != nil { return sendParseError(ctx, req, err) } - v := jsonrpc2.ID{} if n, ok := params.ID.(float64); ok { - v.Number = int64(n) + canceller(*jsonrpc2.NewIntID(int64(n))) } else if s, ok := params.ID.(string); ok { - v.Name = s + canceller(*jsonrpc2.NewStringID(s)) } else { - return sendParseError(ctx, req, fmt.Errorf("Request ID %v malformed", params.ID)) + return sendParseError(ctx, req, fmt.Errorf("request ID %v malformed", params.ID)) } - canceller(v) return req.Reply(ctx, nil, nil) } } @@ -78,8 +76,5 @@ func cancelCall(ctx context.Context, conn *jsonrpc2.Conn, id jsonrpc2.ID) { } func sendParseError(ctx context.Context, req *jsonrpc2.Request, err error) error { - if _, ok := err.(*jsonrpc2.Error); !ok { - err = fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err) - } - return req.Reply(ctx, nil, err) + return req.Reply(ctx, nil, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)) }