Bug 143837 - Need test() method to DatabaseConnection
Need test() method to DatabaseConnection
Status: VERIFIED FIXED
Product: db
Classification: Unclassified
Component: Code
6.x
All All
: P2 (vote)
: 6.x
Assigned To: David Vancouvering
apireviews
: API_REVIEW_FAST
Depends on:
Blocks: 122944 142639 142773 143798
  Show dependency treegraph
 
Reported: 2008-08-14 00:30 UTC by David Vancouvering
Modified: 2008-09-16 10:34 UTC (History)
1 user (show)

See Also:
Issue Type: DEFECT
:


Attachments
Patch file for this API change (5.42 KB, patch)
2008-08-15 19:48 UTC, David Vancouvering
Details | Diff
Updated patch for this API change (8.79 KB, patch)
2008-08-19 22:34 UTC, David Vancouvering
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Vancouvering 2008-08-14 00:30:07 UTC
See Issue 122944 for details
Comment 1 David Vancouvering 2008-08-14 00:35:52 UTC
Andrei suggests 

        try {
            conn.getMetaData().getTables(null, null, " ", new String[] { "TABLE" }).close();
        } catch (SQLException e) {
            return false;
        }
        return true;

but my concern is that this could return a *LOT* of tables for a large schema.  I suggest adding a filter to the table
names so that we either get one or no results if it is successful, e.g.

        try {
            conn.getMetaData().getTables(null, null, "MRBADEA", new String[] { "TABLE" }).close();
        } catch (SQLException e) {
            return false;
        }
        return true;

That way Andrei will also be immortal :)
Comment 2 David Vancouvering 2008-08-14 00:42:16 UTC
Another problem with getTables() - there are a number of drivers that don't support this.  Now we would break these
drivers not only for the explorer but for the SQL Editor and other uses as well.

I wonder if there is another approach.  Let me think about it a bit.
Comment 3 David Vancouvering 2008-08-14 00:48:45 UTC
Well, after further thought, I believe this is probably the best choice, with the caveat that you may get some false
negatives for drivers that don't support DatabaseMetaData.getTables().  Any attempt to issue "generic SQL" will yield
more false negatives -- I don't think there is *any* SQL that works across all vendors :)
Comment 4 David Vancouvering 2008-08-14 00:50:28 UTC
This is a defect rather than a feature because without it we are generating spurious unhelpful exceptions like
"Insufficient data while reading from the network - expected a minimum of 6 bytes and received only -1 bytes" (see Issue
143798)
Comment 5 Andrei Badea 2008-08-14 10:50:24 UTC
What is wrong with sending " " as the table name pattern? It should only return a table named " ", at least according to
the Javadoc.
Comment 6 David Vancouvering 2008-08-15 19:47:15 UTC
This API change allows a user of the API to validate that a connection is working before issuing commands to the
database.  This allows code using this API to display a useful message to the user rather than display exceptions that
are ugly and hard to interpret.  Note the two issues that depend on this already.
Comment 7 David Vancouvering 2008-08-15 19:48:28 UTC
Created attachment 67571 [details]
Patch file for this API change
Comment 8 David Vancouvering 2008-08-18 20:42:33 UTC
Andrei sends an email saying:
===

David,

I don't think it is enough to display an error message. For Derby at least, when the server has been stopped it can just
be started.

Actually, even if the server can't be started, or the network is down, the clients of the test() method should not be
forced to invent their own message. The API should do that for them. So the test() method should put the connection in
the disconnected state, so that the next call to showConnectionDialog() tries to reconnect and it fails with a standard
error message.

Moreover, how the clients are going to use the test() method? They will get a connection through getJDBCConnection(),
then use test(), and eventually call showConnectionDialog() if needed. Looks like a pattern that will end up copy-pasted
throughout the code. Perhaps the clients would actually want a

Connection getJDBCConnection(boolean test);

method, not a test() method. When called with a false parameter, it will do the same as getJDBCConnection(). When called
with true, it will either return null if the connection is not connected, or test the connection and return null or the
java.sql.Connection. When the test fails, the connection is also put in the disconnect state. 
####

and I respond:

