server/prpc: updated server according to protocol

server/prpc/encoding.go

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package prpc
 
 // This file implements encoding of RPC results to HTTP responses.
 
 import (
         "bytes"
-        "io"
         "net/http"
         "sort"
 
         "github.com/golang/protobuf/jsonpb"
         "github.com/golang/protobuf/proto"
         "golang.org/x/net/context"
         "google.golang.org/grpc"
         "google.golang.org/grpc/codes"
-
-        "github.com/luci/luci-go/common/logging"
 )
 
 const (
         headerAccept = "Accept"
+        csrfPrefix   = ")]}'"

[dnj (Google), 2016/01/26 16:13:56]

I believe this should end in a newline, both for clarity and Gerrit parity.

[nodir, 2016/01/26 18:01:06]

Done.

 )
 
 // responseFormat returns the format to be used in a response.
 // Can return only formatBinary (preferred), formatJSONPB or formatText.
-// In case of an error, format is undefined and the error has an HTTP status.
-func responseFormat(acceptHeader string) (format, *httpError) {
+// In case of an error, format is undefined.
+func responseFormat(acceptHeader string) (format, *protocolError) {
         if acceptHeader == "" {
                 return formatBinary, nil
         }
 
         parsed, err := parseAccept(acceptHeader)
         if err != nil {
                 return formatBinary, errorf(http.StatusBadRequest, "Accept header: %s", err)
         }
         assert(len(parsed) > 0)

[dnj (Google), 2016/01/26 16:13:56]

This is just correctness of "parseAccept". No value asserting this here.

[nodir, 2016/01/26 18:01:06]

Done.

         formats := make(acceptFormatSlice, 0, len(parsed))
         for _, at := range parsed {
                 f, err := parseFormat(at.MediaType, at.MediaTypeParams)
                 if err != nil {
                         // Ignore invalid format. Check further.
                         continue
                 }
                 switch f {
 
                 case formatBinary, formatJSONPB, formatText:
                         // fine
 
                 case formatUnspecified:
                         f = formatBinary // prefer binary
 
                 case formatUnrecognized:
                         continue
 
                 default:
                         panicf("cannot happen")
                 }
 
                 assert(f == formatBinary || f == formatJSONPB || f == formatText)

[dnj (Google), 2016/01/26 16:13:56]

This is just correctness. No value using panic to double-check methods. Skip the
format if we don't recognize it (default case == formatUnrecognized case). Then
no panic!

[nodir, 2016/01/26 18:01:06]

Done.

                 formats = append(formats, acceptFormat{f, at.QualityFactor})
         }
         if len(formats) == 0 {
                 return formatBinary, errorf(
                         http.StatusNotAcceptable,
                         "Accept header: specified media types are not not supported. Supported types: %q, %q, %q, %q.",
                         mtPRPCBinary,
                         mtPRPCJSNOPB,
                         mtPRPCText,
                         mtJSON,
                 )
         }
         sort.Sort(formats) // order by quality factor and format preference.
         return formats[0].Format, nil
 }
 
-// writeMessage writes a protobuf message to response in the specified format.
-func writeMessage(w http.ResponseWriter, msg proto.Message, format format) error {
+// respondMessage encodes msg to a response in the specified format.
+func respondMessage(msg proto.Message, format format) *response {
         if msg == nil {
-                panic("msg is nil")
+                panicf("msg is nil")

[dnj (Google), 2016/01/26 16:13:56]

nit: panic would work here, no need for "panicf".

Although since we return pRPC errors for encoding failures, maybe this should be
a status.Internal instead of a panic?

[nodir, 2016/01/26 18:01:06]

err, in another CL you said that we panic with errors, not strings. This the
sole reason I have panicf. Removed it.

This can only happen if caller did not check what is passes (it does), but
whatever, returned err response

[dnj, 2016/01/26 18:50:27]

Ah sorry I meant something like: panic(errors.New("...")), still return errors.
Maybe make panicf => panicErr(string)? Don't really care that much, so this is a
nit.
Thanks:)

         }
         var (
-                contentType string
-                res         []byte
-                err         error
+                res response
+                err error
         )
         switch format {
         case formatBinary:
-                contentType = mtPRPCBinary
-                res, err = proto.Marshal(msg)
+                res.contentType = mtPRPCBinary
+                res.body, err = proto.Marshal(msg)
 
         case formatJSONPB:
-                contentType = mtPRPCJSNOPB
+                res.contentType = mtPRPCJSNOPB
+                var buf bytes.Buffer
+                buf.WriteString(csrfPrefix)
                 m := jsonpb.Marshaler{Indent: "\t"}

[dnj (Google), 2016/01/26 16:13:56]

1) Don't we normally use spaces to indent JSON?
2) Why format the JSON at all here? It's extra data to send. If we want
formatted JSON, there are tools to do that on the client side.

[nodir, 2016/01/26 18:01:07]

1) \t is less bytes
2) I used to curl in the past. I don't mind formatting it in rpc. Done

-                var buf bytes.Buffer
                 err = m.Marshal(&buf, msg)
