DS-3803 Remove db.jndi setting from dspace.cfg

[alanorth]

As of DSpace 6.x this setting is no longer used and is not customizable by the user because it is hard coded in the Hibernate config. We should remove it so we don't confuse users. This is slightly related to the issue in DS-3434.

See: https://wiki.duraspace.org/display/DSDOC6x/Configuration+Reference
See: dspace/config/spring/api/core-hibernate.xml

[tdonohue]

It may be better to update Hibernate to use this db.jndi configuration, so that it is easily configurable in the dspace.cfg again.

I believe, that'd just be a matter of changing this line: https://github.com/DSpace/DSpace/blob/master/dspace/config/spring/api/core-hibernate.xml#L41

To be:

<property name='jndiName' value="java:comp/env/${db.jndi}"/>

I've not tested this, but other configurations in dspace.cfg are similarly pulled into this core-hibernate.xml, so I suspect it should work.

[hardyoyo]

I agree with @tdonohue, @alanorth would you consider revising this PR to instead make the change @tdonohue recommended?

[alanorth]

I've verified that your suggestion to update Hibernate to use the db.jndi setting in dspace.cfg works, @tdonohue. However! I'm a bit uneasy because the db.jndi setting was previous optional, but commented out. Now it would still be optional, but uncommented! It's not obvious that this setting must exist regardless of whether you're using it.

Truth be told, I wish this would behave like it did before and only try to use a database pool from JNDI if you've defined one.

[tdonohue]

@alanorth : what happens if you simply leave db.jndi commented out in dspace.cfg? Does an error get thrown, or does it still all work OK? The reason I ask is that, from reading JavaDocs for org.springframework.jndi.JndiObjectFactoryBean, it sounds like it should just default to using a normal database connection (i.e. the dspaceDataSource bean in that file) if the JNDI connection fails or cannot be found.

So, I was hoping that leaving db.jndi commented out would still be an OK default setting, as it should result in a failed JNDI connection. But, maybe you already tried that and it didn't work as expected?

[alanorth]

That's a good point, @tdonohue. I hadn't actually tried it with db.jndi commented out, but it does indeed cause problems, first of all with ant update:

...
test_database:
     [java] 2018-01-17 17:17:35,187 WARN  org.springframework.context.support.ClassPathXmlApplicationContext @ Exception encountered during context initialization - cancelling refresh attempt: org.springframework.beans.factory.BeanDefinitionStoreException: Invalid bean definition with name 'dataSource' defined in file [/Users/aorth/dspace6/config/spring/api/core-hibernate.xml]: Could not resolve placeholder 'db.jndi' in string value "${db.jndi}"; nested exception is java.lang.IllegalArgumentException: Could not resolve placeholder 'db.jndi' in string value "${db.jndi}"
     [java] 2018-01-17 17:17:35,188 ERROR org.dspace.servicemanager.DSpaceServiceManager @ Failed to startup the DSpace Service Manager: failure starting up spring service manager: Invalid bean definition with name 'dataSource' defined in file [/Users/aorth/dspace6/config/spring/api/core-hibernate.xml]: Could not resolve placeholder 'db.jndi' in string value "${db.jndi}"; nested exception is java.lang.IllegalArgumentException: Could not resolve placeholder 'db.jndi' in string value "${db.jndi}"
     [java] java.lang.IllegalStateException: failure starting up spring service manager: Invalid bean definition with name 'dataSource' defined in file [/Users/aorth/dspace6/config/spring/api/core-hibernate.xml]: Could not resolve placeholder 'db.jndi' in string value "${db.jndi}"; nested exception is java.lang.IllegalArgumentException: Could not resolve placeholder 'db.jndi' in string value "${db.jndi}"

[tdonohue]

Ok, thanks for the quick test there @alanorth.

In that case, I'm not seeing a better option other than simply defaulting the db.jndi to be uncommented. That said, we could update the comments in dspace.cfg to make the new role more clear...something like:

# Name of the JNDI database connection pool to utilize, if configured
# in your servlet container (e.g. Tomcat). If this JNDI resource exists
# in your servlet container's configuration, then the above "db.*"
# settings will be ignored. Otherwise, this setting does nothing.
db.jndi = jdbc/dspace

I'd also like to get @mwoodiupui's opinion on this new default, as I know he uses JNDI on a regular basis and helped get this setup properly in Hibernate to begin with in #1219 (Note: some of the comments in 1219 are now no longer 100% accurate, as at the point that 1219 was merged, all db settings had been temporarily removed from dspace.cfg. They have since been restored, but we neglected to restore db.jndi to usefulness it seems.)

