History | Log In     View a printable version of the current page.  
Issue Details (XML | Word | Printable)

Key: CASTOR-880
Type: Improvement Improvement
Status: Closed Closed
Resolution: Fixed
Priority: Minor Minor
Assignee: Werner Guttmann
Reporter: Martin Fuchs
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
castor

Database.getConnection()

Created: 14/Sep/04 09:39 AM   Updated: 04/Aug/05 03:36 AM
Component/s: JDO
Affects Version/s: 0.9.5.3
Fix Version/s: 0.9.7 M1

Time Tracking:
Original Estimate: 15 minutes
Original Estimate - 15 minutes
Remaining Estimate: 15 minutes
Remaining Estimate - 15 minutes
Time Spent: Not Specified
Remaining Estimate - 15 minutes

File Attachments: 1. Text File castor-database-getconnection.patch (0.8 kb)
2. Text File getjdbcconnection.patch (12 kb)
3. Text File patch.txt (12 kb)
4. Text File patch.txt (12 kb)

Environment:
Operating System: All
Platform: All


 Description  « Hide
While looking through in Castor's source code the following question
hit me one more time: Why is it made so difficult to retrieve the
underlying JDBC connections from Castor?

Yes, I saw the warning "Only for internal / advanced use !" in
DatabaseImplgetConnection().
But some applications need access to the JDBC connection for example
to set some DB parameters at session initialization.

I see everywhere this construct:

Database db = jdo.getDatabase();
db.begin();
Connection conn = (Connection) ((DatabaseImpl)db).getConnection();

A bit complicated just to get a connection object, if you ask me.

First: Why is the cast to DatabaseImpl necessary?
Well, it's needed because getConnection() is only present in DatabaseImpl,
not in the Database interface. What about declaring it in the Database
interface, perhaps with ajusted name getJdbcConnection() and
the return type java.sql.Connection ?
Any other derived database implementation could just return null,
if it doesn't use a JDBC connection. Well - will there ever be a database
implementation in Java, which doesn't use JDBC? I doubt.

Second: Why is the cast to Connection necessary?
DatabaseImpl.getConnection() simply calls TransactionContextImpl.getConnection
(),
which in any case returns a JDBC Connection object. Is there the
possibility to return any another type?

I would propose to change the return type of the getConnection() functions
from Object to java.sql.Connection.

What do you think?



 All   Comments   Work Log   Change History      Sort Order: Ascending order - Click to sort in descending order
Martin Fuchs - 14/Sep/04 09:40 AM
Created an attachment (id=662)
Implementation of Database.getJdbcConnection()

Bruce Snyder (bugzilla) - 15/Sep/04 01:08 AM
This one time, at band camp, Martin Fuchs said:

MF>while looking through in Castor's source code the following question
MF>hit me one more time: Why is it made so difficult to retrieve the
MF>underlying JDBC connections from Castor?
MF>
MF>Yes, I saw the warning "Only for internal / advanced use !" in
DatabaseImplgetConnection().
MF>But some applications need access to the JDBC connection for example
MF>to set some DB parameters at session initialization.
MF>
MF>I see everywhere this construct:
MF>
MF>Database db = jdo.getDatabase();
MF>db.begin();
MF>Connection conn = (Connection) ((DatabaseImpl)db).getConnection();
MF>
MF>A bit complicated just to get a connection object, if you ask me.
MF>
MF>First: Why is the cast to DatabaseImpl necessary?
MF>Well, it's needed because getConnection() is only present in DatabaseImpl,
MF>not in the Database interface. What about declaring it in the Database
MF>interface, perhaps with ajusted name getJdbcConnection() and
MF>the return type java.sql.Connection ?
MF>Any other derived database implementation could just return null,
MF>if it doesn't use a JDBC connection. Well - will there ever be a database
MF>implementation in Java, which doesn't use JDBC? I doubt.

I'm not sure why the getConnection() method is not exposed in the
Database interface. Proabably just an oversight more than anything.
I have no problem exposing this method via the Database interface
but the name cannot change and here's why. All connections in Castor
are not always of type java.sql.Connection. There have always been
plans to make Castor support LDAP persistence. Currently it only
supports querying and marshalling the results to XML. I'm about to
start to work on adding LDAP persistence to Castor to leaving the
getConnection() method signature the way it is currently designed
is important.

