Changes after self review

Signed-off-by: Matt Lord <[email protected]>
vitessio · May 17, 2024 · 98ba421 · 98ba421
1 parent 7c7cb0e
commit 98ba421
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 50 deletions.
diff --git a/go/cmd/vtctldclient/cli/json.go b/go/cmd/vtctldclient/cli/json.go
@@ -21,10 +21,9 @@ import (
 	"fmt"
 
 	"google.golang.org/protobuf/encoding/protojson"
+	"google.golang.org/protobuf/proto"
 
 	"vitess.io/vitess/go/stats"
-
-	"google.golang.org/protobuf/proto"
 )
 
 const (
@@ -81,15 +80,15 @@ func MarshalJSONPretty(obj any) ([]byte, error) {
 	return MarshalJSON(obj, marshalOptions)
 }
 
-// ConvertToSnakeCase converts a string to snake_case or the keys of a
-// map to snake_case. This is useful when converting generic JSON data
-// marshalled to a map[string]interface{} for printing.
+// ConvertToSnakeCase converts strings to snake_case. This is useful when converting
+// generic JSON data marshalled to a map[string]interface{} for printing so that we
+// retain the snake_case used with protobufs and protojson.
 func ConvertToSnakeCase(val any) (any, error) {
 	switch val := val.(type) {
 	case string:
 		skval := stats.GetSnakeName(val)
 		return skval, nil
-	case map[string]interface{}:
+	case map[string]any:
 		for k, v := range val {
 			sk := stats.GetSnakeName(k)
 			// We need to recurse into the map to convert nested maps
@@ -102,15 +101,15 @@ func ConvertToSnakeCase(val any) (any, error) {
 			val[sk] = sv
 		}
 		return val, nil
-	case map[any]interface{}:
+	case map[any]any:
 		for k, v := range val {
 			// We need to recurse into the key to support more complex
 			// key types.
 			sk, err := ConvertToSnakeCase(k)
 			if err != nil {
 				return nil, err
 			}
-			// We need to recurse into the map to convert nested maps
+			// We need to recurse into the map to convert nested types
 			// to snake_case.
 			sv, err := ConvertToSnakeCase(v)
 			if err != nil {
@@ -120,25 +119,25 @@ func ConvertToSnakeCase(val any) (any, error) {
 			val[sk] = sv
 		}
 		return val, nil
-	case []interface{}:
+	case []string:
 		for i, v := range val {
 			// We need to recurse into the slice to convert nested maps
 			// to snake_case.
+			sk := stats.GetSnakeName(v)
+			val[i] = sk
+		}
+		return val, nil
+	case []any:
+		for i, v := range val {
+			// We need to recurse into the slice to convert complex types
+			// to snake_case.
 			sv, err := ConvertToSnakeCase(v)
 			if err != nil {
 				return nil, err
 			}
 			val[i] = sv
 		}
 		return val, nil
-	case []string:
-		for i, v := range val {
-			// We need to recurse into the slice to convert nested maps
-			// to snake_case.
-			sk := stats.GetSnakeName(v)
-			val[i] = sk
-		}
-		return val, nil
 	default:
 		// No need to do any conversion for things like bool.
 		return val, nil

diff --git a/go/cmd/vtctldclient/cli/json_test.go b/go/cmd/vtctldclient/cli/json_test.go
@@ -24,18 +24,22 @@ import (
 
 func TestConvertToSnakeCase(t *testing.T) {
 	tests := []struct {
-		name    string
-		val     any
-		want    any
-		wantErr bool
+		name string
+		val  any
+		want any
 	}{
 		{
 			name: "string",
 			val:  "MyValIsNotCool",
 			want: "my_val_is_not_cool",
 		},
 		{
-			name: "string slice",
+			name: "map[string]bool",
+			val:  map[string]any{"MyValIsNotCool": true},
+			want: map[string]any{"my_val_is_not_cool": true},
+		},
+		{
+			name: "[]string",
 			val: []string{
 				"MyValIsNotCool",
 				"NeitherIsYours",
@@ -46,7 +50,7 @@ func TestConvertToSnakeCase(t *testing.T) {
 			},
 		},
 		{
-			name: "string map",
+			name: "map[string][any]",
 			val: map[string]any{
 				"MyValIsNotCool": "val1",
 				"NeitherIsYours": "val2",
@@ -57,18 +61,7 @@ func TestConvertToSnakeCase(t *testing.T) {
 			},
 		},
 		{
-			name: "string map of slices",
-			val: map[string]any{
-				"MyValIsNotCool": []string{"val1", "val2"},
-				"NeitherIsYours": []string{"val3", "val4"},
-			},
-			want: map[string]any{
-				"my_val_is_not_cool": []string{"val1", "val2"},
-				"neither_is_yours":   []string{"val3", "val4"},
-			},
-		},
-		{
-			name: "string map of slices of string maps",
+			name: "map[any]any",
 			val: map[any]any{
 				"MyValIsNotCool": []any{
 					0: map[any]any{
@@ -114,14 +107,22 @@ func TestConvertToSnakeCase(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "map[any][]any",
+			val: map[any]any{
+				"MyValIsNotCool": []any{"val1", "val2"},
+				"NeitherIsYours": []any{"val3", "val4"},
+			},
+			want: map[any]any{
+				"my_val_is_not_cool": []any{"val1", "val2"},
+				"neither_is_yours":   []any{"val3", "val4"},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			got, err := ConvertToSnakeCase(tt.val)
-			if (err != nil) != tt.wantErr {
-				require.Fail(t, "unexpted error value", "ConvertToSnakeCase() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
+			require.NoError(t, err, "ConvertToSnakeCase() error = %v", err)
 			require.EqualValues(t, tt.want, got, "ConvertToSnakeCase() = %v, want %v", got, tt.want)
 		})
 	}

diff --git a/go/cmd/vtctldclient/command/topology.go b/go/cmd/vtctldclient/command/topology.go
@@ -60,8 +60,11 @@ func commandGetTopologyPath(cmd *cobra.Command, args []string) error {
 	}
 
 	if dataAsJSON {
+		if resp.GetCell() == nil || resp.GetCell().GetData() == "" {
+			return fmt.Errorf("no data found for path %s", path)
+		}
 		m := make(map[string]any)
-		if err := json.Unmarshal([]byte(resp.Cell.Data), &m); err != nil {
+		if err := json.Unmarshal([]byte(resp.GetCell().GetData()), &m); err != nil {
 			return errors.Wrap(err, "failed to unmarshal node data as JSON")
 		}
 		skm, err := cli.ConvertToSnakeCase(m)
@@ -76,7 +79,7 @@ func commandGetTopologyPath(cmd *cobra.Command, args []string) error {
 		return nil
 	}
 
-	data, err := cli.MarshalJSONPretty(resp.Cell)
+	data, err := cli.MarshalJSONPretty(resp.GetCell())
 	if err != nil {
 		return err
 	}

diff --git a/go/vt/vtctl/grpcvtctldserver/server_test.go b/go/vt/vtctl/grpcvtctldserver/server_test.go
@@ -7800,13 +7800,15 @@ func TestGetTopologyPath(t *testing.T) {
 
 	tests := []struct {
 		name      string
-		path      string
+		req       *vtctldatapb.GetTopologyPathRequest
 		shouldErr bool
 		expected  *vtctldatapb.GetTopologyPathResponse
 	}{
 		{
 			name: "root path",
-			path: "/",
+			req: &vtctldatapb.GetTopologyPathRequest{
+				Path: "/",
+			},
 			expected: &vtctldatapb.GetTopologyPathResponse{
 				Cell: &vtctldatapb.TopologyCell{
 					Path:     "/",
@@ -7815,13 +7817,17 @@ func TestGetTopologyPath(t *testing.T) {
 			},
 		},
 		{
-			name:      "invalid path",
-			path:      "",
+			name: "invalid path",
+			req: &vtctldatapb.GetTopologyPathRequest{
+				Path: "",
+			},
 			shouldErr: true,
 		},
 		{
 			name: "global path",
-			path: "/global",
+			req: &vtctldatapb.GetTopologyPathRequest{
+				Path: "/global",
+			},
 			expected: &vtctldatapb.GetTopologyPathResponse{
 				Cell: &vtctldatapb.TopologyCell{
 					Name:     "global",
@@ -7832,7 +7838,9 @@ func TestGetTopologyPath(t *testing.T) {
 		},
 		{
 			name: "terminal data path",
-			path: "/cell1/tablets/cell1-0000000100/Tablet",
+			req: &vtctldatapb.GetTopologyPathRequest{
+				Path: "/cell1/tablets/cell1-0000000100/Tablet",
+			},
 			expected: &vtctldatapb.GetTopologyPathResponse{
 				Cell: &vtctldatapb.TopologyCell{
 					Name: "Tablet",
@@ -7841,16 +7849,28 @@ func TestGetTopologyPath(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "terminal data path with data as json",
+			req: &vtctldatapb.GetTopologyPathRequest{
+				Path:   "/cell1/tablets/cell1-0000000100/Tablet",
+				AsJson: true,
+			},
+			expected: &vtctldatapb.GetTopologyPathResponse{
+				Cell: &vtctldatapb.TopologyCell{
+					Name: "Tablet",
+					Path: "/cell1/tablets/cell1-0000000100/Tablet",
+					Data: `{"alias":{"cell":"cell1","uid":100},"hostname":"localhost","keyspace":"keyspace1","mysqlHostname":"localhost","mysqlPort":17100}`,
+				},
+			},
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 
 			ctx := context.Background()
-			resp, err := vtctld.GetTopologyPath(ctx, &vtctldatapb.GetTopologyPathRequest{
-				Path: tt.path,
-			})
+			resp, err := vtctld.GetTopologyPath(ctx, tt.req)
 
 			if tt.shouldErr {
 				assert.Error(t, err)