diff --git a/enginetest/engine_only_test.go b/enginetest/engine_only_test.go index 2400c76ea9..38602ffae7 100644 --- a/enginetest/engine_only_test.go +++ b/enginetest/engine_only_test.go @@ -647,7 +647,9 @@ func TestTriggerViewWarning(t *testing.T) { baseDb := db.(*memory.HistoryDatabase).BaseDatabase err = baseDb.CreateTrigger(nil, sql.TriggerDefinition{ - Name: "view_trig", + Name: "view_trig", + // TODO: this trigger statement still wouldn't parse correctly in MySQL if it was created on a table. Update to + // a trigger that would otherwise be valid. CreateStatement: "CREATE TRIGGER view_trig BEFORE INSERT ON myview FOR EACH ROW SET i=i+2", }) assert.NoError(t, err) @@ -657,7 +659,8 @@ func TestTriggerViewWarning(t *testing.T) { enginetest.CreateNewConnectionForServerEngine(ctx, e) enginetest.TestQueryWithContext(t, ctx, e, harness, "insert into mytable values (4, 'fourth row')", []sql.Row{{types.NewOkResult(1)}}, nil, nil, nil) - enginetest.TestQueryWithContext(t, ctx, e, harness, "SHOW WARNINGS", []sql.Row{{"Warning", 0, "trigger on view is not supported; 'DROP TRIGGER view_trig' to fix"}}, nil, nil, nil) + enginetest.TestQueryWithContext(t, ctx, e, harness, "SHOW WARNINGS", []sql.Row{{"Warning", 0, "Trigger on view is not supported. Please run 'DROP TRIGGER view_trig;'"}}, nil, nil, nil) + // TODO: this is not correct. MySQL allows inserting into views. https://github.com/dolthub/dolt/issues/10292 enginetest.AssertErrWithCtx(t, e, harness, ctx, "insert into myview values (5, 'fifth row')", nil, nil, "expected insert destination to be resolved or unresolved table") } diff --git a/enginetest/queries/function_queries.go b/enginetest/queries/function_queries.go index 1d1fddbf02..ea0d9fba0a 100644 --- a/enginetest/queries/function_queries.go +++ b/enginetest/queries/function_queries.go @@ -2325,6 +2325,7 @@ var FunctionQueryTests = []QueryTest{ Expected: []sql.Row{{0}}, }, { + // https://github.com/dolthub/dolt/issues/10087 Skip: true, Query: "select extract(day_microsecond from true)", Expected: []sql.Row{{1000000}}, diff --git a/enginetest/queries/trigger_queries.go b/enginetest/queries/trigger_queries.go index 41efecd5bd..495e4c7e69 100644 --- a/enginetest/queries/trigger_queries.go +++ b/enginetest/queries/trigger_queries.go @@ -3918,6 +3918,117 @@ end; }, }, }, + { + // https://github.com/dolthub/dolt/issues/10287 + Name: "only throw 'column not found' error when trigger is actually triggered", + SetUpScript: []string{ + "create table A(col0 int, col1 int)", + "create table B(col0 int, col1 int)", + "create table C(col0 int)", + "create trigger test_trigger after update on A for each row insert into C (col0) select col0 from B where B.col0 = new.col0", + "drop table B", + }, + Assertions: []ScriptTestAssertion{ + { + Query: "show triggers", + // assert trigger is still there + Expected: []sql.Row{ + { + "test_trigger", // Trigger + "UPDATE", // Event + "A", // Table + "insert into C (col0) select col0 from B where B.col0 = new.col0", // Statement + "AFTER", // Timing + time.Unix(0, 0).UTC(), // Created + "", // sql_mode + "", // Definer + sql.Collation_Default.CharacterSet().String(), // character_set_client + sql.Collation_Default.String(), // collation_connection + sql.Collation_Default.String(), // Database Collation + }, + }, + }, + { + Query: "create table B(a int, b int)", + Expected: []sql.Row{ + {types.NewOkResult(0)}, + }, + }, + { + Query: "show triggers", + // assert trigger is still there + Expected: []sql.Row{ + { + "test_trigger", // Trigger + "UPDATE", // Event + "A", // Table + "insert into C (col0) select col0 from B where B.col0 = new.col0", // Statement + "AFTER", // Timing + time.Unix(0, 0).UTC(), // Created + "", // sql_mode + "", // Definer + sql.Collation_Default.CharacterSet().String(), // character_set_client + sql.Collation_Default.String(), // collation_connection + sql.Collation_Default.String(), // Database Collation + }, + }, + }, + { + Query: "insert into C values (12)", + Expected: []sql.Row{ + {types.NewOkResult(1)}, + }, + }, + { + Query: "insert into A values (1, 2)", + Expected: []sql.Row{ + {types.NewOkResult(1)}, + }, + }, + { + Query: "update A set A.col0=2 where true", + ExpectedErr: sql.ErrTableColumnNotFound, + }, + { + Query: "select * from A", + Expected: []sql.Row{{1, 2}}, + }, + { + Query: "show create trigger test_trigger", + // assert trigger info is there + Expected: []sql.Row{ + { + "test_trigger", + "", + "create trigger test_trigger after update on A for each row insert into C (col0) select col0 from B where B.col0 = new.col0", + sql.Collation_Default.CharacterSet().String(), + sql.Collation_Default.String(), + sql.Collation_Default.String(), + time.Unix(0, 0).UTC(), + }, + }, + }, + { + Query: "drop trigger test_trigger", + Expected: []sql.Row{ + {types.NewOkResult(0)}, + }, + }, + { + Query: "show triggers", + // assert trigger has been dropped + Expected: []sql.Row{}, + }, + { + // https://github.com/dolthub/dolt/issues/10291 + Skip: true, + Query: "create trigger test_trigger after update on A for each row insert into C (col0) select col0 from B where B.col0 = new.col0", + Expected: []sql.Row{ + {types.NewOkResult(0)}, + }, + }, + }, + }, } var TriggerCreateInSubroutineTests = []ScriptTest{ diff --git a/sql/analyzer/load_triggers.go b/sql/analyzer/load_triggers.go index d95600d997..8a368bedd5 100644 --- a/sql/analyzer/load_triggers.go +++ b/sql/analyzer/load_triggers.go @@ -108,6 +108,7 @@ func loadTriggersFromDb(ctx *sql.Context, a *Analyzer, db sql.Database, ignorePa // TODO: should perhaps add the auth query handler to the analyzer? does this even use auth? builder := planbuilder.New(ctx, a.Catalog, nil) builder.SetParserOptions(sqlMode.ParserOptions()) + builder.TriggerCtx().LoadOnly = true parsedTrigger, _, _, _, err = builder.Parse(trigger.CreateStatement, nil, false) if err != nil { // We want to be able to drop invalid triggers, so ignore any parser errors and return the name of the trigger diff --git a/sql/analyzer/triggers.go b/sql/analyzer/triggers.go index 8f6a3f3025..9ee85e5bbf 100644 --- a/sql/analyzer/triggers.go +++ b/sql/analyzer/triggers.go @@ -212,42 +212,21 @@ func applyTriggers(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scope, b := planbuilder.New(ctx, a.Catalog, nil) b.DisableAuth() - prevActive := b.TriggerCtx().Active - b.TriggerCtx().Active = true - defer func() { - b.TriggerCtx().Active = prevActive - }() for _, trigger := range triggers { var parsedTrigger sql.Node sqlMode := sql.NewSqlModeFromString(trigger.SqlMode) b.SetParserOptions(sqlMode.ParserOptions()) + b.TriggerCtx().LoadOnly = true parsedTrigger, _, _, _, err = b.Parse(trigger.CreateStatement, nil, false) b.Reset() if err != nil { - return nil, transform.SameTree, err + continue } ct, ok := parsedTrigger.(*plan.CreateTrigger) if !ok { - return nil, transform.SameTree, sql.ErrTriggerCreateStatementInvalid.New(trigger.CreateStatement) - } - transform.Inspect(ct.Body, func(n sql.Node) bool { - call, isCall := n.(*plan.Call) - if !isCall { - return true - } - if call.Procedure == nil { - return true - } - if call.Procedure.ValidationError == nil { - return true - } - err = call.Procedure.ValidationError - return false - }) - if err != nil { - return nil, transform.SameTree, err + continue } var triggerTable string @@ -257,11 +236,11 @@ func applyTriggers(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scope, default: } if stringContains(affectedTables, triggerTable) && triggerEventsMatch(triggerEvent, ct.TriggerEvent) { - // first pass allows unresolved before we know whether trigger is relevant - // TODO store destination table name with trigger, so we don't have to do parse twice + // first pass does not parse the trigger body and is only so we know whether trigger is relevant b.TriggerCtx().Call = true + // TODO: We only need to parse the body here since the other info from the create trigger statement have + // already been parsed. parsedTrigger, _, _, _, err = b.Parse(trigger.CreateStatement, nil, false) - b.TriggerCtx().Call = false b.Reset() if err != nil { return nil, transform.SameTree, err @@ -272,6 +251,24 @@ func applyTriggers(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scope, return nil, transform.SameTree, sql.ErrTriggerCreateStatementInvalid.New(trigger.CreateStatement) } + transform.Inspect(ct.Body, func(n sql.Node) bool { + call, isCall := n.(*plan.Call) + if !isCall { + return true + } + if call.Procedure == nil { + return true + } + if call.Procedure.ValidationError == nil { + return true + } + err = call.Procedure.ValidationError + return false + }) + if err != nil { + return nil, transform.SameTree, err + } + if block, ok := ct.Body.(*plan.BeginEndBlock); ok { ct.Body = plan.NewTriggerBeginEndBlock(block) } diff --git a/sql/planbuilder/builder.go b/sql/planbuilder/builder.go index 5426c599bc..f3d6a2453b 100644 --- a/sql/planbuilder/builder.go +++ b/sql/planbuilder/builder.go @@ -99,6 +99,7 @@ type TriggerContext struct { UnresolvedTables []string Active bool Call bool + LoadOnly bool } // ProcContext allows nested CALLs to use the same database for resolving diff --git a/sql/planbuilder/create_ddl.go b/sql/planbuilder/create_ddl.go index 2fd735e96e..7e4e13ad9b 100644 --- a/sql/planbuilder/create_ddl.go +++ b/sql/planbuilder/create_ddl.go @@ -53,72 +53,76 @@ func (b *Builder) buildCreateTrigger(inScope *scope, subQuery string, fullQuery } } - // resolve table -> create initial scope - prevTriggerCtxActive := b.TriggerCtx().Active - b.TriggerCtx().Active = true - defer func() { - b.TriggerCtx().Active = prevTriggerCtxActive - }() + db, ok := b.resolveDbForTable(c.Table) + if !ok { + b.handleErr(sql.ErrDatabaseSchemaNotFound.New(c.Table.SchemaQualifier.String())) + } - tableName := strings.ToLower(c.Table.Name.String()) tableScope, ok := b.buildResolvedTableForTablename(inScope, c.Table, nil) if !ok { - b.handleErr(sql.ErrTableNotFound.New(tableName)) + b.handleErr(sql.ErrTableNotFound.New(c.Table.Name.String())) } if _, ok := tableScope.node.(*plan.UnresolvedTable); ok { // unknown table in trigger body is OK, but the target table must exist - b.handleErr(sql.ErrTableNotFound.New(tableName)) - } - - // todo scope with new and old columns provided - // insert/update have "new" - // update/delete have "old" - newScope := tableScope.replace() - oldScope := tableScope.replace() - for _, col := range tableScope.cols { - switch c.TriggerSpec.Event { - case ast.InsertStr: - newScope.newColumn(col) - case ast.UpdateStr: - newScope.newColumn(col) - oldScope.newColumn(col) - case ast.DeleteStr: - oldScope.newColumn(col) - } + b.handleErr(sql.ErrTableNotFound.New(c.Table.Name.String())) } - newScope.setTableAlias("new") - oldScope.setTableAlias("old") - triggerScope := tableScope.replace() - - triggerScope.addColumns(newScope.cols) - triggerScope.addColumns(oldScope.cols) - - triggerScope.addExpressions(newScope.exprs) - triggerScope.addExpressions(oldScope.exprs) - bodyStr := strings.TrimSpace(fullQuery[c.SubStatementPositionStart:c.SubStatementPositionEnd]) - bodyScope := b.buildSubquery(triggerScope, c.TriggerSpec.Body, bodyStr, fullQuery) - definer := getCurrentUserForDefiner(b.ctx, c.TriggerSpec.Definer) - - db, ok := b.resolveDbForTable(c.Table) - if !ok { - b.handleErr(sql.ErrDatabaseSchemaNotFound.New(c.Table.SchemaQualifier.String())) - } + triggerCtx := b.TriggerCtx() if _, ok := tableScope.node.(*plan.ResolvedTable); !ok { - if prevTriggerCtxActive { - // previous ctx set means this is an INSERT or SHOW - // old version of Dolt permitted a bad trigger on VIEW - // warn and noop - b.ctx.Warn(0, "trigger on view is not supported; 'DROP TRIGGER %s' to fix", c.TriggerSpec.TrigName.Name.String()) - bodyScope.node = plan.NewResolvedDualTable() + // Old versions of GMS/Dolt permitted creating an invalid trigger on VIEW + if triggerCtx.LoadOnly { + // LoadOnly means a CreateTrigger statement was parsed while loading triggers for SHOW, INSERT, UPDATE, or + // DELETE. Warn and no-op. Since tableScope.node here is not a ResolvedTable, it won't be matched to any + // table during applyTriggers. + b.ctx.Warn(0, "Trigger on view is not supported. Please run 'DROP TRIGGER %s;'", c.TriggerSpec.TrigName.Name.String()) } else { - // top-level call is DDL - err := sql.ErrExpectedTableFoundView.New(tableName) + // Top-level call is DDL, and we should not allow a trigger on a VIEW to be created here. Or somehow, a + // trigger on a VIEW has been matched during applyTriggers, and we should not run that trigger. + err := sql.ErrExpectedTableFoundView.New(c.Table.Name.String()) b.handleErr(err) } } + var bodyNode sql.Node + bodyStr := strings.TrimSpace(fullQuery[c.SubStatementPositionStart:c.SubStatementPositionEnd]) + if !triggerCtx.LoadOnly { + prevTriggerCtxActive := triggerCtx.Active + triggerCtx.Active = true + defer func() { + triggerCtx.Active = prevTriggerCtxActive + }() + + // todo scope with new and old columns provided + // insert/update have "new" + // update/delete have "old" + newScope := tableScope.replace() + oldScope := tableScope.replace() + for _, col := range tableScope.cols { + switch c.TriggerSpec.Event { + case ast.InsertStr: + newScope.newColumn(col) + case ast.UpdateStr: + newScope.newColumn(col) + oldScope.newColumn(col) + case ast.DeleteStr: + oldScope.newColumn(col) + } + } + newScope.setTableAlias("new") + oldScope.setTableAlias("old") + triggerScope := tableScope.replace() + + triggerScope.addColumns(newScope.cols) + triggerScope.addColumns(oldScope.cols) + + triggerScope.addExpressions(newScope.exprs) + triggerScope.addExpressions(oldScope.exprs) + + bodyScope := b.buildSubquery(triggerScope, c.TriggerSpec.Body, bodyStr, fullQuery) + bodyNode = bodyScope.node + } + outScope.node = plan.NewCreateTrigger( db, c.TriggerSpec.TrigName.Name.String(), @@ -126,11 +130,11 @@ func (b *Builder) buildCreateTrigger(inScope *scope, subQuery string, fullQuery c.TriggerSpec.Event, triggerOrder, tableScope.node, - bodyScope.node, + bodyNode, subQuery, bodyStr, b.ctx.QueryTime(), - definer, + getCurrentUserForDefiner(b.ctx, c.TriggerSpec.Definer), ) return outScope }