I like these ideas - keep it simple for the user API and don't require copy/paste of boilerplate code.  I also like the
bit of marking the connection disconnected so that the next call to showConnectionDialog() does the right thing.  Also
the connect() method needs to do the right thing.

I'll do another revision.
Comment 9 David Vancouvering 2008-08-18 22:31:52 UTC
I'm adding getJDBCConnection(test).  But one question I have about your comment, Andrei

<Andrei>
Actually, even if the server can't be started, or the network is down, the clients of the test() method should not be
forced to invent their own message. The API should do that for them.
</Andrei>

What were you thinking here?  The getJDBCConnection() method shouldn't post a dialog if the connection is invalid, as
it's not part of the UI layer.
Comment 10 Andrei Badea 2008-08-19 12:37:26 UTC
I meant that the client shouldn't be forced to do:

Connection conn = dbconn.getJDBCConnection();
if (!dbconn.test()) {
    // Display ad-hoc "connection broken" message, expecting that the connection 
    // can't be reconnected -- which is true in most cases.
}

Instead, they should do:

Connection conn = dbconn.getJDBConnection(true);
// Null is returned here if the connection is broken.
if (conn == null) {
    ConnectionManager.getDefault().showConnectionDialog(dbconn);
}

This way, if the connection is broken beyond repair and it can't be reconnected, the sCD() will display the standard
error message.
Comment 11 David Vancouvering 2008-08-19 22:34:46 UTC
Created attachment 67871 [details]
Updated patch for this API change
Comment 12 David Vancouvering 2008-08-19 22:37:58 UTC
Andrei, et. al. -- I have attached an updated patch which adds the getJDBCConnection(test) method, which marks the
connection as closed if there is a problem.

I modified the original getJDBCConnection() to call getJDBCConnection(false).  I am not sure about this.  I would feel
it would be more robust to have it call getJDBCConnection(true) but I am concerned that this is an incompatible change.

Do others have opinions about this.  I am on the fence.
Comment 13 Andrei Badea 2008-08-20 10:10:55 UTC
The API looks fine, but I have a couple of notes on the documentation:

[AB01] "This change allows you to make sure a DatabaseConnection is valid". You do not test the DatabaseConnection, but
the underlying physical JDBC connection. The DatabaseConnection is always valid. The same applies to the test-connection
use case in the arch.xml.

[AB02] I would suggest dropping "before you use it to issue commands to the database". The user can do whatever he wants
with the connection, so best to keep the description generic.

[AB03] "it marks the DatabaseConnection as invalid". It disconnects the connection, not marks it as invalid.

[AB04] Typo in "Get a JDBCConnection" -- no such thing as a JDBCConnection. Actually, the Javadoc of the new
getJBDCConnection(boolean) method should be based on and include all the details in getJDBCConnection(), while also
documenting the new behavior and parameter.

[AB05] Please state the in Javadoc that the method that getJDBCConnection(true) uses to test the connection is
unspecified, but it can be a long-duration operation such as executing a statement against the database. Also mention
that it should not be called in the event dispatching thread.

Also, the patch seems to include unintentional modifications to DatabaseConnectionTest.java.

You are right that getJDBCConnection() should call getJDBCConnection(false) for backward compatibility reasons, and also
for performance reasons, as stated in [AB05].
Comment 14 David Vancouvering 2008-08-27 19:39:02 UTC
I applied Andrei's comments on the docs and have committed the change.  Now the issues that depend on this feature can
be fixed.

fa862747712d
Comment 15 Quality Engineering 2008-08-28 06:41:15 UTC
Integrated into 'main-golden', available in build *200808280201* on http://bits.netbeans.org/dev/nightly/
Changeset: http://hg.netbeans.org/main/rev/fa862747712d
User: David Van Couvering <davidvc@netbeans.org>
Log: #143837: Need test() method to DatabaseConnection
Comment 16 Quality Engineering 2008-08-30 05:43:08 UTC
Integrated into 'main-golden', available in build *200808300201* on http://bits.netbeans.org/dev/nightly/
Changeset: http://hg.netbeans.org/main/rev/6ac44f704347
User: David Van Couvering <davidvc@netbeans.org>
Log: #143837: Update the version for the spec
Comment 17 Roman Mostyka 2008-09-16 10:34:59 UTC
Verified.


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2014, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo