Client should not allow databaseName to be set with setConnectionAttributes

• Key: DERBY-1130
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Deepa Remesh
• Reporter: Kathey Marsden
• Component/s: Network Client
• Affects Version/s: 10.1.1.0, 10.1.2.1, 10.1.3.1, 10.2.1.6
• Fix Version/s: 10.2.2.1

Description

Per this thread, setConnectionAttributes should not set databaseName.

http://www.nabble.com/double-check-on-checkDataSource-t1187602.html#a3128621

Currently this is allowed for client but should be disabled. I think it is OK to change because we have documented that client will be changed to match embedded for implementation defined behaviour. Hopefully its use is rare as most folks would use the standard setDatabaseName. Still there should be a release not when the change is made and it would be better to change it sooner than later:

Below is the repro.

Here is the output with Client
D>java DatabaseNameWithSetConnAttr
ds.setConnectionAttributes(databaseName=wombat;create=true)
ds.getDatabaseName() = null (should be null)
FAIL: Should not have been able to set databaseName with connection attributes

Also look for tests disabled with this bug number in the test checkDataSource30.java



import java.sql.*;
import java.lang.reflect.Method;


public class DatabaseNameWithSetConnAttr{

public static void main(String[] args) {
try {

String attributes = "databaseName=wombat;create=true";
org.apache.derby.jdbc.ClientDataSource ds = new
org.apache.derby.jdbc.ClientDataSource();

//org.apache.derby.jdbc.EmbeddedDataSource ds = new
//org.apache.derby.jdbc.EmbeddedDataSource();
System.out.println("ds.setConnectionAttributes(" + attributes + ")");
ds.setConnectionAttributes(attributes);
System.out.println("ds.getDatabaseName() = " +
ds.getDatabaseName() + " (should be null)" );

Connection conn = ds.getConnection();

} catch (SQLException e) {
String sqlState = e.getSQLState();
if (sqlState != null && sqlState.equals("XJ041"))
{
System.out.println("PASS: An exception was thrown trying to get a connetion from a datasource after setting databaseName with setConnectionAttributes");
System.out.println("EXPECTED EXCEPTION: " + e.getSQLState()
+ " - " + e.getMessage());
return;
}
while (e != null)
{
System.out.println("FAIL - UNEXPECTED EXCEPTION: " + e.getSQLState());
e.printStackTrace();
e = e.getNextException();
}
return;
}
System.out.println("FAIL: Should not have been able to set databaseName with connection attributes");

}

}

Comments

[Deepa Remesh]

I would like to understand what the behaviour of client driver should be for the following scenario:
call getConnection method when no database name is set for the data source (using setDatabaseName method) and we set the database name using setConnectionAttributes method.

* Code snippet for EmbeddedDataSource:
org.apache.derby.jdbc.EmbeddedDataSource ds = new org.apache.derby.jdbc.EmbeddedDataSource();
ds.setConnectionAttributes("databaseName=wombat;create=true");
Connection conn = ds.getConnection();

In this case, database name is set to " " and it gets trimmed to a zero-length string. And call to getConnection will give following exception:
XJ041 - Failed to create database '', see the next exception for details.

* Code snippet for ClientDataSource:
org.apache.derby.jdbc.ClientDataSource ds = new org.apache.derby.jdbc.ClientDataSource();
ds.setConnectionAttributes("databaseName=wombat;create=true");
Connection conn = ds.getConnection();
 
In the solution I was thinking for ClientDataSource, I plan to catch the error at the client itself and make the client throw the following exception:
08001 - Required property databaseName not set.

Is this solution okay?

[Kathey Marsden]

What is the exception for embedded if create=true is not specified?
 e.g. ds.setConnectionAttributes("databaseName=wombat")

In general I think that the errors should be the same if it is not a tremendous amount of work to make them match. In the case of :
ds.setConnectionAttributes("databaseName=wombat;create=true");
I would think the client would just remove the databaseName part from the string and send the database name as "create=true" and get the same error as embedded.

[Deepa Remesh]

For Kathey's question: What is the exception for embedded if create=true is not specified? e.g. ds.setConnectionAttributes("databaseName=wombat")
Exception for embedded is: XJ004 - Database '' not found.

I think we do not have the same SQL state in the client for the above exception. The corresponding SQLState is "08004 - The connection was refused because the database {0} was not found."

