Strict mode not actually strict

[phemmer]

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.

[phemmer]

Looks like we can do the following for a workaround:

sql.open("mysql", "...?sql_mode="+url.QueryEscape("(SELECT CONCAT(@@sql_mode,',STRICT_ALL_TABLES'))"))

[xaprb]

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]

MySQL's "strict mode" setting is not strict enough to prevent a variety of silent and nasty bugs

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? If strict is set, turn warnings into errors and set sql_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() where err != nil && result != nil).

It would be better to be able to type assert a sql.Result into a native mysql type which has a .Warnings() []MySQLWarnings method. Especially since currently, the only way to get warnings is through this quasi-strict mode.

[timretout]

MySQL's "strict mode" setting is not strict enough to prevent a variety of silent and nasty bugs

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]

This option is stricter than "TRADITIONAL". (known as "kamipo traditional" in Japanese engineers).

sql_mode = TRADITIONAL,NO_AUTO_VALUE_ON_ZERO,ONLY_FULL_GROUP_BY

[julienschmidt]

Isn't it rather a documentation issue then?
The documentation should clearly state what the driver's "strict" mode does and that sql_mode should be used.