|
I ran derbynetclientmatsand dataSourcePermissions_net.java under jcc for this patch and all passed. The patch looks good to me, but there are still some outstanding questions/comments from Dan, so I won't commit after all as I will be off-line for the next week. Hopefully some other committer will pick this up after these issues are resolved.
Dan's comments are in email at: http://mail-archives.apache.org/mod_mbox/db-derby-dev/200507.mbox/%3c42C5952B.8090304@debrunners.com%3e Sorry for any redundancy on this issue but I would like to make sure that everyone is on the same page as per the status of this patch.
The July 1 patch makes the following changes: - Set user to default to "APP" - Set the default servername to "localhost" - Changed databaseName_ = dataSource.getDatabaseName() + attrString; to databaseName_ = dataSource.getDatabaseName() + ";" + attrString; in the connection class to avoid database names like myDBcreate=true when the setConnectionAttributes method is used. - Changed the dataSourcePermissions_net to include additional tests to check bug fixes and changed the associated.out file to match new output. - Commented sections of code associated with the fixes - Provided javadoc for new methods in dataSourcePermissions_net.java and the getPassword() method in ClientBaseDataSource.java - Changed the password, user and servername attributes to private so as to hopefully not conflict with his changes, as per his "Client data source published api javadoc cleanup" email. Can you remove these changes from your patch?
> - Changed the password, user and servername attributes to private so as to hopefully not conflict with his changes, as per his "Client data source published api javadoc cleanup" email. I need some more research to see if these changes are in fact correct. Thanks, Dan. Removed the changes proposed by Dan Debrunner for an alternate patch.
This patch makes the following changes - Sets the username to start with the default value ("APP", Derby-406 Fix) - Sets the server name to start with the default value ("localhost", Derby-410 Fix) - Adds a semi-colon in the connection class to avoid databasenames like myDBcreate=true resulting from no dividing semi-colon (Deby-409 Fix). - Add javadoced methods in the derbynet/dataSourcePermissions_net.java file to test this new functionality - Altered the output file for derbynet/dataSourcePermissions_net.java to accomodate for new tests. Running the suite derbynetclientmats caused no problems. Running derbyall cause a single problem of the form: org.apache.derby.iapi.services.context.ShutdownException: agentThread[DRDAConnThread_5,5,derby.daemons] Further investigation revealed that this was an intermittant problem for both the clean version of derby and my patched version. I ran 3 groups of test, each group running derbynet/dataSourcePermissions_net.java 20 times and these were my results (Apologies if there are formatting errors here): Clean Patched Test 1 1/20 (5%) 5/20 (25%) Test 2 3/20 (15%) 3/20 (15%) Test 3 3/20 (15%) 4/20 (20%) Total 7/60 (11.66%) 12/60 (20%) While the patched version does have a somewhat higher error rate it should be noted that as part of my added tests I add two additional calls to the dataSourcePermissions_net.java shutdown method. I'm working on committing this patch, currently looking at the shutdown exceptions.
I think the issue of intermittent failures of this test is discussed in the
Committed Revision: 209498 ( Derby406_409_410.patch)
As Phil pointed out, I do not believe that this patch fully addresses these issues so they should not be closed, though it is good progress. My concern is about the concept of defaults for DataSource properties, in these cases 406 - user - default of APP 409 - connectionAttributes - default of empty string 410 - serverName - default of localhost All these properties are set to the default when the object is created, however if the values are set to null (e.g. setConnecitonAttributes(null)) then should their values revert to the default or remain at null? Existing data sources do not have any properties that have a default. In the case that the property remains at null, then tests would be needed to ensure null is handled correctly, at least for connectionAttributes I think a NullPointerException will occur. "Existing data sources do not have any properties that have a default."
Perhaps I am misunderstanding your comment. While EmbeddedSimpleDataSource did not have any default values ClientCaseDataSource is more of a mixed bag. There already existed propertyDefaults for loginTimeout, portNumber, securityMechanism, retrieveMessageText and others. Even user had a propertyDefault of "APP" prior to the patch, it was simply not set. This said I acknowledge that it is an ongoing derby goal to have Client functionality more closely match Embedded. However, if this is the case then it might be necessary to undo considerably more then just user and servername. I meant, prior to the client data sources, Derby's embedded implementation of DataSource apis did not have the concept for default values for any of their Java Bean properties. Thus this default concept is new to Derby.
Note that the Derby *engine* provides the default of APP for the user name., not EmbeddedDataSource. Its getUser method (corrresponding to its user property) will return null for a newly created EmbeddedDataSource. So I'm not saying defaults are bad, just that we need to define what happens if the property is set to null, does it mean the property returns to its default value, or does it take on the null value? The point about no prior defaults means there is no prior behaviour to follow. One more minor issue for 409, the code currently has a default of "" (empty string) for connectionAttributes, but the functional spec
http://incubator.apache.org/derby/papers/DerbyClientSpec.html#data_source_props indicates no default for this property. It's probably related to the code that uses the property will give a NPE if connectionAttributes is null. Kathey implies in 409 that it should only be used "if set" Ported this fix from trunk to 10.1 with change number: 209498
Freeing this issue to anyone else who sees a way in which Dan's concerns about a lack of prior behaviour can be addressed.
The July 6, patch addressed the original concerns of this issue. While Dan brought up the potential issue of null handling further investigations appear to indicate null values are handled appropriately.
This did not actually go in until 10.1.2
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
Client DataSource should not require user property to be set
DERBY-406,DERBY-409andDERBY-410#Warning#
When testing this patch there were locale (en_CA vs en_US) and version (10.2.0.0 vs 10.1.0.0) issues which I believe to be unique to my machine. While this should not affect anyone who wishes to test this patch, running the derbynetclientmats suite would be advisable prior to commit.
For local the problem test was:
derbynet/sysinfo.java
For version info the problem tests were:
jdbcapi/dbMetaDataJdbc30.java
jdbcapi/metadata.java
jdbcapi/odbc_metadata.java
jdbcapi/metadataJdbc20.java