Merge pull request #142 from k1LoW/require-fk-index

Add lint rule `requireForeignKeyIndex`

README.md

@@ -442,6 +442,11 @@ lint:
   # check duplicate relations
   duplicateRelations:
     enabled: true
+  # check if the foreign key columns have an index
+  requireForeignKeyIndex:
+    enabled: true
+    exclude:
+      - comments.user_id
 ```
 
 ### Exclude tables

cmd/lint.go

@@ -76,9 +76,6 @@ var lintCmd = &cobra.Command{
 			var v config.Rule
 			r := l.Field(i)
 			v = r.Interface().(config.Rule)
-			if !v.IsEnabled() {
-				continue
-			}
 			ruleWarns = append(ruleWarns, v.Check(s)...)
 		}
 		if len(ruleWarns) > 0 {

config/lint.go

@@ -9,12 +9,13 @@ import (
 
 // Lint is the struct for lint config
 type Lint struct {
-	RequireTableComment  RequireTableComment  `yaml:"requireTableComment"`
-	RequireColumnComment RequireColumnComment `yaml:"requireColumnComment"`
-	UnrelatedTable       UnrelatedTable       `yaml:"unrelatedTable"`
-	ColumnCount          ColumnCount          `yaml:"columnCount"`
-	RequireColumns       RequireColumns       `yaml:"requireColumns"`
-	DuplicateRelations   DuplicateRelations   `yaml:"duplicateRelations"`
+	RequireTableComment    RequireTableComment    `yaml:"requireTableComment"`
+	RequireColumnComment   RequireColumnComment   `yaml:"requireColumnComment"`
+	UnrelatedTable         UnrelatedTable         `yaml:"unrelatedTable"`
+	ColumnCount            ColumnCount            `yaml:"columnCount"`
+	RequireColumns         RequireColumns         `yaml:"requireColumns"`
+	DuplicateRelations     DuplicateRelations     `yaml:"duplicateRelations"`
+	RequireForeignKeyIndex RequireForeignKeyIndex `yaml:"requireForeignKeyIndex"`
 }
 
 // RuleWarn is struct of Rule error
@@ -29,7 +30,7 @@ type Rule interface {
 	Check(*schema.Schema) []RuleWarn
 }
 
-// RequireTableComment check table comment
+// RequireTableComment checks table comment
 type RequireTableComment struct {
 	Enabled bool     `yaml:"enabled"`
 	Exclude []string `yaml:"exclude"`
@@ -42,8 +43,12 @@ func (r RequireTableComment) IsEnabled() bool {
 
 // Check table comment
 func (r RequireTableComment) Check(s *schema.Schema) []RuleWarn {
-	msg := "table comment required."
 	warns := []RuleWarn{}
+	if !r.IsEnabled() {
+		return warns
+	}
+	msg := "table comment required."
+
 	for _, t := range s.Tables {
 		if !contains(r.Exclude, t.Name) && t.Comment == "" {
 			warns = append(warns, RuleWarn{
@@ -55,7 +60,7 @@ func (r RequireTableComment) Check(s *schema.Schema) []RuleWarn {
 	return warns
 }
 
-// RequireColumnComment check column comment
+// RequireColumnComment checks column comment
 type RequireColumnComment struct {
 	Enabled        bool     `yaml:"enabled"`
 	Exclude        []string `yaml:"exclude"`
@@ -69,8 +74,12 @@ func (r RequireColumnComment) IsEnabled() bool {
 
 // Check column comment
 func (r RequireColumnComment) Check(s *schema.Schema) []RuleWarn {
-	msg := "column comment required."
 	warns := []RuleWarn{}
+	if !r.IsEnabled() {
+		return warns
+	}
+	msg := "column comment required."
+
 	for _, t := range s.Tables {
 		if contains(r.ExcludedTables, t.Name) {
 			continue
@@ -87,7 +96,7 @@ func (r RequireColumnComment) Check(s *schema.Schema) []RuleWarn {
 	return warns
 }
 
-// UnrelatedTable check isolated table
+// UnrelatedTable checks isolated table
 type UnrelatedTable struct {
 	Enabled bool     `yaml:"enabled"`
 	Exclude []string `yaml:"exclude"`
@@ -100,8 +109,12 @@ func (r UnrelatedTable) IsEnabled() bool {
 
 // Check table relation
 func (r UnrelatedTable) Check(s *schema.Schema) []RuleWarn {
-	msgFmt := "unrelated (isolated) table exists. [%d]"
 	warns := []RuleWarn{}
+	if !r.IsEnabled() {
+		return warns
+	}
+	msgFmt := "unrelated (isolated) table exists. [%d]"
+
 	tableMap := map[string]*schema.Table{}
 	for _, t := range s.Tables {
 		if contains(r.Exclude, t.Name) {
@@ -122,7 +135,7 @@ func (r UnrelatedTable) Check(s *schema.Schema) []RuleWarn {
 	return warns
 }
 
-// ColumnCount check table column count
+// ColumnCount checks table column count
 type ColumnCount struct {
 	Enabled bool     `yaml:"enabled"`
 	Max     int      `yaml:"max"`
@@ -136,8 +149,12 @@ func (r ColumnCount) IsEnabled() bool {
 
 // Check table column count
 func (r ColumnCount) Check(s *schema.Schema) []RuleWarn {
-	msgFmt := "too many columns. [%d/%d]"
 	warns := []RuleWarn{}
+	if !r.IsEnabled() {
+		return warns
+	}
+	msgFmt := "too many columns. [%d/%d]"
+
 	for _, t := range s.Tables {
 		if !contains(r.Exclude, t.Name) && len(t.Columns) > r.Max {
 			warns = append(warns, RuleWarn{
@@ -158,7 +175,7 @@ func contains(s []string, e string) bool {
 	return false
 }
 
-// RequireColumns check required table columns
+// RequireColumns checks if the table has specified columns
 type RequireColumns struct {
 	Enabled bool                   `yaml:"enabled"`
 	Columns []RequireColumnsColumn `yaml:"columns"`
@@ -178,6 +195,9 @@ func (r RequireColumns) IsEnabled() bool {
 // Check the existence of a table columns
 func (r RequireColumns) Check(s *schema.Schema) []RuleWarn {
 	warns := []RuleWarn{}
+	if !r.IsEnabled() {
+		return warns
+	}
 	for _, t := range s.Tables {
 		for _, cc := range r.Columns {
 			excluded := false
@@ -207,7 +227,7 @@ func (r RequireColumns) Check(s *schema.Schema) []RuleWarn {
 	return warns
 }
 
-// DuplicateRelations check duplicate table relations
+// DuplicateRelations checks duplicate table relations
 type DuplicateRelations struct {
 	Enabled bool `yaml:"enabled"`
 }
@@ -220,6 +240,9 @@ func (r DuplicateRelations) IsEnabled() bool {
 // Check duplicate table relations
 func (r DuplicateRelations) Check(s *schema.Schema) []RuleWarn {
 	warns := []RuleWarn{}
+	if !r.IsEnabled() {
+		return warns
+	}
 	relations := make(map[[4]string]bool)
 	msgFmt := "duplicate relations. [%s -> %s]"
 
@@ -247,3 +270,50 @@ func (r DuplicateRelations) Check(s *schema.Schema) []RuleWarn {
 
 	return warns
 }
+
+// RequireForeignKeyIndex checks if the foreign key columns have an index
+type RequireForeignKeyIndex struct {
+	Enabled bool     `yaml:"enabled"`
+	Exclude []string `yaml:"exclude"`
+}
+
+// IsEnabled return Rule is enabled or not
+func (r RequireForeignKeyIndex) IsEnabled() bool {
+	return r.Enabled
+}
+
+// Check if the foreign key columns have an index
+func (r RequireForeignKeyIndex) Check(s *schema.Schema) []RuleWarn {
+	warns := []RuleWarn{}
+	if !r.IsEnabled() {
+		return warns
+	}
+	msgFmt := "foreign key columns do not have an index. [%s]"
+
+	for _, t := range s.Tables {
+		for _, c := range t.Constraints {
+			for _, c1 := range c.Columns {
+				target := fmt.Sprintf("%s.%s", t.Name, c1)
+				if contains(r.Exclude, c1) || contains(r.Exclude, target) {
+					continue
+				}
+				exist := false
+				for _, i := range t.Indexes {
+					for _, c2 := range i.Columns {
+						if c1 == c2 {
+							exist = true
+						}
+					}
+				}
+				if !exist {
+					warns = append(warns, RuleWarn{
+						Target:  target,
+						Message: fmt.Sprintf(msgFmt, t.Name),
+					})
+				}
+			}
+		}
+	}
+
+	return warns
+}

config/lint_test.go

@@ -18,6 +18,17 @@ func TestRequireTableComment(t *testing.T) {
 	}
 }
 
+func TestIsEnabled(t *testing.T) {
+	r := RequireTableComment{
+		Enabled: false,
+	}
+	s := newTestSchema()
+	warns := r.Check(s)
+	if len(warns) != 0 {
+		t.Errorf("actual %v\nwant %v", len(warns), 1)
+	}
+}
+
 func TestRequireTableCommentWithExclude(t *testing.T) {
 	r := RequireTableComment{
 		Enabled: true,
@@ -44,7 +55,7 @@ func TestRequireColumnComment(t *testing.T) {
 func TestRequireColumnCommentWithExclude(t *testing.T) {
 	r := RequireColumnComment{
 		Enabled: true,
-		Exclude: []string{"b"},
+		Exclude: []string{"b1"},
 	}
 	s := newTestSchema()
 	warns := r.Check(s)
@@ -180,15 +191,41 @@ func TestDuplicateRelations(t *testing.T) {
 	}
 }
 
+func TestRequireForeignKeyIndex(t *testing.T) {
+	tests := []struct {
+		enabled bool
+		exclude []string
+		want    int
+	}{
+		{true, []string{}, 1},
+		{false, []string{}, 0},
+		{true, []string{"a.a1"}, 0},
+		{true, []string{"a1"}, 0},
+	}
+
+	for i, tt := range tests {
+		r := RequireForeignKeyIndex{
+			Enabled: tt.enabled,
+			Exclude: tt.exclude,
+		}
+		s := newTestSchema()
+		warns := r.Check(s)
+		want := tt.want
+		if len(warns) != want {
+			t.Errorf("TestRequireForeignKeyIndex(%d)actual %v\nwant %v", i, len(warns), want)
+		}
+	}
+}
+
 func newTestSchema() *schema.Schema {
 	ca := &schema.Column{
-		Name:     "a",
+		Name:     "a1",
 		Type:     "bigint(20)",
 		Comment:  "column a",
 		Nullable: false,
 	}
 	cb := &schema.Column{
-		Name:     "b",
+		Name:     "b1",
 		Type:     "text",
 		Comment:  "", // empty comment
 		Nullable: true,
@@ -257,6 +294,7 @@ func newTestSchema() *schema.Schema {
 			},
 		},
 	}
+
 	r := &schema.Relation{
 		Table:         ta,
 		Columns:       []*schema.Column{ca},
@@ -266,6 +304,28 @@ func newTestSchema() *schema.Schema {
 	ca.ParentRelations = []*schema.Relation{r}
 	cb.ChildRelations = []*schema.Relation{r}
 
+	ta.Indexes = []*schema.Index{
+		&schema.Index{
+			Name:  "a2_idx",
+			Def:   "a2 index",
+			Table: &ta.Name,
+			Columns: []string{
+				"a2",
+			},
+		},
+	}
+
+	ta.Constraints = []*schema.Constraint{
+		&schema.Constraint{
+			Name:             "a1_b1_fk",
+			Type:             schema.FOREIGN_KEY,
+			Table:            &ta.Name,
+			ReferenceTable:   &tb.Name,
+			Columns:          []string{"a1"},
+			ReferenceColumns: []string{"b1"},
+		},
+	}
+
 	s := &schema.Schema{
 		Name: "testschema",
 		Tables: []*schema.Table{

drivers/mssql/mssql.go

@@ -11,7 +11,7 @@ import (
 )
 
 var defaultSchemaName = "dbo"
-var typeFk = "FOREIGN KEY"
+var typeFk = schema.FOREIGN_KEY
 var typeCheck = "CHECK"
 var reSystemNamed = regexp.MustCompile(`_[^_]+$`)
 

drivers/mysql/mysql.go

@@ -202,6 +202,7 @@ GROUP BY kcu.constraint_name, sub.costraint_type, kcu.referenced_table_name`, ta
 			case "UNIQUE":
 				constraintDef = fmt.Sprintf("UNIQUE KEY %s (%s)", constraintName, constraintColumnName)
 			case "FOREIGN KEY":
