Commons Dbcp
  1. Commons Dbcp
  2. DBCP-105

[dbcp] JNDI problems with SharedPoolDataSource

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      It is currently not possible to bind a SharedPoolDataSource into jndi and
      retrieve it again via a lookup. The problem was seen in tomcat's naming
      implementation, but is likely to be not limited to that implementation.
      The problem is: SharedPoolDataSource implements javax.naming.Referenceable via
      its parent InstanceKeyDataSource. Referenceable specifies that a getReference()
      method is implemented, which returns a javax.naming.Reference object. The
      reference object contains a reference to a factory for the object. In the
      implementation of InstanceKeyDataSource, dbcp's InstanceKeyObjectFactory is
      specified as facory. However, this is invalid as InstanceKeyObjectFactory is
      abstract and cannot be instantiated, so upon a lookup, the object cannot be
      retrieved from jndi.

      There are two obvious solutions. First, getReference() could return the correct
      factory (SharedPoolDataSourceFactory). Second, InstanceKeyDataSource could not
      implement Referenceable any more.

      The problem also occurs for dbcp's PerUserPoolDatasource. A successfull bind can
      be done using dbcp's BasicDataSource.

      I will attach a test case which shows the wrong behaviour. It depends on the
      jars naming-common-5.0.28.jar and naming-java.5.0.28.jar which can be downloaded
      from www.ibiblio.org/maven/tomcat/jars.

        Activity

        Hide
        Thomas Fischer added a comment -

        Created an attachment (id=17293)
        Junit testcase displaying the specified behaviour

        Show
        Thomas Fischer added a comment - Created an attachment (id=17293) Junit testcase displaying the specified behaviour
        Hide
        Phil Steitz added a comment -

        I agree this is a bug and it should be fixed by getting a concrete factory returned.

        Assuming we stick with <class>Factory naming convention, the current setup can
        be made to work by having InstanceKeyDataSource's getReference method pass
        getClass().getName() + "Factory"
        instead of
        InstanceKeyObjectFactory.class.getName()
        as the factory argument to the Reference constructor.

        Show
        Phil Steitz added a comment - I agree this is a bug and it should be fixed by getting a concrete factory returned. Assuming we stick with <class>Factory naming convention, the current setup can be made to work by having InstanceKeyDataSource's getReference method pass getClass().getName() + "Factory" instead of InstanceKeyObjectFactory.class.getName() as the factory argument to the Reference constructor.
        Hide
        Thomas Fischer added a comment -

        In my opinion, it would be cleaner to make the method getReference() abstract in
        InstanceKeyDataSource, and implement it in the subclasses. Then one need not
        rely on naming conventions, but can return XXXFactory.class.getName() in the
        implementations.

        Show
        Thomas Fischer added a comment - In my opinion, it would be cleaner to make the method getReference() abstract in InstanceKeyDataSource, and implement it in the subclasses. Then one need not rely on naming conventions, but can return XXXFactory.class.getName() in the implementations.
        Hide
        Phil Steitz added a comment -

        Yes. That would be cleaner.

        Show
        Phil Steitz added a comment - Yes. That would be cleaner.
        Hide
        Phil Steitz added a comment -

        But unfortunately InstanceKeyDataSource has been released and making
        getReference() abstract in InstanceKeyDataSource would technically be an
        incompatible change - "technically" because binary compatibility problem would
        only surface for those using the broken functionality. Source compatibility is
        another isssue - would bite anyone who has created their own
        InstanceKeyDataSources.

        Show
        Phil Steitz added a comment - But unfortunately InstanceKeyDataSource has been released and making getReference() abstract in InstanceKeyDataSource would technically be an incompatible change - "technically" because binary compatibility problem would only surface for those using the broken functionality. Source compatibility is another isssue - would bite anyone who has created their own InstanceKeyDataSources.
        Hide
        Thomas Fischer added a comment -

        What about change the implementation of InstanceKeyDataSource.getReference() to
        always throw an Exception instead of making it abstract? This would preserve
        binary and source compatibility, but ease finding the cause why binding to jndi
        fails.

        Show
        Thomas Fischer added a comment - What about change the implementation of InstanceKeyDataSource.getReference() to always throw an Exception instead of making it abstract? This would preserve binary and source compatibility, but ease finding the cause why binding to jndi fails.
        Hide
        Sandy McArthur (from Bugzilla import) added a comment -

        Seems to me that changing the second param of Reference in
        InstanceKeyDataSource.getReference() to the following would be a suboptimal but
        working solution.

        public Reference getReference() throws NamingException

        { Reference ref = new Reference(getClass().getName(), getClass().getName() + "Factory", null); ref.add(new StringRefAddr("instanceKey", instanceKey)); return ref; }
        Show
        Sandy McArthur (from Bugzilla import) added a comment - Seems to me that changing the second param of Reference in InstanceKeyDataSource.getReference() to the following would be a suboptimal but working solution. public Reference getReference() throws NamingException { Reference ref = new Reference(getClass().getName(), getClass().getName() + "Factory", null); ref.add(new StringRefAddr("instanceKey", instanceKey)); return ref; }
        Hide
        Sandy McArthur (from Bugzilla import) added a comment -

        Created an attachment (id=17868)
        Dbcp-getReference-fixes.patch

        My suggested fixes to InstanceKeyDataSource.getReference() and subclasses. The
        patch:

        • encourages subclasses of InstanceKeyDataSource to override getReference()
        • changes InstanceKeyDataSource.getReference() so it takes a guess at what the
          Factory class would be named. I think this version will work at least as often
          as the current version though I don't think it should be used. Removing that
          method should wait until a major release.
        • adds implementations of getReference() to PerUserPoolDataSource and
          SharedPoolDataSource.
        Show
        Sandy McArthur (from Bugzilla import) added a comment - Created an attachment (id=17868) Dbcp-getReference-fixes.patch My suggested fixes to InstanceKeyDataSource.getReference() and subclasses. The patch: encourages subclasses of InstanceKeyDataSource to override getReference() changes InstanceKeyDataSource.getReference() so it takes a guess at what the Factory class would be named. I think this version will work at least as often as the current version though I don't think it should be used. Removing that method should wait until a major release. adds implementations of getReference() to PerUserPoolDataSource and SharedPoolDataSource.
        Hide
        Phil Steitz added a comment -

        Last patch, with test case, applied. Thanks!

        Show
        Phil Steitz added a comment - Last patch, with test case, applied. Thanks!

          People

          • Assignee:
            Unassigned
            Reporter:
            Thomas Fischer
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development