Repository: twmb/franz-go (go)

Issue:
sr: segfault in `Client.OptValues("BasicAuth")` when no basic auth is configured
While writing some unit tests, I noticed that calling `Client.OptValues("BasicAuth")` when there is no basic auth configured results in a segfault (nil pointer derefence). After examination of the code, I realised that `basicAuth` in `Client` is a pointer and I overlooked this in PR #963 when returning the values:

https://github.com/twmb/franz-go/blob/37eecbb8927fce0aede1d6af39849839f7b0b3cf/pkg/sr/client.go#L69-L72

https://github.com/twmb/franz-go/blob/37eecbb8927fce0aede1d6af39849839f7b0b3cf/pkg/sr/client.go#L205-L206

I will be fixing this and send another PR, and will write the missing unit tests as well.
I am making this issue mostly as a reminder and for tracking purposes.

Pull request description:
sr: check for nil for basicAuth in Client.OptValues
This PR fixes a crash in `Client.OptValues` when no basic auth is configured (see linked issue for details).

Summary of changes:
* fixes segfault when no basic auth is configured
* also implement testing of default client values in TestOptValue
* also move testing of OptValues to own TestOptValues test

Fixes #1043

Patch:
diff --git a/pkg/sr/client.go b/pkg/sr/client.go
index 885cd4a9..2f45d9b1 100644
--- a/pkg/sr/client.go
+++ b/pkg/sr/client.go
@@ -203,7 +203,10 @@ func (cl *Client) OptValues(opt any) []any {
 	case namefn(DialTLSConfig):
 		return []any{cl.dialTLS}
 	case namefn(BasicAuth):
-		return []any{cl.basicAuth.user, cl.basicAuth.pass}
+		if cl.basicAuth != nil {
+			return []any{cl.basicAuth.user, cl.basicAuth.pass}
+		}
+		return []any{}
 	case namefn(BearerToken):
 		return []any{cl.bearerToken}
 	case namefn(PreReq):


Test patch:
diff --git a/pkg/sr/client_test.go b/pkg/sr/client_test.go
index 5941c43e..82380bb5 100644
--- a/pkg/sr/client_test.go
+++ b/pkg/sr/client_test.go
@@ -8,6 +8,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
+	"time"
 )
 
 func TestSchemaRegistryAPI(t *testing.T) {
@@ -307,15 +308,22 @@ func TestOptValue(t *testing.T) {
 		t.Fatalf("unable to create client: %s", err)
 	}
 
+	cd, err := NewClient()
+	if err != nil {
+		t.Fatalf("unable to create default client: %s", err)
+	}
+
 	tests := []struct {
-		name     string
-		opt      any
-		assertFn func(v any) bool
+		name        string
+		opt         any
+		assertFn    func(v any) bool
+		assertDefFn func(v any) bool
 	}{
 		{
-			name:     "HTTPClient",
-			opt:      "HTTPClient",
-			assertFn: func(v any) bool { return v == httpcl },
+			name:        "HTTPClient",
+			opt:         "HTTPClient",
+			assertFn:    func(v any) bool { return v.(*http.Client) == httpcl },
+			assertDefFn: func(v any) bool { return v.(*http.Client).Timeout == time.Second*5 },
 		},
 		{
 			name: "URLs",
@@ -324,44 +332,52 @@ func TestOptValue(t *testing.T) {
 				vs := v.([]string)
 				return vs[0] == "http://localhost:8081" && vs[1] == "http://localhost2:8082"
 			},
+			assertDefFn: func(v any) bool {
+				vs := v.([]string)
+				return vs[0] == "http://localhost:8081"
+			},
 		},
 		{
-			name:     "UserAgent",
-			opt:      "UserAgent",
-			assertFn: func(v any) bool { return v.(string) == "some-ua" },
+			name:        "UserAgent",
+			opt:         "UserAgent",
+			assertFn:    func(v any) bool { return v.(string) == "some-ua" },
+			assertDefFn: func(v any) bool { return v.(string) == "franz-go" },
 		},
 		{
-			name:     "DialTLSConfig",
-			opt:      "DialTLSConfig",
-			assertFn: func(v any) bool { return v == tlscfg },
+			name:        "DialTLSConfig",
+			opt:         "DialTLSConfig",
+			assertFn:    func(v any) bool { return v.(*tls.Config) == tlscfg },
+			assertDefFn: func(v any) bool { return v.(*tls.Config) == nil },
 		},
 		{
-			name:     "BasicAuth",
-			opt:      BasicAuth,
-			assertFn: func(v any) bool { return v.(string) == "some-user" },
+			name:        "BasicAuth",
+			opt:         BasicAuth,
+			assertFn:    func(v any) bool { return v.(string) == "some-user" },
+			assertDefFn: func(v any) bool { return v == nil },
 		},
 		{
-			name:     "BearerToken",
-			opt:      BearerToken,
-			assertFn: func(v any) bool { return v.(string) == "some-bearer" },
+			name:        "BearerToken",
+			opt:         BearerToken,
+			assertFn:    func(v any) bool { return v.(string) == "some-bearer" },
+			assertDefFn: func(v any) bool { return v.(string) == "" },
 		},
 		{
-			name:     "PreReq",
-			opt:      "PreReq",
-			assertFn: func(v any) bool { return v != nil },
+			name:        "PreReq",
+			opt:         "PreReq",
+			assertFn:    func(v any) bool { return v.(func(*http.Request) error) != nil },
+			assertDefFn: func(v any) bool { return v.(func(*http.Request) error) == nil },
 		},
 		{
-			name: "DefaultParams",
-			opt:  DefaultParams,
-			assertFn: func(v any) bool {
-				vp := v.(Param)
-				return vp.normalize == true && vp.verbose == true
-			},
+			name:        "DefaultParams",
+			opt:         DefaultParams,
+			assertFn:    func(v any) bool { return v.(Param).normalize && v.(Param).verbose },
+			assertDefFn: func(v any) bool { return v.(Param) == Param{} },
 		},
 		{
-			name:     "unknown option name",
-			opt:      "Unknown",
-			assertFn: func(v any) bool { return v == nil },
+			name:        "unknown option name",
+			opt:         "Unknown",
+			assertFn:    func(v any) bool { return v == nil },
+			assertDefFn: func(v any) bool { return v == nil },
 		},
 	}
 	for _, test := range tests {
@@ -370,21 +386,39 @@ func TestOptValue(t *testing.T) {
 			if !test.assertFn(got) {
 				t.Errorf("assertion failed: %q", got)
 			}
+			gotDef := cd.OptValue(test.opt)
+			if !test.assertDefFn(gotDef) {
+				t.Errorf("assertion for default failed: %q", gotDef)
+			}
 		})
 	}