MF>Second: Why is the cast to Connection necessary?
MF>DatabaseImpl.getConnection() simply calls TransactionContextImpl.getConnection(),
MF>which in any case returns a JDBC Connection object. Is there the
MF>possibility to return any another type?

The cast to java.sql.Connection is explained above. Castor has
always had plans to connected to data sources other than relational
databases. By allowing this method call to return a java.lang.Object,
we're allowing other types of connections to be returned.

MF>I would propose to change the return type of the getConnection() functions
MF>from Object to java.sql.Connection.

I disagree here and have explained the reaasons for this above.

MF>What do you think?

I've been pondering this myself over the last month or so. Especially
in just the last couple of days. There are many applications that
need access to the JDBC connection simply to set parameters on it
(e.g. batching, holdability, read-only, transaction isolation
(especially this one!), etc.). The conclusion I've come to is that
we should expose such parameters via method calls. This brings up
another issue, however. We can't place JDBC-specific method calls
on the Database interface. Because of this, I've been thinking about
creating a new interface model like so:

------------

DataSource

------------
#

----------------------+

 

--------------------- ---------------

RelationalDataSource   LdapDataSource

--------------------- ---------------
. .
/\ /\

 
 

------------------------- -------------------

RelationalDataSourceImpl   LdapDataSourceImpl

------------------------- -------------------

For a description of this ASCII UML, please see the following doc:

http://c2.com/cgi/wiki?UmlAsciiArt

In the diagram above, the DataSource interface would replace the
current Database interface. The second layer allows any additional
DataSource type to be added (JCA connectors to AS/400, etc.). The
third layer is a replacemet for the current DatabaseImpl.

Bear in mind that changes this significant would not go into the
TRUNK. We'd need to create a 1_0 or 2_0 branch for it. With the big
changes we're thinking about, we need to leave the TRUNK alone and
only do minor changes and bugs fixes.

Bruce

--------------------------------------------------------

MF>I can show you my typical needs for session intializations:
MF>
MF> Statement stmt = conn.createStatement();
MF>
MF> try { MF> stmt.execute("set role ROLE_XXX identified by PASSWORD"); MF> stmt.execute("alter session set nls_language = GERMAN"); MF> stmt.execute("alter session set nls_territory = GERMANY"); MF> stmt.execute("alter session set nls_numeric_characters = ',.'"); MF> } catch(...
MF>
MF>This just requires the ability to execute some SQL statements.
MF>Would this also be possible with the method calls, you are thinking about?

No, I was only speaking of exposing method calls on the java.sql.Connetion
object. The stmt.execute() calls above can be achieved using the
java.sql.Connection object obtained from Database.getConnection().
Or am I missing something?

Bruce


Martin Fuchs - 15/Sep/04 01:16 AM
OK, the following patch will contain only the extension of the Database
interface by getConnection() to expose DatabaseImpl.getConnection() without the
need for a (DatabaseImpl) typecast.

Martin Fuchs - 15/Sep/04 01:17 AM
Created an attachment (id=663)
Patch for org.exolab.castor.jdo.Database interface

Werner Guttmann - 15/Sep/04 01:53 PM
>I'm not sure why the getConnection() method is not exposed in the
>Database interface.
Not am I, but I thin the original intention was to make it 'quite difficult' to
obtain a JDBC Connection from a Datbase instance. Here's why:

a) Castor JDO is an o/r mapper, first and foremost. We do not want people to
fiddl around with data through direct SQL calls.
b) One must NOT close this connection instance, as this would interfer with
Castor's locking and transaction business.

Because of this, I am really not sure whether we should expose getConnection
even further.

I've seen all the design ideas, bug I do not have the time to comment now.


Martin Fuchs - 16/Sep/04 01:04 AM
> Not am I, but I thin the original intention was to make it 'quite difficult'
to
> obtain a JDBC Connection from a Datbase instance. Here's why:

Then this goal of making it 'quite difficult' has been reached.

> a) Castor JDO is an o/r mapper, first and foremost. We do not want people to
fiddl around with data through direct SQL calls.

