Repository: scionproto/scion
Pull request #3506 (2019-12-10 11:15:11)

Common: Don't panic on nil
Do not panic in common.TypeOf when receiving a non-typed nil.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/scionproto/scion/3506)
<!-- Reviewable:end -->



Patch:
diff --git a/go/lib/common/BUILD.bazel b/go/lib/common/BUILD.bazel
index 6ba917e69..b9c4a311a 100644
--- a/go/lib/common/BUILD.bazel
+++ b/go/lib/common/BUILD.bazel
@@ -21,6 +21,7 @@ go_test(
     name = "go_default_test",
     srcs = [
         "binary_test.go",
+        "defs_test.go",
         "errors_test.go",
     ],
     embed = [":go_default_library"],
diff --git a/go/lib/common/defs.go b/go/lib/common/defs.go
index cc6a3f49b..07225eae8 100644
--- a/go/lib/common/defs.go
+++ b/go/lib/common/defs.go
@@ -59,5 +59,9 @@ func (ifid *IFIDType) UnmarshalText(text []byte) error {
 const IFIDBytes = 8
 
 func TypeOf(v interface{}) string {
-	return reflect.TypeOf(v).String()
+	t := reflect.TypeOf(v)
+	if t != nil {
+		return t.String()
+	}
+	return "<nil>"
 }
diff --git a/go/lib/infra/BUILD.bazel b/go/lib/infra/BUILD.bazel
index 53a846a64..526be2f9d 100644
--- a/go/lib/infra/BUILD.bazel
+++ b/go/lib/infra/BUILD.bazel
@@ -36,5 +36,6 @@ go_test(
         "//go/lib/infra/mock_infra:go_default_library",
         "//go/proto:go_default_library",
         "@com_github_golang_mock//gomock:go_default_library",
+        "@com_github_stretchr_testify//assert:go_default_library",
     ],
 )
diff --git a/go/lib/infra/common.go b/go/lib/infra/common.go
index edb05f8dd..9d4ce3a15 100644
--- a/go/lib/infra/common.go
+++ b/go/lib/infra/common.go
@@ -380,6 +380,9 @@ type ResponseWriter interface {
 }
 
 func ResponseWriterFromContext(ctx context.Context) (ResponseWriter, bool) {
+	if ctx == nil {
+		return nil, false
+	}
 	rw, ok := ctx.Value(responseWriterContextKey).(ResponseWriter)
 	return rw, ok
 }


Test patch:
diff --git a/go/lib/common/defs_test.go b/go/lib/common/defs_test.go
new file mode 100644
index 000000000..33efd0ca3
--- /dev/null
+++ b/go/lib/common/defs_test.go
@@ -0,0 +1,40 @@
+// Copyright 2019 Anapaya Systems
+//
+// 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 common_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/scionproto/scion/go/lib/common"
+)
+
+func TestTypeOf(t *testing.T) {
+	type A struct{}
+	tests := map[string]interface{}{
+		"nil":       nil,
+		"typed nil": (*A)(nil),
+		"ptr":       &A{},
+		"struct":    A{},
+	}
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			var result string
+			assert.NotPanics(t, func() { result = common.TypeOf(test) })
+			assert.NotEmpty(t, result)
+		})
+	}
+}
diff --git a/go/lib/infra/common_test.go b/go/lib/infra/common_test.go
index 4ebb013cd..e115b3ac6 100644
--- a/go/lib/infra/common_test.go
+++ b/go/lib/infra/common_test.go
@@ -19,6 +19,7 @@ import (
 	"testing"
 
 	"github.com/golang/mock/gomock"
+	"github.com/stretchr/testify/assert"
 
 	"github.com/scionproto/scion/go/lib/ctrl/ack"
 	"github.com/scionproto/scion/go/lib/infra"
@@ -59,3 +60,40 @@ func TestResourceHealth(t *testing.T) {
 	req := infra.NewRequest(ctx, nil, nil, nil, 1)
 	rHandler.Handle(req)
 }
+
+func TestResponseWriterFromContext(t *testing.T) {
+	tests := map[string]struct {
+		Ctx        context.Context
+		ExpectedRW infra.ResponseWriter
+		ExpectedOK bool
+	}{
+		"nil context": {
+			Ctx:        nil,
+			ExpectedRW: nil,
+			ExpectedOK: false,
+		},
+		"key not found": {
+			Ctx:        context.Background(),
+			ExpectedRW: nil,
+			ExpectedOK: false,
+		},
+		"value is nil": {
+			Ctx:        infra.NewContextWithResponseWriter(context.Background(), nil),
+			ExpectedRW: nil,
+			ExpectedOK: false,
+		},
+		"valid": {
+			Ctx: infra.NewContextWithResponseWriter(context.Background(),
+				&mock_infra.MockResponseWriter{}),
+			ExpectedRW: &mock_infra.MockResponseWriter{},
+			ExpectedOK: true,
+		},
+	}
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			rw, ok := infra.ResponseWriterFromContext(test.Ctx)
+			assert.Equal(t, test.ExpectedRW, rw)
+			assert.Equal(t, test.ExpectedOK, ok)
+		})
+	}
+}