jdaugherty commented on code in PR #16625: URL: https://github.com/apache/grails-core/pull/16625#discussion_r4265121484
########## grails-data-hibernate5/core/src/test/groovy/grails/gorm/tests/proxy/traitinitializer/TraitInitializerProxySpec.groovy: ########## @@ -0,0 +1,98 @@ +/* + * 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 grails.gorm.tests.proxy.traitinitializer + +import grails.gorm.annotation.Entity +import org.hibernate.Hibernate +import org.grails.orm.hibernate.HibernateDatastore +import spock.lang.AutoCleanup +import spock.lang.Issue +import spock.lang.Shared +import spock.lang.Specification + +/** + * Regression test for the construction of Hibernate proxies for Groovy entities whose trait + * declares fields with default values. + * + * Groovy generates a static {@code $init$} helper for such traits and calls it from the entity + * constructor. When Hibernate builds the lazy proxy, the proxy constructor runs while the + * interceptor has not been assigned yet; the generated proxy intercepts {@code getMetaClass()} + * (Groovy 3+ no longer marks it as synthetic, so Hibernate's synthetic-only filter does not + * exclude it) and the trait initializer fails with a {@code NullPointerException}. + * + * {@code Domain.load()} must return a usable, uninitialized proxy for both entities. + */ +@Issue('https://github.com/apache/grails-core/issues/16600') +class TraitInitializerProxySpec extends Specification { + + @Shared @AutoCleanup HibernateDatastore datastore = new HibernateDatastore(getClass().getPackage()) + + void "load() of an entity whose trait declares default values creates the proxy"() { Review Comment: This stops at `load()` returning an uninitialized proxy, which is the symptom from the issue, but it does not show the proxy works afterwards: nothing dispatches through the new `getMetaClass()` override once the interceptor is installed, nothing reads the trait defaults off the loaded target, and `load()` is not how most applications hit this; a lazy many-to-one to an entity that implements the trait is. Could you extend the spec along these lines (this passes against this branch on both modules): ```groovy when: 'the proxy is initialized through dynamic dispatch' InitializedTraitEntity proxy = null boolean uninitializedBefore = false String name = null String createdBy = null Class<?> unwrapped = null InitializedTraitEntity.withTransaction { proxy = InitializedTraitEntity.load(id) uninitializedBefore = !Hibernate.isInitialized(proxy) name = proxy.name createdBy = proxy.createdBy unwrapped = datastore.mappingContext.proxyHandler.unwrap(proxy).getClass() } then: proxy instanceof HibernateProxy proxy instanceof InitializedTrait uninitializedBefore proxy.id == id name == 'seed' createdBy == 'N/A' Hibernate.isInitialized(proxy) unwrapped == InitializedTraitEntity ``` plus a second feature with an owner entity (`InitializedTraitEntity target`) that asserts `owner.target` is an uninitialized `HibernateProxy` after `TraitEntityOwner.get(ownerId)` in a new session, and that `owner.target.name` and `owner.target.createdBy` resolve to the saved values. The same applies to the Hibernate 7 copy of this spec. ########## grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/proxy/traitinitializer/TraitInitializerProxySpec.groovy: ########## @@ -0,0 +1,98 @@ +/* + * 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 grails.gorm.tests.proxy.traitinitializer + +import grails.gorm.annotation.Entity +import org.hibernate.Hibernate +import org.grails.orm.hibernate.HibernateDatastore +import spock.lang.AutoCleanup +import spock.lang.Issue +import spock.lang.Shared +import spock.lang.Specification + +/** + * Regression test for the construction of Hibernate proxies for Groovy entities whose trait + * declares fields with default values. + * + * Groovy generates a static {@code $init$} helper for such traits and calls it from the entity + * constructor. When Hibernate builds the lazy proxy, the proxy constructor runs while the + * interceptor has not been assigned yet; the generated proxy intercepts {@code getMetaClass()} + * (Groovy 3+ no longer marks it as synthetic, so Hibernate's synthetic-only filter does not + * exclude it) and the trait initializer fails with a {@code NullPointerException}. + * + * {@code Domain.load()} must return a usable, uninitialized proxy for both entities. + */ +@Issue('https://github.com/apache/grails-core/issues/16600') +class TraitInitializerProxySpec extends Specification { + + @Shared @AutoCleanup HibernateDatastore datastore = new HibernateDatastore(getClass().getPackage()) + + void "load() of an entity whose trait declares default values creates the proxy"() { Review Comment: Same ask as on the Hibernate 5 copy of this spec: initialize the proxy through dynamic dispatch, assert the trait defaults on the loaded target and the proxy handler unwrap, and add the lazy association case. ########## grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/proxy/GrailsBytecodeProviderSpec.groovy: ########## @@ -33,6 +33,12 @@ class GrailsBytecodeProviderSpec extends Specification { provider.proxyHelper != null } + def "exposes the ByteBuddyState its proxy helper loads proxy classes through"() { + expect: + provider.byteBuddyState != null + provider.byteBuddyState.is(provider.byteBuddyState) Review Comment: `provider.byteBuddyState.is(provider.byteBuddyState)` is always true. What this PR changed is that the helper and the Groovy wrapper share one `ByteBuddyState`, and that `GrailsProxyFactoryFactory` now passes it through, and neither is covered (`GrailsProxyFactoryFactory` has no spec of its own). A check that exercises the wiring: two `ByteBuddyGroovyProxyFactory` instances built from the same provider (`new ByteBuddyGroovyProxyFactory(provider.proxyHelper, provider.byteBuddyState, false)`, or through `provider.proxyFactoryFactory.buildProxyFactory(...)`) call `postInstantiate` for the same entity and end up with the same proxy class. With a fresh `ByteBuddyState` per factory they would not, because the stock class is cached per state. ########## grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/proxy/GroovyProxyClassBuilder.java: ########## @@ -0,0 +1,94 @@ +/* + * 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 groovy.lang.GroovyObject; +import groovy.lang.MetaClass; + +import net.bytebuddy.ByteBuddy; +import net.bytebuddy.dynamic.scaffold.subclass.ConstructorStrategy; +import net.bytebuddy.implementation.MethodDelegation; +import org.hibernate.HibernateException; +import org.hibernate.proxy.ProxyConfiguration; +import org.hibernate.proxy.pojo.bytebuddy.ByteBuddyProxyHelper; + +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.returns; +import static org.hibernate.bytecode.spi.ClassLoadingStrategyHelper.resolveClassLoadingStrategy; + +/** + * Builds the proxy class Hibernate instantiates for an entity. + * + * <p>The class is the stock Hibernate proxy class. For a Groovy entity it is wrapped in a subclass that + * answers {@code getMetaClass()} through {@link GroovyProxyMetaClassDispatcher} while the interceptor has + * not been assigned yet, which Groovy trait initializers running in the proxy constructor depend on. + * + * <p>Hibernate 5.6 gives no access to the {@code ByteBuddyState} that loads its proxy classes, so the + * wrapper is generated with ByteBuddy directly and loaded into the entity's class loader, as the stock + * proxy class is. + * + * <p>Everything that can fail about the generated class fails here, when the proxy factory is + * instantiated, rather than on the first lazy load. Mirrors the Hibernate 7 implementation in + * grails-data-hibernate7, which loads the wrapper through Hibernate's {@code ByteBuddyState}. + * + * @since 8.0.1 + */ +public final class GroovyProxyClassBuilder { + + private final ByteBuddyProxyHelper proxyHelper; + + public GroovyProxyClassBuilder(ByteBuddyProxyHelper proxyHelper) { + this.proxyHelper = proxyHelper; + } + + /** + * @param entityName the name of the entity, used to say which entity a failure is about + * @param persistentClass the entity class + * @param interfaces the interfaces the proxy has to implement + * @return the proxy class, which implements {@link ProxyConfiguration} so an interceptor can be assigned + * @throws HibernateException when the proxy class cannot be generated or cannot take an interceptor + */ + public Class<?> build(String entityName, Class<?> persistentClass, Class<?>[] interfaces) throws HibernateException { + final Class<?> hibernateProxyClass = proxyHelper.buildProxy(persistentClass, interfaces); + final boolean groovyEntity = GroovyObject.class.isAssignableFrom(persistentClass); + final Class<?> proxyClass = groovyEntity ? wrap(entityName, persistentClass, hibernateProxyClass) : hibernateProxyClass; + if (!ProxyConfiguration.class.isAssignableFrom(proxyClass)) { + throw new HibernateException("Generated proxy class for " + entityName + + " does not implement ProxyConfiguration, so no interceptor can be installed: " + proxyClass.getName()); + } + return proxyClass; + } + + private static Class<?> wrap(String entityName, Class<?> persistentClass, Class<?> hibernateProxyClass) { + try { + return new ByteBuddy() Review Comment: Non-blocking: nothing caches this wrapper, so every `postInstantiate` for a Groovy entity defines a new class in the entity's class loader. Hibernate caches the stock class in its `ByteBuddyState`, and the Hibernate 7 builder rides on that cache, so H7 ends with one wrapper per entity; here it is one per SessionFactory build, which mostly shows up in the test suite where many `HibernateDatastore` instances share a loader. One SessionFactory per application makes this a non-issue in production. If you want parity with H7, a `TypeCache.WithInlineExpunction<TypeCache.SimpleKey>` keyed on the stock proxy class keeps it at one per entity. -- 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]
