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

peer and metadata: Implement the Stringer interface for Peer and Metadata #7137

Merged
merged 9 commits into from
May 6, 2024
15 changes: 15 additions & 0 deletions metadata/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,21 @@ func Pairs(kv ...string) MD {
return md
}

// String implements the Stringer interface for pretty-printing a MD.
// Ordering of the values is non-deterministic as it ranges over a map.
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about sorting the keys before generating the string representation and ensuring that the output is deterministic. This will also make the unit test much simpler than what it is currently.

func (md MD) String() string {
var sb strings.Builder
fmt.Fprintf(&sb, "MD{")
arvindbr8 marked this conversation as resolved.
Show resolved Hide resolved
for k, v := range md {
if sb.Len() > 3 {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about defining a bool first and using that as the conditional here, instead of this magic check which took me sometime to understand.

fmt.Fprintf(&sb, ", ")
}
arvindbr8 marked this conversation as resolved.
Show resolved Hide resolved
fmt.Fprintf(&sb, "%s=[%s]", k, strings.Join(v, ", "))
}
fmt.Fprintf(&sb, "}")
return sb.String()
}

// Len returns the number of items in md.
func (md MD) Len() int {
return len(md)
Expand Down
21 changes: 21 additions & 0 deletions metadata/metadata_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"context"
"reflect"
"strconv"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -338,6 +339,26 @@ func (s) TestAppendToOutgoingContext_FromKVSlice(t *testing.T) {
}
}

func TestStringerMD(t *testing.T) {
for _, test := range []struct {
md MD
want []string
arvindbr8 marked this conversation as resolved.
Show resolved Hide resolved
}{
{MD{}, []string{"MD{}"}},
{MD{"k1": []string{}}, []string{"MD{k1=[]}"}},
{MD{"k1": []string{"v1", "v2"}}, []string{"MD{k1=[v1, v2]}"}},
{MD{"k1": []string{"v1"}}, []string{"MD{k1=[v1]}"}},
{MD{"k1": []string{"v1", "v2"}, "k2": []string{}, "k3": []string{"1", "2", "3"}}, []string{"MD{", "k1=[v1, v2]", "k2=[]", "k3=[1, 2, 3]", "}"}},
} {
got := test.md.String()
for _, want := range test.want {
if !strings.Contains(got, want) {
t.Fatalf("Metadata string %q is missing %q", got, want)
}
}
}
}

