Derby
  1. Derby
  2. DERBY-4073

Creation/configuration of ClientXDataSource fails because of two setSsl methods

    Details

    • Issue & fix info:
      Release Note Needed

      Description

      Applications using reflection (and JavaBean conventions) have problems configuring the Derby client data sources.
      Depending on how things are done, the user may or may not see the problems.
      For instance, some applications obtain all valid data source properties and list them with their default settings. In the case of SSL, this will be "Ssl" with value "off". When the application is trying to call setSsl("off") through reflection it may invoke setSsl(int) instead of setSsl(String), failing because "off" cannot be converted to an integer. In some implementations both methods will be invoked.

      There are two ways to look at this, and I don't know which one is correct:
      o the reflection code of the third-party applications using Derby isn't written well enough.
      o Derby is to blame for the problem by providing two setSsl-methods.

      I don't know if providing overloading setters violates the JavaBean spec, or any other relevant spec we should follow.

      The easiest technical solution is to rename one of the methods or possibly making one of them private. Both of these will break existing applications using that method to configure a Derby client data source.
      Is doing this, and providing a release note, sufficient?
      Does anyone see any other solutions?

      It should be noted that in some applications, it is impossible to configure ClientConnectionPoolDataSource or ClientXADataSource to use SSL. The reasons are the problem described here and DERBY-4067. One typical class of software with this problem is application servers. A workaround is to avoid setting the SSL property, which isn't doable if you need SSL of course...

      A related issue is whether it should be allowed to set the SSL property both through the setter method(s) and as a connection attribute.

      1. releaseNote.html
        4 kB
        Kristian Waagan
      2. releaseNote.html
        4 kB
        Kristian Waagan
      3. derby-4073-1a-add_docs_and_remove_setSsl_int.diff
        4 kB
        Kristian Waagan

        Activity

        Hide
        Rick Hillegas added a comment -

        Hi Kristian,

        I'm not an expert on Java Beans. It may be that the calling application falls back on ordinary introspection to find the bean methods. There could be a bug in the the application or in the Introspector class provided with the JRE. It may be possible to hide the setSsl(int) method by providing BeanInfo classes in our public api packages. E.g., a ClientDataSource40BeanInfo class which just exposes the getters and setters we really want to publish. There is some explanation of this pattern in the BeanInfo javadoc and here: http://java.sun.com/docs/books/tutorial/javabeans/introspection/index.html This may be an annoying amount of work which still doesn't fix the problem.

        It seems to me that the Introspector or the calling application ought to be smart enough to look for the setSsl() overload which matches the return type of getSsl(). It's not clear to me that this is a Derby bug.

        Show
        Rick Hillegas added a comment - Hi Kristian, I'm not an expert on Java Beans. It may be that the calling application falls back on ordinary introspection to find the bean methods. There could be a bug in the the application or in the Introspector class provided with the JRE. It may be possible to hide the setSsl(int) method by providing BeanInfo classes in our public api packages. E.g., a ClientDataSource40BeanInfo class which just exposes the getters and setters we really want to publish. There is some explanation of this pattern in the BeanInfo javadoc and here: http://java.sun.com/docs/books/tutorial/javabeans/introspection/index.html This may be an annoying amount of work which still doesn't fix the problem. It seems to me that the Introspector or the calling application ought to be smart enough to look for the setSsl() overload which matches the return type of getSsl(). It's not clear to me that this is a Derby bug.
        Hide
        Kristian Waagan added a comment -

        Thanks for the comment, Rick.

        Even if this isn't a Derby bug, I think we should consider addressing the issue.

        The allowed values are; str_value=off|basic|peerAuthentication
        Why do we have three ways to set the same property?
        a) connectionAttributes (ssl=<str_value>)
        b) setSsl(String) ; <str_values> can be used
        c) setSsl(int) ; 0, 1 and 2 are valid values, but the input isn't validated. When getSsl() is called, invalid values will cause "off" to be returned.

        To me, it looks as if setSsl(int) is more of an internal helper method, and it shouldn't be exposed. I also find using a string value is more informative.

        Show
        Kristian Waagan added a comment - Thanks for the comment, Rick. Even if this isn't a Derby bug, I think we should consider addressing the issue. The allowed values are; str_value=off|basic|peerAuthentication Why do we have three ways to set the same property? a) connectionAttributes (ssl=<str_value>) b) setSsl(String) ; <str_values> can be used c) setSsl(int) ; 0, 1 and 2 are valid values, but the input isn't validated. When getSsl() is called, invalid values will cause "off" to be returned. To me, it looks as if setSsl(int) is more of an internal helper method, and it shouldn't be exposed. I also find using a string value is more informative.
        Hide
        Rick Hillegas added a comment -

        Hi Kristian,

        According to my interpretation of ClientBaseDataSource.setConnectionAttributes(), option (a) is forbidden and results in unpredictable behavior. Users should not use setConnectionAttributes() to set the ssl attribute.

        Neither of the overloads for setSsl() has any javadoc explaining what the legal values are. For setSsl(String), I suppose that someone can extrapolate the values from the "Setting attributes for the database connection URL" section of the Reference Guide. But how does a user figure out what the legal values of setSsl(int) are? If there is no user documentation on this overload, then the method has no contract and it ought to be ok to remove it.

        Show
        Rick Hillegas added a comment - Hi Kristian, According to my interpretation of ClientBaseDataSource.setConnectionAttributes(), option (a) is forbidden and results in unpredictable behavior. Users should not use setConnectionAttributes() to set the ssl attribute. Neither of the overloads for setSsl() has any javadoc explaining what the legal values are. For setSsl(String), I suppose that someone can extrapolate the values from the "Setting attributes for the database connection URL" section of the Reference Guide. But how does a user figure out what the legal values of setSsl(int) are? If there is no user documentation on this overload, then the method has no contract and it ought to be ok to remove it.
        Hide
        Kristian Waagan added a comment -

        Thanks again, Rick.
        Patch 1a adds some JavaDoc comments, removes the methods setSsl(int) and adds string constants for the valid values denoting SSL modes.
        I would also like to backport this fix to 10.4.

        Patch ready for review.
        I have started the regression tests

        Show
        Kristian Waagan added a comment - Thanks again, Rick. Patch 1a adds some JavaDoc comments, removes the methods setSsl(int) and adds string constants for the valid values denoting SSL modes. I would also like to backport this fix to 10.4. Patch ready for review. I have started the regression tests
        Hide
        Kristian Waagan added a comment -

        Regression tests (suites.All & derbyall) passed.

        Show
        Kristian Waagan added a comment - Regression tests (suites.All & derbyall) passed.
        Hide
        Myrna van Lunteren added a comment -

        Does this warrant getting marked as 'existing application impact'?
        i.e. could anyone have tried to use getSsl(int) sucessfully and now have to change their app?

        Show
        Myrna van Lunteren added a comment - Does this warrant getting marked as 'existing application impact'? i.e. could anyone have tried to use getSsl(int) sucessfully and now have to change their app?
        Hide
        Kristian Waagan added a comment -

        That is possible.
        To use the method, the user would have to use either brute force or inspect the source code to determine the valid values. To my knowledge, setSsl(int) isn't documented anywhere. Leaning on the statement Rick made, the valid values for setSsl(String) could be extrapolated from the documentation of connection URL attributes.
        I think we should write a release note for this fix.

        Note that if we make Derby throw an exception if the user specifies a property that has its own setter method in connectionAttributes, we definitely have to write a release note.

        Show
        Kristian Waagan added a comment - That is possible. To use the method, the user would have to use either brute force or inspect the source code to determine the valid values. To my knowledge, setSsl(int) isn't documented anywhere. Leaning on the statement Rick made, the valid values for setSsl(String) could be extrapolated from the documentation of connection URL attributes. I think we should write a release note for this fix. Note that if we make Derby throw an exception if the user specifies a property that has its own setter method in connectionAttributes, we definitely have to write a release note.
        Hide
        Kristian Waagan added a comment -

        Added first version of a release note.
        Ready for review.

        Show
        Kristian Waagan added a comment - Added first version of a release note. Ready for review.
        Hide
        Dag H. Wanvik added a comment -

        Release note, looks good. A clarification suggestion:

        • "Having two setSsl-methods caused problems for applications using
          Derby data sources, configuring the data source through
          introspection.

        suggest:

        • "Having two setSsl-methods caused choice problems for some applications using
          Derby data sources and also configuring the data source through
          introspection, since there were two method called setSsl;
          setSsl(int) and setSsl(String).
        Show
        Dag H. Wanvik added a comment - Release note, looks good. A clarification suggestion: "Having two setSsl-methods caused problems for applications using Derby data sources, configuring the data source through introspection. suggest: "Having two setSsl-methods caused choice problems for some applications using Derby data sources and also configuring the data source through introspection, since there were two method called setSsl; setSsl(int) and setSsl(String).
        Hide
        Kristian Waagan added a comment -

        Thanks, Dag.

        Updated the release note.

        Show
        Kristian Waagan added a comment - Thanks, Dag. Updated the release note.
        Hide
        Kristian Waagan added a comment -

        Committed patch 1a to trunk with revision 753176.
        Backported to the 10.4 branch with revision 753177, and to the 10.3 branch with revision 753179.

        Will perform some testing with an appserver before closing.

        Show
        Kristian Waagan added a comment - Committed patch 1a to trunk with revision 753176. Backported to the 10.4 branch with revision 753177, and to the 10.3 branch with revision 753179. Will perform some testing with an appserver before closing.
        Hide
        Myrna van Lunteren added a comment -

        I think the usage of the @code tag may not be ok in 10.3. I believe it should be possible with 10.3 to use the 1.4.2 compiler, which wouldn't support @code, right?
        At least, when I use the 1.4.2 compiler for building the javadoc, I see:
        Since revision 753179 in 10.3 I see the following javadoc warning in 10.3:
        <path_to_10.3>\java\client\org\apache\derby\jdbc\ClientBaseDataSource.java:202: warning - @code is an unknown tag.
        Can you suggest another way to improve the comment?

        Show
        Myrna van Lunteren added a comment - I think the usage of the @code tag may not be ok in 10.3. I believe it should be possible with 10.3 to use the 1.4.2 compiler, which wouldn't support @code, right? At least, when I use the 1.4.2 compiler for building the javadoc, I see: Since revision 753179 in 10.3 I see the following javadoc warning in 10.3: <path_to_10.3>\java\client\org\apache\derby\jdbc\ClientBaseDataSource.java:202: warning - @code is an unknown tag. Can you suggest another way to improve the comment?
        Hide
        Kristian Waagan added a comment -

        Replacing

        {@code string}

        with <code>string</code> is a simple solution.
        However, I'm slightly confused. When I grep the 10.3 source files, I find at least 72 occurrences of @code. Why aren't those a problem?
        Second, if I run 'ant derbydocs' with JAVA_HOME pointing to Java 1.4.2, it fails when processing the files containing Java 5.0 features (in my case I see it fail when processing ConcurrentLockSet).

        Is my ant.properties file wrong?

        Show
        Kristian Waagan added a comment - Replacing {@code string} with <code>string</code> is a simple solution. However, I'm slightly confused. When I grep the 10.3 source files, I find at least 72 occurrences of @code. Why aren't those a problem? Second, if I run 'ant derbydocs' with JAVA_HOME pointing to Java 1.4.2, it fails when processing the files containing Java 5.0 features (in my case I see it fail when processing ConcurrentLockSet). Is my ant.properties file wrong?
        Hide
        Kristian Waagan added a comment -

        Replaced @code tag on the 10.3 branch with revision 756441.
        Closing issue.

        Show
        Kristian Waagan added a comment - Replaced @code tag on the 10.3 branch with revision 756441. Closing issue.

          People

          • Assignee:
            Kristian Waagan
            Reporter:
            Kristian Waagan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development