But you see, there is the need for some way to get access to the underlying SQL
connection in many applications. I see two application cases here:

  • Initializing the database connection by setting some DB parameters or calling
    some stored procedures in the database.
  • Calling stored procedures later on to process business logic.

So we should try to come up with a more convenient solution.

> b) One must NOT close this connection instance, as this would interfer with
Castor's locking and transaction business.

If forbidding to call close() for the connection instance is the main reason
for the design decision, what do you think about exposing two methods
createJdbcStatement() and createPreparedJdbcStatement()? This would be
sufficient for most application cases, while not allowing direct access to the
connection object.

Btw. a callback mechanism to allow initializing a database connection just
after Castor created it, would also be a very kind thing.


Werner Guttmann - 17/Sep/04 03:01 AM
> Initializing the database connection by setting some DB parameters
> or calling some stored procedures in the database.
In the case of setting parameters, are you doing this by setting properties.
Iow, can you please give a short example of what you'd be doing (e.g. using
vanilla JDBC calls).

> Calling stored procedures later on to process business logic.
Why couldn't you use SQL CALL statements ? Just wondering ?


Werner Guttmann - 17/Sep/04 03:04 AM
> Btw. a callback mechanism to allow initializing a database connection
> just after Castor created it, would also be a very kind thing.
Imho, I'd really favour implementing such a feature. As far as I can see, one
might have to deal with the fact that the underlying JDBC connection is not
created at Database instantiation time. But I still believe that - as long as a
handler is registered properly - it will be called, eventually.

Martin Fuchs - 17/Sep/04 03:34 AM
This example is already in the mails from above, but OK - here again.

This is an example of changing some database parameters using JDBC execute
calls:

Statement stmt = conn.createStatement();

try { stmt.execute("set role ROLE_XXX identified by PASSWORD"); stmt.execute("alter session set nls_language = GERMAN"); stmt.execute("alter session set nls_territory = GERMANY"); stmt.execute("alter session set nls_numeric_characters = ',.'"); } catch(...

To use the "CALL SQL" syntax I would have to create a OQLQuery. (Correct me if
I have overseen another possibility.)
But OQLQuery expects a query with result set. I can't just issue a DDL command
or call a database procedure without return data. For example this doesn't
work, I get an 'Stored procedure call must end with "AS <class-name>"'
exception:

OQLQuery oql = db.getOQLQuery("CALL SQL alter session set nls_language =
GERMAN");


Werner Guttmann - 17/Sep/04 07:41 AM
> This example is already in the mails from above, but OK - here again.
Oops, sorry , but I was picking up on the use of the word 'properties' again ...
.

Werner Guttmann - 17/Sep/04 07:43 AM
> To use the "CALL SQL" syntax I would have to create a OQLQuery. (Correct
> me if I have overseen another possibility.) But OQLQuery expects a query with
> result set.
Correct, and I wish I could correct you ... but unfortunately your assessment is
true. Maybe we should allow for 'updates' to be used as part of a CALL SQL
statement.

Martin Fuchs - 30/Oct/04 10:50 AM
Extending OQL to allow 'update' statements may also be a possibility.
You think about something like the following, right?
db.execute("CALL SQL alter session set nls_language = GERMAN");

But then why no immediately allow to issue SQL statements, using e.g.:
db.executeSQL("alter session set nls_language = GERMAN");

Exposing an Database.execute() function as in the first example would imply,
there is also the possibility to execute OQL update statements like:
db.execute("update someClass set attrib1 = 1234");
And this is something, I think, we don't want to provide. Instead Castor can be
used to unmarshal the object using a OQL query, altered in memory and updated
automatically at commit time.


Werner Guttmann - 22/Apr/05 06:18 AM
Alright, there's too many threads in here (un)related to the original issue, i.e. exposing DatabaseImpl.getConnection() more formally (through the interface Database.java) and change its return type).
  • Can we please create new issues for any "unrelated" topics ?
  • How does everybofy feel about Martin's request/patch ?

Me thinks that if we really, really expose a getConnection() method through the Database interface, we will have to provide very good documentation on this subject, as misuse can easily cause quite some problems.


Ralf Joachim - 22/Apr/05 12:18 PM
What about a getConnection() method in JDOManager that will return a java.sql.Connection. As that connection will not be associated with a TransactionManager the interference with castor would only be the cached objects. Its implementation would also be easy as we only need to call DatabaseRegistry.createConnection().

I also don't like to expose the DatabaseImpl.getConnection() method according to the reasons Werner mentioned.


Gregory Block - 22/Apr/05 05:45 PM
I think the ability to retrieve the DatabaseImpl connection is vital for several reasons:

1) In a transaction-oriented environment, the only way to get a safe set of operations is to do so on a single connection, in that single transaction, which can then guarantee rollback capability.

2) While calling passthrough SQL for all possible operations is possible, doing so will require the support of all of the semantics; that means allowing me to call any stored procedure to get any ResultSet I choose, not requiring me to create non-trivial mappings for a bunch of trivial SQL.

