db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From myrna <my...@Golux.Com>
Subject Re: [jira] Commented: (DERBY-95) NPE when passing in url instead of database name to EmbeddedXADataSource
Date Thu, 20 Jan 2005 18:19:41 GMT
Shreyas,

I like this message...The behavior is now fine with me. But now we don't 
need the imports to MessageID, ExceptionSeverity, and MessageService 
anymore, correct?

Also, do we feel this bug warrants a test case in one of the test 
suites? If so, it could maybe be added to 
org.apache.derbyTesting.functionTests.tests.jdbcapi.checkDataSource30 ? 
(I could do that).

Myrna


Shreyas Kaushik wrote:

> Here is the latest version of the patch with the aprropriate messages.
>
> thanks
> Shreyas
>
> Daniel John Debrunner wrote:
>
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> myrna wrote:
>>
>>  
>>
>>> It does no longer throw an NPE, and it passes all tests....
>>>
>>> Could we leave EmbeddedDataSource alone & only change
>>> EmbeddedXADataSource? Only with the xa datasource did we get a NPE...
>>> Although strictly speaking, it should behave the same, so, with
>>> EmbeddedDataSource too, we should accept the databasename only, it's
>>> been able to resolve the url fine. I'd be apprehensive about any
>>> possible cloudscape customers that have mistakenly used this
>>> functionality...
>>>   
>>
>>
>> I agree with Shreyas that his patch is logically correct but I want to
>> ensure that your concerns are addressed. Can you be explicit about what
>> you believe the patch breaks for existing users of Derby. I tried this
>> and it results in a NullPointerException if the return from
>> getConnection() is referenced, e.g. in the conn.close() here.
>> (this is before the patch is applied)
>>
>> org.apache.derby.jdbc.EmbeddedDataSource ds = new
>> org.apache.derby.jdbc.EmbeddedDataSource();
>>
>> ds.setDatabaseName("jdbc:derby:fred");
>>
>> Connection conn = ds.getConnection();
>>
>> // NPE here because conn is null
>> conn.close();
>>
>>
>> The only issue I have with the patch is that the error thrown is
>> misleading, "Database Not Available" implies to the user or application
>> developer that they need to do something to make the database available.
>> A database not found exception with the name obtained from
>> DataSource.getDatabaseName() might make more sense. Or maybe even
>> better, invalid property setting error, since databaseName is a Java
>> Bean property of the datasource. E.g. SQLState.PROPERTY_INVALID_VALUE,
>> resulting in an error like
>>
>> Invalid value for property 'databaseName'='jdbc:derby:fred'.
>>
>> Thanks,
>> Dan.
>> -----BEGIN PGP SIGNATURE-----
>> Version: GnuPG v1.2.5 (MingW32)
>> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>>
>> iD8DBQFB7rJnIv0S4qsbfuQRApTGAKDj+MxB4kOY/GNFCMs/9R5Js3zK2wCgqYSm
>> DrAWFRgPOfwMHin0vUwGi70=
>> =zL6e
>> -----END PGP SIGNATURE-----
>>
>>  
>>
>------------------------------------------------------------------------
>
>Index: java/engine/org/apache/derby/jdbc/EmbeddedDataSource.java
>===================================================================
>--- java/engine/org/apache/derby/jdbc/EmbeddedDataSource.java	(revision 125587)
>+++ java/engine/org/apache/derby/jdbc/EmbeddedDataSource.java	(working copy)
>@@ -34,6 +34,11 @@
> 
> 
> import org.apache.derby.iapi.reference.Attribute;
>+import org.apache.derby.iapi.reference.MessageId;
>+import org.apache.derby.iapi.reference.SQLState;
>+import org.apache.derby.iapi.error.ExceptionSeverity;
>+import org.apache.derby.iapi.services.i18n.MessageService;
>+import org.apache.derby.impl.jdbc.Util;
> 
> /** 
> 	
>@@ -431,6 +436,8 @@
> 	final Connection getConnection(String username, String password, boolean requestPassword)
> 		throws SQLException {
> 
>+        Connection toClose = null;
>+
> 		Properties info = new Properties();
> 		if (username != null)
> 			info.put(Attribute.USERNAME_ATTR, username);
>@@ -450,6 +457,7 @@
> 
> 		if (attributesAsPassword && requestPassword && password != null) {
> 
>+
> 			StringBuffer sb = new StringBuffer(url.length() + password.length() + 1);
> 
> 			sb.append(url);
>@@ -459,8 +467,12 @@
> 			url = sb.toString();
> 
> 		}
>+		toClose =  findDriver().connect(url, info);
> 
>-		return findDriver().connect(url, info);
>+        if(toClose == null)
>+           throw Util.generateCsSQLException(SQLState.PROPERTY_INVALID_VALUE,Attribute.DBNAME_ATTR,getDatabaseName());
>+
>+        return toClose;
> 	}
>    
> 	Driver169 findDriver() throws SQLException
>Index: java/engine/org/apache/derby/jdbc/EmbeddedXADataSource.java
>===================================================================
>--- java/engine/org/apache/derby/jdbc/EmbeddedXADataSource.java	(revision 125587)
>+++ java/engine/org/apache/derby/jdbc/EmbeddedXADataSource.java	(working copy)
>@@ -132,6 +132,9 @@
> 
> 	private void setupResourceAdapter(String user, String password, boolean requestPassword)
throws SQLException
> 	{
>+
>+        Connection toClose = null;
>+
> 		synchronized(this)
> 		{
> 			if (ra == null || !ra.isActive())
>@@ -157,18 +160,24 @@
> 						// If database is not found, try connecting to it.  This
> 						// boots and/or creates the database.  If database cannot
> 						// be found, this throws SQLException.
>-						if (requestPassword)
>-							getConnection(user, password).close();
>-						else
>-							getConnection().close();
>-
>+						if (requestPassword) {
>+							   toClose = getConnection(user, password);
>+                               if(toClose != null)
>+                                   toClose.close();
>+                        }
>+						else {
>+							toClose = getConnection();
>+                            if(toClose != null)
>+                                toClose.close();
>+                        }
> 						// now try to find it again
> 						database = (Database)
> 							Monitor.findService(Property.DATABASE_MODULE, dbName); 
> 					}
> 
>-					if (database != null)
>+					if (database != null)  {
> 						ra = (ResourceAdapter) database.getResourceAdapter();
>+                    }
> 				}
> 
> 				if (ra == null)
>@@ -178,9 +187,8 @@
> 
> 
> 				// If database is already up, we need to set up driver
>-				// seperately. 
>+				// seperately.
> 				findDriver();
>-
> 				if (driver == null)
> 					throw new SQLException(MessageService.getTextMessage(MessageId.CORE_DRIVER_NOT_AVAILABLE),
> 										   "08006",
>  
>


Mime
View raw message