codeconsole commented on code in PR #16535:
URL: https://github.com/apache/grails-core/pull/16535#discussion_r4199671377
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/HibernateDatastore.java:
##########
@@ -554,6 +559,35 @@ public void destroy() {
}
}
+ /**
+ * Closes the connection sources created for schema tenants. Each one owns
a SessionFactory,
+ * but none of them is part of {@link #connectionSources}, so closing
those does not reach them.
+ */
+ private void closeSchemaTenantConnectionSources() {
+ for (ConnectionSource<SessionFactory,
HibernateConnectionSourceSettings> tenantConnectionSource :
schemaTenantConnectionSources) {
+ try {
+ tenantConnectionSource.close();
+ } catch (IOException e) {
+ LOG.error("There was an error closing the connection source of
schema tenant [" + tenantConnectionSource.getName() + "]: " + e.getMessage(),
e);
Review Comment:
The Hibernate 7 version of this method uses placeholders; this one
concatenates. `LOG` is SLF4J here too, so it can match:
```suggestion
LOG.error("There was an error closing the connection source
of schema tenant [{}]: {}", tenantConnectionSource.getName(), e.getMessage(),
e);
```
##########
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/HibernateDatastoreDestroySpec.groovy:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.orm.hibernate
+
+import grails.gorm.MultiTenant
+import grails.gorm.annotation.Entity
+import org.grails.datastore.gorm.GormRegistry
+import org.grails.datastore.mapping.core.DatastoreUtils
+import org.grails.datastore.mapping.multitenancy.AllTenantsResolver
+import
org.grails.datastore.mapping.multitenancy.resolvers.SystemPropertyTenantResolver
+import org.hibernate.SessionFactory
+import org.hibernate.dialect.H2Dialect
+import spock.lang.Specification
+import spock.util.environment.RestoreSystemProperties
+
+/**
+ * Verifies that destroying a {@link HibernateDatastore} releases what it
created for its
+ * additional connection sources, so that nothing keeps the destroyed
datastore reachable.
+ */
+@RestoreSystemProperties
+class HibernateDatastoreDestroySpec extends Specification {
+
+ HibernateDatastore datastore
+
+ void cleanup() {
+ datastore?.destroy()
+ }
+
+ void "destroying a datastore removes the datastores of its additional data
sources from the GORM registry"() {
+ given: 'a datastore with a secondary data source'
+ datastore = new
HibernateDatastore(DatastoreUtils.createPropertyResolver([
+ 'dataSource.url' :
'jdbc:h2:mem:destroyDefaultDB;LOCK_TIMEOUT=10000',
+ 'dataSource.dbCreate' : 'create-drop',
+ 'dataSource.dialect' : H2Dialect.name,
+ 'dataSources.secondary': [url:
'jdbc:h2:mem:destroySecondaryDB;LOCK_TIMEOUT=10000'],
+ ]), DestroyMultiDataSourceBook)
+ HibernateDatastore secondary =
datastore.getDatastoreForConnection('secondary')
+ GormRegistry registry = GormRegistry.instance
+
+ expect: 'the registry resolves the secondary data source to its
datastore'
+ registry.getDatastore(DestroyMultiDataSourceBook,
'secondary').is(secondary)
+ registry.datastoresByQualifier.values().any { it.is(secondary) }
+
+ when:
+ datastore.destroy()
+
+ then: 'the registry no longer holds the secondary datastore'
+ registry.getDatastore(DestroyMultiDataSourceBook, 'secondary') == null
+ !registry.datastoresByQualifier.values().any { it.is(secondary) }
+ }
+
+ void "destroying a datastore closes the session factories of its schema
tenants"() {
+ given: 'a datastore using a schema per tenant'
+ System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, '')
+ datastore = new
HibernateDatastore(DatastoreUtils.createPropertyResolver([
+ 'grails.gorm.multiTenancy.mode' : 'SCHEMA',
+ 'grails.gorm.multiTenancy.tenantResolverClass':
DestroySchemaTenantsResolver,
+ 'dataSource.url' :
'jdbc:h2:mem:destroySchemaTenantDB;LOCK_TIMEOUT=10000',
+ 'dataSource.dbCreate' : 'update',
+ 'dataSource.dialect' : H2Dialect.name,
+ 'hibernate.hbm2ddl.auto' : 'create',
+ ]), DestroySchemaTenantBook)
+
+ and: 'a tenant added after the datastore was created'
+ datastore.addTenantForSchema('destroyTenantC')
+
+ and:
+ List<String> tenantIds = ['destroyTenantA', 'destroyTenantB',
'destroyTenantC']
+ List<SessionFactory> tenantSessionFactories = tenantIds.collect {
String tenantId ->
+ datastore.getDatastoreForConnection(tenantId).sessionFactory
+ }
+
+ expect: 'every tenant has its own open session factory'
+ tenantSessionFactories.unique(false) { System.identityHashCode(it)
}.size() == tenantIds.size()
+ !tenantSessionFactories.any { it.is(datastore.sessionFactory) }
+ tenantSessionFactories.every { it.isOpen() }
+
+ when:
+ datastore.destroy()
+
+ then: 'the tenant session factories are closed'
+ tenantSessionFactories.every { it.isClosed() }
Review Comment:
This feature checks that the tenants' session factories are closed, but not
the other half of the fix for them: their child datastores leaving the GORM
registry. The first feature covers that only for a secondary data source.
The suggestion below adds it. I ran it in both modules: it passes on this
branch, and with the base `HibernateDatastore` tenants A and B are still in
`datastoresByQualifier` after `destroy()`, so it fails there. It checks only
the tenants resolved at startup because, in Hibernate 7, `destroyTenantC`
(added with `addTenantForSchema`) is never put in the registry:
`SchemaTenantGormEnhancer.allQualifiers()` only adds the ids from
`resolveTenantIds()`.
The Hibernate 5 copy of this spec takes the same change.
```suggestion
and:
List<String> tenantIds = ['destroyTenantA', 'destroyTenantB',
'destroyTenantC']
List<SessionFactory> tenantSessionFactories = tenantIds.collect {
String tenantId ->
datastore.getDatastoreForConnection(tenantId).sessionFactory
}
List<HibernateDatastore> startupTenantDatastores =
['destroyTenantA', 'destroyTenantB'].collect { String tenantId ->
datastore.getDatastoreForConnection(tenantId)
}
GormRegistry registry = GormRegistry.instance
expect: 'every tenant has its own open session factory'
tenantSessionFactories.unique(false) { System.identityHashCode(it)
}.size() == tenantIds.size()
!tenantSessionFactories.any { it.is(datastore.sessionFactory) }
tenantSessionFactories.every { it.isOpen() }
and: 'the registry holds the datastores of the tenants resolved at
startup'
startupTenantDatastores.every { HibernateDatastore tenant ->
registry.datastoresByQualifier.values().any { it.is(tenant) }
}
when:
datastore.destroy()
then: 'the tenant session factories are closed'
tenantSessionFactories.every { it.isClosed() }
and: 'the registry no longer holds the tenant datastores'
!startupTenantDatastores.any { HibernateDatastore tenant ->
registry.datastoresByQualifier.values().any { it.is(tenant) }
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
