Insufficient connection checking in getUpdateSql() / getInsertSql()

[NathanGibbs3]

Description

When trying to build a testcase for #993 it was discovered that these functions do not check the connected state of the passed AdoConnection object.

• GetInsertSQL() throws warnings,
• GetUpdateSQL() crashes.

Environment

• ADOdb version: < 5.22.7
• Driver or Module: All
• Database type and version: All
• PHP version: All
• Platform: All

• I have tested that the problem is reproducible in the latest release / master branch / hotfix branch

Steps to reproduce

Detailed, step-by-step instructions to reproduce the behavior, including:

• code snippet

// TestCase for 993
$lib = 0; // 0 use 5.22.6 release 1 use snapshot of master branch.
$driver = 'mysqli';

$svr = 'db';
$usr = 'test';
$pwd = 'password';

$DB = 'test';
$tbl = 'test';
$fld = 'test';

if ( $lib == 0 ){
    include('../../ADOdb-5.22.6/adodb.inc.php');
}else{
    include('../../ADOdb/adodb.inc.php');
}
$tmp = 'notatable';

$db = newAdoConnection($driver);
// No Connection Valid and Invalid Table
print $db->GetInsertSQL($tbl, array($fld=>'test'))."\n";
print $db->GetUpdateSQL($tbl, array($fld=>'What'))."\n";
print $db->GetInsertSQL($tmp, array($fld=>'test'))."\n";
print $db->GetUpdateSQL($tmp, array($fld=>'What'))."\n";
$db->connect($svr, $usr, $pwd, $DB);
// Connection Valid and Invalid Table
print $db->GetInsertSQL($tbl, array($fld=>'test'))."\n";
print $db->GetUpdateSQL($tbl, array($fld=>'What'))."\n";
print $db->GetInsertSQL($tmp, array($fld=>'test'))."\n";
print $db->GetUpdateSQL($tmp, array($fld=>'What'))."\n";

• SQL to create and populate database objects used by the code snippet

CREATE TABLE test ( test VARCHAR(255) );

Expected behavior

Expected to see the 6 error messages based on ADODB_BAD_RS.

Additional context

Related Issues: #899 #993

If this is something that would be considered critical, I'll provide backports.

[NathanGibbs3]

Connection checking may be an across the codebase issue.
I tried this to see what happened.

• Set debug to true for an unconnected ADOConnection object.
• Called serverinfo()

Got PHP Warnings & stack traces.
Which is not as helpful to the end user as a "Not Connected" error message.

Example Code:

$driver = 'mysqli';
$db = newAdoConnection($driver);
print "ADOdb   Ver: " . $db->Version()."\n";
$db->debug = true;
// No Connection, Valid and Invalid Table / Field
print "ADOdb Error: " . $db->ErrorMsg()."\n";
print "DB Server Desc: " . $db->serverInfo()['description']."\n";
print "DB Server  Ver: " . $db->serverInfo()['version']."\n";

It seems that some functions assume that if we have an ADOConnection object, that it is connected to a DB, and we can just start issuing SQL commands. Adding connection checking to these, and more importantly providing a standard not connected error message would make troubleshooting easier down the road. What I've put into a PR could be a start along these lines.

We may want to provide a general not connected error message, that we could use in other places in the codebase.
I'm thinking a more general error message for the proposed ADODB_BAD_CONNECTON vs. the current one that is based on ADODB_BAD_RS

How do we want to word it?

[dregad]

Connection checking may be an across the codebase issue.

That depends on how you look at it.

Attempting to use most of the ADOConnection object's methods without a connected database does not make any sense, and IMO is most likely an error / design problem in the client code that should check the connection's status before continuing. As the documentation says

The method returns true if the connection was successfully established, false otherwise. This should be checked before continuing with code execution.

With this in mind, I believe it's probably a good thing to have ADOdb throw some kind of error instead of displaying a nice and gentle error message, sometimes a hard fail the right thing to do... And it avoids having to add an extra to check this in pretty much each and every ADOConnection method, which I'm not sure is worth the effort and overhead in CPU cycles.

At some point we're going to move towards Exceptions, and when that happens ADOConnection::connect() will probably throw instead of returning false.

[NathanGibbs3]

I understand, wrapping every method in connection checking code is a lot of work.
I've had to do this with calls to ADOdb in my own project because ADOdb will fail catastrophically on transient DB connectivity issues.

It's out of scope for this issue, but adding connection checking to the ADOConnection->Execute() method would catch most of this as many ADOConnection methods as well as client code that require a connection go through there anyways.

Ok, back to the issue at hand.
Fixing this in the context of GetInsertSQL & GetUpadteSQL() partially fixes #993 and is in line with the discussions in #899 .

However trying to test #993 did raise the point.

providing a standard not connected error message would make troubleshooting easier down the road. What I've put into a PR could be a start along these lines.

We may want to provide a general not connected error message, that we could use in other places in the codebase. I'm thinking a more general error message for the proposed ADODB_BAD_CONNECTON vs. the current one that is based on ADODB_BAD_RS

The question is, what would we want ADODB_BAD_CONNECTION to be?

• Based on ADODB_BAD_RS, like in Closes #994 #996 '<p>Bad $rs in %s. Not Connected. Try using $connection->debug=true;</p>'
• Something more general, that we could use in other places if / when we wanted to.

I'm thinking of issues like #875 where better error messages provided by ADOdb would go a long way towards troubleshooting.

[mnewnham]

I don't think that adding any more load to execute () is appropriate. For users who need the safety the autoexecute() method provides additional layers of validation. We've spent a good bit of time increasing the speed of the function by adding things like recordset caching. As execute is the lowest level of record manipulation we provide it has to assumed that a robust connection is in place for db access

[NathanGibbs3]

OK, are we keeping connection checking for GetInsertSQL(), GetUpdateSQL(), & rs2html(); or are we completely disregarding the discussion in #899 about making the error message less ambiguous?

Another thought I had around this as well.
The Error message states Connection or SQL invalid.. It's ambiguous what the actual issue is. With some additional code we could separate the connection issue from the invalid SQL issue and return a more meaningful error message.

I'll close down #993, this issue, & #996 if we are not going to do any connection checking.
Should I change the error message in #991 to remove reference to the connection?

I don't wish to waste our collective time building things for ADOdb that are neither needed or wanted, aka backports to the 5.20x line. LOL! 😄

[NathanGibbs3]

It's out of scope for this issue, but was also exposed by my testcase for #993, code that never ran as expected.
GetInsertSQL() & GetUpdateSQL() do not check for the existence of tables & fields,
This could be implemented by calls to MetaTables() & MetacolumnNames().
BTW, those two don't show up in the Reference Manual

Is table & field checking for GetInsertSQL() & GetUpdateSQL() something that is even needed / wanted?