+				constraintType = schema.FOREIGN_KEY
 				constraintDef = fmt.Sprintf("FOREIGN KEY (%s) REFERENCES %s (%s)", constraintColumnName, constraintRefTableName.String, constraintRefColumnName.String)
 				relation := &schema.Relation{
 					Table: table,

drivers/postgres/postgres.go

@@ -472,7 +472,7 @@ func convertConstraintType(t string) string {
 	case "u":
 		return "UNIQUE"
 	case "f":
-		return "FOREIGN KEY"
+		return schema.FOREIGN_KEY
 	case "c":
 		return "CHECK"
 	case "t":

drivers/sqlite/sqlite.go

@@ -190,7 +190,7 @@ WHERE name != 'sqlite_sequence' AND (type = 'table' OR type = 'view');`)
 				strings.Join(f.ColumnNames, ", "), f.ForeignTableName, strings.Join(f.ForeignColumnNames, ", "), f.OnUpdate, f.OnDelete, f.Match) // #nosec
 			constraint := &schema.Constraint{
 				Name:             fmt.Sprintf("- (Foreign key ID: %s)", f.ID),
-				Type:             "FOREIGN KEY",
+				Type:             schema.FOREIGN_KEY,
 				Def:              foreignKeyDef,
 				Table:            &table.Name,
 				Columns:          f.ColumnNames,

schema/schema.go

@@ -9,6 +9,10 @@ import (
 	"github.com/pkg/errors"
 )
 
+const (
+	FOREIGN_KEY = "FOREIGN KEY"
+)
+
 // Index is the struct for database index
 type Index struct {
 	Name    string   `json:"name"`