Merge pull request #2345 from dolthub/nicktobey/mysql-errors

Return correct MySQL error code when inserting into nonexistent columns.

Author: nicktobey

enginetest/queries/load_queries.go

@@ -19,7 +19,6 @@ import (
 	"time"
 
 	"github.com/dolthub/go-mysql-server/sql"
-	"github.com/dolthub/go-mysql-server/sql/plan"
 )
 
 var LoadDataScripts = []ScriptTest{
@@ -279,15 +278,15 @@ var LoadDataErrorScripts = []ScriptTest{
 		Assertions: []ScriptTestAssertion{
 			{
 				Query:       "LOAD DATA INFILE './testdata/test1.txt' INTO TABLE loadtable FIELDS ENCLOSED BY '\"' (fake_col, pk, i)",
-				ExpectedErr: plan.ErrInsertIntoNonexistentColumn,
+				ExpectedErr: sql.ErrUnknownColumn,
 			},
 			{
 				Query:       "LOAD DATA INFILE './testdata/test1.txt' INTO TABLE loadtable FIELDS ENCLOSED BY '\"' (pk, fake_col, i)",
-				ExpectedErr: plan.ErrInsertIntoNonexistentColumn,
+				ExpectedErr: sql.ErrUnknownColumn,
 			},
 			{
 				Query:       "LOAD DATA INFILE './testdata/test1.txt' INTO TABLE loadtable FIELDS ENCLOSED BY '\"' (pk, i, fake_col)",
-				ExpectedErr: plan.ErrInsertIntoNonexistentColumn,
+				ExpectedErr: sql.ErrUnknownColumn,
 			},
 		},
 	},

server/handler_test.go

@@ -212,6 +212,76 @@ func TestHandlerOutput(t *testing.T) {
 	})
 }
 
+func TestHandlerErrors(t *testing.T) {
+	e, pro := setupMemDB(require.New(t))
+	dbFunc := pro.Database
+
+	dummyConn := newConn(1)
+	handler := &Handler{
+		e: e,
+		sm: NewSessionManager(
+			testSessionBuilder(pro),
+			sql.NoopTracer,
+			dbFunc,
+			sql.NewMemoryManager(nil),
+			sqle.NewProcessList(),
+			"foo",
+		),
+		readTimeout: time.Second,
+	}
+	handler.NewConnection(dummyConn)
+
+	type expectedValues struct {
+		callsToCallback  int
+		lenLastBatch     int
+		lastRowsAffected uint64
+	}
+
+	setupCommands := []string{"CREATE TABLE `test_table` ( `id` INT NOT NULL PRIMARY KEY, `v` INT );"}
+
+	tests := []struct {
+		name              string
+		query             string
+		expectedErrorCode int
+	}{
+		{
+			name:              "insert with nonexistent field name",
+			query:             "INSERT INTO `test_table` (`id`, `v_`) VALUES (1, 2)",
+			expectedErrorCode: mysql.ERBadFieldError,
+		},
+		{
+			name:              "insert into nonexistent table",
+			query:             "INSERT INTO `test`.`no_such_table` (`id`, `v`) VALUES (1, 2)",
+			expectedErrorCode: mysql.ERNoSuchTable,
+		},
+		{
+			name:              "insert into same column twice",
+			query:             "INSERT INTO `test`.`test_table` (`id`, `id`, `v`) VALUES (1, 2, 3)",
+			expectedErrorCode: mysql.ERFieldSpecifiedTwice,
+		},
+	}
+
+	handler.ComInitDB(dummyConn, "test")
+	for _, setupCommand := range setupCommands {
+		err := handler.ComQuery(dummyConn, setupCommand, func(res *sqltypes.Result, more bool) error {
+			return nil
+		})
+		require.NoError(t, err)
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			err := handler.ComQuery(dummyConn, test.query, func(res *sqltypes.Result, more bool) error {
+				return nil
+			})
+			require.NotNil(t, err)
+			sqlErr, isSqlError := err.(*mysql.SQLError)
+			require.True(t, isSqlError)
+			require.Equal(t, test.expectedErrorCode, sqlErr.Number())
+		})
+	}
+}
+
 // TestHandlerComReset asserts that the Handler.ComResetConnection method correctly clears all session
 // state (e.g. table locks, prepared statements, user variables, session variables), and keeps the current
 // database selected.

sql/errors.go

@@ -883,6 +883,8 @@ var (
 	ErrInsertIntoMismatchValueCount = errors.NewKind("number of values does not match number of columns provided")
 
 	ErrInvalidTypeForLimit = errors.NewKind("invalid limit. expected %T, found %T")
+
+	ErrColumnSpecifiedTwice = errors.NewKind("column '%v' specified twice")
 )
 
 // CastSQLError returns a *mysql.SQLError with the error code and in some cases, also a SQL state, populated for the
@@ -949,6 +951,10 @@ func CastSQLError(err error) *mysql.SQLError {
 		code = 1553 // TODO: Needs to be added to vitess
 	case ErrInvalidValue.Is(err):
 		code = mysql.ERTruncatedWrongValueForField
+	case ErrUnknownColumn.Is(err):
+		code = mysql.ERBadFieldError
+	case ErrColumnSpecifiedTwice.Is(err):
+		code = mysql.ERFieldSpecifiedTwice
 	case ErrLockDeadlock.Is(err):
 		// ER_LOCK_DEADLOCK signals that the transaction was rolled back
 		// due to a deadlock between concurrent transactions.

sql/plan/insert.go

@@ -29,8 +29,6 @@ var ErrReplaceIntoNotSupported = errors.NewKind("table doesn't support REPLACE I
 var ErrOnDuplicateKeyUpdateNotSupported = errors.NewKind("table doesn't support ON DUPLICATE KEY UPDATE")
 var ErrAutoIncrementNotSupported = errors.NewKind("table doesn't support AUTO_INCREMENT")
 var ErrInsertIntoUnsupportedValues = errors.NewKind("%T is unsupported for inserts")
-var ErrInsertIntoDuplicateColumn = errors.NewKind("duplicate column name %v")
-var ErrInsertIntoNonexistentColumn = errors.NewKind("invalid column name %v")
 var ErrInsertIntoIncompatibleTypes = errors.NewKind("cannot convert type %s to %s")
 
 // cc: https://dev.mysql.com/doc/refman/8.0/en/sql-mode.html#sql-mode-strict

sql/planbuilder/dml.go

@@ -81,7 +81,7 @@ func (b *Builder) buildInsert(inScope *scope, i *ast.Insert) (outScope *scope) {
 	if rt != nil {
 		sch = b.resolveSchemaDefaults(destScope, rt.Schema())
 	}
-	srcScope := b.insertRowsToNode(inScope, i.Rows, columns, sch)
+	srcScope := b.insertRowsToNode(inScope, i.Rows, columns, tableName, sch)
 
 	// TODO: on duplicate expressions need to reference both VALUES and
 	//  derived columns equally in ON DUPLICATE UPDATE expressions.
@@ -120,20 +120,20 @@ func (b *Builder) buildInsert(inScope *scope, i *ast.Insert) (outScope *scope) {
 	return
 }
 
-func (b *Builder) insertRowsToNode(inScope *scope, ir ast.InsertRows, columnNames []string, destSchema sql.Schema) (outScope *scope) {
+func (b *Builder) insertRowsToNode(inScope *scope, ir ast.InsertRows, columnNames []string, tableName string, destSchema sql.Schema) (outScope *scope) {
 	switch v := ir.(type) {
 	case ast.SelectStatement:
 		return b.buildSelectStmt(inScope, v)
 	case ast.Values:
-		outScope = b.buildInsertValues(inScope, v, columnNames, destSchema)
+		outScope = b.buildInsertValues(inScope, v, columnNames, tableName, destSchema)
 	default:
 		err := sql.ErrUnsupportedSyntax.New(ast.String(ir))
 		b.handleErr(err)
 	}
 	return
 }
 
-func (b *Builder) buildInsertValues(inScope *scope, v ast.Values, columnNames []string, destSchema sql.Schema) (outScope *scope) {
+func (b *Builder) buildInsertValues(inScope *scope, v ast.Values, columnNames []string, tableName string, destSchema sql.Schema) (outScope *scope) {
 	columnDefaultValues := make([]*sql.ColumnDefaultValue, len(columnNames))
 
 	for i, columnName := range columnNames {
@@ -142,7 +142,7 @@ func (b *Builder) buildInsertValues(inScope *scope, v ast.Values, columnNames []
 			if !b.TriggerCtx().Call && len(b.TriggerCtx().UnresolvedTables) > 0 {
 				continue
 			}
-			err := plan.ErrInsertIntoNonexistentColumn.New(columnName)
+			err := sql.ErrUnknownColumn.New(columnName, tableName)
 			b.handleErr(err)
 		}
 

sql/planbuilder/dml_validate.go

@@ -103,15 +103,15 @@ func validateColumns(tableName string, columnNames []string, dstSchema sql.Schem
 	for i, columnName := range columnNames {
 		dstCol, exists := dstColNames[columnName]
 		if !exists {
-			return plan.ErrInsertIntoNonexistentColumn.New(columnName)
+			return sql.ErrUnknownColumn.New(columnName, tableName)
 		}
 		if dstCol.Generated != nil && !validGeneratedColumnValue(i, source) {
 			return sql.ErrGeneratedColumnValue.New(dstCol.Name, tableName)
 		}
 		if _, exists := usedNames[columnName]; !exists {
 			usedNames[columnName] = struct{}{}
 		} else {
-			return plan.ErrInsertIntoDuplicateColumn.New(columnName)
+			return sql.ErrColumnSpecifiedTwice.New(columnName)
 		}
 	}
 	return nil