As the SQLStates do not match, I was thinking it would be okay to catch the exception at client itself and throw "08001 - Required property databaseName not set. " if database name is not set using setDatabaseName on the client data source. In case the database name is set using setDatabaseName and we try to over-ride it using "databaseName" property in setConnectionAttributes, the over-riding will fail. Only the database name set using setDatabaseName will be used. This was the behaviour in the patch I was working on.

However, if the above exception is confusing, I'll try to set a dummy database name on the client and send it to server to get back "08004 - The connection was refused because the database {0} was not found.". Please share your thoughts on this.

[Kathey Marsden]

Your approach sounds ok to me, since the SQL states are already different.

[Deepa Remesh]

Attaching a patch 'derby-1130-v1.diff' which ensures that database name set using setConnectionAttributes does not get used by client driver. Changes are:

* Appends the attributes in setConnectionAttributes method to database name only if database name has been already set on the data source. Database name is a required property and if it is not set, DataSource.getConnection method will throw the following exception:
08001 - Required property databaseName not set.

Without the patch, if database name was not set, the code was using "null" as database name and creating a database named null if "create=true" is specified or throwing an exception that it cannot connect to database named null.

* Adds test to jdbcapi/checkDataSource.java. Adds a new method "testClientDSConnectionAttributes". As we are testing the methods specific to Derby data sources, we cannot re-use the method used to test embedded data sources.

Ran derbynetclientmats with this patch using Sun jdk 1.4.2 on Windows XP. No failures. Also ran the modified tests jdbcapi/checkDataSource.java and jdbcapi/checkDataSource30.java in embedded mode. Please review/commit this patch. Thanks.

[David Van Couvering]

Hi, Deepa. I looked at the patch, and have a couple of questions/thoughts

- The driving requirement is that you can't set databaseName as a connection attribute

- I think it's confusing that when someone does try to use connectionAttributes to set the database name, the error they get is "Required property databaseName is not set". They'll scratch their heads and say "doggone it, right here in my code I'm setting it using connectionAttributes! What the heck am I doing wrong? Doth mine eyes deceive me?"

I'm not sure exactly how the logic of connection properties is managed in Derby, but this method seems to be working with a data source. Shouldn't we just check to see if databaseName is in the attributes string (if connAttrs.contains("databaseName")) and raise an exception if it does?

I also think it's problematic that we just ignore the connection attributes if the database name is null. Shouldn't we raise an exception at that point saying something like "you can't set connection attributes for this connection because the database name is not specified" ?

I actually think the exception on the embedded side similarly is confusing, saying the database is not found. I would like something that explicitly says something like "you can not set the database name using connection attributes. Please use setDatabaseName"

Finally, do we need to fix the client so that it has the same SQL State as the embedded driver when the database is not found? I think so, perhaps that should be a separate bug.

Thanks,

David

[Daniel John Debrunner]

David wrote:
  - I think it's confusing that when someone does try to use connectionAttributes to set the database name, the error they get is "Required property databaseName is not set". They'll scratch their heads and say "doggone it, right here in my code I'm setting it using connectionAttributes! What the heck am I doing wrong? Doth mine eyes deceive me?"
 
    djd> Probably not always clear, but 'connectionAttributes' sets JDBC attributes for the connection and here we have 'databaseName' being an *attribute* in connectionAttributes and a Java bean *property* for the DataSource object. Thus the message is technically correct, the *property* has not been set.

David again:

 - I actually think the exception on the embedded side similarly is confusing, saying the database is not found. I would like something that explicitly says something like "you can not set the database name using connection attributes. Please use setDatabaseName"

   djd> I think such a mesage needs to be more generic in how the Java bean property databaseName is set rather than stating use setDatabaseName. Most users will not be calling methods against Derby's DataSourrce implementations, but instead setting fields in a web GUI or modifying configuration files.

Maybe the solution is to be more specific on the term property, replace with 'Derby DataSource property'?
 

[Deepa Remesh]

Thanks David and Dan for looking at this issue.

As David suggests, I think it would be better to throw an exception if database name property is set in setConnectionAttributes method for a DataSource. I think this will make both these cases clearer:
1. We need to set the "databaseName" Java bean *property* for the DataSource object. We cannot expect that the databaseName property in connectionAttributes will get used.
2. If we have set "databaseName" Java bean *property* for the DataSource object, we cannot over-ride it using databaseName property in connection attributes.