There's no doubt that this isn't an 'entry level' operation - direct SQL calls can change, for example, the transaction type to READ_UNCOMMITTED, and watch your dataset torch itself when you do.

However, without doing this, there's no way to build hybrid applications - you're 100% OQL, or you're 100% SQL, and never the twain shall meet - because I can't afford for my SQL operation to lock dead, waiting on an OQL transaction that can't complete until my SQL operation is done.

DBLocked implies a requirement of the existence of that SQL Connection API.

And, to boot, if the concern is that we don't want to expose an API because we can't guess at what the type is, build a typesafe implementation that does a

public Object getNativeConnection(Class expectedType);

that verifies that the connection we've got is of the same type as we're expecting, and return a null if they don't match the expected results.

All in all, this is something that we need to add. We haven't got a choice; it's unreasonable to expect users to never build hybrid applications, and DBLocked is a very important feature for anyone who needs to build one.

If you're THAT concerned about it's usage, have the getNativeConnection() class fail unless a flag is turned on in castor.properties, where it logs an error stating that you attempted to get a native connection from the current DB instance without first configuring it to allow it.

But not implementing it, IMO, just isn't an option.


Ralf Joachim - 22/Apr/05 06:25 PM
Very good point Greogory. Your arguments conviced me to change my opinon to offer the Database.getConnection() method.

None the less I think we also should offer JDOManager.getConnection() for those SQL statements that need not to be in the same transaction with OQL statements. The javadoc for both methods should state when to use one or the other. What do you think about that?


Werner Guttmann - 23/Apr/05 11:02 AM
> None the less I think we also should offer JDOManager.getConnection() for those
> SQL statements that need not to be in the same transaction with OQL statements.
> The javadoc for both methods should state when to use one or the other. What do
> you think about that?
Not sure, to be honest. What transactional semantics would be applied to such a scenario ? If you allow a user to obtain a connection from JDOManager, will 'auto-commit' mode be used or something else ? Do we want this to be configurable. As I've said, not sure.

Werner Guttmann - 23/Apr/05 11:03 AM
Other than this, let's move forward with this issue. Iow, +1.

Gregory Block - 23/Apr/05 05:10 PM
Everything happens within someone's transactional context; as such, I see zero issue with asking a user to get a database object in order to get the connection...

The higher-level equivalent would be to ask JDOManager for the DataSource, and I don't necessarily feel that's wise. I don't see any issue with restricting it to Database only.

And I look forward to getting rid of my DatabaseImpl cast.


Werner Guttmann - 25/Apr/05 02:43 AM
Fwiw, I'll commit this tonight.

Werner Guttmann - 25/Apr/05 03:46 PM
This is an update of the previous patch (plus some more Javadocs), and the removal of e.printStackTrace() calls.

Werner Guttmann - 25/Apr/05 03:49 PM
One remaining question, though: in DatabaseImpl#getJdbcConnection(), do we really want to just log the PersistenceException being thrown in TransactionContext ?

Werner Guttmann - 26/Apr/05 02:51 AM
Based upon a short discussion yesterday, we've settled to add a throws clause to Database.getJdbcConnection(). Will update the patch in due time and commit.

Werner Guttmann - 27/Apr/05 04:25 AM
Updated patch to reflect the exception handling as discussed above.

Werner Guttmann - 27/Apr/05 01:52 PM
Btw, any issues covered in this issue report that have not been addressed by the patch should be moved to new issues.