Merge pull request #319 from github/rename-comment-fix

added tests to verify no false positives rename-column found

go/logic/migrator.go

@@ -229,7 +229,7 @@ func (this *Migrator) validateStatement() (err error) {
 	if this.parser.HasNonTrivialRenames() && !this.migrationContext.SkipRenamedColumns {
 		this.migrationContext.ColumnRenameMap = this.parser.GetNonTrivialRenames()
 		if !this.migrationContext.ApproveRenamedColumns {
-			return fmt.Errorf("gh-ost believes the ALTER statement renames columns, as follows: %v; as precation, you are asked to confirm gh-ost is correct, and provide with `--approve-renamed-columns`, and we're all happy. Or you can skip renamed columns via `--skip-renamed-columns`, in which case column data may be lost", this.parser.GetNonTrivialRenames())
+			return fmt.Errorf("gh-ost believes the ALTER statement renames columns, as follows: %v; as precaution, you are asked to confirm gh-ost is correct, and provide with `--approve-renamed-columns`, and we're all happy. Or you can skip renamed columns via `--skip-renamed-columns`, in which case column data may be lost", this.parser.GetNonTrivialRenames())
 		}
 		log.Infof("Alter statement has column(s) renamed. gh-ost finds the following renames: %v; --approve-renamed-columns is given and so migration proceeds.", this.parser.GetNonTrivialRenames())
 	}

go/sql/parser.go

@@ -8,10 +8,12 @@ package sql
 import (
 	"regexp"
 	"strconv"
+	"strings"
 )
 
 var (
-	renameColumnRegexp = regexp.MustCompile(`(?i)change\s+(column\s+|)([\S]+)\s+([\S]+)\s+`)
+	sanitizeQuotesRegexp = regexp.MustCompile("('[^']*')")
+	renameColumnRegexp   = regexp.MustCompile(`(?i)\bchange\s+(column\s+|)([\S]+)\s+([\S]+)\s+`)
 )
 
 type Parser struct {
@@ -24,17 +26,54 @@ func NewParser() *Parser {
 	}
 }
 
-func (this *Parser) ParseAlterStatement(alterStatement string) (err error) {
+func (this *Parser) tokenizeAlterStatement(alterStatement string) (tokens []string, err error) {
-	allStringSubmatch := renameColumnRegexp.FindAllStringSubmatch(alterStatement, -1)
+	terminatingQuote := rune(0)
-	for _, submatch := range allStringSubmatch {
+	f := func(c rune) bool {
-		if unquoted, err := strconv.Unquote(submatch[2]); err == nil {
+		switch {
-			submatch[2] = unquoted
+		case c == terminatingQuote:
-		}
+			terminatingQuote = rune(0)
-		if unquoted, err := strconv.Unquote(submatch[3]); err == nil {
+			return false
-			submatch[3] = unquoted
+		case terminatingQuote != rune(0):
+			return false
+		case c == '\'':
+			terminatingQuote = c
+			return false
+		case c == '(':
+			terminatingQuote = ')'
+			return false
+		default:
+			return c == ','
 		}
+	}
 
-		this.columnRenameMap[submatch[2]] = submatch[3]
+	tokens = strings.FieldsFunc(alterStatement, f)
+	for i := range tokens {
+		tokens[i] = strings.TrimSpace(tokens[i])
+	}
+	return tokens, nil
+}
+
+func (this *Parser) sanitizeQuotesFromAlterStatement(alterStatement string) (strippedStatement string) {
+	strippedStatement = alterStatement
+	strippedStatement = sanitizeQuotesRegexp.ReplaceAllString(strippedStatement, "''")
+	return strippedStatement
+}
+
+func (this *Parser) ParseAlterStatement(alterStatement string) (err error) {
+	alterTokens, _ := this.tokenizeAlterStatement(alterStatement)
+	for _, alterToken := range alterTokens {
+		alterToken = this.sanitizeQuotesFromAlterStatement(alterToken)
+		allStringSubmatch := renameColumnRegexp.FindAllStringSubmatch(alterToken, -1)
+		for _, submatch := range allStringSubmatch {
+			if unquoted, err := strconv.Unquote(submatch[2]); err == nil {
+				submatch[2] = unquoted
+			}
+			if unquoted, err := strconv.Unquote(submatch[3]); err == nil {
+				submatch[3] = unquoted
+			}
+
+			this.columnRenameMap[submatch[2]] = submatch[3]
+		}
 	}
 	return nil
 }

go/sql/parser_test.go

@@ -6,6 +6,7 @@
 package sql
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/outbrain/golib/log"
@@ -66,3 +67,58 @@ func TestParseAlterStatementNonTrivial(t *testing.T) {
 		test.S(t).ExpectEquals(renames["f"], "fl")
 	}
 }
+
+func TestTokenizeAlterStatement(t *testing.T) {
+	parser := NewParser()
+	{
+		alterStatement := "add column t int"
+		tokens, _ := parser.tokenizeAlterStatement(alterStatement)
+		test.S(t).ExpectTrue(reflect.DeepEqual(tokens, []string{"add column t int"}))
+	}
+	{
+		alterStatement := "add column t int, change column i int"
+		tokens, _ := parser.tokenizeAlterStatement(alterStatement)
+		test.S(t).ExpectTrue(reflect.DeepEqual(tokens, []string{"add column t int", "change column i int"}))
+	}
+	{
+		alterStatement := "add column t int, change column i int 'some comment'"
+		tokens, _ := parser.tokenizeAlterStatement(alterStatement)
+		test.S(t).ExpectTrue(reflect.DeepEqual(tokens, []string{"add column t int", "change column i int 'some comment'"}))
+	}
+	{
+		alterStatement := "add column t int, change column i int 'some comment, with comma'"
+		tokens, _ := parser.tokenizeAlterStatement(alterStatement)
+		test.S(t).ExpectTrue(reflect.DeepEqual(tokens, []string{"add column t int", "change column i int 'some comment, with comma'"}))
+	}
+	{
+		alterStatement := "add column t int, add column d decimal(10,2)"
+		tokens, _ := parser.tokenizeAlterStatement(alterStatement)
+		test.S(t).ExpectTrue(reflect.DeepEqual(tokens, []string{"add column t int", "add column d decimal(10,2)"}))
+	}
+	{
+		alterStatement := "add column t int, add column e enum('a','b','c')"
+		tokens, _ := parser.tokenizeAlterStatement(alterStatement)
+		log.Errorf("%#v", tokens)
+		test.S(t).ExpectTrue(reflect.DeepEqual(tokens, []string{"add column t int", "add column e enum('a','b','c')"}))
+	}
+	{
+		alterStatement := "add column t int(11), add column e enum('a','b','c')"
+		tokens, _ := parser.tokenizeAlterStatement(alterStatement)
+		log.Errorf("%#v", tokens)
+		test.S(t).ExpectTrue(reflect.DeepEqual(tokens, []string{"add column t int(11)", "add column e enum('a','b','c')"}))
+	}
+}
+
+func TestSanitizeQuotesFromAlterStatement(t *testing.T) {
+	parser := NewParser()
+	{
+		alterStatement := "add column e enum('a','b','c')"
+		strippedStatement := parser.sanitizeQuotesFromAlterStatement(alterStatement)
+		test.S(t).ExpectEquals(strippedStatement, "add column e enum('','','')")
+	}
+	{
+		alterStatement := "change column i int 'some comment, with comma'"
+		strippedStatement := parser.sanitizeQuotesFromAlterStatement(alterStatement)
+		test.S(t).ExpectEquals(strippedStatement, "change column i int ''")
+	}
+}

localtests/rename-none-column/create.sql

@@ -0,0 +1,8 @@
+drop table if exists gh_ost_test;
+create table gh_ost_test (
+  id int auto_increment,
+  c1 int not null,
+  primary key (id)
+) auto_increment=1;
+
+drop event if exists gh_ost_test;

localtests/rename-none-column/extra_args

@@ -0,0 +1 @@
+--alter="add column exchange double comment 'exchange rate used for pay in your own currency'"

localtests/rename-none-comment/create.sql

@@ -0,0 +1,8 @@
+drop table if exists gh_ost_test;
+create table gh_ost_test (
+  id int auto_increment,
+  c1 int not null,
+  primary key (id)
+) auto_increment=1;
+
+drop event if exists gh_ost_test;

localtests/rename-none-comment/extra_args

@@ -0,0 +1 @@
+--alter="add column exchange_rate double comment 'change rate used for pay in your own currency'"