As Dan suggests, use of "Derby DataSource property" in the message may help clarify the situation. However, I tend to think it could still be confusing why the databaseName property in connectionAttributes is not getting used. I plan to change setConnectionAttributes method in both embedded and client data sources to check that "databaseName" property cannot be set using this method. Please let me know if anyone thinks this is not okay.

David, I did not understand the following comment:
"
I also think it's problematic that we just ignore the connection attributes if the database name is null. Shouldn't we raise an exception at that point saying something like "you can't set connection attributes for this connection because the database name is not specified" ?
"

Isn't the check for the databaseName property (which is a required property) enough for this? If this required property is not set, then we will not be able to use the DataSource and will be throwing an exception.

[Deepa Remesh]

Attaching a patch 'd1130-client-v1.diff' which disallows databaseName attribute to be set using setConnectionAttributes method in client data sources. Tests added to jdbcapi/checkDataSource.java.

Ran derbynetclientmats using Sun jdk1.4.2 on Windows XP. No new failures. Please take a look at this patch. If this is okay, I plan to upload another patch to add a similar check for embedded data sources.

[David Van Couvering]

Hi, Deepa. I apologize, I don't know how I missed your response on July 19th. Almost a month later I'm getting back to you. That's embarassing. Thank goodness for the 'patch available' report.

I am not sure how to reach closure on this. What I am seeing is the following:

- If the databaseName property is not set, then we get an exception that makes sense - "Required property databaseName not set"

- If the databaseName property is not set, but the database name is set through connection attributes, we still get - "Required property databaseName not set." Now if I were a naive user this would be confusing - I set the databaseName property, why are you complaining? (Ignorant that I am not allowed to set it via connection attributes.) A much more helpful messages would be "You can not set the databaseName property through connection attributes. Please set this as a Derby DataSource property"

- If the databaseName property *is* set, everything works right

- If the databaseName property *is* set *and* the databaseName is specified through connection attributes, this is again potentially confusing. The user sees that they are connecting to or creating database 'wombat' when they explicitly set the database name to 'kangaroo' (ignorant perhaps that they are setting it to 'wombat' elsewhere in their code. What is that about? And that is even harder to track down. Here I think there is value in saying something like "WARNING: property databaseName can not be set through connection attributes. Using the databaseName property 'wombat' that was set through the Derby DataSource."

Thanks,

David

[Deepa Remesh]

Thanks David for revisiting this issue. I too got back to this only this week. Yesterday, I uploaded a new patch 'd1130-client-v1.diff' for client which disallows databaseName attribute to be set using setConnectionAttributes method in client data sources. Please take a look at the new patch to see if the solution is acceptable. If yes, I'll upload a similar patch for embedded.

[Sunitha Kambhampati]

Thanks Deepa for the d1130-client-v1 patch. The patch applies cleanly in my eclipse ide [with a fuzz factor of 24 :) ]

-- the error messages that is thrown look good to me and it seems clean that we are doing the check in setConnectionAttributes itself.

I just have one question. Can you please help me understand why the following change is necessary:
===================================================================
--- java/client/org/apache/derby/jdbc/ClientDataSource.java (revision 429370)
@@ -181,7 +181,6 @@
         try
         {
             LogWriter dncLogWriter = super.computeDncLogWriterForNewConnection("_sds");
- updateDataSourceValues(tokenizeAttributes(getConnectionAttributes(), null));

-----------
Thanks,
Sunitha.

[Deepa Remesh]

Thanks Sunitha for looking at the patch.

I moved updateDataSourceValues method call from getConnection to setConnectionAttributes method. This method looks at the connectionAttributes and updates the fields relevant to client datasource. I have added the check for databaseName attribute to this method. Calling this earlier from setConnectionAttributes allows to throw an exception from here itself. Hope this answers the question.

[Sunitha Kambhampati]

Thats makes sense. Thanks Deepa for the explanation.
The patch (d1130-client-v1.patch) looks good to me. My +1 for commit.

[Kathey Marsden]

Thanks Deepa for the patch and Sunitha for the review.

I checked in the patch for this issue. I marked Existing Application impact because some existing applications may be trying to set the databaseName this way and hit issues upgrading to 10.2. Can you add a release note?

  
Date: Thu Aug 10 20:34:22 2006
New Revision: 430641

URL: http://svn.apache.org/viewvc?rev=430641&view=rev
Log:
DERBY-1130 Client should not allow databaseName to be set with setConnectionAttributes

Attaching a patch 'd1130-client-v1.diff' which disallows databaseName attribute to be set using setConnectionAttributes method in client data sources. Tests added to jdbcapi/checkDataSource.java.

[David Van Couvering]

Somehow again I missed your comments, Deepa. Thanks for making the change, it looks good. A small improvement might be if the databaseName property is null in the DataSource and databaseName is set in connectionAttributes, to raise the exception "databaseName attribute cannot be set using the DataSource method setConnectionAttributes" rather than "required property databaseName not set." -- e.g. to give the former exception precedence.

Thanks,

David

[Deepa Remesh]

Thanks Sunitha, Kathey and David for looking at the patch.

I noticed this patch is causing a failure in jdbcapi/dataSourceReference.java. Sorry about that. I had only run derbynetclientmats before submitting the patch as this was a client-only change. But found that client data sources are also tested in jdbcapi/dataSourceReference.java which is run only in embedded framework. I am attaching a patch 'd1130-test-dataSourceReference.diff' which modifies dataSourceReference test to resolve this failure. The failure was because we now check the connection attributes in the call to setConnectionAttributes itself. Before the patch, this check was deferred to getConnection method. The patch only modifies this test and I checked that the test passes with it. Please take a look and commit if okay.

For David's suggestion, "if the databaseName property is null in the DataSource and databaseName is set in connectionAttributes, to raise the exception "databaseName attribute cannot be set using the DataSource method setConnectionAttributes" rather than "required property databaseName not set." -- e.g. to give the former exception precedence."

If I understand correctly, this is already being handled as David suggests. This is tested in jdbcapi/checkDataSource.java in '"testClientDSConnectionAttributes" method.

Kathey, I will post a release note for this.

[Kathey Marsden]

I will commit the test patch to get the test running again, but it seems problematic to me that the client jar is required to run the embeded tests or derby.jar is required for client tests. Perhaps another issue should be filed for that for the test.

[Daniel John Debrunner]

With this change the setConnectionAttributes method now is declared to throw a SQLException.

I thought such methods should not throw exceptions since it is a Java Bean property setter method.

The suggestion above that this method throw an exception if databaseName has not been set but is set with setConnectionAttributes is invalid.
Since DataSource properties may be set from a configuration file, the data source should not impose any ordering on the setting of the properties.

I think typically any exception would be raised when a connection is requested.

Also for any error text, it's clearer if the usage refers to setting the property connectionAttributes rather than instructions to use (or not use) setConnectionAttributes (the setter methods). Most configuration of these data sources is through gui's and sets of properties not through Java code directly.

[Daniel John Debrunner]

Also throwing SQLException on setConnectionAttributes is a regression from 10.1 and may cause applications to break, since previously this method did not throw the exception.

[Deepa Remesh]

I was going to post a release note about the existing application impact. But based on what Dan just said, this change in the method does not look correct and maybe we need to revert it. Any thoughts on this? Other options I can think of are:

1) Move the check of connectionAttributes into getConnection method. This way we throw the exception(that we cannot set databaseName attribute on a data source) when getConnection is called.

2) Go back to the first solution (derby-1130-v1.diff) which just ensures that database name set using setConnectionAttributes does not get used by client driver. This is more like what is being done in embedded driver , i.e, we just ignore the databaseName attribute. Also, modify the error message to explicitly indicate "Derby datasource property" and thus differentiate it from databaseName "attribute".

3) other options ?

Please provide feedback about how to proceed on this issue. Thanks.

[David Van Couvering]

Option 2 looks good to me if it's feasible.

[Deepa Remesh]

