gapic: wrap errors from REST better (#745)

Author: software-dov

internal/gengapic/genrest.go

@@ -608,8 +608,8 @@ func (g *generator) pagingRESTCall(servName string, m *descriptor.MethodDescript
 	p("  }")
 	p("  defer httpRsp.Body.Close()")
 	p("")
-	p("  if httpRsp.StatusCode != http.StatusOK {")
-	p(`    return nil, "", fmt.Errorf(httpRsp.Status)`)
+	p("  if err = googleapi.CheckResponse(httpRsp); err != nil {")
+	p(`    return nil, "", err`)
 	p("  }")
 	p("")
 	p("  buf, err := ioutil.ReadAll(httpRsp.Body)")
@@ -629,6 +629,7 @@ func (g *generator) pagingRESTCall(servName string, m *descriptor.MethodDescript
 	g.imports[pbinfo.ImportSpec{Path: "google.golang.org/api/iterator"}] = true
 	g.imports[pbinfo.ImportSpec{Path: "google.golang.org/protobuf/proto"}] = true
 	g.imports[pbinfo.ImportSpec{Path: "google.golang.org/protobuf/encoding/protojson"}] = true
+	g.imports[pbinfo.ImportSpec{Path: "google.golang.org/api/googleapi"}] = true
 	g.imports[inSpec] = true
 	g.imports[outSpec] = true
 
@@ -743,13 +744,12 @@ func (g *generator) emptyUnaryRESTCall(servName string, m *descriptor.MethodDesc
 	p("}")
 	p("defer httpRsp.Body.Close()")
 	p("")
-	p("if httpRsp.StatusCode != http.StatusOK {")
-	p("  return fmt.Errorf(httpRsp.Status)")
-	p("}")
-	p("")
-	p("return nil")
+	p("// Returns nil if there is no error, otherwise wraps")
+	p("// the response code and body into a non-nil error")
+	p("return googleapi.CheckResponse(httpRsp)")
 	p("}")
 
+	g.imports[pbinfo.ImportSpec{Path: "google.golang.org/api/googleapi"}] = true
 	g.imports[inSpec] = true
 	return nil
 }
@@ -845,8 +845,8 @@ func (g *generator) unaryRESTCall(servName string, m *descriptor.MethodDescripto
 	p("}")
 	p("defer httpRsp.Body.Close()")
 	p("")
-	p("if httpRsp.StatusCode != http.StatusOK {")
-	p("  return nil, fmt.Errorf(httpRsp.Status)")
+	p("if err = googleapi.CheckResponse(httpRsp); err != nil {")
+	p("  return nil, err")
 	p("}")
 	p("")
 	p("buf, err := ioutil.ReadAll(httpRsp.Body)")
@@ -867,10 +867,9 @@ func (g *generator) unaryRESTCall(servName string, m *descriptor.MethodDescripto
 	}
 	p(ret)
 	p("}")
-	g.imports[pbinfo.ImportSpec{Path: "google.golang.org/protobuf/encoding/protojson"}] = true
-	g.imports[inSpec] = true
-	g.imports[outSpec] = true
 
+	g.imports[pbinfo.ImportSpec{Path: "google.golang.org/api/googleapi"}] = true
+	g.imports[pbinfo.ImportSpec{Path: "google.golang.org/protobuf/encoding/protojson"}] = true
 	g.imports[inSpec] = true
 	g.imports[outSpec] = true
 	return nil

internal/gengapic/genrest_test.go

@@ -481,6 +481,7 @@ func TestGenRestMethod(t *testing.T) {
 			options: &options{diregapic: true},
 			imports: map[pbinfo.ImportSpec]bool{
 				{Path: "google.golang.org/protobuf/encoding/protojson"}:          true,
+				{Path: "google.golang.org/api/googleapi"}:                        true,
 				{Name: "foopb", Path: "google.golang.org/genproto/cloud/foo/v1"}: true,
 			},
 		},

internal/gengapic/testdata/rest_CustomOp.want

@@ -20,8 +20,8 @@ func (c *fooRESTClient) CustomOp(ctx context.Context, req *foopb.Foo, opts ...ga
 	}
 	defer httpRsp.Body.Close()
 
-	if httpRsp.StatusCode != http.StatusOK {
-		return nil, fmt.Errorf(httpRsp.Status)
+	if err = googleapi.CheckResponse(httpRsp); err != nil {
+		return nil, err
 	}
 
 	buf, err := ioutil.ReadAll(httpRsp.Body)

showcase/echo_test.go

@@ -17,6 +17,7 @@ package showcase
 import (
 	"context"
 	"io"
+	"net/http"
 	"os"
 	"strings"
 	"testing"
@@ -25,6 +26,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	showcase "github.com/googleapis/gapic-showcase/client"
 	showcasepb "github.com/googleapis/gapic-showcase/server/genproto"
+	"google.golang.org/api/googleapi"
 	"google.golang.org/api/iterator"
 	"google.golang.org/api/option"
 	locationpb "google.golang.org/genproto/googleapis/cloud/location"
@@ -68,19 +70,25 @@ func TestEcho_error(t *testing.T) {
 		},
 	}
 	for typ, client := range map[string]*showcase.EchoClient{"grpc": echo, "rest": echoREST} {
-		if typ == "rest" {
-			// TODO(dovs): currently erroring with 2, want 1
-			continue
-		}
-
 		resp, err := client.Echo(context.Background(), req)
-		if resp != nil {
+		if resp != nil || err == nil {
 			t.Errorf("%s Echo() = %v, wanted error %d", typ, resp, val)
 		}
-		status, _ := status.FromError(err)
-		if status.Code() != val {
-			t.Errorf("%s Echo() errors with %d, want %d", typ, status.Code(), val)
+		if typ == "grpc" {
+			status, _ := status.FromError(err)
+			if status.Code() != val {
+				t.Errorf("%s Echo() errors with %d, want %d", typ, status.Code(), val)
+			}
+		} else {
+			const ERR_CODE int = http.StatusInternalServerError
+			err, ok := err.(*googleapi.Error)
+			if !ok {
+				t.Errorf("%s Echo() returned unexpected error type: %v", typ, err)
+			} else if err.Code != ERR_CODE {
+				t.Errorf("%s Echo() errors with %d, want %d", typ, err.Code, ERR_CODE)
+			}
 		}
+
 	}
 
 }