From b9eb9f67688945616e796a81e215a79cbc57df10 Mon Sep 17 00:00:00 2001
From: Weizhen Wang <wangweizhen@pingcap.com>
Date: Thu, 23 Dec 2021 11:59:48 +0800
Subject: [PATCH] ddl: add format error for incorrect dict syntax in the
 placement rule (#30919)

close pingcap/tidb#30454
---
 ddl/placement/errors.go    |  4 +++
 ddl/placement/rule.go      | 16 ++++++++++++
 ddl/placement/rule_test.go | 53 ++++++++++++++++++++++++++------------
 3 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/ddl/placement/errors.go b/ddl/placement/errors.go
index 4f98e0fa4c2ec..b609827bd4ce7 100644
--- a/ddl/placement/errors.go
+++ b/ddl/placement/errors.go
@@ -43,4 +43,8 @@ var (
 	ErrNoRulesToDrop = errors.New("no rule of such role to drop")
 	// ErrInvalidPlacementOptions is from bundle.go.
 	ErrInvalidPlacementOptions = errors.New("invalid placement option")
+	// ErrInvalidConstraintsMappingWrongSeparator is wrong separator in mapping.
+	ErrInvalidConstraintsMappingWrongSeparator = errors.New("mappings use a colon and space (“: ”) to mark each key/value pair")
+	// ErrInvalidConstraintsMappingNoColonFound is no colon found in mapping.
+	ErrInvalidConstraintsMappingNoColonFound = errors.New("no colon found")
 )
diff --git a/ddl/placement/rule.go b/ddl/placement/rule.go
index 88cd5067153f8..518dd369414b2 100644
--- a/ddl/placement/rule.go
+++ b/ddl/placement/rule.go
@@ -16,6 +16,7 @@ package placement
 
 import (
 	"fmt"
+	"regexp"
 	"strings"
 
 	"gopkg.in/yaml.v2"
@@ -61,6 +62,18 @@ func NewRule(role PeerRoleType, replicas uint64, cnst Constraints) *Rule {
 	}
 }
 
+var wrongSeparatorRegexp = regexp.MustCompile(`[^"':]+:\d`)
+
+func getYamlMapFormatError(str string) error {
+	if !strings.Contains(str, ":") {
+		return ErrInvalidConstraintsMappingNoColonFound
+	}
+	if wrongSeparatorRegexp.MatchString(str) {
+		return ErrInvalidConstraintsMappingWrongSeparator
+	}
+	return nil
+}
+
 // NewRules constructs []*Rule from a yaml-compatible representation of
 // 'array' or 'dict' constraints.
 // Refer to https://github.com/pingcap/tidb/blob/master/docs/design/2020-06-24-placement-rules-in-sql.md.
@@ -86,6 +99,9 @@ func NewRules(role PeerRoleType, replicas uint64, cnstr string) ([]*Rule, error)
 		ruleCnt := 0
 		for labels, cnt := range constraints2 {
 			if cnt <= 0 {
+				if err := getYamlMapFormatError(string(cnstbytes)); err != nil {
+					return rules, err
+				}
 				return rules, fmt.Errorf("%w: count of labels '%s' should be positive, but got %d", ErrInvalidConstraintsMapcnt, labels, cnt)
 			}
 			ruleCnt += cnt
diff --git a/ddl/placement/rule_test.go b/ddl/placement/rule_test.go
index 9432448127a4a..f38819c278998 100644
--- a/ddl/placement/rule_test.go
+++ b/ddl/placement/rule_test.go
@@ -16,21 +16,20 @@ package placement
 
 import (
 	"errors"
+	"reflect"
+	"testing"
 
 	. "github.com/pingcap/check"
+	"github.com/stretchr/testify/require"
 )
 
-var _ = Suite(&testRuleSuite{})
-
-type testRuleSuite struct{}
-
-func (t *testRuleSuite) TestClone(c *C) {
+func TestClone(t *testing.T) {
 	rule := &Rule{ID: "434"}
 	newRule := rule.Clone()
 	newRule.ID = "121"
 
-	c.Assert(rule, DeepEquals, &Rule{ID: "434"})
-	c.Assert(newRule, DeepEquals, &Rule{ID: "121"})
+	require.Equal(t, &Rule{ID: "434"}, rule)
+	require.Equal(t, &Rule{ID: "121"}, newRule)
 }
 
 func matchRules(t1, t2 []*Rule, prefix string, c *C) {
@@ -50,7 +49,22 @@ func matchRules(t1, t2 []*Rule, prefix string, c *C) {
 	}
 }
 
-func (t *testRuleSuite) TestNewRuleAndNewRules(c *C) {
+func matchRulesT(t1, t2 []*Rule, prefix string, t *testing.T) {
+	require.Equal(t, len(t2), len(t1), prefix)
+	for i := range t1 {
+		found := false
+		for j := range t2 {
+			ok := reflect.DeepEqual(t2[j], t1[i])
+			if ok {
+				found = true
+				break
+			}
+		}
+		require.True(t, found, "%s\n\ncan not found %d rule\n%+v\n%+v", prefix, i, t1[i], t2)
+	}
+}
+
+func TestNewRuleAndNewRules(t *testing.T) {
 	type TestCase struct {
 		name     string
 		input    string
@@ -58,7 +72,7 @@ func (t *testRuleSuite) TestNewRuleAndNewRules(c *C) {
 		output   []*Rule
 		err      error
 	}
-	tests := []TestCase{}
+	var tests []TestCase
 
 	tests = append(tests, TestCase{
 		name:     "empty constraints",
@@ -175,14 +189,21 @@ func (t *testRuleSuite) TestNewRuleAndNewRules(c *C) {
 		err:      ErrInvalidConstraintFormat,
 	})
 
-	for _, t := range tests {
-		comment := Commentf("[%s]", t.name)
-		output, err := NewRules(Voter, t.replicas, t.input)
-		if t.err == nil {
-			c.Assert(err, IsNil, comment)
-			matchRules(t.output, output, comment.CheckCommentString(), c)
+	tests = append(tests, TestCase{
+		name:     "invalid map separator",
+		input:    `{+region=us-east-2:2}`,
+		replicas: 6,
+		err:      ErrInvalidConstraintsMappingWrongSeparator,
+	})
+
+	for _, tt := range tests {
+		comment := Commentf("[%s]", tt.name)
+		output, err := NewRules(Voter, tt.replicas, tt.input)
+		if tt.err == nil {
+			require.NoError(t, err, comment)
+			matchRulesT(tt.output, output, comment.CheckCommentString(), t)
 		} else {
-			c.Assert(errors.Is(err, t.err), IsTrue, Commentf("[%s]\n%s\n%s\n", t.name, err, t.err))
+			require.True(t, errors.Is(err, tt.err), "[%s]\n%s\n%s\n", tt.name, err, tt.err)
 		}
 	}
 }