Repository: prometheus/common
Pull request #181 (2019-03-29 06:36:20)

Don't panic when route.Param has no binding.
I've adopted route/common for a Prom related project, and had panics
due to a middleware attempting to grab a Param that had no binding in
the URL path in some cases.

I do like the safety aspect of panicing here, but strings have a
useful zero-value of "", which seems like an appropriate, and better
choice for this functionality.

(This will aide in
https://github.com/prometheus/pushgateway/issues/149 as some paths
don't have a *labels binding, but probably want to share some of the
same processing of labels code.)

cc / @brian-brazil 

brian-brazil: Where exactly is this a problem? I want to check there isn't a logic bug.

@beorn7 FYI
beorn7: I understand the reasoning. In principle, existing code could depend on the current behavior, but it is unlikely.
apghero: In pushgateway, the problem will be this:


`main.go`
```
	pushAPIPath := *routePrefix + "/metrics"
	r.PUT(pushAPIPath+"/job/:job/*labels", handler.Push(ms, true))
	r.POST(pushAPIPath+"/job/:job/*labels", handler.Push(ms, false))
	r.DELETE(pushAPIPath+"/job/:job/*labels", handler.Delete(ms))
	r.PUT(pushAPIPath+"/job/:job", handler.Push(ms, true))
	r.POST(pushAPIPath+"/job/:job", handler.Push(ms, false))
	r.DELETE(pushAPIPath+"/job/:job", handler.Delete(ms))
```

This reuses the same handler for routes with `*labels` and routes without. 

In `common/route` [these lines](https://github.com/prometheus/pushgateway/blob/master/handler/push.go#L57-L58) naturally become:

```
		job := route.Param(r.Context(), "job")
		labelsString := route.Param(r.Context(), "labels")
```

However, this will panic for the routes without `*labels`.

Here's a test case that shows the existing behavior:

```
$ cat panic_test.go
package route

import (
	"net/http"
	"testing"
)

func TestContextShowPanic(t *testing.T) {
	router := New()
	router.Get("/test/panic", func(w http.ResponseWriter, r *http.Request) {
		Param(r.Context(), "foo")
	})
	r, err := http.NewRequest("GET", "http://localhost:9090/test/panic/", nil)
	if err != nil {
		t.Fatalf("Error building test request: %s", err)
	}
	router.ServeHTTP(nil, r)
}
$ $ go test -run TestContextShowPanic ./...
ok      github.com/prometheus/common/config     (cached) [no tests to run]
ok      github.com/prometheus/common/expfmt     (cached) [no tests to run]
ok      github.com/prometheus/common/internal/bitbucket.org/ww/goautoneg        (cached) [no tests to run]
ok      github.com/prometheus/common/log        (cached) [no tests to run]
ok      github.com/prometheus/common/model      (cached) [no tests to run]
?       github.com/prometheus/common/promlog    [no test files]
?       github.com/prometheus/common/promlog/flag       [no test files]
--- FAIL: TestContextShowPanic (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x12510f0]

goroutine 4 [running]:
testing.tRunner.func1(0xc000140100)
        /usr/local/go/src/testing/testing.go:830 +0x392
panic(0x12c54a0, 0x1568400)
        /usr/local/go/src/runtime/panic.go:522 +0x1b5
net/http.Redirect(0x0, 0x0, 0xc000140200, 0xc0000141e0, 0x20, 0x12d)
        /usr/local/go/src/net/http/server.go:2087 +0x90
github.com/julienschmidt/httprouter.(*Router).ServeHTTP(0xc000020200, 0x0, 0x0, 0xc000140200)
        /Users/agwozdziewycz/go/pkg/mod/github.com/julienschmidt/httprouter@v1.2.0/router.go:350 +0x69f
github.com/prometheus/common/route.(*Router).ServeHTTP(...)
        /Users/agwozdziewycz/Devel/common/route/route.go:116
github.com/prometheus/common/route.TestContextShowPanic(0xc000140100)
        /Users/agwozdziewycz/Devel/common/route/route_test.go:68 +0x168
testing.tRunner(0xc000140100, 0x132e5f0)
        /usr/local/go/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
        /usr/local/go/src/testing/testing.go:916 +0x35a
FAIL    github.com/prometheus/common/route      0.482s
?       github.com/prometheus/common/version    [no test files]
```




@beorn7: If you're truly concerned about backwards compatibility, I can keep `Param` as is, and offer `ParamDefault(context.Context, key, default string) string`. 

apghero: @beorn7 @brian-brazil: Happy to make a backwards compatible change here if you wish -- for that, I'd go with the `ParamDefault`. Please advise.
brian-brazil: I'm thinking more that you shouldn't be requesting params that aren't defined in the first place.
apghero: @brian-brazil So in your opinion, "just don't do [this](https://github.com/prometheus/pushgateway/blob/master/main.go#L92-L97)"?

I truly agree that you should not request something that's not supposed to be there. But, a handler has no way of knowing how it was attached to a route, and there's not exactly any static guarantees that can be made at compile time here. There's also not anyway to do this via DbC (especially given Go has no actual support for this...). The handler being used in the above link is [ublic. It doesn't document that `*labels` are required.
beorn7: I guess you could give `handler.Push` an extra parameter that tells it if it should expect `labels` or not, and then pass it in as appropriate [here](https://github.com/prometheus/pushgateway/blob/master/main.go#L92-L97).

On the other hand, I see the convenience if `route.Param` would just return `""` for a non-existing param. The existing PGW code uses the same convenience as provided by the `httprouter.Params.ByName` method [here](https://github.com/prometheus/pushgateway/blob/master/handler/push.go#L58).

I would argue along two lines:

For one, our `route` packages should have the same convenience with the same semantics as the `httprouter` package we are using.

Then we should consider that `router.Param` is essentially just a convenience around `ctx.Value`. `ctx.Value` would _not_ panic if we asked it for a non-existing key. It would simply return `nil`. By adding the convenience code that is handling the conversion to a string for us, we should not change that semantics but return the zero value of string in case the underlying `ctx.Value` returns `nil`

Thus, both lines of argument make me agree with the suggested change.



Patch:
diff --git a/route/route.go b/route/route.go
index ec45bdf..9512de4 100644
--- a/route/route.go
+++ b/route/route.go
@@ -22,9 +22,13 @@ import (
 
 type param string
 
-// Param returns param p for the context.
+// Param returns param p for the context, or the empty string when
+// param does not exist in context.
 func Param(ctx context.Context, p string) string {
-	return ctx.Value(param(p)).(string)
+	if v, ok := ctx.Value(param(p)).(string); ok {
+		return v
+	}
+	return ""
 }
 
 // WithParam returns a new context with param p set to v.


Test patch:
diff --git a/route/route_test.go b/route/route_test.go
index 1019317..4d90203 100644
--- a/route/route_test.go
+++ b/route/route_test.go
@@ -93,6 +93,23 @@ func TestContextWithValue(t *testing.T) {
 	router.ServeHTTP(nil, r)
 }
 
+func TestContextWithoutValue(t *testing.T) {
+	router := New()
+	router.Get("/test", func(w http.ResponseWriter, r *http.Request) {
+		want := ""
+		got := Param(r.Context(), "foo")
+		if want != got {
+			t.Fatalf("Unexpected context value: want %q, got %q", want, got)
+		}
+	})
+
+	r, err := http.NewRequest("GET", "http://localhost:9090/test", nil)
+	if err != nil {
+		t.Fatalf("Error building test request: %s", err)
+	}
+	router.ServeHTTP(nil, r)
+}
+
 func TestInstrumentation(t *testing.T) {
 	var got string
 	cases := []struct {