Register the default dataSource bean without dataSource configuration

[matrei]

Fixes #16455

With GORM for Hibernate, an application without a dataSource block got no dataSource bean and no DataSource bean at all, although the datastore created and used a default data source (jdbc:h2:mem:grailsDB). Any dataSource block, even one that only repeats a default, made the bean appear.

HibernateDatastoreSpringInitializer.configureDataSources only added the default connection source to the names HibernateDatastoreConnectionSourcesRegistrar registers beans for when the dataSource map was non-empty. It now always includes it, as the config == null branch already did, since HibernateDatastore always creates the default connection source. Additional dataSources are added as before.

Always including the default exposed a second problem. configureForDataSource(DataSource) registers the given data source as a dataSource singleton before the registrar runs, and the registrar only checked for an existing bean definition, so it replaced the given data source with its own. The registrar now skips a data source name that is already in use (BeanDefinitionRegistry.isBeanNameInUse), which also covers singletons and aliases. Before this change, the same replacement happened whenever configureForDataSource was combined with a dataSource block.

The change is applied to both grails-data-hibernate7 and grails-data-hibernate5, which had the same code.

The DataSource section of the guide now says that the default data source is always available as the dataSource bean, also without a dataSource block.

Tests

In both Hibernate modules:

• HibernateDatastoreSpringInitializerSpec:
  • with no data source configuration, dataSource is the only DataSource bean, it is the datastore's default data source, and it connects to jdbc:h2:mem:grailsDB
  • with only dataSources.books configured, both dataSource and dataSource_books are registered
  • the data source passed to configureForDataSource remains the dataSource bean
• HibernateDatastoreConnectionSourcesRegistrarSpec: a dataSource registered beforehand as a bean definition or as a singleton is kept

Without the fix, the first two initializer features fail (checked with Hibernate 7). With the old containsBeanDefinition check in the registrar, the configureForDataSource feature fails in both modules, and the singleton registrar feature fails (checked with Hibernate 7).

[jdaugherty]

This is now a LinkedHashSet instead of a HashSet. It's a minor detail, but probably worth covering since you're changing the specific type of this.dataSources.

[matrei]

Good point. [ConnectionSource.DEFAULT] as Set<String> gives a LinkedHashSet, which is what the dataSources field initialiser and the config == null branch already produced. Only the branch for a non-null configuration used a HashSet. The declared type of dataSources stays Set<String>.

With insertion order, the default connection source now always comes first, followed by the additional data sources in the order the configuration returns the dataSources map. Before, the order was whatever HashSet gave. The names are only iterated to register the dataSource_<name>, sessionFactory_<name> and transactionManager_<name> beans, so nothing depends on the order.

I've added a feature to HibernateDatastoreSpringInitializerSpec in both modules that checks the contents of dataSources with no configuration, with only dataSource, with only dataSources, and with both, and that the default connection source comes first.

[bito-code-review]

The change from HashSet to LinkedHashSet for dataSourceNames ensures that the order of data sources is preserved during initialization. This is appropriate because the order of connection sources can be significant in Grails/GORM configuration, and LinkedHashSet provides predictable iteration order while maintaining the uniqueness guarantees of a Set.

grails-data-hibernate5/grails-plugin/src/main/groovy/grails/orm/bootstrap/HibernateDatastoreSpringInitializer.groovy

-        Set<String> dataSourceNames = new HashSet<String>()
+        // The datastore always creates the default connection source, whether or not it is configured
+        Set<String> dataSourceNames = [ConnectionSource.DEFAULT] as Set<String>

[jdaugherty]

This is only true when gorm is applied, yes? How would you disable the datasource? Is there no way after this PR?

[matrei]

Yes, only when GORM for Hibernate (grails-data-hibernate7 or grails-data-hibernate5) is applied. Without it, DataSourceGrailsPlugin still registers a dataSource bean only when the application configures one, so nothing changes there. I've reworded the sentence to make that clearer.

As for disabling it: there was no way to do that before this PR either. HibernateDatastore always creates the default connection source, configured or not. In the issue's reproduction, an application without a dataSource block has a working HikariDataSource for jdbc:h2:mem:grailsDB behind hibernateDatastore.connectionSources.defaultConnectionSource, and GORM uses it. Before, that data source just wasn't registered as a bean. This PR only registers the data source GORM already creates and uses. It doesn't add one.

Without the H2 driver on the classpath, this default data source can't connect. That doesn't change with this PR, and the guide now says it needs the H2 driver. I checked with H2 excluded from the test runtime classpath and no dataSource block, before and after this PR, with the same results:

• without hibernate.dialect, startup fails when hibernateDatastore is created ("Unable to determine Dialect without JDBC metadata"), because Hibernate needs a connection to detect the dialect
• with hibernate.dialect set, the application starts, and the first real query fails with ClassNotFoundException: org.h2.Driver

The only difference is that the dataSource bean now exists in the second case. Creating it doesn't open a connection, so it can't make startup fail.

An application that defines its own dataSource bean keeps it. The registrar leaves a dataSource name that is already in use alone, and with this PR that includes singletons. The only way to not have a default data source is to not apply GORM for Hibernate.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.2788%. Comparing base (d13aa33) to head (d8a001d).
⚠️ Report is 59 commits behind head on 8.0.x.

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #16458        +/-   ##
==================================================
+ Coverage     58.9274%   59.2788%   +0.3513%     
- Complexity      24440      24770       +330     
==================================================
  Files            2174       2170         -4     
  Lines          107366     108118       +752     
  Branches        19557      19707       +150     
==================================================
+ Hits            63268      64091       +823     
+ Misses          35235      35151        -84     
- Partials         8863       8876        +13     

see 44 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jdaugherty]

Let's be explicit on the type if we're going to change it?

[jdaugherty]

@matrei for some reason my comments were removed when submitting them pending. 2 concerns here:

1. if we're going to change the type (and I think it's for the better), can we please be explicit about that by using LinkedHashSet instead of Set?
2. originally you could disable a grails plugin and it's beans would not be wired, did we break that in Grails 8? That was supposed to be how you disable hibernate.

[jdaugherty]

Going to go ahead and merge this, let's iterate if you disagree with the changes.