-                buf.WriteString("\n")
-                res = buf.Bytes()
+                res.body = buf.Bytes()
+                res.newLine = true
 
         case formatText:
-                contentType = mtPRPCText
+                res.contentType = mtPRPCText
                 var buf bytes.Buffer
                 err = proto.MarshalText(&buf, msg)
-                res = buf.Bytes()
+                res.body = buf.Bytes()
         }
         if err != nil {
-                return err
+                return errResponse(codes.Internal, 0, err.Error())
         }
-        w.Header().Set(headerContentType, contentType)
-        _, err = w.Write(res)
-        return err
+
+        return &res
+}
+
+// respondProtocolError creates a response for a pRPC protocol error.
+func respondProtocolError(err *protocolError) *response {
+        return errResponse(codes.InvalidArgument, err.status, err.err.Error())
+}
+
+// errorCode returns a most appropriate gRPC code for an error
+func errorCode(err error) codes.Code {
+        switch err {
+        case nil:
+                panicf("err is nil")

[dnj (Google), 2016/01/26 16:13:56]

Default to something like Unknown if nil, rather than panic.

[nodir, 2016/01/26 18:01:06]

returned ok

+                fallthrough
+
+        case context.DeadlineExceeded:
+                return codes.DeadlineExceeded
+
+        case context.Canceled:
+                return codes.Canceled
+
+        default:
+                return grpc.Code(err)
+        }
 }
 
 // codeToStatus maps gRPC codes to HTTP statuses.
 // This map may need to be corrected when
 // https://github.com/grpc/grpc-common/issues/210
 // is closed.
 var codeToStatus = map[codes.Code]int{
         codes.OK:                 http.StatusOK,
         codes.Canceled:           http.StatusNoContent,
         codes.Unknown:            http.StatusInternalServerError,
         codes.InvalidArgument:    http.StatusBadRequest,
         codes.DeadlineExceeded:   http.StatusServiceUnavailable,
         codes.NotFound:           http.StatusNotFound,
         codes.AlreadyExists:      http.StatusConflict,
         codes.PermissionDenied:   http.StatusForbidden,
         codes.Unauthenticated:    http.StatusUnauthorized,
         codes.ResourceExhausted:  http.StatusServiceUnavailable,
         codes.FailedPrecondition: http.StatusPreconditionFailed,
         codes.Aborted:            http.StatusInternalServerError,
         codes.OutOfRange:         http.StatusBadRequest,
         codes.Unimplemented:      http.StatusNotImplemented,
         codes.Internal:           http.StatusInternalServerError,
         codes.Unavailable:        http.StatusServiceUnavailable,
         codes.DataLoss:           http.StatusInternalServerError,
 }
 
-// ErrorStatus returns HTTP status for an error.
-// In particular, it maps gRPC codes to HTTP statuses.
-// Status of nil is 200.
-//
-// See also grpc.Code.
-func ErrorStatus(err error) int {
-        if err, ok := err.(*httpError); ok {
-                return err.status
+// codeStatus maps gRPC codes to HTTP status codes.
+// Falls back to http.StatusInternalServerError.
+func codeStatus(code codes.Code) int {

[dnj (Google), 2016/01/26 16:13:56]

If it falls back to internal server error, you should remove the entries in the
map that map to internal server error.

[nodir, 2016/01/26 18:01:07]

Done.

+        if status, ok := codeToStatus[code]; ok {
+                return status
         }
-
-        status, ok := codeToStatus[grpc.Code(err)]
-        if !ok {
-                status = http.StatusInternalServerError
-        }
-        return status
+        return http.StatusInternalServerError
 }
-
-// ErrorDesc returns the error description of err if it was produced by pRPC or gRPC.
-// Otherwise, it returns err.Error() or empty string when err is nil.
-//
-// See also grpc.ErrorDesc.
-func ErrorDesc(err error) string {
-        if err == nil {
-                return ""
-        }
-        if e, ok := err.(*httpError); ok {
-                err = e.err
-        }
-        return grpc.ErrorDesc(err)
-}
-
-// writeError writes an error to an HTTP response.
-//
-// HTTP status is determined by ErrorStatus.
-// If it is http.StatusInternalServerError, prints only "Internal server error",
-// otherwise uses ErrorDesc.
-//
-// Logs all errors with status >= 500.
-func writeError(c context.Context, w http.ResponseWriter, err error) {
-        if err == nil {
-                panic("err is nil")
-        }
-
-        status := ErrorStatus(err)
-        if status >= 500 {
-                logging.Errorf(c, "HTTP %d: %s", status, ErrorDesc(err))
-        }
-
-        w.Header().Set(headerContentType, "text/plain")
-        w.WriteHeader(status)
-
-        var body string
-        if status == http.StatusInternalServerError {
-                body = "Internal server error"
-        } else {
-                body = ErrorDesc(err)
-        }
-        if _, err := io.WriteString(w, body+"\n"); err != nil {
-                logging.Errorf(c, "could not write error: %s", err)
-        }
-}
-
-func assert(condition bool) {
-        if !condition {
-                panicf("assertion failed")
-        }
-}