// Old/slow approach to adding metadata to context
func Benchmark_AddingMetadata_ContextManipulationApproach(b *testing.B) {
// TODO: Add in N=1-100 tests once Go1.6 support is removed.
Expand Down
30 changes: 30 additions & 0 deletions peer/peer.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ package peer

import (
"context"
"fmt"
"net"
"strings"

"google.golang.org/grpc/credentials"
)
Expand All @@ -39,6 +41,34 @@ type Peer struct {
AuthInfo credentials.AuthInfo
}

// String ensures the Peer types implements the Stringer interface in order to
// allow to print a context with a peerKey value effectively.
func (p *Peer) String() string {
if p == nil {
return "Peer<nil>"
}
arvindbr8 marked this conversation as resolved.
Show resolved Hide resolved
sb := &strings.Builder{}
sb.WriteString("Peer{")
if p.Addr != nil {
arvindbr8 marked this conversation as resolved.
Show resolved Hide resolved
fmt.Fprintf(sb, "Addr: '%s', ", p.Addr.String())
} else {
fmt.Fprintf(sb, "Addr: <nil>, ")
}
if p.LocalAddr != nil {
fmt.Fprintf(sb, "LocalAddr: '%s', ", p.LocalAddr.String())
} else {
fmt.Fprintf(sb, "LocalAddr: <nil>, ")
}
if p.AuthInfo != nil {
AnomalRoil marked this conversation as resolved.
Show resolved Hide resolved
fmt.Fprintf(sb, "AuthInfo: '%s'", p.AuthInfo.AuthType())
} else {
fmt.Fprintf(sb, "AuthInfo: <nil>")
}
sb.WriteString("}")

return sb.String()
}

type peerKey struct{}

// NewContext creates a new context with peer information attached.
Expand Down
103 changes: 103 additions & 0 deletions peer/peer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/*
*
* Copyright 2024 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package peer

import (
"context"
"fmt"
"testing"

"google.golang.org/grpc/credentials"
)

// A struct that implements AuthInfo interface and implements CommonAuthInfo() method.
type testAuthInfo struct {
credentials.CommonAuthInfo
}

func (ta testAuthInfo) AuthType() string {
return fmt.Sprintf("testAuthInfo-%d", ta.SecurityLevel)
}

type addr struct {
ipAddress string
}

func (addr) Network() string { return "" }

func (a *addr) String() string { return a.ipAddress }

func TestPeerStringer(t *testing.T) {
testCases := []struct {
name string
peer *Peer
want string
}{
{
name: "+Addr-LocalAddr+ValidAuth",
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI, go test std library already does something similar by replacing \ss with _s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to convey information about whether the test case included + or didn't include - that specific field.

Copy link
Contributor

Choose a reason for hiding this comment

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

The use of + and - to convey the presence or absence of certain fields within the Peer isn't very readable. How about the following:

  • withAddrAndValidAuth
  • withAddrLocalAddrAndValidAuth
  • withAddrAndEmptyAuth
  • withAddrLocalAddrAndEmptyAuth
  • nilPeer

The above names in the test output are more readable I feel.

peer: &Peer{Addr: &addr{"example.com:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: credentials.PrivacyAndIntegrity}}},
want: "Peer{Addr: 'example.com:1234', LocalAddr: <nil>, AuthInfo: 'testAuthInfo-3'}",
},
{
name: "+Addr+LocalAddr+ValidAuth",
peer: &Peer{Addr: &addr{"example.com:1234"}, LocalAddr: &addr{"example.com:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: credentials.PrivacyAndIntegrity}}},
want: "Peer{Addr: 'example.com:1234', LocalAddr: 'example.com:1234', AuthInfo: 'testAuthInfo-3'}",
},
{
name: "+Addr-LocalAddr+emptyAuth",
peer: &Peer{Addr: &addr{"1.2.3.4:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{}}},
want: "Peer{Addr: '1.2.3.4:1234', LocalAddr: <nil>, AuthInfo: 'testAuthInfo-0'}",
},
{
name: "-Addr-LocalAddr+emptyAuth",
peer: &Peer{AuthInfo: testAuthInfo{}},
want: "Peer{Addr: <nil>, LocalAddr: <nil>, AuthInfo: 'testAuthInfo-0'}",
},
{
name: "zeroedPeer",
peer: &Peer{},
want: "Peer{Addr: <nil>, LocalAddr: <nil>, AuthInfo: <nil>}",
},
{
name: "nilPeer",
peer: nil,
want: "Peer<nil>",
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ctx := NewContext(context.Background(), tc.peer)
p, ok := FromContext(ctx)
if !ok {
t.Fatalf("Unable to get peer from context")
}
if p.String() != tc.want {
t.Fatalf("Error using peer String(): expected %q, got %q", tc.want, p.String())
}
AnomalRoil marked this conversation as resolved.
Show resolved Hide resolved
})
}
}

func TestPeerStringerOnContext(t *testing.T) {
ctx := NewContext(context.Background(), &Peer{Addr: &addr{"1.2.3.4:1234"}, AuthInfo: testAuthInfo{credentials.CommonAuthInfo{SecurityLevel: credentials.PrivacyAndIntegrity}}})
want := "context.Background.WithValue(type peer.peerKey, val Peer{Addr: '1.2.3.4:1234', LocalAddr: <nil>, AuthInfo: 'testAuthInfo-3'})"
if got := fmt.Sprintf("%v", ctx); got != want {
t.Fatalf("Unexpected stringer output, got: %q; want: %q", got, want)
}
}
Loading