From 4b1db5fe8a5217bf17941627ec16ae8830aa794f Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Wed, 7 Aug 2019 19:26:45 +0200 Subject: [PATCH] spanner: remove redundant x-goog-api-client header The Spanner client now uses the gapic client, which automatically sets the x-goog-api-client header, so this header should no longer be set by the client itself. Fixes #1524 Change-Id: Ib7523878a7d94f7e5a05b5caec6fc3312316c72c Reviewed-on: https://code-review.googlesource.com/c/gocloud/+/43671 Reviewed-by: kokoro Reviewed-by: Emmanuel Odeke --- spanner/client.go | 10 +------ spanner/client_test.go | 49 ++++++++++++++++++++++++++++++++++ spanner/mocked_inmem_server.go | 21 +++++++++++---- 3 files changed, 66 insertions(+), 14 deletions(-) diff --git a/spanner/client.go b/spanner/client.go index c3e7acb705c4..a6b4a11bd63b 100644 --- a/spanner/client.go +++ b/spanner/client.go @@ -25,7 +25,6 @@ import ( "time" "cloud.google.com/go/internal/trace" - "cloud.google.com/go/internal/version" vkit "cloud.google.com/go/spanner/apiv1" "cloud.google.com/go/spanner/internal/backoff" "google.golang.org/api/option" @@ -41,10 +40,6 @@ const ( // resourcePrefixHeader is the name of the metadata header used to indicate // the resource being operated on. resourcePrefixHeader = "google-cloud-resource-prefix" - - // xGoogHeaderKey is the name of the metadata header used to indicate client - // information. - xGoogHeaderKey = "x-goog-api-client" ) const ( @@ -57,7 +52,6 @@ const ( var ( validDBPattern = regexp.MustCompile("^projects/[^/]+/instances/[^/]+/databases/[^/]+$") - xGoogHeaderVal = fmt.Sprintf("gl-go/%s gccl/%s grpc/%s", version.Go(), version.Repo, grpc.Version) ) func validDatabaseName(db string) error { @@ -125,9 +119,7 @@ func NewClient(ctx context.Context, database string, opts ...option.ClientOption func NewClientWithConfig(ctx context.Context, database string, config ClientConfig, opts ...option.ClientOption) (c *Client, err error) { c = &Client{ database: database, - md: metadata.Pairs( - resourcePrefixHeader, database, - xGoogHeaderKey, xGoogHeaderVal), + md: metadata.Pairs(resourcePrefixHeader, database), } // Make a copy of labels. diff --git a/spanner/client_test.go b/spanner/client_test.go index 57a77b740a12..912e159f0204 100644 --- a/spanner/client_test.go +++ b/spanner/client_test.go @@ -26,7 +26,9 @@ import ( "cloud.google.com/go/spanner/internal/benchserver" "cloud.google.com/go/spanner/internal/testutil" "google.golang.org/api/iterator" + "google.golang.org/grpc" "google.golang.org/grpc/codes" + "google.golang.org/grpc/metadata" gstatus "google.golang.org/grpc/status" ) @@ -435,3 +437,50 @@ func TestNewClient_ConnectToEmulator(t *testing.T) { t.Fatal(err) } } + +func TestClient_ApiClientHeader(t *testing.T) { + t.Parallel() + server, client := newSpannerInMemTestServerWithInterceptor(t, func( + ctx context.Context, + method string, + req interface{}, + reply interface{}, + cc *grpc.ClientConn, + invoker grpc.UnaryInvoker, + opts ...grpc.CallOption, + ) error { + // Check that the x-goog-api-client is set. + headers, ok := metadata.FromOutgoingContext(ctx) + if !ok { + return spannerErrorf(codes.Internal, "could not get outgoing metadata") + } + token, ok := headers["x-goog-api-client"] + if !ok { + return spannerErrorf(codes.Internal, "could not get api client token") + } + if token == nil { + return spannerErrorf(codes.Internal, "nil-value for outgoing api client token") + } + if len(token) != 1 { + return spannerErrorf(codes.Internal, "unexpected number of api client token headers: %v", len(token)) + } + if !strings.HasPrefix(token[0], "gl-go/") { + return spannerErrorf(codes.Internal, "unexpected api client token: %v", token[0]) + } + return invoker(ctx, method, req, reply, cc, opts...) + }, + ) + defer server.teardown(client) + ctx := context.Background() + iter := client.Single().Query(ctx, NewStatement(selectSingerIDAlbumIDAlbumTitleFromAlbums)) + defer iter.Stop() + for { + _, err := iter.Next() + if err == iterator.Done { + break + } + if err != nil { + t.Fatal(err) + } + } +} diff --git a/spanner/mocked_inmem_server.go b/spanner/mocked_inmem_server.go index b98b17724ac7..ab96f71b9fe0 100644 --- a/spanner/mocked_inmem_server.go +++ b/spanner/mocked_inmem_server.go @@ -56,18 +56,25 @@ func newSpannerInMemTestServer(t *testing.T) (*spannerInMemTestServer, *Client) return s, client } +// Create a spannerInMemTestServer with default configuration and a client interceptor. +func newSpannerInMemTestServerWithInterceptor(t *testing.T, interceptor grpc.UnaryClientInterceptor) (*spannerInMemTestServer, *Client) { + s := &spannerInMemTestServer{} + client := s.setupWithConfig(t, ClientConfig{}, interceptor) + return s, client +} + // Create a spannerInMemTestServer with the specified configuration. func newSpannerInMemTestServerWithConfig(t *testing.T, config ClientConfig) (*spannerInMemTestServer, *Client) { s := &spannerInMemTestServer{} - client := s.setupWithConfig(t, config) + client := s.setupWithConfig(t, config, nil) return s, client } func (s *spannerInMemTestServer) setup(t *testing.T) *Client { - return s.setupWithConfig(t, ClientConfig{}) + return s.setupWithConfig(t, ClientConfig{}, nil) } -func (s *spannerInMemTestServer) setupWithConfig(t *testing.T, config ClientConfig) *Client { +func (s *spannerInMemTestServer) setupWithConfig(t *testing.T, config ClientConfig, interceptor grpc.UnaryClientInterceptor) *Client { s.testSpanner = testutil.NewInMemSpannerServer() s.setupFooResults() s.setupSingersResults() @@ -83,11 +90,15 @@ func (s *spannerInMemTestServer) setupWithConfig(t *testing.T, config ClientConf serverAddress := lis.Addr().String() ctx := context.Background() var formattedDatabase = fmt.Sprintf("projects/%s/instances/%s/databases/%s", "[PROJECT]", "[INSTANCE]", "[DATABASE]") - client, err := NewClientWithConfig(ctx, formattedDatabase, config, + opts := []option.ClientOption{ option.WithEndpoint(serverAddress), option.WithGRPCDialOption(grpc.WithInsecure()), option.WithoutAuthentication(), - ) + } + if interceptor != nil { + opts = append(opts, option.WithGRPCDialOption(grpc.WithUnaryInterceptor(interceptor))) + } + client, err := NewClientWithConfig(ctx, formattedDatabase, config, opts...) if err != nil { t.Fatal(err) }