Add SQLState to MySQLError

[thopos]

Description

Allow library users to distinguish user-defined from client / server errors.

Checklist

• Code compiles correctly
• Created tests which fail without the change (if possible)
• All tests passing
• Extended the README / documentation, if necessary
• Added myself / the copyright holder to the AUTHORS file

[shogo82148]

it needs some little memory allocations, but not so frequently.
#1322 is acceptable for me.

@jmhodges @arnehormann @methane How do you think about it?

[shogo82148]

👍🏼

[methane]

Please remove IsWithSQLState.

[thopos]

Thank you @shogo82148 and @methane for your patience and thorough review.

[iseki0]

Why you add it as a field instead of a method?
So sadly, all user of the driver has to hard-dependent it.

FYI:
https://github.com/jackc/pgx/blob/a95cfbb433143477082823c8e93d75b0eafc71da/pgconn/errors.go#L57-L60

[arnehormann]

@iseki0 thank you for that input. but even if you are right - first, we cannot change it now. second, that would have been a wonderful code review comment. why did you sadly have to add it more than a year after it was merged and not when it was still open?

Honestly, this is passive aggressive and not constructive and it contributes to maintainer burnout. why?

something like: "To bad it's too late now, but for similar future consideration: making it a method instead of a function would have helped for situation xyz." which still gets the point across and helps. but without one-upping.

[iseki0]

@iseki0 thank you for that input. but even if you are right - first, we cannot change it now. second, that would have been a wonderful code review comment. why did you sadly have to add it more than a year after it was merged and not when it was still open?

Honestly, this is passive aggressive and not constructive and it contributes to maintainer burnout. why?

something like: "To bad it's too late now, but for similar future consideration: making it a method instead of a function would have helped for situation xyz." which still gets the point across and helps. but without one-upping.

@arnehormann
Sorry for my irrational and negative comments.
I just curious why it's a field. And, I think we have a chance to improve the compatibility:
Can we use the Unwrap() error method to returns an error object that implement the SQLState() string method?
I see it smells... a bit ugly. But it will works with other famous SQL drivers, such as the PostgreSQL.

My native language is not English. Sorry for my poor English.

[iseki0]

Can we use the Unwrap() error method to returns an error object that implement the SQLState() string method?

In Go, we often use fmt.Errorf("prefix: %w", e) to wrap an error, because in Go we haven't a stacktrace.
So I think if people want to read the SQL state from an error object, they will use errors.As() like:

var errorWithSQLState interface{ SQLState() string }
if errors.As(e, &errorWithSQLState) {
  fmt.Println(errorWithSQLState.SQLState())
}

If we use the Unwrap() error method, we will have a chance to keep compatibility with the PostgreSQL driver.
At least... Many SQL state code is standarized, so many program can use it to handling DB error.

[methane]

I just curious why it's a field.

Because I never think about such use cases. Your comment is the first time I heard about using SQLState as DB-independent error code.

Both of database/sql and DB API in Python defines SQLState. sqlite3 error code doesn't mention about SQLState. They are my only experience.

Is there any famous ORM implementation (not limited to Go) using SQLState as DB-independent error code?

But it will works with other famous SQL drivers, such as the PostgreSQL?

Could you list several other SQL drivers?

Currently, I feel abusing Unrap() for this is not worth enough.

[iseki0]

Could you list several other SQL drivers?

https://pkg.go.dev/github.com/godror/godror#OraErr.SQLState
https://github.com/asifjalil/cli/blob/2598d7f01fdb79080f4a7837561a9fbe019eda39/error.go#L87-L89

Both of database/sql and DB API in Python defines SQLState.

😦Could you show me where the definition in the database/sql? I haven't found. I think if go standarized it, we should obey the standards. Just like the JDBC

[iseki0]

Is there any famous ORM implementation (not limited to Go) using SQLState as DB-independent error code?

You can see the function in hibernate:
https://github.com/hibernate/hibernate-orm/blob/eb03db74012665a738bbaea737bbbdc91ac15ad8/hibernate-core/src/main/java/org/hibernate/exception/internal/SQLStateConversionDelegate.java#L80-L124
It convert some well-known SQL state to program exceptions...

[iseki0]

Fortunately, we have Unwrap() []error now. We have a chance, even if we already use error wrapping.

[methane]

😦Could you show me where the definition in the database/sql?

https://pkg.go.dev/database/sql

[methane]

Is there any Go ORM that want to use SQLState for DB-independent error handler?

[iseki0]

😦Could you show me where the definition in the database/sql?

https://pkg.go.dev/database/sql

Show me the paragraph, thank you

[methane]

No paragraph at all. I meant database/sql doesn't provide a way to DB-independent API for SQLState. And this driver is based on it.

And there is no feature request to support SQLState on database/sql nor PyMySQL nor mysqlclient-python. I doubt that using SQLState is common practice.

[arnehormann]

@iseki0 thank you for that input. but even if you are right - first, we cannot change it now. second, that would have been a wonderful code review comment. why did you sadly have to add it more than a year after it was merged and not when it was still open?
Honestly, this is passive aggressive and not constructive and it contributes to maintainer burnout. why?
something like: "To bad it's too late now, but for similar future consideration: making it a method instead of a function would have helped for situation xyz." which still gets the point across and helps. but without one-upping.

@arnehormann Sorry for my irrational and negative comments. I just curious why it's a field. And, I think we have a chance to improve the compatibility: Can we use the Unwrap() error method to returns an error object that implement the SQLState() string method? I see it smells... a bit ugly. But it will works with other famous SQL drivers, such as the PostgreSQL.

My native language is not English. Sorry for my poor English.

thank you for this, it's the basis for a more constructive discussion.

[iseki0]

I meant database/sql doesn't provide a way to DB-independent API for SQLState.

That's the problem. 😥😫

[arnehormann]

@iseki0 this driver aligns to database/sql. There might be capabilities not included there (e.g. providing io.Reader for LOAD DATA INFILE LOCAL), but we decide on an api without any global standard.
If it were a method, it could have been named differently or provided a different return code ([]byte vs [6]byte vs string ...). Without an interface in database/sql, it might still be wrong.
Constructively, you can still get it without adding the driver as a dependency if you use reflect to retrieve the field. Then you just have to know what it looks like - which is not a huge problem as it won't change.
Might this help you?
And as an alternative, you can of course also propose it as an addition to database/sql. which we would adopt as soon as we can.

[iseki0]

@iseki0 this driver aligns to database/sql. There might be capabilities not included there (e.g. providing io.Reader for LOAD DATA INFILE LOCAL), but we decide on an api without any global standard.
If it were a method, it could have been named differently or provided a different return code ([]byte vs [6]byte vs string ...). Without an interface in database/sql, it might still be wrong.
Constructively, you can still get it without adding the driver as a dependency if you use reflect to retrieve the field. Then you just have to know what it looks like - which is not a huge problem as it won't change.
Might this help you?
And as an alternative, you can of course also propose it as an addition to database/sql. which we would adopt as soon as we can.

Your reply is helpful. I'm also afraid that the Go team might give a standard which conflict with the current behaviour. The problem is Go's.