Repository navigation
Strict mode not actually strict #376
Description
The strict parameter is not using the MySQL strict mode, just returning errors when mysql throws a warning. This is extremely dangerous behavior that can lead to data corruption.
MySQL has a strict mode that should be used for this: https://dev.mysql.com/doc/refman/5.6/en/sql-mode.html#sql-mode-strict
For example:
package main
import (
"database/sql"
"fmt"
"os"
"time"
_ "github.com/go-sql-driver/mysql"
)
/*
CREATE DATABASE test;
use test;
CREATE TABLE test (
id BIGINT NOT NULL PRIMARY KEY,
value VARCHAR(4)
);
*/
func main() {
db, err := sql.Open("mysql", "root:root@tcp(localhost:3306)/test?strict=true")
if err != nil {
fmt.Printf("Error: %v\n", err)
os.Exit(1)
}
// show some stuff to confirm we aren't using any STRICT modes
row := db.QueryRow("show variables like 'sql_mode'")
var varname string
var sql_mode string
err = row.Scan(&varname, &sql_mode)
if err != nil {
fmt.Printf("Error: %v\n", err)
os.Exit(1)
}
fmt.Printf("sql_mode=%q\n", sql_mode)
row = db.QueryRow("select @@GLOBAL.sql_mode")
err = row.Scan(&sql_mode)
if err != nil {
fmt.Printf("Error: %v\n", err)
os.Exit(1)
}
fmt.Printf("@@GLOBAL.sql_mode=%q\n", sql_mode)
// now perform the test
id := time.Now().UnixNano()
value := "1234567890"
_, err = db.Exec("insert into test (id, value) values (?,?)", id, value)
fmt.Printf("Insert error: %v\n", err)
row = db.QueryRow("select value from test where id=?", id)
err = row.Scan(&value)
fmt.Printf("Select error: %v\n", err)
fmt.Printf("Selected value: %q\n", value)
}Which results in:
sql_mode="NO_ENGINE_SUBSTITUTION"
@@GLOBAL.sql_mode="NO_ENGINE_SUBSTITUTION"
Insert error: Warning 1265: Data truncated for column 'value' at row 1
Select error: <nil>
Selected value: "1234"
As you can see, the INSERT threw an error, but the insert was actually performed. I now have this row in my database that I'm not expecting to be there.
If this had been an UPDATE, I could have overwritten valid data with now invalid, truncated, data.
Activity
phemmer commentedon Oct 22, 2015
Looks like we can do the following for a workaround:
xaprb commentedon Oct 22, 2015
I would not say the functionality is wrong, but rather that the naming may need to be reconsidered. MySQL's "strict mode" setting is not strict enough to prevent a variety of silent and nasty bugs that will lead to problems in production, and has the same issues you mention about the warnings-to-errors flag in this driver. I've implemented functionality to check for warnings and promote them to errors in a variety of jobs I've had; usually only enabling in development, of course :)
phemmer commentedon Oct 22, 2015
That's the first I've ever heard of that. Do you have examples or references?
And if that is the case, then why not both? Ifstrictis set, turn warnings into errors and setsql_mode=STRICT_ALL_TABLES.Edit: nix that, bad idea. This behavior really needs to just go away and use the DB's strict mode. With this behavior, it's difficult to differentiate between a failed operation, and partial success. It's also very unusual in go to return an error and a valid value (for example,
result, err := db.Exec()whereerr != nil && result != nil).It would be better to be able to type assert a
sql.Resultinto a nativemysqltype which has a.Warnings() []MySQLWarningsmethod. Especially since currently, the only way to get warnings is through this quasi-strict mode.timretout commentedon Nov 18, 2015
If you're using a transactional table backend like InnoDB, it's much improved, because it rolls back the transaction when it hits a problem. There's also "TRADITIONAL" sql_mode, which is described as "turn warnings into errors", and is STRICT_ALL_TABLES plus some other stuff: https://dev.mysql.com/doc/refman/5.7/en/sql-mode.html
I'm sure there might be yet more subtle problems, but it's a big step forwards. So I agree with @phemmer that the "strict" flag in this package is confusing if you know about MySQL's strict sql_mode, and it's not as strict as it could be.
methane commentedon Nov 18, 2015
This option is stricter than "TRADITIONAL". (known as "kamipo traditional" in Japanese engineers).
julienschmidt commentedon Jan 13, 2016
Isn't it rather a documentation issue then?
The documentation should clearly state what the driver's "strict" mode does and that
sql_modeshould be used.README: Clarify strict mode
README: Clarify strict mode (#500)