As the current solution does not seem to be okay, can a committer please revert this change (svn revision# 430641)?

Also, I have unchecked patch available as there is no need to commit the test patch 'd1130-test-dataSourceReference.diff'. I will upload a revised patch based on option 2 in some time.

[Kathey Marsden]

I Backed out the change. I didn't see David's mail right away.

Thanks Dan for catching this. It is good to have many, many eyes on the code and always could use two more before and after commit.

Date: Fri Aug 11 13:50:29 2006
New Revision: 430891

URL: http://svn.apache.org/viewvc?rev=430891&view=rev

[Daniel John Debrunner]

Does it need to be backed out of the 10.2 branch, maybe not now, but at some point once the branch has stablized.
Or would the change to trunk just be merged up to 10.2?

[Deepa Remesh]

Attaching a patch 'd1130-v2.diff' which ensures that database name set using setConnectionAttributes does not get used by client data sources. Changes are:

1) Appends the attributes in setConnectionAttributes method to database name only if database name has been already set on the data source. This will handle both these cases successfully:

a) When database name is not set as a DataSource property - In this case, if we pass in database name as a connection attribute, it will not get used. databaseName is a required Derby DataSource property. If it is not set, we cannot get a connection using the DataSource. It will fail with the following exception:
08001 - Required Derby DataSource property databaseName not set.
So, there is no need to append the connectionAttributes to the database name variable if databaseName DataSource property is not set. This way, we can avoid using database name in case it is passed in as a connection attribute.

Without the patch, if database name was not set, the code was using "null" as database name and creating a database named null if "create=true" is specified or throwing an exception that it cannot connect to database named null.

b) When database name is set as a DataSource property - In this case, if we pass in database name as a connection attribute, it will not be used. This is because database name set as DataSource property will over-ride it. This case is correctly handled (even without the patch).

2) The exception message is changed to indicate we are referring to "Derby DataSource" property:
08001 - Required Derby DataSource property databaseName not set.

3) Adds test to jdbcapi/checkDataSource.java. Adds a new method "testClientDSConnectionAttributes" which is run only in client framework. Modifies master files.

Ran derbyall with this patch using Sun jdk 1.4.2 on Windows XP. No new failures. (6 failures also seen in tinderbox tests). Please take a look at this patch. Thanks.

[Deepa Remesh]

Marking patch'd1130-v2.diff' available for review/commit.

[David Van Couvering]

+1, this looks good, thanks for everyone's effort on this.

[Mike Matrigali]

thanks for the review david, I'll run the tests and commit if they pass. If anyone else is going to review let me know.

[Mike Matrigali]

Committed to trunk, thanks for the review David. I ran full set of tests against sun 1.4.2 jvm on XP.

m3_142:1>svn commit

Sending java\client\org\apache\derby\client\am\Connection.java
Sending java\engine\org\apache\derby\loc\messages_en.properties
Sending java\testing\org\apache\derbyTesting\functionTests\master\DerbyNetClient\checkDataSource.out
Sending java\testing\org\apache\derbyTesting\functionTests\master\DerbyNetClient\checkDataSource30.out
Sending java\testing\org\apache\derbyTesting\functionTests\tests\jdbcapi\checkDataSource.java
Transmitting file data .....
Committed revision 438122.

[Mike Matrigali]

patch merged to 10.2 as part of "mega-merge" change 439370

[Deepa Remesh]

Verified the fix is in trunk (10.3) and 10.2 branch.

Here is the release note for this issue:

PROBLEM:

Derby's client DataSources were using a wrong database name when getting a connection in the following case:
* databaseName is not set as a Derby DataSource property
* databaseName is set as a connection attribute using setConnectionAttributes method

SYMPTOM:

Instead of throwing an exception saying databaseName is a required Derby DataSource property and must be set, client driver was using "null" as database name and returning a connection to database named "null".

CAUSE:

The database name was constructed wrongly in the client driver.

SOLUTION:

This was solved by setting the internal database name property in the client driver correctly. Also ensured that databaseName set as a connection attribute will not be used by Derby's client DataSources.. This fix will be available in Derby versions 10.2 and above.

WORKAROUND:

If using release prior to version 10.2, make sure database name is set only as a DataSource property when using Derby's client DataSources.

[Rick Hillegas]

Re-opening the issue to remove 10.3 from the "fix in" list. It should be assumed that this fix appears in all releases after 10.2. Marking this as "fix in" 10.3 causes the 10.3 release notes to describe a larger delta than the delta from 10.2.

[Rick Hillegas]

This issue was fixed in 10.2. Adjusting the "fix in" field to note that the issue was fixed in 10.2 and it may just be assumed that it is fixed in 10.3 and later releases.