jdaugherty commented on code in PR #16721: URL: https://github.com/apache/grails-core/pull/16721#discussion_r4407949370
########## grails-core/src/test/groovy/org/grails/core/artefact/DomainClassArtefactHandlerSpec.groovy: ########## @@ -0,0 +1,53 @@ +/* + * 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.core.artefact + +import grails.core.artefact.test.Entity +import spock.lang.Specification + +class DomainClassArtefactHandlerSpec extends Specification { + + void "isDomainClass walks every generated layer when proxies are allowed"() { + expect: + DomainClassArtefactHandler.isDomainClass(DomainClassArtefactProxyEntity) + DomainClassArtefactHandler.isDomainClass(DomainClassArtefactProxyEntity, true) + DomainClassArtefactHandler.isDomainClass(DomainClassArtefactProxyEntity, false) + !DomainClassArtefactHandler.isDomainClass(DomainClassArtefactProxyEntity$Proxy) + !DomainClassArtefactHandler.isDomainClass(DomainClassArtefactProxyEntity$Proxy$Wrapper) + DomainClassArtefactHandler.isDomainClass(DomainClassArtefactProxyEntity$Proxy, true) + DomainClassArtefactHandler.isDomainClass(DomainClassArtefactProxyEntity$Proxy$Wrapper, true) + !DomainClassArtefactHandler.isDomainClass(DomainClassArtefactProxyEntity$Proxy, false) + !DomainClassArtefactHandler.isDomainClass(DomainClassArtefactProxyEntity$Proxy$Wrapper, false) + !DomainClassArtefactHandler.isDomainClass(DomainClassArtefactProxyPlain, true) + !DomainClassArtefactHandler.isDomainClass(null, true) + } +} + +@Entity Review Comment: Use the real `grails.persistence.Entity` here and drop `grails.core.artefact.test.Entity`. The stand-in passes only because it satisfies the handler's `grails.*` package plus `Entity` simple-name check, so the spec proves the loop against a marker no production class carries. The real annotation lives in this module, and `ClassPathScannerSpec` in this same test source set already annotates its `Foo` fixture with it, so the transform is not an obstacle here. It is not `@Inherited` either, so the generated layers still miss it and the walk is still what the spec exercises. ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/NameUtils.java: ########## @@ -98,15 +98,16 @@ public static String getGetterName(String propertyName, boolean useBooleanPrefix } /** - * Get the class name, taking into account proxies + * Get the class name, taking into account proxies. + * A {@code $} in the simple name is taken to mean a generated subclass, + * which is also how a nested class has always been treated. Review Comment: Non-blocking: this sentence says the opposite of what happens. `Class.getSimpleName()` drops the outer-class prefix, so a nested class has no `$` in its simple name (`Outer$Inner` reports `Inner`) and this method has never unwrapped one. Only a generated class, or a top-level class literally named with a `$` like the spec fixtures, is walked. Suggest: `A {@code $} in the simple name marks a generated subclass; a nested class is unaffected, since its simple name carries no {@code $}.` ########## grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormApiResolver.groovy: ########## @@ -156,7 +156,8 @@ class GormApiResolver { PersistentEntity findEntity(Class entity, String qualifier = null) { String resolvedQualifier = qualifier ?: findTenantId(entity) - return findDatastore(entity, resolvedQualifier)?.mappingContext?.getPersistentEntity(entity.name) + String className = entity != null ? NameUtils.getClassName(entity) : null + return findDatastore(entity, resolvedQualifier)?.mappingContext?.getPersistentEntity(className) Review Comment: Non-blocking, anchored here because the line is outside the hunk: `findTenantId`, two methods down, is the one remaining `entity.name` in this chain. `GormRegistry.normalizeEntityKey(String)` only trims, so for a proxy class of a `MultiTenant` entity the `getDatastoreByString(entity.name, DEFAULT)` lookup misses, no tenant is resolved, and this `findEntity` reaches `findDatastore` with `DEFAULT` instead of the current tenant. `registry.getDatastore(entity, ConnectionSource.DEFAULT)` takes the `Class` overload, which normalizes through `NameUtils`, and closes it. Same bug one more time, so worth taking while you are here. ########## grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/proxy/ByteBuddyGroovyProxyFactorySpec.groovy: ########## @@ -41,6 +43,30 @@ class ByteBuddyGroovyProxyFactorySpec extends Specification { ByteBuddyState state = new ByteBuddyState() ByteBuddyProxyHelper helper = new ByteBuddyProxyHelper(state) + void "a wrapped Hibernate proxy resolves the entity name and GORM APIs without initialization"() { + given: + Session session = datastore.sessionFactory.openSession() + + when: + def proxy = session.getReference(GroovyProxyFactoryEntity, 1L) + Class proxyClass = proxy.getClass() + def proxyValidationApi = GormEnhancer.findValidationApi(proxyClass) + def entityValidationApi = GormEnhancer.findValidationApi(GroovyProxyFactoryEntity) + + then: + proxyClass != GroovyProxyFactoryEntity + NameUtils.getClassName(proxyClass) == GroovyProxyFactoryEntity.name + proxyValidationApi != null + proxyValidationApi.is(entityValidationApi) + GormEnhancer.findStaticApi(proxyClass).is(GormEnhancer.findStaticApi(GroovyProxyFactoryEntity)) + GormEnhancer.findInstanceApi(proxyClass).is(GormEnhancer.findInstanceApi(GroovyProxyFactoryEntity)) Review Comment: Non-blocking: `GormEnhancer.findEntity` is the one method this commit changes that no spec runs; `GormApiResolverSpec` covers the resolver's `findEntity` against a mocked mapping context. A `GormEnhancer.findEntity(proxyClass).is(GormEnhancer.findEntity(GroovyProxyFactoryEntity))` line here and in the H5 twin would run it against the real Hibernate 5 and 7 chains. -- 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]
