matrei commented on code in PR #15568: URL: https://github.com/apache/grails-core/pull/15568#discussion_r3452200617
########## grails-data-hibernate7/grails-plugin/src/main/groovy/grails/plugin/hibernate/commands/SchemaExportCommand.groovy: ########## @@ -4,14 +4,14 @@ * 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 + * '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 + * 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY Review Comment: License header changed? ########## grails-data-hibernate7/grails-plugin/src/main/groovy/grails/test/hibernate/HibernateSpec.groovy: ########## @@ -56,36 +74,124 @@ abstract class HibernateSpec extends Specification { @Shared @AutoCleanup HibernateDatastore hibernateDatastore @Shared PlatformTransactionManager transactionManager + @Shared HibernateProxyHandler proxyHandler = new HibernateProxyHandler() + @Shared @AutoCleanup('close') ApplicationContext applicationContext Review Comment: `close` is the default method used and does not need to be specified in `@AutoCleanup`. ########## grails-data-hibernate7/grails-plugin/src/main/groovy/grails/test/hibernate/HibernateSpec.groovy: ########## @@ -4,25 +4,28 @@ * 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 + * '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 + * 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY Review Comment: License header changed? ########## grails-data-hibernate7/grails-plugin/src/main/groovy/grails/plugin/hibernate/HibernateGrailsPlugin.groovy: ########## @@ -4,14 +4,14 @@ * 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 + * '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 + * 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * KIND, either express or implied. See the License for the Review Comment: License header changed? ########## grails-data-hibernate7/grails-plugin/src/main/groovy/grails/test/hibernate/HibernateSpec.groovy: ########## @@ -56,36 +74,124 @@ abstract class HibernateSpec extends Specification { @Shared @AutoCleanup HibernateDatastore hibernateDatastore @Shared PlatformTransactionManager transactionManager + @Shared HibernateProxyHandler proxyHandler = new HibernateProxyHandler() + @Shared @AutoCleanup('close') ApplicationContext applicationContext - void setupSpec() { + static class TestGrailsBytecodeProvider extends GrailsBytecodeProvider { - List<PropertySourceLoader> propertySourceLoaders = SpringFactoriesLoader.loadFactories(PropertySourceLoader, getClass().getClassLoader()) - ResourceLoader resourceLoader = new DefaultResourceLoader() - MutablePropertySources propertySources = new MutablePropertySources() - PropertySourceLoader ymlLoader = propertySourceLoaders.find { it.getFileExtensions().toList().contains('yml') } - if (ymlLoader) { - load(resourceLoader, ymlLoader, 'application.yml').each { - propertySources.addLast(it) - } - } - PropertySourceLoader groovyLoader = propertySourceLoaders.find { it.getFileExtensions().toList().contains('groovy') } - if (groovyLoader) { - load(resourceLoader, groovyLoader, 'application.groovy').each { - propertySources.addLast(it) + @Override + @CompileStatic(TypeCheckingMode.SKIP) + protected ByteBuddyProxyHelper createProxyHelper() { + try { + def byteBuddyStateClass = Class.forName('org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState') + def byteBuddyStateConstructor = byteBuddyStateClass.getDeclaredConstructor() + byteBuddyStateConstructor.setAccessible(true) + def byteBuddyState = byteBuddyStateConstructor.newInstance() + return new ByteBuddyProxyHelper(byteBuddyState as org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState) + } catch (e) { + throw new RuntimeException('Failed to instantiate ByteBuddyState using reflection', e) } } - propertySources.addFirst(new MapPropertySource('defaults', getConfiguration())) - Config config = new PropertySourcesConfig(propertySources) + } + + @CompileStatic(TypeCheckingMode.SKIP) + void setupSpec() { + Config config List<Class> domainClasses = getDomainClasses() - String packageName = getPackageToScan(config) + HibernateDatastoreSpringInitializer initializer + + if (applicationContext == null) { + System.out.println('HibernateSpec: applicationContext is null, creating new one.') + List<PropertySourceLoader> propertySourceLoaders = SpringFactoriesLoader.loadFactories(PropertySourceLoader, getClass().getClassLoader()) + ResourceLoader resourceLoader = new DefaultResourceLoader() + MutablePropertySources propertySources = new MutablePropertySources() + PropertySourceLoader ymlLoader = propertySourceLoaders.find { it.getFileExtensions().toList().contains('yml') } + if (ymlLoader) { + load(resourceLoader, ymlLoader, 'application.yml').each { + propertySources.addLast(it) + } + } + PropertySourceLoader groovyLoader = propertySourceLoaders.find { it.getFileExtensions().toList().contains('groovy') } + if (groovyLoader) { + load(resourceLoader, groovyLoader, 'application.groovy').each { + propertySources.addLast(it) + } + } + propertySources.addFirst(new MapPropertySource('defaults', getConfiguration())) + config = new PropertySourcesConfig(propertySources) + PropertyResolver propertyResolver = DatastoreUtils.preparePropertyResolver(config) + + if (!domainClasses) { + String packageName = getPackageToScan(config) + initializer = new HibernateDatastoreSpringInitializer(propertyResolver, packageName) + } else { + initializer = new HibernateDatastoreSpringInitializer(propertyResolver, domainClasses) + } + + initializer.beanDefinitions = { -> + dataSource(org.springframework.jdbc.datasource.DriverManagerDataSource) { + driverClassName = 'org.h2.Driver' + url = 'jdbc:h2:mem:test;DB_CLOSE_DELAY=-1' + username = 'sa' + password = '' + } + hibernateBytecodeProvider(TestGrailsBytecodeProvider) + } - if (!domainClasses) { - Package packageToScan = Package.getPackage(packageName) ?: getClass().getPackage() - hibernateDatastore = new HibernateDatastore((PropertyResolver) config, packageToScan) + applicationContext = initializer.configure() } else { - hibernateDatastore = new HibernateDatastore((PropertyResolver) config, domainClasses as Class[]) + System.out.println("HibernateSpec: applicationContext already exists (${applicationContext.class.name}), registering beans.") Review Comment: Do we need to print a debug message here? ########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/proxy/GrailsBytecodeProvider.java: ########## @@ -0,0 +1,75 @@ +/* + * 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.proxy; + +import java.util.Map; + +import org.hibernate.bytecode.enhance.spi.EnhancementContext; +import org.hibernate.bytecode.enhance.spi.Enhancer; +import org.hibernate.bytecode.spi.BytecodeProvider; +import org.hibernate.bytecode.spi.ProxyFactoryFactory; +import org.hibernate.bytecode.spi.ReflectionOptimizer; +import org.hibernate.property.access.spi.PropertyAccess; +import org.hibernate.proxy.pojo.bytebuddy.ByteBuddyProxyHelper; + +/** + * A {@link BytecodeProvider} implementation for Hibernate 7 that provides Groovy-aware proxies. + * + * @author Graeme Rocher + * @since 7.0 + */ Review Comment: ? ########## grails-data-hibernate7/grails-plugin/src/main/groovy/grails/plugin/hibernate/HibernateGrailsPlugin.groovy: ########## Review Comment: Grails Plugin metadata is not updated. ########## grails-data-hibernate7/grails-plugin/src/main/groovy/grails/test/hibernate/HibernateSpec.groovy: ########## @@ -56,36 +74,124 @@ abstract class HibernateSpec extends Specification { @Shared @AutoCleanup HibernateDatastore hibernateDatastore @Shared PlatformTransactionManager transactionManager + @Shared HibernateProxyHandler proxyHandler = new HibernateProxyHandler() + @Shared @AutoCleanup('close') ApplicationContext applicationContext - void setupSpec() { + static class TestGrailsBytecodeProvider extends GrailsBytecodeProvider { - List<PropertySourceLoader> propertySourceLoaders = SpringFactoriesLoader.loadFactories(PropertySourceLoader, getClass().getClassLoader()) - ResourceLoader resourceLoader = new DefaultResourceLoader() - MutablePropertySources propertySources = new MutablePropertySources() - PropertySourceLoader ymlLoader = propertySourceLoaders.find { it.getFileExtensions().toList().contains('yml') } - if (ymlLoader) { - load(resourceLoader, ymlLoader, 'application.yml').each { - propertySources.addLast(it) - } - } - PropertySourceLoader groovyLoader = propertySourceLoaders.find { it.getFileExtensions().toList().contains('groovy') } - if (groovyLoader) { - load(resourceLoader, groovyLoader, 'application.groovy').each { - propertySources.addLast(it) + @Override + @CompileStatic(TypeCheckingMode.SKIP) + protected ByteBuddyProxyHelper createProxyHelper() { + try { + def byteBuddyStateClass = Class.forName('org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState') + def byteBuddyStateConstructor = byteBuddyStateClass.getDeclaredConstructor() + byteBuddyStateConstructor.setAccessible(true) + def byteBuddyState = byteBuddyStateConstructor.newInstance() + return new ByteBuddyProxyHelper(byteBuddyState as org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState) + } catch (e) { Review Comment: - Should we be using a class from an `internal` package? - Why are we instantiating it reflectively? ########## grails-data-hibernate7/grails-plugin/src/main/groovy/grails/test/hibernate/HibernateSpec.groovy: ########## Review Comment: This class should probably be moved to a `testFixtures` source set. ########## grails-data-hibernate7/grails-plugin/src/main/groovy/grails/test/hibernate/HibernateSpec.groovy: ########## @@ -56,36 +74,124 @@ abstract class HibernateSpec extends Specification { @Shared @AutoCleanup HibernateDatastore hibernateDatastore @Shared PlatformTransactionManager transactionManager + @Shared HibernateProxyHandler proxyHandler = new HibernateProxyHandler() + @Shared @AutoCleanup('close') ApplicationContext applicationContext - void setupSpec() { + static class TestGrailsBytecodeProvider extends GrailsBytecodeProvider { - List<PropertySourceLoader> propertySourceLoaders = SpringFactoriesLoader.loadFactories(PropertySourceLoader, getClass().getClassLoader()) - ResourceLoader resourceLoader = new DefaultResourceLoader() - MutablePropertySources propertySources = new MutablePropertySources() - PropertySourceLoader ymlLoader = propertySourceLoaders.find { it.getFileExtensions().toList().contains('yml') } - if (ymlLoader) { - load(resourceLoader, ymlLoader, 'application.yml').each { - propertySources.addLast(it) - } - } - PropertySourceLoader groovyLoader = propertySourceLoaders.find { it.getFileExtensions().toList().contains('groovy') } - if (groovyLoader) { - load(resourceLoader, groovyLoader, 'application.groovy').each { - propertySources.addLast(it) + @Override + @CompileStatic(TypeCheckingMode.SKIP) + protected ByteBuddyProxyHelper createProxyHelper() { + try { + def byteBuddyStateClass = Class.forName('org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState') + def byteBuddyStateConstructor = byteBuddyStateClass.getDeclaredConstructor() + byteBuddyStateConstructor.setAccessible(true) + def byteBuddyState = byteBuddyStateConstructor.newInstance() + return new ByteBuddyProxyHelper(byteBuddyState as org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState) + } catch (e) { + throw new RuntimeException('Failed to instantiate ByteBuddyState using reflection', e) } } - propertySources.addFirst(new MapPropertySource('defaults', getConfiguration())) - Config config = new PropertySourcesConfig(propertySources) + } + + @CompileStatic(TypeCheckingMode.SKIP) + void setupSpec() { + Config config List<Class> domainClasses = getDomainClasses() - String packageName = getPackageToScan(config) + HibernateDatastoreSpringInitializer initializer + + if (applicationContext == null) { + System.out.println('HibernateSpec: applicationContext is null, creating new one.') Review Comment: Do we need to print a debug message here? ########## grails-data-hibernate7/grails-plugin/src/main/groovy/org/grails/plugin/hibernate/support/HibernatePersistenceContextInterceptor.java: ########## @@ -97,47 +98,48 @@ public void destroy() { try { disconnected.clear(); SessionFactoryUtils.closeSession(holder.getSession()); - } - catch (RuntimeException ex) { + } catch (RuntimeException ex) { LOG.error("Unexpected exception on closing Hibernate Session", ex); } } public void disconnect() { - if (getSessionFactory() == null) return; - try { - disconnected.add( - getSession(false).disconnect() - ); - - } - catch (Exception e) { - // no session ignore - } + throw new UnsupportedOperationException("disconnect is not supported by Hibernate 6"); } public void reconnect() { Review Comment: Should we use `Hibernate 6` in the message? ########## grails-data-hibernate7/grails-plugin/src/main/groovy/grails/test/hibernate/HibernateSpec.groovy: ########## @@ -108,8 +214,43 @@ abstract class HibernateSpec extends Specification { /** * @return The configuration */ - Map getConfiguration() { - Collections.singletonMap(Settings.SETTING_DB_CREATE, 'create-drop') + Map<String,Object> getConfiguration() { + [ + (Settings.SETTING_DB_CREATE): 'create-drop', + 'hibernate.proxy_factory_class': 'org.grails.orm.hibernate.proxy.ByteBuddyGroovyProxyFactory', + 'hibernate.dialect': 'org.hibernate.dialect.H2Dialect', + 'jakarta.persistence.validation.mode': 'none' + ] as Map<String, Object> + } + + @CompileStatic(TypeCheckingMode.SKIP) Review Comment: Why `TypeCheckingMode.SKIP`? ########## grails-data-hibernate7/grails-plugin/src/main/groovy/grails/test/hibernate/HibernateSpec.groovy: ########## @@ -56,36 +74,124 @@ abstract class HibernateSpec extends Specification { @Shared @AutoCleanup HibernateDatastore hibernateDatastore @Shared PlatformTransactionManager transactionManager + @Shared HibernateProxyHandler proxyHandler = new HibernateProxyHandler() + @Shared @AutoCleanup('close') ApplicationContext applicationContext - void setupSpec() { + static class TestGrailsBytecodeProvider extends GrailsBytecodeProvider { - List<PropertySourceLoader> propertySourceLoaders = SpringFactoriesLoader.loadFactories(PropertySourceLoader, getClass().getClassLoader()) - ResourceLoader resourceLoader = new DefaultResourceLoader() - MutablePropertySources propertySources = new MutablePropertySources() - PropertySourceLoader ymlLoader = propertySourceLoaders.find { it.getFileExtensions().toList().contains('yml') } - if (ymlLoader) { - load(resourceLoader, ymlLoader, 'application.yml').each { - propertySources.addLast(it) - } - } - PropertySourceLoader groovyLoader = propertySourceLoaders.find { it.getFileExtensions().toList().contains('groovy') } - if (groovyLoader) { - load(resourceLoader, groovyLoader, 'application.groovy').each { - propertySources.addLast(it) + @Override + @CompileStatic(TypeCheckingMode.SKIP) + protected ByteBuddyProxyHelper createProxyHelper() { + try { + def byteBuddyStateClass = Class.forName('org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState') + def byteBuddyStateConstructor = byteBuddyStateClass.getDeclaredConstructor() + byteBuddyStateConstructor.setAccessible(true) + def byteBuddyState = byteBuddyStateConstructor.newInstance() + return new ByteBuddyProxyHelper(byteBuddyState as org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState) + } catch (e) { + throw new RuntimeException('Failed to instantiate ByteBuddyState using reflection', e) } } - propertySources.addFirst(new MapPropertySource('defaults', getConfiguration())) - Config config = new PropertySourcesConfig(propertySources) + } + + @CompileStatic(TypeCheckingMode.SKIP) + void setupSpec() { + Config config List<Class> domainClasses = getDomainClasses() - String packageName = getPackageToScan(config) + HibernateDatastoreSpringInitializer initializer + + if (applicationContext == null) { + System.out.println('HibernateSpec: applicationContext is null, creating new one.') + List<PropertySourceLoader> propertySourceLoaders = SpringFactoriesLoader.loadFactories(PropertySourceLoader, getClass().getClassLoader()) + ResourceLoader resourceLoader = new DefaultResourceLoader() + MutablePropertySources propertySources = new MutablePropertySources() + PropertySourceLoader ymlLoader = propertySourceLoaders.find { it.getFileExtensions().toList().contains('yml') } + if (ymlLoader) { + load(resourceLoader, ymlLoader, 'application.yml').each { + propertySources.addLast(it) + } + } + PropertySourceLoader groovyLoader = propertySourceLoaders.find { it.getFileExtensions().toList().contains('groovy') } + if (groovyLoader) { + load(resourceLoader, groovyLoader, 'application.groovy').each { + propertySources.addLast(it) + } + } + propertySources.addFirst(new MapPropertySource('defaults', getConfiguration())) + config = new PropertySourcesConfig(propertySources) + PropertyResolver propertyResolver = DatastoreUtils.preparePropertyResolver(config) + + if (!domainClasses) { + String packageName = getPackageToScan(config) + initializer = new HibernateDatastoreSpringInitializer(propertyResolver, packageName) + } else { + initializer = new HibernateDatastoreSpringInitializer(propertyResolver, domainClasses) + } + + initializer.beanDefinitions = { -> + dataSource(org.springframework.jdbc.datasource.DriverManagerDataSource) { + driverClassName = 'org.h2.Driver' + url = 'jdbc:h2:mem:test;DB_CLOSE_DELAY=-1' + username = 'sa' + password = '' + } + hibernateBytecodeProvider(TestGrailsBytecodeProvider) + } - if (!domainClasses) { - Package packageToScan = Package.getPackage(packageName) ?: getClass().getPackage() - hibernateDatastore = new HibernateDatastore((PropertyResolver) config, packageToScan) + applicationContext = initializer.configure() } else { - hibernateDatastore = new HibernateDatastore((PropertyResolver) config, domainClasses as Class[]) + System.out.println("HibernateSpec: applicationContext already exists (${applicationContext.class.name}), registering beans.") + // Context already exists (e.g. from ControllerUnitTest), register our beans into it + try { + config = applicationContext.getBean('grailsConfig', Config) + } catch (e) { + // Fallback: create a new config if grailsConfig bean is missing + System.out.println('HibernateSpec: grailsConfig bean not found, creating fallback.') Review Comment: Do we need to print a debug message here? ########## grails-data-hibernate7/core/src/main/groovy/grails/gorm/hibernate/HibernateEntity.groovy: ########## Review Comment: Are the method javadoc up-to-date in this class (GString requirement)? I do not like the renaming of the methods, for example `findAllWithSql` -> `findAllWithNativeSql`. Sql is Sql. What is native Sql? I suggest we revert that, unless there has been a broader discussion/consensus around the change. ########## grails-data-hibernate7/grails-plugin/src/main/groovy/org/grails/plugin/hibernate/support/HibernatePersistenceContextInterceptor.java: ########## @@ -97,47 +98,48 @@ public void destroy() { try { disconnected.clear(); SessionFactoryUtils.closeSession(holder.getSession()); - } - catch (RuntimeException ex) { + } catch (RuntimeException ex) { LOG.error("Unexpected exception on closing Hibernate Session", ex); } } public void disconnect() { - if (getSessionFactory() == null) return; - try { - disconnected.add( - getSession(false).disconnect() - ); - - } - catch (Exception e) { - // no session ignore - } + throw new UnsupportedOperationException("disconnect is not supported by Hibernate 6"); Review Comment: Should we use `Hibernate 6` in the message? -- 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]
