Fix 4 trigger handling bugs (#1626)

* fix: remove double-transformation in trigger length validation

ValidateGhostTriggerLengthBelowMaxLength was calling GetGhostTriggerName
on an already-transformed name, adding the suffix twice. This caused valid
trigger names (ghost name <= 64 chars) to be falsely rejected.

The caller in inspect.go:627 already transforms the name via
GetGhostTriggerName before passing it, so the validation function
should check the length as-is.

Unit tests updated to reflect the correct call pattern: transform first
with GetGhostTriggerName, then validate the result. Added boundary tests
for exactly 64 and 65 char names.

* fix: return error from trigger creation during atomic cut-over

During atomic cut-over, if CreateTriggersOnGhost failed, the error was
logged but not returned. The migration continued and completed without
triggers, silently losing them.

The two-step cut-over (line 793) already correctly returns the error.
This aligns the atomic cut-over to do the same.

* fix: check trigger name uniqueness per schema, not per table

validateGhostTriggersDontExist was filtering by event_object_table,
only checking if the ghost trigger name existed on the original table.
MySQL trigger names are unique per schema, so a trigger with the same
name on any other table would block CREATE TRIGGER but pass validation.

Remove the event_object_table filter to check trigger_name + trigger_schema
only, matching MySQL's uniqueness constraint.

* fix: use parameterized query in GetTriggers to prevent SQL injection

GetTriggers used fmt.Sprintf with string interpolation for database and
table names, causing SQL syntax errors with special characters and
potential SQL injection. Switched to parameterized query with ? placeholders,
matching the safe pattern already used in inspect.go:553-559.

* test: add regression tests for trigger handling bugs

Add two integration tests:
- trigger-long-name-validation: verifies 60-char trigger names
  (64-char ghost name) are not falsely rejected by double-transform
- trigger-ghost-name-conflict: verifies validation detects ghost
  trigger name conflicts on other tables in the same schema

* style: gofmt context_test.go

---------

Co-authored-by: Yakir Gibraltar <yakir.g@taboola.com>
Co-authored-by: meiji163 <meiji163@github.com>

Author: 3 people

go/base/context.go

@@ -970,9 +970,8 @@ func (this *MigrationContext) GetGhostTriggerName(triggerName string) string {
 	return triggerName + this.TriggerSuffix
 }
 
-// validateGhostTriggerLength check if the ghost trigger name length is not more than 64 characters
+// ValidateGhostTriggerLengthBelowMaxLength checks if the given trigger name (already transformed
+// by GetGhostTriggerName) does not exceed the maximum allowed length.
 func (this *MigrationContext) ValidateGhostTriggerLengthBelowMaxLength(triggerName string) bool {
-	ghostTriggerName := this.GetGhostTriggerName(triggerName)
-
-	return utf8.RuneCountInString(ghostTriggerName) <= mysql.MaxTableNameLength
+	return utf8.RuneCountInString(triggerName) <= mysql.MaxTableNameLength
 }

go/base/context_test.go

@@ -86,38 +86,69 @@ func TestGetTriggerNames(t *testing.T) {
 }
 
 func TestValidateGhostTriggerLengthBelowMaxLength(t *testing.T) {
+	// Tests simulate the real call pattern: GetGhostTriggerName first, then validate the result.
 	{
+		// Short trigger name with suffix appended: well under 64 chars
 		context := NewMigrationContext()
 		context.TriggerSuffix = "_gho"
-		require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength("my_trigger"))
+		ghostName := context.GetGhostTriggerName("my_trigger") // "my_trigger_gho" = 14 chars
+		require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName))
 	}
 	{
+		// 64-char original + "_ghost" suffix = 70 chars → exceeds limit
 		context := NewMigrationContext()
 		context.TriggerSuffix = "_ghost"
-		require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 4))) // 64 characters + "_ghost"
+		ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 4)) // 64 + 6 = 70
+		require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName))
 	}
 	{
+		// 48-char original + "_ghost" suffix = 54 chars → valid
 		context := NewMigrationContext()
 		context.TriggerSuffix = "_ghost"
-		require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 3))) // 48 characters + "_ghost"
+		ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 3)) // 48 + 6 = 54
+		require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName))
 	}
 	{
+		// RemoveTriggerSuffix: 64-char name ending in "_ghost" → suffix removed → 58 chars → valid
 		context := NewMigrationContext()
 		context.TriggerSuffix = "_ghost"
 		context.RemoveTriggerSuffix = true
-		require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 4))) // 64 characters + "_ghost" removed
+		ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 4)) // suffix removed → 58
+		require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName))
 	}
 	{
+		// RemoveTriggerSuffix: name doesn't end in suffix → suffix appended → 65 + 6 = 71 chars → exceeds
 		context := NewMigrationContext()
 		context.TriggerSuffix = "_ghost"
 		context.RemoveTriggerSuffix = true
-		require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 4)+"X")) // 65 characters + "_ghost" not removed
+		ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 4) + "X") // no match, appended → 71
+		require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName))
 	}
 	{
+		// RemoveTriggerSuffix: 70-char name ending in "_ghost" → suffix removed → 64 chars → exactly at limit → valid
 		context := NewMigrationContext()
 		context.TriggerSuffix = "_ghost"
 		context.RemoveTriggerSuffix = true
-		require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(strings.Repeat("my_trigger_ghost", 4)+"_ghost")) // 70 characters + last "_ghost"  removed
+		ghostName := context.GetGhostTriggerName(strings.Repeat("my_trigger_ghost", 4) + "_ghost") // suffix removed → 64
+		require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName))
+	}
+	{
+		// Edge case: exactly 64 chars after transformation → valid (boundary test)
+		context := NewMigrationContext()
+		context.TriggerSuffix = "_ght"
+		originalName := strings.Repeat("x", 60)                // 60 chars
+		ghostName := context.GetGhostTriggerName(originalName) // 60 + 4 = 64
+		require.Equal(t, 64, len(ghostName))
+		require.True(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName))
+	}
+	{
+		// Edge case: 65 chars after transformation → exceeds (boundary test)
+		context := NewMigrationContext()
+		context.TriggerSuffix = "_ght"
+		originalName := strings.Repeat("x", 61)                // 61 chars
+		ghostName := context.GetGhostTriggerName(originalName) // 61 + 4 = 65
+		require.Equal(t, 65, len(ghostName))
+		require.False(t, context.ValidateGhostTriggerLengthBelowMaxLength(ghostName))
 	}
 }
 