+}
+
+func TestOptValues(t *testing.T) {
+	c, err := NewClient(
+		BasicAuth("some-user", "some-pass"),
+	)
+	if err != nil {
+		t.Fatalf("unable to create client: %s", err)
+	}
+
+	cd, err := NewClient()
+	if err != nil {
+		t.Fatalf("unable to create default client: %s", err)
+	}
 
-	t.Run("BasicAuthValues", func(t *testing.T) {
+	t.Run("BasicAuth", func(t *testing.T) {
 		got := c.OptValues(BasicAuth)
 		if len(got) != 2 {
 			t.Errorf("number of return values: expected 2, got %d", len(got))
 		}
-		expected := "some-user"
-		if got[0] != expected {
-			t.Errorf("username: expected %q, got %q", expected, got[0])
+		expected := []string{"some-user", "some-pass"}
+		if got[0] != expected[0] || got[1] != expected[1] {
+			t.Errorf("values: expected %q, got %q", expected, got)
 		}
-		expected = "some-pass"
-		if got[1] != expected {
-			t.Errorf("password: expected %q, got %q", expected, got[1])
+		gotDef := cd.OptValues(BasicAuth)
+		if len(gotDef) != 0 {
+			t.Errorf("number of return values for default: expected 0, got %d", len(gotDef))
 		}
 	})
 }