jdaugherty commented on code in PR #16299: URL: https://github.com/apache/grails-core/pull/16299#discussion_r3921014025
########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/DevToolsClassLoaders.java: ########## @@ -0,0 +1,70 @@ +/* + * 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.datastore.mapping.reflect; + +/** + * Resolves the class loader GORM and Hibernate should use when Spring Boot DevTools + * is on the classpath. + * + * <p>DevTools splits the classpath across a base loader (third-party jars) and a + * {@code RestartClassLoader} (application classes). Hibernate's JPA metamodel and + * GORM's entity registry key entities by {@link Class} identity, so domain classes + * loaded by the restart loader are "not an entity" if Hibernate resolved them + * through the base loader. Preferring the restart loader — typically the thread + * context class loader on {@code restartedMain} — keeps those identities aligned.</p> + * + * @since 8.0 + */ +public final class DevToolsClassLoaders { + + private static final String RESTART_CLASS_LOADER_SIMPLE_NAME = "RestartClassLoader"; + + private DevToolsClassLoaders() { + } + + /** + * @param classLoader the loader to inspect, possibly {@code null} + * @return {@code true} when {@code classLoader} is Spring Boot DevTools' + * {@code RestartClassLoader} + */ + public static boolean isRestartClassLoader(ClassLoader classLoader) { Review Comment: This heuristic was tolerable as a private inline check, but promoting it to a public static method in a published module makes it API surface that has to keep working. devtools' loader is always `org.springframework.boot.devtools.restart.classloader.RestartClassLoader`, so matching the FQCN first (with the simple name as a fallback for shaded/relocated cases) would be both tighter and self-documenting. As written, a user class coincidentally named `RestartClassLoader` — or `restartclassloader`, given the `equalsIgnoreCase` — matches. ########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/HibernateMappingContextConfiguration.java: ########## @@ -153,10 +154,8 @@ public void setApplicationContext(@Nullable ApplicationContext applicationContex properties.put("hibernate.enhancer.enableLazyInitialization", FALSE_LITERAL); properties.put("hibernate.enhancer.enableDirtyTracking", FALSE_LITERAL); properties.put("hibernate.enhancer.enableAssociationManagement", FALSE_LITERAL); - ClassLoader classLoader = applicationContext.getClassLoader(); - if (classLoader != null) { - properties.put(AvailableSettings.CLASSLOADERS, classLoader); - } + properties.put(AvailableSettings.CLASSLOADERS, + DevToolsClassLoaders.resolve(applicationContext.getClassLoader())); Review Comment: Behavior change worth confirming is intentional: the old code only set `CLASSLOADERS` when `applicationContext.getClassLoader()` was non-null. `resolve` never returns null, so when the app context loader is null this now writes `DevToolsClassLoaders.class.getClassLoader()` (grails-datastore-core's loader) instead of leaving the key absent and letting `buildSessionFactory` fall back to `HibernateMappingContextConfiguration.class.getClassLoader()` (hibernate7-core's loader). Same result on a flat classpath, but it's an unintended semantic change. Preserving the null check, or having `resolve` be null-preserving, would keep the old fallback. ########## grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/cfg/HibernateMappingContextConfiguration.java: ########## @@ -292,13 +283,9 @@ public SessionFactory buildSessionFactory() throws HibernateException { SessionFactory sessionFactory; Object classLoaderObject = getProperties().get(AvailableSettings.CLASSLOADERS); - ClassLoader appClassLoader; - - if (classLoaderObject instanceof ClassLoader) { - appClassLoader = (ClassLoader) classLoaderObject; - } else { - appClassLoader = getClass().getClassLoader(); - } + ClassLoader storedClassLoader = classLoaderObject instanceof ClassLoader ? + (ClassLoader) classLoaderObject : getClass().getClassLoader(); + ClassLoader appClassLoader = DevToolsClassLoaders.resolve(storedClassLoader); Review Comment: Two things on this third `resolve` call: 1. **It silently overrides an explicit setting.** Both setters already resolve, and they run on the same thread immediately before this, so this can only differ when `CLASSLOADERS` was populated by something else — a `configClass` subclass, `configuration.addProperties(...)`, or `HibernateMappingContextSessionFactoryBean.newConfiguration()`. In those cases a deliberate loader choice gets replaced by whatever the TCCL happens to be. Either drop it or document why re-deriving is intended. 2. **It's untested.** This is the one hunk that could plausibly change the resolved loader relative to the setters, and no spec exercises it. The existing `"buildSessionFactory handles classloader object when it is a ClassLoader"` test never calls `buildSessionFactory` — it puts a value into the properties and asserts it's still there. Per CLAUDE.md rules 11/13 this branch needs real coverage. ########## grails-doc/src/en/guide/gettingStarted/developmentReloading.adoc: ########## @@ -25,6 +25,8 @@ Spring Boot Developer Tools is a feature of Spring Boot that provides automatic For larger applications, you may need to adjust the default settings for optimal performance. This works well until your application becomes very large, at which point restarts may take longer or fail. +When Spring Boot Developer Tools restart is active, Grails bootstraps Hibernate with the restart class loader. That keeps domain class identity consistent with Hibernate's metamodel without moving GORM or Grails jars onto the restart loader. Without that alignment, GORM calls such as `Role.count()` and `save()` can fail with `IllegalArgumentException: Not an entity` while `get()` and HQL still work. Review Comment: The second sentence documents the symptom of a bug that is now fixed, and uses `Role.count()` — a class the reader has no context for in this section. That reads as release-note material rather than guide content. Suggest keeping only the first sentence, or dropping the paragraph and putting it in the changelog. ########## grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/cfg/HibernateMappingContextConfigurationSpec.groovy: ########## @@ -0,0 +1,114 @@ +/* + * 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.cfg + +import javax.sql.DataSource + +import org.grails.datastore.gorm.jdbc.connections.DataSourceSettings +import org.grails.datastore.mapping.core.connections.ConnectionSource +import org.hibernate.cfg.AvailableSettings +import org.springframework.context.ApplicationContext +import spock.lang.Specification + +class HibernateMappingContextConfigurationSpec extends Specification { + + ClassLoader originalContextClassLoader + + def setup() { + originalContextClassLoader = Thread.currentThread().contextClassLoader + } + + def cleanup() { + Thread.currentThread().contextClassLoader = originalContextClassLoader + } + + void "setApplicationContext uses the context class loader when DevTools is not active"() { + given: + def config = new HibernateMappingContextConfiguration() + ClassLoader cl = new URLClassLoader([] as URL[], originalContextClassLoader) + ApplicationContext appCtx = Stub(ApplicationContext) { + containsBean("dataSource") >> false + getClassLoader() >> cl + } + + when: + config.setApplicationContext(appCtx) + + then: + config.getProperties().get(AvailableSettings.CLASSLOADERS).is(cl) + } + + void "setApplicationContext prefers RestartClassLoader thread context class loader over the context class loader"() { + given: + def config = new HibernateMappingContextConfiguration() + ClassLoader contextLoader = new URLClassLoader([] as URL[], originalContextClassLoader) + ApplicationContext appCtx = Stub(ApplicationContext) { + containsBean("dataSource") >> false + getClassLoader() >> contextLoader + } + ClassLoader restartLoader = new GroovyClassLoader().parseClass( + 'class RestartClassLoader extends ClassLoader {}' + ).getDeclaredConstructor().newInstance() as ClassLoader + + when: + Thread.currentThread().contextClassLoader = restartLoader + config.setApplicationContext(appCtx) + + then: + config.getProperties().get(AvailableSettings.CLASSLOADERS).is(restartLoader) + } + + void "setDataSourceConnectionSource uses RestartClassLoader thread context class loader"() { + given: + def config = new HibernateMappingContextConfiguration() + DataSource ds = Stub(DataSource) + ClassLoader restartLoader = new GroovyClassLoader().parseClass( + 'class RestartClassLoader extends ClassLoader {}' + ).getDeclaredConstructor().newInstance() as ClassLoader + + when: + Thread.currentThread().contextClassLoader = restartLoader + config.setDataSourceConnectionSource(Stub(ConnectionSource) { + getSource() >> ds + getName() >> "default" + }) + + then: + config.getProperties().get(AvailableSettings.CLASSLOADERS).is(restartLoader) + config.getProperties().get(org.hibernate.cfg.Environment.DATASOURCE).is(ds) + } + + void "setDataSourceConnectionSource uses the connection source class loader when DevTools is not active"() { + given: + def config = new HibernateMappingContextConfiguration() + DataSource ds = Stub(DataSource) + ConnectionSource<DataSource, DataSourceSettings> connSrc = Stub(ConnectionSource) { + getName() >> "secondary" + getSource() >> ds + } + + when: + config.setDataSourceConnectionSource(connSrc) + + then: + config.dataSourceName == "secondary" + config.getProperties().containsKey(AvailableSettings.CLASSLOADERS) Review Comment: This assertion doesn't test what the feature name claims. `containsKey` plus `!= null` passes regardless of which loader was stored, including a wrong one — `resolve` could be changed to always return `DevToolsClassLoaders.class.getClassLoader()` and this would still be green. Assert the actual loader: `config.getProperties().get(AvailableSettings.CLASSLOADERS).is(connSrc.getClass().getClassLoader())`. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/DevToolsClassLoaders.java: ########## @@ -0,0 +1,70 @@ +/* + * 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.datastore.mapping.reflect; + +/** + * Resolves the class loader GORM and Hibernate should use when Spring Boot DevTools + * is on the classpath. + * + * <p>DevTools splits the classpath across a base loader (third-party jars) and a + * {@code RestartClassLoader} (application classes). Hibernate's JPA metamodel and + * GORM's entity registry key entities by {@link Class} identity, so domain classes + * loaded by the restart loader are "not an entity" if Hibernate resolved them + * through the base loader. Preferring the restart loader — typically the thread + * context class loader on {@code restartedMain} — keeps those identities aligned.</p> + * + * @since 8.0 + */ +public final class DevToolsClassLoaders { + + private static final String RESTART_CLASS_LOADER_SIMPLE_NAME = "RestartClassLoader"; + + private DevToolsClassLoaders() { + } + + /** + * @param classLoader the loader to inspect, possibly {@code null} + * @return {@code true} when {@code classLoader} is Spring Boot DevTools' + * {@code RestartClassLoader} + */ + public static boolean isRestartClassLoader(ClassLoader classLoader) { + return classLoader != null && + RESTART_CLASS_LOADER_SIMPLE_NAME.equalsIgnoreCase(classLoader.getClass().getSimpleName()); + } + + /** + * Prefer the thread context class loader when it is DevTools' + * {@code RestartClassLoader}; otherwise return {@code fallback}, or this + * class's loader when {@code fallback} is {@code null}. + * + * @param fallback the loader to use when DevTools is not active + * @return a non-null class loader + */ + @SuppressWarnings("PMD.UseProperClassLoader") + public static ClassLoader resolve(ClassLoader fallback) { Review Comment: This prefers the TCCL unconditionally, which discards a strictly better loader when `fallback` is a *descendant* of the restart loader — the child can see everything the restart loader sees plus its own classes, and returning the parent loses those. A cheap guard is to keep `fallback` when the restart loader is already in its parent chain. That doesn't weaken the intended fix: in the devtools scenario the base loader is the restart loader's *parent*, not a descendant, so the guard still returns the restart loader there. Also worth a one-line comment justifying the `@SuppressWarnings("PMD.UseProperClassLoader")` — the reason (`DevToolsClassLoaders.class.getClassLoader()` is the deliberate last-resort fallback, not a TCCL oversight) isn't obvious from the code. Nit: `resolve(x)` doesn't read well at the call sites. `preferRestartClassLoader(x)` says what it actually does. ########## grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/cfg/HibernateMappingContextConfiguration.java: ########## @@ -239,14 +236,9 @@ public SessionFactory buildSessionFactory() throws HibernateException { SessionFactory sessionFactory; Object classLoaderObject = getProperties().get(AvailableSettings.CLASSLOADERS); - ClassLoader appClassLoader; - - if (classLoaderObject instanceof ClassLoader) { - appClassLoader = (ClassLoader) classLoaderObject; - } - else { - appClassLoader = getClass().getClassLoader(); - } + ClassLoader storedClassLoader = classLoaderObject instanceof ClassLoader ? + (ClassLoader) classLoaderObject : getClass().getClassLoader(); + ClassLoader appClassLoader = DevToolsClassLoaders.resolve(storedClassLoader); Review Comment: Same as the hibernate7 counterpart: this re-resolve can only differ from what the setter stored when `CLASSLOADERS` came from somewhere other than the two setters, in which case it discards an explicit choice — and it has no test coverage. -- 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]