go/logic/inspect.go

@@ -596,7 +596,7 @@ func (this *Inspector) validateGhostTriggersDontExist() error {
 		var foundTriggers []string
 		for _, trigger := range this.migrationContext.Triggers {
 			triggerName := this.migrationContext.GetGhostTriggerName(trigger.Name)
-			query := "select 1 from information_schema.triggers where trigger_name = ? and trigger_schema = ? and event_object_table = ?"
+			query := "select 1 from information_schema.triggers where trigger_name = ? and trigger_schema = ?"
 			err := sqlutils.QueryRowsMap(this.db, query, func(rowMap sqlutils.RowMap) error {
 				triggerExists := rowMap.GetInt("1")
 				if triggerExists == 1 {
@@ -606,7 +606,6 @@ func (this *Inspector) validateGhostTriggersDontExist() error {
 			},
 				triggerName,
 				this.migrationContext.DatabaseName,
-				this.migrationContext.OriginalTableName,
 			)
 			if err != nil {
 				return err

go/logic/migrator.go

@@ -842,7 +842,7 @@ func (this *Migrator) atomicCutOver() (err error) {
 	// If we need to create triggers we need to do it here (only create part)
 	if this.migrationContext.IncludeTriggers && len(this.migrationContext.Triggers) > 0 {
 		if err := this.applier.CreateTriggersOnGhost(); err != nil {
-			this.migrationContext.Log.Errore(err)
+			return this.migrationContext.Log.Errore(err)
 		}
 	}
 

go/mysql/utils.go

@@ -248,9 +248,9 @@ func Kill(db *gosql.DB, connectionID string) error {
 
 // GetTriggers reads trigger list from given table
 func GetTriggers(db *gosql.DB, databaseName, tableName string) (triggers []Trigger, err error) {
-	query := fmt.Sprintf(`select trigger_name as name, event_manipulation as event, action_statement as statement, action_timing as timing
-	from information_schema.triggers 
-	where trigger_schema = '%s' and event_object_table = '%s'`, databaseName, tableName)
+	query := `select trigger_name as name, event_manipulation as event, action_statement as statement, action_timing as timing
+	from information_schema.triggers
+	where trigger_schema = ? and event_object_table = ?`
 
 	err = sqlutils.QueryRowsMap(db, query, func(rowMap sqlutils.RowMap) error {
 		triggers = append(triggers, Trigger{
@@ -260,7 +260,7 @@ func GetTriggers(db *gosql.DB, databaseName, tableName string) (triggers []Trigg
 			Timing:    rowMap.GetString("timing"),
 		})
 		return nil
-	})
+	}, databaseName, tableName)
 	if err != nil {
 		return nil, err
 	}

localtests/trigger-ghost-name-conflict/create.sql

@@ -0,0 +1,35 @@
+-- Bug #3 regression test: validateGhostTriggersDontExist must check whole schema
+-- MySQL trigger names are unique per SCHEMA, not per table.
+-- The validation must detect a trigger with the ghost name on ANY table in the schema.
+
+drop trigger if exists gh_ost_test_ai_ght;
+drop trigger if exists gh_ost_test_ai;
+drop table if exists gh_ost_test_other;
+drop table if exists gh_ost_test;
+
+create table gh_ost_test (
+  id int auto_increment,
+  i int not null,
+  primary key(id)
+) auto_increment=1;
+
+-- This trigger has the _ght suffix (simulating a previous migration left it).
+-- Ghost name = "gh_ost_test_ai" (suffix removed).
+create trigger gh_ost_test_ai_ght
+  after insert on gh_ost_test for each row
+  set @dummy = 1;
+
+-- Create ANOTHER table with a trigger named "gh_ost_test_ai" (the ghost name).
+-- Validation must detect this conflict even though the trigger is on a different table.
+create table gh_ost_test_other (
+  id int auto_increment,
+  primary key(id)
+);
+
+create trigger gh_ost_test_ai
+  after insert on gh_ost_test_other for each row
+  set @dummy = 1;
+
+insert into gh_ost_test values (null, 11);
+insert into gh_ost_test values (null, 13);
+insert into gh_ost_test values (null, 17);

localtests/trigger-ghost-name-conflict/destroy.sql

@@ -0,0 +1,2 @@
+drop trigger if exists gh_ost_test_ai;
+drop table if exists gh_ost_test_other;

localtests/trigger-ghost-name-conflict/expect_failure

@@ -0,0 +1 @@
+Found gh-ost triggers

localtests/trigger-ghost-name-conflict/extra_args

@@ -0,0 +1 @@
+--include-triggers --trigger-suffix=_ght --remove-trigger-suffix-if-exists

localtests/trigger-long-name-validation/create.sql

@@ -0,0 +1,38 @@
+-- Bug #1: Double-transformation in trigger length validation
+-- A trigger with a 60-char name should be valid: ghost name = 60 + 4 (_ght) = 64 chars (max allowed).
+-- But validateGhostTriggersLength() applies GetGhostTriggerName() twice,
+-- computing 60 + 4 + 4 = 68, which falsely exceeds the 64-char limit.
+
+drop table if exists gh_ost_test;
+
+create table gh_ost_test (
+  id int auto_increment,
+  i int not null,
+  primary key(id)
+) auto_increment=1;
+
+-- Trigger name is exactly 60 characters (padded to 60).
+-- Ghost name with _ght suffix = 64 chars = exactly at the MySQL limit.
+-- 60 chars: trigger_long_name_padding_aaaaaaaaaaaaaaaaaaaaa_60chars_xxxx
+create trigger trigger_long_name_padding_aaaaaaaaaaaaaaaaaaaaa_60chars_xxxx
+  after insert on gh_ost_test for each row
+  set @dummy = 1;
+
+insert into gh_ost_test values (null, 11);
+insert into gh_ost_test values (null, 13);
+insert into gh_ost_test values (null, 17);
+
+drop event if exists gh_ost_test;
+delimiter ;;
+create event gh_ost_test
+  on schedule every 1 second
+  starts current_timestamp
+  ends current_timestamp + interval 60 second
+  on completion not preserve
+  enable
+  do
+begin
+  insert into gh_ost_test values (null, 23);
+  update gh_ost_test set i=i+1 where id=1;
+end ;;
+delimiter ;