Merge pull request #3042 from dolthub/angela/triggers

Apply multiple triggers to `UPDATE JOIN`

Author: angelamayxie

enginetest/queries/script_queries.go

@@ -517,15 +517,15 @@ SET entity_test.value = joined.value;`,
 				Expected: []sql.Row{{1, "john", "doe", 0, 42}},
 			},
 			{
-				Query:    "UPDATE test_users JOIN (SELECT id, 1 FROM test_users) AS tu SET test_users.favorite_number = 420;",
+				Query:    "UPDATE test_users JOIN (SELECT 1 FROM test_users) AS tu SET test_users.favorite_number = 420;",
 				Expected: []sql.Row{{NewUpdateResult(1, 1)}},
 			},
 			{
 				Query:    "select * from test_users;",
 				Expected: []sql.Row{{1, "john", "doe", 0, 420}},
 			},
 			{
-				Query:    "UPDATE test_users JOIN (SELECT id, 1 FROM test_users) AS tu SET test_users.deleted = 1;",
+				Query:    "UPDATE test_users JOIN (SELECT 1 FROM test_users) AS tu SET test_users.deleted = 1;",
 				Expected: []sql.Row{{NewUpdateResult(1, 1)}},
 			},
 			{

enginetest/queries/update_queries.go

@@ -534,7 +534,7 @@ var UpdateScriptTests = []ScriptTest{
 		Name:    "UPDATE join – multiple tables, with trigger",
 		SetUpScript: []string{
 			"CREATE TABLE a (id INT PRIMARY KEY, x INT);",
-			"CREATE TABLE b (id INT PRIMARY KEY, y INT);",
+			"CREATE TABLE b (pk INT PRIMARY KEY, y INT);",
 			"CREATE TABLE logbook (entry TEXT);",
 			`CREATE TRIGGER trig_a AFTER UPDATE ON a FOR EACH ROW
 		 BEGIN
