From a646f9215d203efd19b12dea83bc05eebd4c4da2 Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Mon, 10 Jun 2024 21:57:05 +0200 Subject: [PATCH 01/11] Add e2e test to recreate failure. Fix code bug Signed-off-by: Rohit Nayak --- go/cmd/vtctldclient/command/routing_rules.go | 2 +- .../vreplication_vtctldclient_cli_test.go | 76 +++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/go/cmd/vtctldclient/command/routing_rules.go b/go/cmd/vtctldclient/command/routing_rules.go index 0ffee0c2c24..58d9bc01991 100644 --- a/go/cmd/vtctldclient/command/routing_rules.go +++ b/go/cmd/vtctldclient/command/routing_rules.go @@ -82,7 +82,7 @@ func commandApplyRoutingRules(cmd *cobra.Command, args []string) error { } rr := &vschemapb.RoutingRules{} - if err := json2.Unmarshal(rulesBytes, &rr); err != nil { + if err := json2.Unmarshal(rulesBytes, rr); err != nil { return err } diff --git a/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go b/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go index bca51512a3c..f5e75c556fd 100644 --- a/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go +++ b/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go @@ -19,10 +19,13 @@ package vreplication import ( "encoding/json" "fmt" + "os" "slices" "strings" "testing" + "vitess.io/vitess/go/json2" + "github.com/stretchr/testify/require" "golang.org/x/exp/maps" "google.golang.org/protobuf/encoding/protojson" @@ -61,6 +64,9 @@ func TestVtctldclientCLI(t *testing.T) { workflowName := "wf1" targetTabs := setupMinimalCustomerKeyspace(t) + t.Run("RoutingRulesCommands", func(t *testing.T) { + testRoutingRulesCommands(t) + }) t.Run("WorkflowList", func(t *testing.T) { testWorkflowList(t, sourceKeyspaceName, targetKeyspaceName) }) @@ -88,6 +94,76 @@ func TestVtctldclientCLI(t *testing.T) { }) } +func testRoutingRulesCommands(t *testing.T) { + rr := &vschemapb.RoutingRules{ + Rules: []*vschemapb.RoutingRule{ + { + FromTable: "from1", + ToTables: []string{"to1", "to2"}, + }, + }, + } + rules, err := json2.MarshalPB(rr) + require.NoError(t, err) + + type routingRulesTest struct { + name string + typ string + rules string + useFile bool + } + tests := []routingRulesTest{ + { + name: "inline", + typ: "RoutingRules", + rules: string(rules), + }, + { + name: "file", + typ: "RoutingRules", + rules: string(rules), + useFile: true, + }, + { + name: "empty", + typ: "RoutingRules", + rules: "{}", + }, + } + for _, tt := range tests { + t.Run(tt.typ+"/"+tt.name, func(t *testing.T) { + var args []string + apply := fmt.Sprintf("Apply%s", tt.typ) + get := fmt.Sprintf("Get%s", tt.typ) + args = append(args, apply) + if tt.useFile { + tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s_rules.json", tt.name)) + require.NoError(t, err) + defer os.Remove(tmpFile.Name()) + _, err = tmpFile.WriteString(tt.rules) + require.NoError(t, err) + args = append(args, "--rules-file", tmpFile.Name()) + } else { + args = append(args, "--rules", tt.rules) + } + var output string + var err error + if output, err = vc.VtctldClient.ExecuteCommandWithOutput(args...); err != nil { + require.FailNowf(t, "failed action", apply, "%v: %s", err, output) + } + if output, err = vc.VtctldClient.ExecuteCommandWithOutput(get); err != nil { + require.FailNowf(t, "failed action", get, "%v: %s", err, output) + } + var want = &vschemapb.RoutingRules{} + require.NoError(t, json2.Unmarshal([]byte(tt.rules), want)) + var got = &vschemapb.RoutingRules{} + require.NoError(t, json2.Unmarshal([]byte(output), got)) + require.EqualValues(t, want, got) + }) + } + +} + // Tests several create flags and some complete flags and validates that some of them are set correctly for the workflow. func testMoveTablesFlags1(t *testing.T, mt *iMoveTables, sourceKeyspace, targetKeyspace, workflowName string, targetTabs map[string]*cluster.VttabletProcess) { tables := "customer,customer2" From 703efa5e2660be78773b412b715819a5288b9111 Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Mon, 10 Jun 2024 23:34:49 +0200 Subject: [PATCH 02/11] Add tests and fixes for Shard and Keyspace Routing Rules Signed-off-by: Rohit Nayak --- .../command/keyspace_routing_rules.go | 2 +- .../command/shard_routing_rules.go | 2 +- .../vreplication_vtctldclient_cli_test.go | 201 +++++++++++------- 3 files changed, 132 insertions(+), 73 deletions(-) diff --git a/go/cmd/vtctldclient/command/keyspace_routing_rules.go b/go/cmd/vtctldclient/command/keyspace_routing_rules.go index 7d1134d3abf..ba1e95edd18 100644 --- a/go/cmd/vtctldclient/command/keyspace_routing_rules.go +++ b/go/cmd/vtctldclient/command/keyspace_routing_rules.go @@ -82,7 +82,7 @@ func commandApplyKeyspaceRoutingRules(cmd *cobra.Command, args []string) error { } krr := &vschemapb.KeyspaceRoutingRules{} - if err := json2.Unmarshal(rulesBytes, &krr); err != nil { + if err := json2.Unmarshal(rulesBytes, krr); err != nil { return err } diff --git a/go/cmd/vtctldclient/command/shard_routing_rules.go b/go/cmd/vtctldclient/command/shard_routing_rules.go index 10ce7e81747..26d95087c0b 100644 --- a/go/cmd/vtctldclient/command/shard_routing_rules.go +++ b/go/cmd/vtctldclient/command/shard_routing_rules.go @@ -87,7 +87,7 @@ func commandApplyShardRoutingRules(cmd *cobra.Command, args []string) error { } srr := &vschemapb.ShardRoutingRules{} - if err := json2.Unmarshal(rulesBytes, &srr); err != nil { + if err := json2.Unmarshal(rulesBytes, srr); err != nil { return err } // Round-trip so when we display the result it's readable. diff --git a/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go b/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go index f5e75c556fd..ca6d42f7da1 100644 --- a/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go +++ b/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go @@ -65,7 +65,7 @@ func TestVtctldclientCLI(t *testing.T) { targetTabs := setupMinimalCustomerKeyspace(t) t.Run("RoutingRulesCommands", func(t *testing.T) { - testRoutingRulesCommands(t) + testAllRoutingRulesCommands(t) }) t.Run("WorkflowList", func(t *testing.T) { testWorkflowList(t, sourceKeyspaceName, targetKeyspaceName) @@ -94,76 +94,6 @@ func TestVtctldclientCLI(t *testing.T) { }) } -func testRoutingRulesCommands(t *testing.T) { - rr := &vschemapb.RoutingRules{ - Rules: []*vschemapb.RoutingRule{ - { - FromTable: "from1", - ToTables: []string{"to1", "to2"}, - }, - }, - } - rules, err := json2.MarshalPB(rr) - require.NoError(t, err) - - type routingRulesTest struct { - name string - typ string - rules string - useFile bool - } - tests := []routingRulesTest{ - { - name: "inline", - typ: "RoutingRules", - rules: string(rules), - }, - { - name: "file", - typ: "RoutingRules", - rules: string(rules), - useFile: true, - }, - { - name: "empty", - typ: "RoutingRules", - rules: "{}", - }, - } - for _, tt := range tests { - t.Run(tt.typ+"/"+tt.name, func(t *testing.T) { - var args []string - apply := fmt.Sprintf("Apply%s", tt.typ) - get := fmt.Sprintf("Get%s", tt.typ) - args = append(args, apply) - if tt.useFile { - tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s_rules.json", tt.name)) - require.NoError(t, err) - defer os.Remove(tmpFile.Name()) - _, err = tmpFile.WriteString(tt.rules) - require.NoError(t, err) - args = append(args, "--rules-file", tmpFile.Name()) - } else { - args = append(args, "--rules", tt.rules) - } - var output string - var err error - if output, err = vc.VtctldClient.ExecuteCommandWithOutput(args...); err != nil { - require.FailNowf(t, "failed action", apply, "%v: %s", err, output) - } - if output, err = vc.VtctldClient.ExecuteCommandWithOutput(get); err != nil { - require.FailNowf(t, "failed action", get, "%v: %s", err, output) - } - var want = &vschemapb.RoutingRules{} - require.NoError(t, json2.Unmarshal([]byte(tt.rules), want)) - var got = &vschemapb.RoutingRules{} - require.NoError(t, json2.Unmarshal([]byte(output), got)) - require.EqualValues(t, want, got) - }) - } - -} - // Tests several create flags and some complete flags and validates that some of them are set correctly for the workflow. func testMoveTablesFlags1(t *testing.T, mt *iMoveTables, sourceKeyspace, targetKeyspace, workflowName string, targetTabs map[string]*cluster.VttabletProcess) { tables := "customer,customer2" @@ -514,3 +444,132 @@ func validateMoveTablesWorkflow(t *testing.T, workflows []*vtctldatapb.Workflow) require.Equal(t, binlogdatapb.OnDDLAction_STOP, bls.OnDdl) require.True(t, bls.StopAfterCopy) } + +func testAllRoutingRulesCommands(t *testing.T) { + var getRules func() string + var validateRules func(want, got string) + typs := []string{"RoutingRules", "ShardRoutingRules", "KeyspaceRoutingRules"} + for _, typ := range typs { + switch typ { + case "RoutingRules": + rr := &vschemapb.RoutingRules{ + Rules: []*vschemapb.RoutingRule{ + { + FromTable: "from1", + ToTables: []string{"to1", "to2"}, + }, + }, + } + rules, err := json2.MarshalPB(rr) + require.NoError(t, err) + getRules = func() string { + return string(rules) + } + validateRules = func(want, got string) { + var wantRules = &vschemapb.RoutingRules{} + require.NoError(t, json2.Unmarshal([]byte(want), wantRules)) + var gotRules = &vschemapb.RoutingRules{} + require.NoError(t, json2.Unmarshal([]byte(got), gotRules)) + require.EqualValues(t, wantRules, gotRules) + } + case "ShardRoutingRules": + srr := &vschemapb.ShardRoutingRules{ + Rules: []*vschemapb.ShardRoutingRule{ + { + FromKeyspace: "from1", + ToKeyspace: "to1", + Shard: "-80", + }, + }, + } + rules, err := json2.MarshalPB(srr) + require.NoError(t, err) + getRules = func() string { + return string(rules) + } + validateRules = func(want, got string) { + var wantRules = &vschemapb.ShardRoutingRules{} + require.NoError(t, json2.Unmarshal([]byte(want), wantRules)) + var gotRules = &vschemapb.ShardRoutingRules{} + require.NoError(t, json2.Unmarshal([]byte(got), gotRules)) + require.EqualValues(t, wantRules, gotRules) + } + case "KeyspaceRoutingRules": + krr := &vschemapb.KeyspaceRoutingRules{ + Rules: []*vschemapb.KeyspaceRoutingRule{ + { + FromKeyspace: "from1", + ToKeyspace: "to1", + }, + }, + } + rules, err := json2.MarshalPB(krr) + require.NoError(t, err) + getRules = func() string { + return string(rules) + } + validateRules = func(want, got string) { + var wantRules = &vschemapb.KeyspaceRoutingRules{} + require.NoError(t, json2.Unmarshal([]byte(want), wantRules)) + var gotRules = &vschemapb.KeyspaceRoutingRules{} + require.NoError(t, json2.Unmarshal([]byte(got), gotRules)) + require.EqualValues(t, wantRules, gotRules) + } + + } + testOneRoutingRulesCommand(t, typ, getRules, validateRules) + } + +} + +func testOneRoutingRulesCommand(t *testing.T, typ string, getRules func() string, validateRules func(want, got string)) { + type routingRulesTest struct { + name string + rules string + useFile bool + } + rules := getRules() + tests := []routingRulesTest{ + { + name: "inline", + rules: string(rules), + }, + { + name: "file", + rules: string(rules), + useFile: true, + }, + { + name: "empty", + rules: "{}", + }, + } + for _, tt := range tests { + t.Run(typ+"/"+tt.name, func(t *testing.T) { + var args []string + apply := fmt.Sprintf("Apply%s", typ) + get := fmt.Sprintf("Get%s", typ) + args = append(args, apply) + if tt.useFile { + tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s_rules.json", tt.name)) + require.NoError(t, err) + defer os.Remove(tmpFile.Name()) + _, err = tmpFile.WriteString(tt.rules) + require.NoError(t, err) + args = append(args, "--rules-file", tmpFile.Name()) + } else { + args = append(args, "--rules", tt.rules) + } + var output string + var err error + if output, err = vc.VtctldClient.ExecuteCommandWithOutput(args...); err != nil { + require.FailNowf(t, "failed action", apply, "%v: %s", err, output) + } + if output, err = vc.VtctldClient.ExecuteCommandWithOutput(get); err != nil { + require.FailNowf(t, "failed action", get, "%v: %s", err, output) + } + validateRules(tt.rules, output) + }) + } + +} From 9b8a530d98fa37f0088814a27e4034ce6b483e9c Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Tue, 11 Jun 2024 00:31:22 +0200 Subject: [PATCH 03/11] Test both camel case and snake case inputs Signed-off-by: Rohit Nayak --- .../vreplication_vtctldclient_cli_test.go | 59 +++++++++++-------- 1 file changed, 36 insertions(+), 23 deletions(-) diff --git a/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go b/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go index ca6d42f7da1..d8a633d3963 100644 --- a/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go +++ b/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go @@ -448,7 +448,7 @@ func validateMoveTablesWorkflow(t *testing.T, workflows []*vtctldatapb.Workflow) func testAllRoutingRulesCommands(t *testing.T) { var getRules func() string var validateRules func(want, got string) - typs := []string{"RoutingRules", "ShardRoutingRules", "KeyspaceRoutingRules"} + typs := []string{"RoutingRules"} // , "ShardRoutingRules", "KeyspaceRoutingRules"} for _, typ := range typs { switch typ { case "RoutingRules": @@ -546,29 +546,42 @@ func testOneRoutingRulesCommand(t *testing.T, typ string, getRules func() string } for _, tt := range tests { t.Run(typ+"/"+tt.name, func(t *testing.T) { - var args []string - apply := fmt.Sprintf("Apply%s", typ) - get := fmt.Sprintf("Get%s", typ) - args = append(args, apply) - if tt.useFile { - tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s_rules.json", tt.name)) - require.NoError(t, err) - defer os.Remove(tmpFile.Name()) - _, err = tmpFile.WriteString(tt.rules) - require.NoError(t, err) - args = append(args, "--rules-file", tmpFile.Name()) - } else { - args = append(args, "--rules", tt.rules) + done := false + wantRules := tt.rules + typ2 := "camelCase" + for !done { + t.Run(typ2, func(t *testing.T) { + var args []string + apply := fmt.Sprintf("Apply%s", typ) + get := fmt.Sprintf("Get%s", typ) + args = append(args, apply) + if tt.useFile { + tmpFile, err := os.CreateTemp("", fmt.Sprintf("%s_rules.json", tt.name)) + require.NoError(t, err) + defer os.Remove(tmpFile.Name()) + _, err = tmpFile.WriteString(wantRules) + require.NoError(t, err) + args = append(args, "--rules-file", tmpFile.Name()) + } else { + args = append(args, "--rules", wantRules) + } + var output string + var err error + if output, err = vc.VtctldClient.ExecuteCommandWithOutput(args...); err != nil { + require.FailNowf(t, "failed action", apply, "%v: %s", err, output) + } + if output, err = vc.VtctldClient.ExecuteCommandWithOutput(get); err != nil { + require.FailNowf(t, "failed action", get, "%v: %s", err, output) + } + validateRules(wantRules, output) + if typ2 == "snake_case" { + done = true + } else { + wantRules = output + typ2 = "snake_case" + } + }) } - var output string - var err error - if output, err = vc.VtctldClient.ExecuteCommandWithOutput(args...); err != nil { - require.FailNowf(t, "failed action", apply, "%v: %s", err, output) - } - if output, err = vc.VtctldClient.ExecuteCommandWithOutput(get); err != nil { - require.FailNowf(t, "failed action", get, "%v: %s", err, output) - } - validateRules(tt.rules, output) }) } From e88eda50b81f4ed883f85c4bfa1ae1555d905e2f Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Tue, 11 Jun 2024 10:50:17 +0200 Subject: [PATCH 04/11] Add UnmarshalPB helper Signed-off-by: Rohit Nayak --- go/cmd/vtctldclient/command/keyspace_routing_rules.go | 2 +- go/cmd/vtctldclient/command/routing_rules.go | 2 +- go/cmd/vtctldclient/command/shard_routing_rules.go | 2 +- go/json2/marshal_test.go | 6 ++++++ go/json2/unmarshal.go | 6 ++++++ go/json2/unmarshal_test.go | 11 +++++++++++ 6 files changed, 26 insertions(+), 3 deletions(-) diff --git a/go/cmd/vtctldclient/command/keyspace_routing_rules.go b/go/cmd/vtctldclient/command/keyspace_routing_rules.go index ba1e95edd18..68aaa35b8bb 100644 --- a/go/cmd/vtctldclient/command/keyspace_routing_rules.go +++ b/go/cmd/vtctldclient/command/keyspace_routing_rules.go @@ -82,7 +82,7 @@ func commandApplyKeyspaceRoutingRules(cmd *cobra.Command, args []string) error { } krr := &vschemapb.KeyspaceRoutingRules{} - if err := json2.Unmarshal(rulesBytes, krr); err != nil { + if err := json2.UnmarshalPB(rulesBytes, krr); err != nil { return err } diff --git a/go/cmd/vtctldclient/command/routing_rules.go b/go/cmd/vtctldclient/command/routing_rules.go index 58d9bc01991..8a228589098 100644 --- a/go/cmd/vtctldclient/command/routing_rules.go +++ b/go/cmd/vtctldclient/command/routing_rules.go @@ -82,7 +82,7 @@ func commandApplyRoutingRules(cmd *cobra.Command, args []string) error { } rr := &vschemapb.RoutingRules{} - if err := json2.Unmarshal(rulesBytes, rr); err != nil { + if err := json2.UnmarshalPB(rulesBytes, rr); err != nil { return err } diff --git a/go/cmd/vtctldclient/command/shard_routing_rules.go b/go/cmd/vtctldclient/command/shard_routing_rules.go index 26d95087c0b..2214269d0f3 100644 --- a/go/cmd/vtctldclient/command/shard_routing_rules.go +++ b/go/cmd/vtctldclient/command/shard_routing_rules.go @@ -87,7 +87,7 @@ func commandApplyShardRoutingRules(cmd *cobra.Command, args []string) error { } srr := &vschemapb.ShardRoutingRules{} - if err := json2.Unmarshal(rulesBytes, srr); err != nil { + if err := json2.UnmarshalPB(rulesBytes, srr); err != nil { return err } // Round-trip so when we display the result it's readable. diff --git a/go/json2/marshal_test.go b/go/json2/marshal_test.go index b155126fb17..93c39b9948f 100644 --- a/go/json2/marshal_test.go +++ b/go/json2/marshal_test.go @@ -36,6 +36,12 @@ func TestMarshalPB(t *testing.T) { require.NoErrorf(t, err, "MarshalPB(%+v) error", col) want := "{\"name\":\"c1\",\"type\":\"VARCHAR\"}" assert.Equalf(t, want, string(b), "MarshalPB(%+v)", col) + + // Test MarshalPB with nil proto. + b, err = MarshalPB(nil) + require.NoErrorf(t, err, "MarshalPB(nil) error") + want = "{}" + assert.Equalf(t, want, string(b), "MarshalPB(nil)") } func TestMarshalIndentPB(t *testing.T) { diff --git a/go/json2/unmarshal.go b/go/json2/unmarshal.go index e382b8ad47a..b5f50ff1864 100644 --- a/go/json2/unmarshal.go +++ b/go/json2/unmarshal.go @@ -53,3 +53,9 @@ func annotate(data []byte, err error) error { return fmt.Errorf("line: %d, position %d: %v", line, pos, err) } + +// UnmarshalPB is similar to Unmarshal but specifically for proto.Message to add type safety. +func UnmarshalPB(data []byte, pb proto.Message) error { + opts := protojson.UnmarshalOptions{DiscardUnknown: true} + return annotate(data, opts.Unmarshal(data, pb)) +} diff --git a/go/json2/unmarshal_test.go b/go/json2/unmarshal_test.go index ff18a29def8..1616a368976 100644 --- a/go/json2/unmarshal_test.go +++ b/go/json2/unmarshal_test.go @@ -91,3 +91,14 @@ func TestAnnotate(t *testing.T) { require.Equal(t, tcase.err, err, "annotate(%s, %v) error", string(tcase.data), tcase.err) } } + +func TestUnmarshalPB(t *testing.T) { + protoData := &emptypb.Empty{} + protoJSONData, err := protojson.Marshal(protoData) + require.Nil(t, err) + + var pb emptypb.Empty + err = UnmarshalPB(protoJSONData, &pb) + require.Nil(t, err) + require.Equal(t, protoData, &pb) +} From 103930ffef118309c0fbec463eb48e4dff03310a Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Tue, 11 Jun 2024 11:05:07 +0200 Subject: [PATCH 05/11] Refactor e2e test Signed-off-by: Rohit Nayak --- .../vreplication_vtctldclient_cli_test.go | 26 +++++++------------ 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go b/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go index d8a633d3963..d95bc7397a7 100644 --- a/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go +++ b/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go @@ -446,7 +446,8 @@ func validateMoveTablesWorkflow(t *testing.T, workflows []*vtctldatapb.Workflow) } func testAllRoutingRulesCommands(t *testing.T) { - var getRules func() string + var rulesBytes []byte + var err error var validateRules func(want, got string) typs := []string{"RoutingRules"} // , "ShardRoutingRules", "KeyspaceRoutingRules"} for _, typ := range typs { @@ -460,11 +461,8 @@ func testAllRoutingRulesCommands(t *testing.T) { }, }, } - rules, err := json2.MarshalPB(rr) + rulesBytes, err = json2.MarshalPB(rr) require.NoError(t, err) - getRules = func() string { - return string(rules) - } validateRules = func(want, got string) { var wantRules = &vschemapb.RoutingRules{} require.NoError(t, json2.Unmarshal([]byte(want), wantRules)) @@ -482,11 +480,8 @@ func testAllRoutingRulesCommands(t *testing.T) { }, }, } - rules, err := json2.MarshalPB(srr) + rulesBytes, err = json2.MarshalPB(srr) require.NoError(t, err) - getRules = func() string { - return string(rules) - } validateRules = func(want, got string) { var wantRules = &vschemapb.ShardRoutingRules{} require.NoError(t, json2.Unmarshal([]byte(want), wantRules)) @@ -503,11 +498,8 @@ func testAllRoutingRulesCommands(t *testing.T) { }, }, } - rules, err := json2.MarshalPB(krr) + rulesBytes, err = json2.MarshalPB(krr) require.NoError(t, err) - getRules = func() string { - return string(rules) - } validateRules = func(want, got string) { var wantRules = &vschemapb.KeyspaceRoutingRules{} require.NoError(t, json2.Unmarshal([]byte(want), wantRules)) @@ -515,20 +507,20 @@ func testAllRoutingRulesCommands(t *testing.T) { require.NoError(t, json2.Unmarshal([]byte(got), gotRules)) require.EqualValues(t, wantRules, gotRules) } - + default: + t.Fatalf("Unknown type %s", typ) } - testOneRoutingRulesCommand(t, typ, getRules, validateRules) + testOneRoutingRulesCommand(t, typ, string(rulesBytes), validateRules) } } -func testOneRoutingRulesCommand(t *testing.T, typ string, getRules func() string, validateRules func(want, got string)) { +func testOneRoutingRulesCommand(t *testing.T, typ string, rules string, validateRules func(want, got string)) { type routingRulesTest struct { name string rules string useFile bool } - rules := getRules() tests := []routingRulesTest{ { name: "inline", From b75de501ed2563d78194fef69a53e7f9cc60a3f7 Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Tue, 11 Jun 2024 11:09:54 +0200 Subject: [PATCH 06/11] Revert unnecessary change to marshal_test.go Signed-off-by: Rohit Nayak --- go/json2/marshal_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/go/json2/marshal_test.go b/go/json2/marshal_test.go index 93c39b9948f..b155126fb17 100644 --- a/go/json2/marshal_test.go +++ b/go/json2/marshal_test.go @@ -36,12 +36,6 @@ func TestMarshalPB(t *testing.T) { require.NoErrorf(t, err, "MarshalPB(%+v) error", col) want := "{\"name\":\"c1\",\"type\":\"VARCHAR\"}" assert.Equalf(t, want, string(b), "MarshalPB(%+v)", col) - - // Test MarshalPB with nil proto. - b, err = MarshalPB(nil) - require.NoErrorf(t, err, "MarshalPB(nil) error") - want = "{}" - assert.Equalf(t, want, string(b), "MarshalPB(nil)") } func TestMarshalIndentPB(t *testing.T) { From 1d9de8d98a65aca9cc76d5ea767a58d2856d9352 Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Tue, 11 Jun 2024 11:17:32 +0200 Subject: [PATCH 07/11] Minor refactor unit test Signed-off-by: Rohit Nayak --- go/json2/unmarshal_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/go/json2/unmarshal_test.go b/go/json2/unmarshal_test.go index 1616a368976..1079a464306 100644 --- a/go/json2/unmarshal_test.go +++ b/go/json2/unmarshal_test.go @@ -93,12 +93,12 @@ func TestAnnotate(t *testing.T) { } func TestUnmarshalPB(t *testing.T) { - protoData := &emptypb.Empty{} - protoJSONData, err := protojson.Marshal(protoData) + want := &emptypb.Empty{} + json, err := protojson.Marshal(want) require.Nil(t, err) - var pb emptypb.Empty - err = UnmarshalPB(protoJSONData, &pb) + var got emptypb.Empty + err = UnmarshalPB(json, &got) require.Nil(t, err) - require.Equal(t, protoData, &pb) + require.Equal(t, want, &got) } From a195191b6fd1da26444d1d5e3d6c20f98121cb2c Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Tue, 11 Jun 2024 11:36:26 +0200 Subject: [PATCH 08/11] Refactor e2e test Signed-off-by: Rohit Nayak --- .../vreplication_vtctldclient_cli_test.go | 56 +++++++++---------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go b/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go index d95bc7397a7..6040af45d28 100644 --- a/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go +++ b/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go @@ -24,12 +24,11 @@ import ( "strings" "testing" - "vitess.io/vitess/go/json2" - "github.com/stretchr/testify/require" "golang.org/x/exp/maps" "google.golang.org/protobuf/encoding/protojson" + "vitess.io/vitess/go/json2" "vitess.io/vitess/go/test/endtoend/cluster" binlogdatapb "vitess.io/vitess/go/vt/proto/binlogdata" @@ -64,8 +63,8 @@ func TestVtctldclientCLI(t *testing.T) { workflowName := "wf1" targetTabs := setupMinimalCustomerKeyspace(t) - t.Run("RoutingRulesCommands", func(t *testing.T) { - testAllRoutingRulesCommands(t) + t.Run("RoutingRulesApply", func(t *testing.T) { + testRoutingRulesApplyCommands(t) }) t.Run("WorkflowList", func(t *testing.T) { testWorkflowList(t, sourceKeyspaceName, targetKeyspaceName) @@ -445,12 +444,14 @@ func validateMoveTablesWorkflow(t *testing.T, workflows []*vtctldatapb.Workflow) require.True(t, bls.StopAfterCopy) } -func testAllRoutingRulesCommands(t *testing.T) { +// Test that routing rules can be applied using the vtctldclient CLI for all types of routing rules. +func testRoutingRulesApplyCommands(t *testing.T) { var rulesBytes []byte var err error var validateRules func(want, got string) - typs := []string{"RoutingRules"} // , "ShardRoutingRules", "KeyspaceRoutingRules"} - for _, typ := range typs { + + ruleTypes := []string{"RoutingRules", "ShardRoutingRules", "KeyspaceRoutingRules"} + for _, typ := range ruleTypes { switch typ { case "RoutingRules": rr := &vschemapb.RoutingRules{ @@ -465,9 +466,9 @@ func testAllRoutingRulesCommands(t *testing.T) { require.NoError(t, err) validateRules = func(want, got string) { var wantRules = &vschemapb.RoutingRules{} - require.NoError(t, json2.Unmarshal([]byte(want), wantRules)) + require.NoError(t, json2.UnmarshalPB([]byte(want), wantRules)) var gotRules = &vschemapb.RoutingRules{} - require.NoError(t, json2.Unmarshal([]byte(got), gotRules)) + require.NoError(t, json2.UnmarshalPB([]byte(got), gotRules)) require.EqualValues(t, wantRules, gotRules) } case "ShardRoutingRules": @@ -484,9 +485,9 @@ func testAllRoutingRulesCommands(t *testing.T) { require.NoError(t, err) validateRules = func(want, got string) { var wantRules = &vschemapb.ShardRoutingRules{} - require.NoError(t, json2.Unmarshal([]byte(want), wantRules)) + require.NoError(t, json2.UnmarshalPB([]byte(want), wantRules)) var gotRules = &vschemapb.ShardRoutingRules{} - require.NoError(t, json2.Unmarshal([]byte(got), gotRules)) + require.NoError(t, json2.UnmarshalPB([]byte(got), gotRules)) require.EqualValues(t, wantRules, gotRules) } case "KeyspaceRoutingRules": @@ -502,9 +503,9 @@ func testAllRoutingRulesCommands(t *testing.T) { require.NoError(t, err) validateRules = func(want, got string) { var wantRules = &vschemapb.KeyspaceRoutingRules{} - require.NoError(t, json2.Unmarshal([]byte(want), wantRules)) + require.NoError(t, json2.UnmarshalPB([]byte(want), wantRules)) var gotRules = &vschemapb.KeyspaceRoutingRules{} - require.NoError(t, json2.Unmarshal([]byte(got), gotRules)) + require.NoError(t, json2.UnmarshalPB([]byte(got), gotRules)) require.EqualValues(t, wantRules, gotRules) } default: @@ -515,34 +516,37 @@ func testAllRoutingRulesCommands(t *testing.T) { } +// For a given routing rules type, test that the rules can be applied using the vtctldclient CLI. +// We test both inline and file-based rules. +// The test also validates that both camelCase and snake_case key names work correctly. func testOneRoutingRulesCommand(t *testing.T, typ string, rules string, validateRules func(want, got string)) { type routingRulesTest struct { name string rules string - useFile bool + useFile bool // if true, use a file to pass the rules } tests := []routingRulesTest{ { name: "inline", - rules: string(rules), + rules: rules, }, { name: "file", - rules: string(rules), + rules: rules, useFile: true, }, { - name: "empty", + name: "empty", // finally, dcleanup rules rules: "{}", }, } for _, tt := range tests { t.Run(typ+"/"+tt.name, func(t *testing.T) { - done := false wantRules := tt.rules - typ2 := "camelCase" - for !done { - t.Run(typ2, func(t *testing.T) { + // The input rules are in camelCase, since they are the output of json2.MarshalPB + // The first iteration uses the output of routing rule Gets which are in snake_case. + for _, keyCase := range []string{"camelCase", "snake_case"} { + t.Run(keyCase, func(t *testing.T) { var args []string apply := fmt.Sprintf("Apply%s", typ) get := fmt.Sprintf("Get%s", typ) @@ -566,15 +570,11 @@ func testOneRoutingRulesCommand(t *testing.T, typ string, rules string, validate require.FailNowf(t, "failed action", get, "%v: %s", err, output) } validateRules(wantRules, output) - if typ2 == "snake_case" { - done = true - } else { - wantRules = output - typ2 = "snake_case" - } + // output of GetRoutingRules is in snake_case and we use it for the next iteration which + // tests applying rules with snake_case keys. + wantRules = output }) } }) } - } From 62f9a6bf8e301169d03d109bead2411331e1a469 Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Tue, 11 Jun 2024 11:48:30 +0200 Subject: [PATCH 09/11] Reuse UnmarshalPB in Unmarshal Signed-off-by: Rohit Nayak --- go/json2/unmarshal.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/go/json2/unmarshal.go b/go/json2/unmarshal.go index b5f50ff1864..e2034fa71c9 100644 --- a/go/json2/unmarshal.go +++ b/go/json2/unmarshal.go @@ -33,8 +33,7 @@ var carriageReturn = []byte("\n") // efficient and should not be used for high QPS operations. func Unmarshal(data []byte, v any) error { if pb, ok := v.(proto.Message); ok { - opts := protojson.UnmarshalOptions{DiscardUnknown: true} - return annotate(data, opts.Unmarshal(data, pb)) + return UnmarshalPB(data, pb) } return annotate(data, json.Unmarshal(data, v)) } From ee32b2a9aed143458682ff0668f0eb8bc29fbf0c Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Tue, 11 Jun 2024 16:28:03 +0200 Subject: [PATCH 10/11] Address review comments Signed-off-by: Rohit Nayak --- go/json2/unmarshal_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/json2/unmarshal_test.go b/go/json2/unmarshal_test.go index 1079a464306..1ba3368d5ca 100644 --- a/go/json2/unmarshal_test.go +++ b/go/json2/unmarshal_test.go @@ -95,10 +95,10 @@ func TestAnnotate(t *testing.T) { func TestUnmarshalPB(t *testing.T) { want := &emptypb.Empty{} json, err := protojson.Marshal(want) - require.Nil(t, err) + require.NoError(t, err) var got emptypb.Empty err = UnmarshalPB(json, &got) - require.Nil(t, err) + require.NoError(t, err) require.Equal(t, want, &got) } From dd0236b4a0553c905415e8cef8696a471435274a Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Wed, 12 Jun 2024 07:05:51 +0200 Subject: [PATCH 11/11] Address review comments Signed-off-by: Rohit Nayak --- .../vreplication/vreplication_vtctldclient_cli_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go b/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go index 6040af45d28..4a3f16a1cc9 100644 --- a/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go +++ b/go/test/endtoend/vreplication/vreplication_vtctldclient_cli_test.go @@ -509,7 +509,7 @@ func testRoutingRulesApplyCommands(t *testing.T) { require.EqualValues(t, wantRules, gotRules) } default: - t.Fatalf("Unknown type %s", typ) + require.FailNow(t, "Unknown type %s", typ) } testOneRoutingRulesCommand(t, typ, string(rulesBytes), validateRules) } @@ -536,7 +536,7 @@ func testOneRoutingRulesCommand(t *testing.T, typ string, rules string, validate useFile: true, }, { - name: "empty", // finally, dcleanup rules + name: "empty", // finally, cleanup rules rules: "{}", }, }