[alanorth]

My fix to remove db.jndi from dspace.cfg was trivial, and only meant to reflect the current 6.x reality, but I think it would be better to restore the previous functionality where DSpace only tried to use a pre-configured database pool if db.jndi is uncommented.

I'm curious to hear @mwoodiupui thoughts too. By the way, I've also started using JNDI in DSpace 5.x and it was during my testing of DSpace 6.2 last week where I stumbled on DS-3434—that DSpace is actually not even able to start if a database pool is provided from JNDI. This is no longer trivial!

[mwoodiupui]

I definitely do not want to lose the JNDI support! @tdonohue 's default looks reasonable. Here I leave it set to that and match the <ResourceLink> to the appropriate <Resource>, which works smoothly even with multiple DSpace instances in a single Tomcat each looking for (its own) jdbc/dspace.

Please see DS-3434 which will, I think, be needed to make this work in 6+.

[alanorth]

Hold on, nobody said anything about removing JNDI support—my pull request was to merely remove the setting from dspace.cfg because it is literally not used, but also because DSpace 6.x seems to have been designed like that. According to the DSpace 6.x Configuration Reference:

Earlier releases of DSpace provided a configuration property db.jndi to specify the name to be looked up, but that has been removed. The name is specified in config/spring/api/core-hibernate.xml if you really need to change it.

I'm glad that @mwoodiupui is making progress figuring out this rather serious regression (DS-3434).

[mwoodiupui]

Yes, I should read more deeply instead of going with my initial "hey, I need that!" reaction.
If we need for this to be configurable, then @tdonohue's suggestion seems good. But as I have thought about it, I haven't come up with a scenario in which we need to configure this in DSpace. It seems to me that the Servlet spec. is well designed in this area, with all the flexibility we need for any case I can think of. So, if nobody has a use case that cannot be addressed with the standard Resource/ResourceLink feature, then I would suggest going with the original idea and just retiring dspace.jndi.

[alanorth]

I agree, @mwoodiupui. I was happy to learn that DSpace 6.x just tries to find it and falls back to the other db.* settings from dspace.cfg if it doesn't. Also, it's not so bad that the name is hard coded as jdbc/dspace because you can configure it globally with a descriptive name in Tomcat's server.xml and then use jdbc/dspace in each context.

For example, I've been doing this in development lately. Configure separate pools for "web" and "api" requests in server.xml:

    <Resource name="jdbc/dspaceWeb" auth="Container" type="javax.sql.DataSource"
          driverClassName="org.postgresql.Driver"
          url="jdbc:postgresql://localhost:5432/dspace?ApplicationName=dspaceWeb"
...
          testOnBorrow='true' />

    <Resource name="jdbc/dspaceApi" auth="Container" type="javax.sql.DataSource"
          driverClassName="org.postgresql.Driver"
          url="jdbc:postgresql://localhost:5432/dspace?ApplicationName=dspaceApi"
...
          testOnBorrow='true' />

Then each web application context, ie ROOT.xml:

<Context docBase="[dspace]/webapps/xmlui">                                                                                                                             
  <ResourceLink global="jdbc/dspaceWeb" name="jdbc/dspace" type="javax.sql.DataSource"/>                                                                                          
</Context>

I can name them anything I want at the global level (and notice that I use JDBC trickery to set the pool name using ?ApplicationName=, which sets the application name used in pg_stat_activity!).

[tdonohue]

After discussion with @mwoodiupui in today's DevMtg, I'll give the original change suggested by @alanorth (to simply remove the db.jndi) my support 👍

In the DevMtg, we've decided that it's better to enhance the documentation on using DSpace 6 + JNDI. @mwoodiupui volunteered to take the lead on that (and hopefully @alanorth can provide feedback/review at a minimum).

DevMtg logs: http://irclogs.duraspace.org/index.php?date=2018-01-24

So, this PR looks good to me, provided we enhance our 6.x Documentation around how to setup DSpace + JNDI properly: https://wiki.duraspace.org/display/DSDOC6x/

[alanorth]

Perfect, @tdonohue. I'm happy to help with the documentation on setting up Tomcat with JNDI for DSpace. I spent quite a bit of time writing my verbose message to the dspace-tech mailing list a few weeks ago with the thought that I'd eventually need to document the process in a blog post or wiki page eventually anyways.

[mwoodiupui]

+1 by inspection

[alanorth]

Happy this is moving forward. For the record, @mwoodiupui and I added quite a bit more notes on setting up DSpace with JNDI on the DSpace 6.x Installing DSpace wiki page, covering use cases, JDBC driver installation, and Tomcat configuration. Looking good!