mridoni / gixsql

GixSQL is an ESQL preprocessor and a series of runtime libraries to enable GnuCOBOL to access PostgreSQL, ODBC, MySQL, Oracle and SQLite databases.
GNU General Public License v3.0
16 stars 8 forks source link

sqlstate by driver may set 00000 for errors (in GixSQL?) #114

Closed GitMensch closed 1 year ago

GitMensch commented 2 years ago

Given https://github.com/mridoni/gixsql/blob/6394c173f8855db8d9617d67c25f05bc09ddeba4/runtime/libgixsql/gixsql.cpp#L1260-L1272 there's a check for "only use driver code when != 0", something similar seems necessary for sqlstate.

Of course there is the question "should this happen" or is there an internal error in this case? I don't know but in the case I've had "invalid data" which was not invalid but wrong checked by gixsql the driver said "the last query was perfect", so only could return 0 + "00000".

... and applications that check the sqlstate then possibly loop...

I think there should at least be a warning message if gixsql wants to set a non-zero state and the driver says "code zero" and/or "state zero".

mridoni commented 2 years ago

Replacing line 1266 with:

if (!sqlstate.empty() && sqlstate != "00000") {

should work, basically:

1) SQLSTATE is initialized to 00000 at line 1102 (the sqlca_initialize call) 2) if an error code was passed to setState, it is used to set the defaults for SQLSTATE and SQLERRM 3) if (and only if) the driver reports something different from the "all ok" condition, we defer to the driver-provided information instead of the ones provided in 2) because they are supposedly more accurate.

GitMensch commented 2 years ago

This ensures a "manual" error is not reset by the driver, which is reasonable. The question is: should there be a case where this happens? I suggest the change is done as mentioned and additional add an else logger.warn("internally set sqlstate {} while driver sets 00000", st->sqlstate); [at least logger.trace]

mridoni commented 2 years ago

I rewrote it like this:

    // if the driver provides an sqlstate we use it instead of the generic one above
    std::string sqlstate = dbi->get_state();
    if (!sqlstate.empty() && sqlstate != "00000") {
        if (sqlstate.size() > 5)
            sqlstate = sqlstate.substr(0, 5);

        memset(st->sqlstate, '0', 5);
        memcpy(st->sqlstate, sqlstate.c_str(), sqlstate.size());
    }
    else {  // SQLSTATE might be empty due to the "&&" in the "if" above, we check for the specific set of conditions
        if (err != DBERR_NO_ERROR && sqlstate == "00000") {
            spdlog::warn("internally set sqlstate {} while driver sets 00000", st->sqlstate);
        }
    }
GitMensch commented 1 year ago

Shouldn't that be fixed-closed?

mridoni commented 1 year ago

Shouldn't that be fixed-closed?

Yes, thanks