Fix 4 trigger handling bugs #1626
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
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.
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.
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
yakirgb
requested review from
meiji163,
rashiq and
timvaillancourt
as code owners
Contributor
|
Thanks for the PR @yakirgb ! Can you |
Contributor
Author
meiji163
approved these changes
Feb 10, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1625 — four related bugs found by code inspection of the trigger handling logic.
Bug 1: Double-transformation in trigger length validation
ValidateGhostTriggerLengthBelowMaxLengthcalledGetGhostTriggerNameon an already-transformed name, adding the suffix twice and falsely rejecting valid trigger names.Bug 2: Atomic cut-over silently ignores trigger creation errors
CreateTriggersOnGhosterror was logged but not returned during atomic cut-over, causing silent trigger loss.returnto match the two-step cut-over behavior.Bug 3:
validateGhostTriggersDontExistchecks wrong table scopeevent_object_table, missing conflicts on other tables in the same schema.Bug 4: SQL injection in
GetTriggers()fmt.Sprintfstring interpolation instead of parameterized query.?placeholders, matching the safe pattern ininspect.go.Test plan
go/base/context_test.go— tests now simulate the real call pattern (transform first, then validate) with boundary tests at 64/65 charslocaltests/trigger-long-name-validation— verifies 60-char trigger names are not falsely rejected (Bug 1)localtests/trigger-ghost-name-conflict— verifies ghost name conflicts on other tables are detected (Bug 3)go build ./...— cleango test ./go/base/ ./go/mysql/ ./go/sql/— all passThank you, Yakir Gibraltar