jdaugherty commented on code in PR #16625: URL: https://github.com/apache/grails-core/pull/16625#discussion_r4289257788
########## 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: Addressed in 72c44111e5: initialization through dynamic dispatch, the trait defaults on the loaded target, the proxy-handler unwrap and the lazy many-to-one are all covered, and pass locally on both modules. ########## 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: Addressed in 72c44111e5, same coverage as the Hibernate 5 copy. ########## grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/proxy/ByteBuddyGroovyProxyFactorySpec.groovy: ########## @@ -0,0 +1,175 @@ +/* + * 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 grails.gorm.annotation.Entity +import groovy.transform.CompileStatic + +import org.grails.orm.hibernate.HibernateDatastore +import org.hibernate.Hibernate +import org.hibernate.HibernateException +import org.hibernate.Session +import org.hibernate.engine.spi.SharedSessionContractImplementor +import org.hibernate.proxy.HibernateProxy +import org.hibernate.proxy.LazyInitializer +import org.hibernate.proxy.pojo.bytebuddy.ByteBuddyProxyHelper +import spock.lang.AutoCleanup +import spock.lang.Shared +import spock.lang.Specification + +class ByteBuddyGroovyProxyFactorySpec extends Specification { + + @Shared @AutoCleanup HibernateDatastore datastore = new HibernateDatastore(GroovyProxyFactoryEntity) + + ByteBuddyProxyHelper helper = new GrailsBytecodeProvider().getByteBuddyProxyHelper() + + void "factory can be instantiated"() { + expect: + new ByteBuddyGroovyProxyFactory(Mock(ByteBuddyProxyHelper)) != null + } + + void "postInstantiate wraps the Hibernate proxy class so getMetaClass() can answer before the interceptor is assigned"() { + given: + def factory = new ByteBuddyGroovyProxyFactory(helper) + + when: + factory.postInstantiate('GroovyProxyFactoryEntity', GroovyProxyFactoryEntity, [HibernateProxy] as Set, null, null, null) + + then: 'the proxy class subclasses the Hibernate proxy class and overrides getMetaClass()' + def proxyClass = proxyClassOf(factory) + def stock = proxyClass.superclass + HibernateProxy.isAssignableFrom(stock) + GroovyProxyFactoryEntity.isAssignableFrom(proxyClass) + proxyClass.getMethod('getMetaClass').declaringClass == proxyClass + } + + void "postInstantiate fails when the generated proxy class cannot take an interceptor, naming the entity"() { + given: + def stockHelper = Mock(ByteBuddyProxyHelper) { + buildProxy(Object, _ as Class[]) >> String + } + def factory = new ByteBuddyGroovyProxyFactory(stockHelper) + + when: + factory.postInstantiate('PlainEntity', Object, [] as Set, null, null, null) + + then: 'it fails when the factory is set up, not on the first lazy load' + HibernateException e = thrown() + e.message.contains('PlainEntity') + } + + void "postInstantiate fails when the entity has no default constructor, naming the entity"() { + given: + def factory = new ByteBuddyGroovyProxyFactory(helper) + + when: + factory.postInstantiate('GroovyProxyNoDefaultConstructorEntity', GroovyProxyNoDefaultConstructorEntity, [HibernateProxy] as Set, null, null, null) + + then: + HibernateException e = thrown() + e.message.contains('GroovyProxyNoDefaultConstructorEntity') + } + + void "getProxy returns a proxy with the Groovy interceptor installed"() { + given: + def factory = new ByteBuddyGroovyProxyFactory(helper) + factory.postInstantiate(GroovyProxyFactoryEntity.name, GroovyProxyFactoryEntity, [HibernateProxy] as Set, + GroovyProxyFactoryEntity.getMethod('getId'), GroovyProxyFactoryEntity.getMethod('setId', Long), null) + def session = datastore.sessionFactory.openSession() as SharedSessionContractImplementor + + when: + HibernateProxy proxy = factory.getProxy(1L, session) + + then: + proxy instanceof GroovyProxyFactoryEntity + proxy.getClass().interfaces.contains(HibernateProxy) + lazyInitializerOf(proxy) instanceof ByteBuddyGroovyInterceptor + lazyInitializerOf(proxy).identifier == 1L + lazyInitializerOf(proxy).entityName == GroovyProxyFactoryEntity.name + + cleanup: + session?.close() + } + + void "getProxy wraps a failure in HibernateException"() { + given: 'a factory that was never set up, so it has no proxy class to instantiate' + def factory = new ByteBuddyGroovyProxyFactory(helper) + + when: + factory.getProxy(1L, Mock(SharedSessionContractImplementor)) + + then: + thrown(HibernateException) + } + + void "a proxy from getProxy survives Java serialization as the proxy its SessionFactory hands out"() { + given: + def factory = new ByteBuddyGroovyProxyFactory(helper) + factory.postInstantiate(GroovyProxyFactoryEntity.name, GroovyProxyFactoryEntity, [HibernateProxy] as Set, + GroovyProxyFactoryEntity.getMethod('getId'), GroovyProxyFactoryEntity.getMethod('setId', Long), null) + Session session = datastore.sessionFactory.openSession() + HibernateProxy proxy = factory.getProxy(1L, session as SharedSessionContractImplementor) + + when: + def deserialized = roundTrip(proxy) + + then: 'the serialized form rebuilt a Groovy-aware proxy of the class the session itself hands out' + deserialized instanceof GroovyProxyFactoryEntity + deserialized.getClass().is(session.getReference(GroovyProxyFactoryEntity, 1L).getClass()) + !Hibernate.isInitialized(deserialized) + lazyInitializerOf(deserialized) instanceof ByteBuddyGroovyInterceptor + lazyInitializerOf(deserialized).identifier == 1L + + cleanup: + session?.close() + } + + @CompileStatic + private static Object roundTrip(Object object) { + ByteArrayOutputStream bytes = new ByteArrayOutputStream() + new ObjectOutputStream(bytes).withCloseable { ObjectOutputStream out -> out.writeObject(object) } + new ObjectInputStream(new ByteArrayInputStream(bytes.toByteArray())).withCloseable { ObjectInputStream input -> + input.readObject() + } + } + + @CompileStatic + private static LazyInitializer lazyInitializerOf(HibernateProxy proxy) { + proxy.getHibernateLazyInitializer() + } + + private static Class<?> proxyClassOf(ByteBuddyGroovyProxyFactory factory) { Review Comment: Non-blocking: this is the only place in the spec that reaches past the public surface, and it does not need to. The proxy class is observable the way `GrailsBytecodeProviderSpec` now does it: `factory.getProxy(1L, session).getClass()` with the real session this spec already opens for the interceptor case, and `.superclass` for the stock class. The three assertions in the `postInstantiate wraps ...` feature hold unchanged against that class. ########## 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: Addressed in 87a3f936e5 with a static `TypeCache` keyed on (entity, interfaces). Agreed that keying on the stock class would never hit on Hibernate 5.6, given the random names and a provider per SessionFactory; and reusing the first provider's stock class across SessionFactories is safe, since the generated class holds no reference to the state that built it. ########## 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: Addressed in 49fa7e809e: the tautology is gone, and the new feature reaches the shared `ByteBuddyState` through `getProxyFactoryFactory().buildProxyFactory(...)` and `getProxy(...)` rather than the field. ########## grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/proxy/ByteBuddyGroovyProxyFactorySpec.groovy: ########## @@ -18,46 +18,160 @@ */ package org.grails.orm.hibernate.proxy +import grails.gorm.annotation.Entity +import groovy.transform.CompileStatic + +import org.grails.orm.hibernate.HibernateDatastore +import org.hibernate.Hibernate import org.hibernate.HibernateException +import org.hibernate.Session +import org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState import org.hibernate.engine.spi.SharedSessionContractImplementor +import org.hibernate.proxy.HibernateProxy +import org.hibernate.proxy.LazyInitializer import org.hibernate.proxy.pojo.bytebuddy.ByteBuddyProxyHelper +import spock.lang.AutoCleanup +import spock.lang.Shared import spock.lang.Specification class ByteBuddyGroovyProxyFactorySpec extends Specification { - def "factory can be instantiated"() { + @Shared @AutoCleanup HibernateDatastore datastore = new HibernateDatastore(GroovyProxyFactoryEntity) + + ByteBuddyState state = new ByteBuddyState() + ByteBuddyProxyHelper helper = new ByteBuddyProxyHelper(state) + + void "factory can be instantiated"() { expect: new ByteBuddyGroovyProxyFactory(Mock(ByteBuddyProxyHelper)) != null } - def "postInstantiate configures entity name, class, and interfaces"() { + void "postInstantiate wraps the Hibernate proxy class so getMetaClass() can answer before the interceptor is assigned"() { + given: + def factory = new ByteBuddyGroovyProxyFactory(helper, state, false) + + when: + factory.postInstantiate('GroovyProxyFactoryEntity', GroovyProxyFactoryEntity, [HibernateProxy] as Set, null, null, null) + + then: 'the proxy class is a subclass of the Hibernate-built proxy that overrides getMetaClass()' + def proxyClass = proxyClassOf(factory) + def stock = helper.buildProxy(GroovyProxyFactoryEntity, [HibernateProxy] as Class[]) + proxyClass != stock + stock.isAssignableFrom(proxyClass) + proxyClass.getMethod('getMetaClass').declaringClass == proxyClass + } + + void "postInstantiate fails when the generated proxy class cannot take an interceptor, naming the entity"() { given: - def helper = Mock(ByteBuddyProxyHelper) { - buildProxy(String, _ as Class[]) >> String + def stockHelper = Mock(ByteBuddyProxyHelper) { + buildProxy(Object, _ as Class[]) >> String } - def factory = new ByteBuddyGroovyProxyFactory(helper) + def factory = new ByteBuddyGroovyProxyFactory(stockHelper, state, false) + + when: + factory.postInstantiate('PlainEntity', Object, [] as Set, null, null, null) + + then: 'it fails when the factory is set up, not on the first lazy load' + HibernateException e = thrown() + e.message.contains('PlainEntity') + } + + void "postInstantiate fails when the entity has no default constructor, naming the entity"() { + given: + def factory = new ByteBuddyGroovyProxyFactory(helper, state, false) when: - factory.postInstantiate("MyEntity", String, [] as Set, null, null, null) + factory.postInstantiate('GroovyProxyNoDefaultConstructorEntity', GroovyProxyNoDefaultConstructorEntity, [HibernateProxy] as Set, null, null, null) then: - noExceptionThrown() + HibernateException e = thrown() + e.message.contains('GroovyProxyNoDefaultConstructorEntity') } - def "getProxy wraps instantiation failure in HibernateException"() { - given: "a factory where proxyClass cannot be cast to HibernateProxy" - def helper = Mock(ByteBuddyProxyHelper) { - buildProxy(String, _ as Class[]) >> String - } - def factory = new ByteBuddyGroovyProxyFactory(helper) - factory.postInstantiate("MyEntity", String, [] as Set, null, null, null) - def session = Mock(SharedSessionContractImplementor) + void "getProxy returns a proxy with the Groovy interceptor installed"() { + given: + def factory = new ByteBuddyGroovyProxyFactory(helper, state, false) + factory.postInstantiate(GroovyProxyFactoryEntity.name, GroovyProxyFactoryEntity, [HibernateProxy] as Set, + GroovyProxyFactoryEntity.getMethod('getId'), GroovyProxyFactoryEntity.getMethod('setId', Long), null) + def session = datastore.sessionFactory.openSession() as SharedSessionContractImplementor + + when: + HibernateProxy proxy = factory.getProxy(1L, session) + + then: + proxy instanceof GroovyProxyFactoryEntity + proxy.getClass().interfaces.contains(HibernateProxy) + lazyInitializerOf(proxy) instanceof ByteBuddyGroovyInterceptor + lazyInitializerOf(proxy).identifier == 1L + lazyInitializerOf(proxy).entityName == GroovyProxyFactoryEntity.name + + cleanup: + session?.close() + } + + void "getProxy wraps a failure in HibernateException"() { + given: 'a factory that was never set up, so it has no proxy class to instantiate' + def factory = new ByteBuddyGroovyProxyFactory(helper, state, false) - when: "getProxy is called — new String() cannot cast to HibernateProxy" - factory.getProxy(1L, session) + when: + factory.getProxy(1L, Mock(SharedSessionContractImplementor)) then: - def e = thrown(HibernateException) - e.message.contains("MyEntity") + thrown(HibernateException) + } + + void "a proxy from getProxy survives Java serialization as the proxy its SessionFactory hands out"() { + given: + def factory = new ByteBuddyGroovyProxyFactory(helper) + factory.postInstantiate(GroovyProxyFactoryEntity.name, GroovyProxyFactoryEntity, [HibernateProxy] as Set, + GroovyProxyFactoryEntity.getMethod('getId'), GroovyProxyFactoryEntity.getMethod('setId', Long), null) + Session session = datastore.sessionFactory.openSession() + HibernateProxy proxy = factory.getProxy(1L, session as SharedSessionContractImplementor) + + when: + def deserialized = roundTrip(proxy) + + then: 'the serialized form rebuilt a Groovy-aware proxy of the class the session itself hands out' + deserialized instanceof GroovyProxyFactoryEntity + deserialized.getClass().is(session.getReference(GroovyProxyFactoryEntity, 1L).getClass()) + !Hibernate.isInitialized(deserialized) + lazyInitializerOf(deserialized) instanceof ByteBuddyGroovyInterceptor + lazyInitializerOf(deserialized).identifier == 1L + + cleanup: + session?.close() + } + + @CompileStatic + private static Object roundTrip(Object object) { + ByteArrayOutputStream bytes = new ByteArrayOutputStream() + new ObjectOutputStream(bytes).withCloseable { ObjectOutputStream out -> out.writeObject(object) } + new ObjectInputStream(new ByteArrayInputStream(bytes.toByteArray())).withCloseable { ObjectInputStream input -> + input.readObject() + } + } + + @CompileStatic + private static LazyInitializer lazyInitializerOf(HibernateProxy proxy) { + proxy.getHibernateLazyInitializer() + } + + private static Class<?> proxyClassOf(ByteBuddyGroovyProxyFactory factory) { Review Comment: Non-blocking, same as the Hibernate 5 copy: `factory.getProxy(1L, session).getClass()` in place of the reflective field read. -- 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]