@@ -550,13 +550,10 @@ var UpdateScriptTests = []ScriptTest{
 		Assertions: []ScriptTestAssertion{
 			{
 				Query: `UPDATE a
-					JOIN b ON a.id = 5 AND b.id = 6
+					JOIN b ON a.id = 5 AND b.pk = 6
 					SET a.x = 101, b.y = 201;`,
 			},
 			{
-				// TODO: UPDATE ... JOIN does not properly apply triggers when multiple tables are being updated,
-				//       and will currently only apply triggers from one of the tables.
-				Skip:  true,
 				Query: "SELECT * FROM logbook ORDER BY entry;",
 				Expected: []sql.Row{
 					{"a updated"},
@@ -565,6 +562,82 @@ var UpdateScriptTests = []ScriptTest{
 			},
 		},
 	},
+	{
+		Dialect: "mysql",
+		Name:    "UPDATE join – multiple tables with triggers that reference row values",
+		SetUpScript: []string{
+			"create table customers (id int primary key, name text, tier text)",
+			"create table orders (order_id int primary key, customer_id int, status text)",
+			"create table trigger_log (msg text)",
+			`CREATE TRIGGER after_orders_update after update on orders for each row
+				begin
+					insert into trigger_log (msg) values(
+						concat('Order ', OLD.order_id, ' status changed from ', OLD.status, ' to ', NEW.status));
+				end;`,
+			`Create trigger after_customers_update after update on customers for each row
+					begin
+						insert into trigger_log (msg) values(
+							concat('Customer ', OLD.id, ' tier changed from ', OLD.tier, ' to ', NEW.tier));
+					end;`,
+			"insert into customers values(1, 'Alice', 'silver'), (2, 'Bob', 'gold');",
+			"insert into orders values (101, 1, 'pending'), (102, 2, 'pending');",
+			"update customers c join orders o on c.id = o.customer_id " +
+				"set c.tier = 'platinum', o.status = 'shipped' where o.status = 'pending'",
+		},
+		Assertions: []ScriptTestAssertion{
+			{
+				Query: "SELECT * FROM trigger_log order by msg;",
+				Expected: []sql.Row{
+					{"Customer 1 tier changed from silver to platinum"},
+					{"Customer 2 tier changed from gold to platinum"},
+					{"Order 101 status changed from pending to shipped"},
+					{"Order 102 status changed from pending to shipped"},
+				},
+			},
+		},
+	},
+	{
+		Dialect: "mysql",
+		Name:    "UPDATE join – multiple tables with same column names with triggers",
+		SetUpScript: []string{
+			"create table customers (id int primary key, name text, tier text)",
+			"create table orders (id int primary key, customer_id int, status text)",
+			"create table trigger_log (msg text)",
+			`CREATE TRIGGER after_orders_update after update on orders for each row
+				begin
+					insert into trigger_log (msg) values(
+						concat('Order ', OLD.id, ' status changed from ', OLD.status, ' to ', NEW.status));
+				end;`,
+			`Create trigger after_customers_update after update on customers for each row
+					begin
+						insert into trigger_log (msg) values(
+							concat('Customer ', OLD.id, ' tier changed from ', OLD.tier, ' to ', NEW.tier));
+					end;`,
+			"insert into customers values(1, 'Alice', 'silver'), (2, 'Bob', 'gold');",
+			"insert into orders values (101, 1, 'pending'), (102, 2, 'pending');",
+		},
+		Assertions: []ScriptTestAssertion{
+			{
+				Query: "update customers c join orders o on c.id = o.customer_id " +
+					"set c.tier = 'platinum', o.status = 'shipped' where o.status = 'pending'",
+				// TODO: we shouldn't expect an error once we're able to handle conflicting column names
+				// https://github.com/dolthub/dolt/issues/9403
+				ExpectedErrStr: "Unable to apply triggers when joined tables have columns with the same name",
+			},
+			{
+				// TODO: unskip once we're able to handle conflicting column names
+				// https://github.com/dolthub/dolt/issues/9403
+				Skip:  true,
+				Query: "SELECT * FROM trigger_log order by msg;",
+				Expected: []sql.Row{
+					{"Customer 1 tier changed from silver to platinum"},
+					{"Customer 2 tier changed from gold to platinum"},
+					{"Order 101 status changed from pending to shipped"},
+					{"Order 102 status changed from pending to shipped"},
+				},
+			},
+		},
+	},
 }
 
 var SpatialUpdateTests = []WriteQueryTest{

sql/analyzer/tables.go

@@ -22,7 +22,7 @@ import (
 	"github.com/dolthub/go-mysql-server/sql/transform"
 )
 
-// Returns the underlying table name for the node given
+// Returns the underlying table name, unaliased, for the node given
 func getTableName(node sql.Node) string {
 	var tableName string
 	transform.Inspect(node, func(node sql.Node) bool {
@@ -43,27 +43,6 @@ func getTableName(node sql.Node) string {
 	return tableName
 }
 
-// Returns the underlying table name for the node given, ignoring table aliases
-func getUnaliasedTableName(node sql.Node) string {
-	var tableName string
-	transform.Inspect(node, func(node sql.Node) bool {
-		switch node := node.(type) {
-		case *plan.ResolvedTable:
-			tableName = node.Name()
-			return false
-		case *plan.UnresolvedTable:
-			tableName = node.Name()
-			return false
-		case *plan.IndexedTableAccess:
-			tableName = node.Name()
-			return false
-		}
-		return true
-	})
-
-	return tableName
-}
-
 // Finds first table node that is a descendant of the node given
 func getTable(node sql.Node) sql.Table {
 	var table sql.Table

sql/analyzer/triggers.go

@@ -15,6 +15,7 @@
 package analyzer
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 
@@ -158,7 +159,15 @@ func applyTriggers(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scope,
 				db = n.Database().Name()
 			}
 		case *plan.Update:
-			affectedTables = append(affectedTables, getTableName(n))
+			if n.IsJoin {
+				uj := n.Child.(*plan.UpdateJoin)
+				updateTargets := uj.UpdateTargets
+				for _, updateTarget := range updateTargets {
+					affectedTables = append(affectedTables, getTableName(updateTarget))
+				}
+			} else {
+				affectedTables = append(affectedTables, getTableName(n))
+			}
 			triggerEvent = plan.UpdateTrigger
 			if n.Database() != "" {
 				db = n.Database()
@@ -355,18 +364,18 @@ func applyTrigger(ctx *sql.Context, a *Analyzer, originalNode, n sql.Node, scope
 		}
 	}
 
-	return transform.NodeWithCtx(n, nil, func(c transform.Context) (sql.Node, transform.TreeIdentity, error) {
+	canApplyTriggerExecutor := func(c transform.Context) bool {
 		// Don't double-apply trigger executors to the bodies of triggers. To avoid this, don't apply the trigger if the
-		// parent is a trigger body.
-		// TODO: this won't work for BEGIN END blocks, stored procedures, etc. For those, we need to examine all ancestors,
-		//  not just the immediate parent. Alternately, we could do something like not walk all children of some node types
-		//  (probably better).
+		// parent is a trigger body. Having this as a selector function will also prevent walking the child nodes in the
+		// trigger execution logic.
 		if _, ok := c.Parent.(*plan.TriggerExecutor); ok {
 			if c.ChildNum == 1 { // Right child is the trigger execution logic
-				return c.Node, transform.SameTree, nil
+				return false
 			}
 		}
-
+		return true
+	}
+	return transform.NodeWithCtx(n, canApplyTriggerExecutor, func(c transform.Context) (sql.Node, transform.TreeIdentity, error) {
 		switch n := c.Node.(type) {
 		case *plan.InsertInto:
 			qFlags.Set(sql.QFlagTrigger)
@@ -404,9 +413,9 @@ func applyTrigger(ctx *sql.Context, a *Analyzer, originalNode, n sql.Node, scope
 			//       like we need something like a MultipleTriggerExecutor node
 			//       that could execute multiple triggers on the same row from its
 			//       wrapped iterator. There is also an issue with running triggers
-			//       because their field indexes assume the row they evalute will
+			//       because their field indexes assume the row they evaluate will
 			//       only ever contain the columns from the single table the trigger
-			//       is based on, but this isn't true with UPDATE JOIN or DELETE JOIN.
+			//       is based on.
 			if n.HasExplicitTargets() {
 				return nil, transform.SameTree, fmt.Errorf("delete from with explicit target tables " +
 					"does not support triggers; retry with single table deletes")
@@ -472,6 +481,12 @@ func getTriggerLogic(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scop
 				),
 			)
 		} else {
+			// TODO: We should be able to handle duplicate column names by masking columns that aren't part of the
+			// triggered table https://github.com/dolthub/dolt/issues/9403
+			err = validateNoConflictingColumnNames(updateSrc.Child.Schema())
+			if err != nil {
+				return nil, err
+			}
 			// The scopeNode for an UpdateJoin should contain every node in the updateSource as new and old.
 			scopeNode = plan.NewProject(
 				[]sql.Expression{expression.NewStar()},
@@ -497,6 +512,19 @@ func getTriggerLogic(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scop
 	return triggerLogic, err
 }
 
+// validateNoConflictingColumnNames checks the columns of a joined table to make sure there are no conflicting column
+// names
+func validateNoConflictingColumnNames(sch sql.Schema) error {
+	columnNames := make(map[string]struct{})
+	for _, col := range sch {
+		if _, ok := columnNames[col.Name]; ok {
+			return errors.New("Unable to apply triggers when joined tables have columns with the same name")
+		}
+		columnNames[col.Name] = struct{}{}
+	}
+	return nil
+}
+
 // validateNoCircularUpdates returns an error if the trigger logic attempts to update the table that invoked it (or any
 // table being updated in an outer scope of this analysis)
 func validateNoCircularUpdates(trigger *plan.CreateTrigger, n sql.Node, scope *plan.Scope) error {
@@ -505,8 +533,8 @@ func validateNoCircularUpdates(trigger *plan.CreateTrigger, n sql.Node, scope *p
 		switch node := node.(type) {
 		case *plan.Update, *plan.InsertInto, *plan.DeleteFrom:
 			for _, n := range append([]sql.Node{n}, scope.MemoNodes()...) {
-				invokingTableName := getUnaliasedTableName(n)
-				updatedTable := getUnaliasedTableName(node)
+				invokingTableName := getTableName(n)
+				updatedTable := getTableName(node)
 				// TODO: need to compare DB as well
 				if updatedTable == invokingTableName {
 					circularRef = sql.ErrTriggerTableInUse.New(updatedTable)