Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly handle user-defined exceptions. #1322

Closed
thopos opened this issue Apr 6, 2022 · 1 comment
Closed

Properly handle user-defined exceptions. #1322

thopos opened this issue Apr 6, 2022 · 1 comment

Comments

@thopos
Copy link
Contributor

thopos commented Apr 6, 2022

Issue description

The library should include the SQLState in MySQLError. User-defined exceptions (SQLState '45000') can use any error number and will likely collide with build in error numbers. Without the SQLState it is impossible to distinguish client / server errors from user-defined errors.

This is related to #207. There was no argument brought forward in support for including SQLState though, so i will try to make it here:
We use stored routines a lot and some of them SIGNAL failure with user-defined exceptions. This library would be unusable for us
without the possibility to distinguish the SQLState.

Example

DELIMITER $$

CREATE PROCEDURE `CloseConnection`()
BEGIN
    SIGNAL SQLSTATE '45000' 
        SET MYSQL_ERRNO = 1792, MESSAGE_TEXT ='no connection for you';
END$$

DELIMITER ;

While having a look at handleErrorPacket I noticed that calling the procedure above would close the connection when config option RejectReadOnly is set. This is addressed in PR #1320.

package main

import (
        "database/sql"
        "log"

        "github.com/go-sql-driver/mysql"
        driver "github.com/go-sql-driver/mysql"
)

func main() {
        db, err := newDB()
        if err != nil {
                log.Fatal(err)
        }

        _, err = db.Query(`
                CREATE PROCEDURE CloseConnection()
                BEGIN
                        SIGNAL SQLSTATE '45000' 
                                SET MYSQL_ERRNO = 1792, MESSAGE_TEXT ='no connection for you';
                END`)
        if err != nil {
                log.Fatal(err)
        }

        _, err = db.Query("CALL CloseConnection()")
        myErr, ok := err.(*mysql.MySQLError)
        if !ok {
                log.Fatalf("not a MySQLError: %s", err)
        } else {
                if myErr.Number != 1792 || myErr.Message != "no connection for you" {
                        log.Fatalf("unexpected error: %s", err)
                }
        }
}

func newDB() (*sql.DB, error) {
        driverCfg := driver.NewConfig()

        driverCfg.User = "root"
        driverCfg.Passwd = "root"
        driverCfg.Net = "tcp"
        driverCfg.Addr = "127.0.0.1:3306"
        driverCfg.DBName = "test"

        driverCfg.RejectReadOnly = true

        return sql.Open("mysql", driverCfg.FormatDSN())
}

Test with:

docker run --rm -p 127.0.0.1:3306:3306 -e MYSQL_ROOT_PASSWORD=root -e MYSQL_DATABASE=test mysql:5.7 # or 8.0

logs:

# server
2022-04-06T11:07:36.697562Z 2 [Note] Aborted connection 2 to db: 'test' user: 'root' host: '172.17.0.1' (Got an error reading communication packets)

# client
2022/04/06 13:07:36 not a MySQLError: driver: bad connection

MySQLError

PR #1321, depending on the PR above, adds SQLState to MySQLError. I would argue that MySQLError.Is is wrong in not including the SQLState in the comparison as stated above. Changeing MySQLError.Is might break more client code than necessary. Therefore a new method IsWithSQLState was added, which is not as convenient since it does not implement errors.Is but might still be useful.

Configuration

Driver version (or git SHA): eff3908

Go version: go1.18 linux/amd64

Server version: MySQL 5.7, 8.0

Server OS: Docker (docker.io/library/mysql)

@thopos
Copy link
Contributor Author

thopos commented Apr 13, 2022

Resolution:

  • User-defined exceptions are not handled differently when RejectReadOnly is set since similar libraries don't do it either.
  • SQLState is exposed to the user in MySQLError.

@thopos thopos closed this as completed Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant