Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to forward grpc binary metadata #737

Merged

Conversation

timonwong
Copy link
Contributor

@timonwong timonwong commented Aug 27, 2018

Fixes #218

@codecov-io
Copy link

codecov-io commented Aug 27, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@e679739). Click here to learn what that means.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #737   +/-   ##
=========================================
  Coverage          ?   55.86%           
=========================================
  Files             ?       30           
  Lines             ?     3170           
  Branches          ?        0           
=========================================
  Hits              ?     1771           
  Misses            ?     1225           
  Partials          ?      174
Impacted Files Coverage Δ
runtime/mux.go 43.69% <ø> (ø)
runtime/context.go 71.6% <72.72%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e679739...400ef68. Read the comment docs.

pairs = append(pairs, "authorization", val)
}
if h, ok := mux.incomingHeaderMatcher(key); ok {
// Handles "-bin" metadata in grpc, since grpc will do another base64
// encode before sending to server, we need to decode it first.
if strings.HasSuffix(key, metadataHeaderBinarySuffix) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand the context here, but should there be a TrimSuffix somewhere? Or do we intentionally preserve the -bin?

Copy link
Contributor Author

@timonwong timonwong Aug 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -9,6 +9,7 @@ import (
"context"
"github.com/grpc-ecosystem/grpc-gateway/runtime"
"google.golang.org/grpc/metadata"
"encoding/base64"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in the previous block of imports? (to be consistent with runtime/context.go)

@@ -68,6 +69,30 @@ func TestAnnotateContext_ForwardsGrpcMetadata(t *testing.T) {
}
}

func TestAnnotateContext_ForwardGrpcBinaryMetadata(t *testing.T) {
ctx := context.Background()
request ,err := http.NewRequest("GET", "http://www.example.com", nil)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

", " -- has this gone through gofmt?

t.Errorf("Expected %d metadata items in context; got %v", emptyForwardMetaCount+1, md)
}
if got, want := md["test-bin"], []string{string(binData)}; !reflect.DeepEqual(got, want) {
t.Errorf(`md["grpcgateway-test-bin"] = %q want %q`, got, want)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how come this isn't md["test-bin"]?

@johanbrandhorst
Copy link
Collaborator

LGTM, thanks for your contribution!

@johanbrandhorst johanbrandhorst merged commit 8976e60 into grpc-ecosystem:master Aug 29, 2018
@timonwong timonwong deleted the feature/binary-header branch August 29, 2018 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants