jdaugherty commented on code in PR #16155:
URL: https://github.com/apache/grails-core/pull/16155#discussion_r4146884964
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/proxy/GroovyProxyFactory.groovy:
##########
@@ -70,7 +71,8 @@ class GroovyProxyFactory implements ProxyFactory {
}
}
- @groovy.transform.CompileDynamic
+ @CompileDynamic
+ @SuppressWarnings('GrMethodMayBeStatic')
Review Comment:
Keeping these as instance methods with the suppression was the right call:
`protected` hooks that subclasses may override must stay virtual. The
suppression matches the existing `GrMethodMayBeStatic` usage in `GrailsBanner`
and `AbstractDatastoreInitializer`, so no convention concern here.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/proxy/GroovyProxyFactorySpec.groovy:
##########
@@ -0,0 +1,385 @@
+/*
+ * 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.gorm.proxy
+
+import org.grails.datastore.mapping.core.Session
+import org.grails.datastore.mapping.engine.AssociationQueryExecutor
+import org.grails.datastore.mapping.engine.EntityPersister
+import org.grails.datastore.mapping.model.MappingContext
+import org.grails.datastore.mapping.model.PersistentEntity
+import org.grails.datastore.mapping.reflect.EntityReflector
+import org.springframework.dao.DataIntegrityViolationException
+import spock.lang.Specification
+
+class GroovyProxyFactorySpec extends Specification {
+
+ GroovyProxyFactory proxyFactory = new GroovyProxyFactory()
+
+ void "createProxy returns an uninitialized proxy whose identifier is
available without loading"() {
Review Comment:
Renamed from "returns an initialized-looking instance": the feature asserts
`!isInitialized(proxy)`, so the old name said the opposite of what it checks.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/proxy/ProxyInstanceMetaClassSpec.groovy:
##########
@@ -0,0 +1,475 @@
+/*
+ * 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.gorm.proxy
+
+import org.grails.datastore.mapping.core.Session
+import org.springframework.dao.DataIntegrityViolationException
+import spock.lang.Specification
+
+class ProxyInstanceMetaClassSpec extends Specification {
+
+ Session session = Mock(Session)
+ MetaClass delegate = Mock(MetaClass)
+ ProxyInstanceTestTarget target = new ProxyInstanceTestTarget()
+ Object proxy = new Object()
Review Comment:
Worth being aware of when reading this spec: with a `Mock(MetaClass)`
delegate and a plain `Object` receiver, it can only verify the three-argument
methods it calls directly. It cannot detect which `MetaClass` variant Groovy
actually dispatches through, which is how the property-assignment bug slipped
past full line and branch coverage. The cases that drive a real proxy through
Groovy dispatch (`proxy.name`, `proxy.name = ...`, `proxy.@name`,
`proxy.describe()`) live in `GroovyProxyFactorySpec`; this spec now also pins
the sender-aware variants directly.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/proxy/GroovyProxyFactorySpec.groovy:
##########
@@ -0,0 +1,385 @@
+/*
+ * 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.gorm.proxy
+
+import org.grails.datastore.mapping.core.Session
+import org.grails.datastore.mapping.engine.AssociationQueryExecutor
+import org.grails.datastore.mapping.engine.EntityPersister
+import org.grails.datastore.mapping.model.MappingContext
+import org.grails.datastore.mapping.model.PersistentEntity
+import org.grails.datastore.mapping.reflect.EntityReflector
+import org.springframework.dao.DataIntegrityViolationException
+import spock.lang.Specification
+
+class GroovyProxyFactorySpec extends Specification {
+
+ GroovyProxyFactory proxyFactory = new GroovyProxyFactory()
+
+ void "createProxy returns an uninitialized proxy whose identifier is
available without loading"() {
+ given:
+ Session session = Mock(Session)
+ session.getPersister(ProxyFactoryTestDomain) >> null
+ session.getMappingContext() >> null
+
+ when:
+ ProxyFactoryTestDomain proxy = proxyFactory.createProxy(session,
ProxyFactoryTestDomain, 42L)
+
+ then:
+ proxyFactory.isProxy(proxy)
+ proxyFactory.getIdentifier(proxy) == 42L
+ !proxyFactory.isInitialized(proxy)
+ 0 * session.retrieve(_, _)
+ }
+
+ void "createProxy uses the session's persister to set the object
identifier when available"() {
+ given:
+ Session session = Mock(Session)
+ EntityPersister persister = Mock(EntityPersister)
+ session.getPersister(ProxyFactoryTestDomain) >> persister
+
+ when:
+ ProxyFactoryTestDomain proxy = proxyFactory.createProxy(session,
ProxyFactoryTestDomain, 99L)
+
+ then:
+ 1 * persister.setObjectIdentifier(_, 99L)
+ proxyFactory.isProxy(proxy)
+ }
+
+ void "createProxy falls back to the mapping context's entity reflector
when there is no persister"() {
+ given:
+ Session session = Mock(Session)
+ MappingContext mappingContext = Mock(MappingContext)
+ PersistentEntity entity = Mock(PersistentEntity)
+ EntityReflector reflector = Mock(EntityReflector)
+ session.getPersister(ProxyFactoryTestDomain) >> null
+ session.getMappingContext() >> mappingContext
+ mappingContext.getPersistentEntity(ProxyFactoryTestDomain.name) >>
entity
+ mappingContext.getEntityReflector(entity) >> reflector
+
+ when:
+ ProxyFactoryTestDomain proxy = proxyFactory.createProxy(session,
ProxyFactoryTestDomain, 5L)
+
+ then:
+ 1 * reflector.setIdentifier(_ as ProxyFactoryTestDomain, 5L)
+ proxyFactory.isProxy(proxy)
+ proxyFactory.getIdentifier(proxy) == 5L
+ }
+
+ void "createProxy sets the id property directly when the mapping context
does not know the type"() {
+ given:
+ Session session = Mock(Session)
+ MappingContext mappingContext = Mock(MappingContext)
+ session.getPersister(ProxyFactoryTestDomain) >> null
+ session.getMappingContext() >> mappingContext
+ mappingContext.getPersistentEntity(ProxyFactoryTestDomain.name) >> null
+
+ when:
+ ProxyFactoryTestDomain proxy = proxyFactory.createProxy(session,
ProxyFactoryTestDomain, 6L)
+
+ then: 'the underlying instance carries the identifier, bypassing the
proxy metaClass to read it'
+ ProxyFactoryTestDomain.getMethod('getId').invoke(proxy) == 6L
+ proxyFactory.isProxy(proxy)
+ 0 * session.retrieve(_, _)
+ }
+
+ void "createProxy still produces a proxy when identifier assignment
fails"() {
+ given:
+ Session session = Mock(Session)
+ session.getPersister(ProxyFactoryTestDomain) >> null
+ session.getMappingContext() >> { throw new IllegalStateException('no
mapping context') }
+
+ when:
+ ProxyFactoryTestDomain proxy = proxyFactory.createProxy(session,
ProxyFactoryTestDomain, 8L)
+
+ then:
+ proxyFactory.isProxy(proxy)
+ proxyFactory.getIdentifier(proxy) == 8L
+ }
+
+ void "a proxy answers identity and proxy-state queries through Groovy
dispatch without loading"() {
+ given:
+ Session session = Mock(Session)
+ ProxyFactoryTestDomain proxy = proxyFactory.createProxy(session,
ProxyFactoryTestDomain, 42L)
+
+ when:
+ Object id = proxy.id
+ Object idFromGetter = proxy.getId()
+ Object isProxy = proxy.isProxy()
+ Object initialized = proxy.initialized
+ Object initializedFromGetter = proxy.isInitialized()
+ Object metaClass = proxy.metaClass
+ Object clazz = proxy.class
+
+ then:
+ id == 42L
+ idFromGetter == 42L
+ isProxy == true
+ initialized == false
+ initializedFromGetter == false
+ metaClass instanceof ProxyInstanceMetaClass
+ clazz == ProxyFactoryTestDomain
+ 0 * session.retrieve(_, _)
+ }
+
+ void "reading a regular property through Groovy dispatch loads the target
once and reads from it"() {
+ given:
+ Session session = Mock(Session)
+ ProxyFactoryTestDomain target = new ProxyFactoryTestDomain(id: 42L,
name: 'loaded')
+ ProxyFactoryTestDomain proxy = proxyFactory.createProxy(session,
ProxyFactoryTestDomain, 42L)
+
+ when:
+ String first = proxy.name
+ String second = proxy.name
+
+ then:
+ 1 * session.retrieve(ProxyFactoryTestDomain, 42L) >> target
+ first == 'loaded'
+ second == 'loaded'
+ proxy.initialized
+ proxy.target.is(target)
+ }
+
+ void "writing a regular property through Groovy dispatch loads the target
and writes to it"() {
Review Comment:
This is the case that first exposed the dispatch bug. Against the PR's
original `ProxyInstanceMetaClass` it failed with `session.retrieve` never
invoked: the assignment wrote `changed` into the proxy shell's own `name`
field, `target.name` stayed `loaded`, and the proxy remained uninitialized.
`proxy.setName(...)` and `proxy.setProperty(...)` did work, which is why the
mock-delegate spec could not see it.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/proxy/ProxyInstanceMetaClass.java:
##########
@@ -118,48 +147,93 @@ public boolean isProxyInitiated() {
@Override
public Object getProperty(Object object, String property) {
- if (property.equals("id")) {
- return getKey();
- } else if (property.equals("proxy")) {
- return true;
- } else if (property.equals("initialized")) {
- return isProxyInitiated();
- } else if (property.equals("target")) {
- return getProxyTarget();
- } else if (property.equals("metaClass")) {
- return this;
- } else if (property.equals("class") || property.equals("domainClass"))
{
+ Object result = proxyPropertyValue(property);
+ if (result != DELEGATE) {
+ return result;
+ }
+ return delegate.getProperty(propertyReceiver(object, property),
property);
+ }
+
+ @Override
+ public Object getProperty(Class sender, Object object, String property,
boolean useSuper, boolean fromInsideClass) {
+ Object result = proxyPropertyValue(property);
+ if (result != DELEGATE) {
+ return result;
+ }
+ return delegate.getProperty(sender, propertyReceiver(object,
property), property, useSuper, fromInsideClass);
+ }
+
+ private Object proxyPropertyValue(String property) {
+ return switch (property) {
+ case "id" -> getKey();
+ case "proxy" -> true;
+ case "initialized" -> isProxyInitiated();
+ case "target" -> getProxyTarget();
+ case "metaClass" -> this;
+ default -> DELEGATE;
+ };
+ }
+
+ private Object propertyReceiver(Object proxy, String property) {
+ if (property.equals("class") || property.equals("domainClass")) {
// return correct class only if loaded, otherwise hope for the best
- return delegate.getProperty(isProxyInitiated() ? proxyTarget :
object, property);
- } else {
- return delegate.getProperty(getProxyTarget(), property);
+ return isProxyInitiated() ? proxyTarget : proxy;
}
+ return getProxyTarget();
}
@Override
public void setProperty(Object object, String property, Object newValue) {
- boolean resolveTarget = true;
- if (property.equals("metaClass") && (newValue == null || newValue
instanceof MetaClass)) {
- resolveTarget = false;
- }
- delegate.setProperty(resolveTarget ? getProxyTarget() : object,
property, newValue);
+ delegate.setProperty(setPropertyReceiver(object, property, newValue),
property, newValue);
+ }
+
+ @Override
+ public void setProperty(Class sender, Object object, String property,
Object newValue, boolean useSuper,
Review Comment:
This is the fix for the bug the new dispatch tests exposed.
`ScriptBytecodeAdapter.setGroovyObjectProperty` checks whether the receiver's
`setProperty` is the `GroovyObject` default method (true for every modern POGO,
including GORM entities: `Location` in the TCK has no generated `setProperty`)
and then calls `getMetaClass().setProperty(senderClass, receiver, name, value,
false, false)`. `DelegatingMetaClass` implements that six-argument variant by
forwarding to the delegate with the *proxy* as receiver, so the three-argument
override above was never consulted for `proxy.name = 'x'`. The same applies to
`getGroovyObjectProperty` (five-argument `getProperty`), `getGroovyObjectField`
(four-argument `getAttribute`) and `setGroovyObjectField` (six-argument
`setAttribute`); the six-argument `invokeMethod` is covered too for consistency.
Rather than duplicate the rules, each pair of overrides now shares a private
lookup (`proxyPropertyValue` etc.) that returns the proxy-handled value or the
`DELEGATE` marker, plus a receiver-selection helper. The
sender/useSuper/fromInsideClass arguments are passed through unchanged so
delegate semantics are preserved.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/proxy/ProxyInstanceMetaClassSpec.groovy:
##########
@@ -0,0 +1,475 @@
+/*
+ * 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.gorm.proxy
+
+import org.grails.datastore.mapping.core.Session
+import org.springframework.dao.DataIntegrityViolationException
+import spock.lang.Specification
+
+class ProxyInstanceMetaClassSpec extends Specification {
+
+ Session session = Mock(Session)
+ MetaClass delegate = Mock(MetaClass)
+ ProxyInstanceTestTarget target = new ProxyInstanceTestTarget()
+ Object proxy = new Object()
+
+ void setup() {
+ delegate.getTheClass() >> ProxyInstanceTestTarget
+ }
+
+ ProxyInstanceMetaClass newMetaClass() {
+ new ProxyInstanceMetaClass(delegate, session, 11L)
+ }
+
+ void "getKey returns the identifier without resolving the target"() {
+ given:
+ ProxyInstanceMetaClass metaClass = newMetaClass()
+
+ when:
+ Serializable key = metaClass.getKey()
+ boolean initiated = metaClass.isProxyInitiated()
+
+ then:
+ key == 11L
+ !initiated
+ 0 * session.retrieve(_, _)
+ }
+
+ void "getProxyTarget lazily loads and caches the target from the
session"() {
+ given:
+ ProxyInstanceMetaClass metaClass = newMetaClass()
+
+ when:
+ Object first = metaClass.getProxyTarget()
+ Object second = metaClass.getProxyTarget()
+
+ then:
+ 1 * session.retrieve(ProxyInstanceTestTarget, 11L) >> target
+ first.is(target)
+ second.is(target)
+ metaClass.isProxyInitiated()
+ }
+
+ void "getProxyTarget throws DataIntegrityViolationException when the
associated instance no longer exists"() {
+ given:
+ ProxyInstanceMetaClass metaClass = newMetaClass()
+ session.retrieve(ProxyInstanceTestTarget, 11L) >> null
+
+ when:
+ metaClass.getProxyTarget()
+
+ then:
+ thrown(DataIntegrityViolationException)
+ }
+
+ void "invokeMethod handles proxy-aware methods without resolving the
target"() {
+ given:
+ ProxyInstanceMetaClass metaClass = newMetaClass()
+
+ when:
+ Object isProxyResult = metaClass.invokeMethod(proxy, 'isProxy', [] as
Object[])
+ Object getIdResult = metaClass.invokeMethod(proxy, 'getId', [] as
Object[])
+ Object isInitializedResult = metaClass.invokeMethod(proxy,
'isInitialized', [] as Object[])
+ Object getMetaClassResult = metaClass.invokeMethod(proxy,
'getMetaClass', [] as Object[])
+
+ then:
+ isProxyResult == true
+ getIdResult == 11L
+ isInitializedResult == false
+ getMetaClassResult.is(metaClass)
+ 0 * session.retrieve(_, _)
+ }
+
+ void "invokeMethod returns the resolved target for getTarget/initialize
without delegating"() {
Review Comment:
`getTarget`/`initialize` return `getProxyTarget()` directly without calling
the delegate, so the previous `delegate.invokeMethod(...) >> target` stub was
never hit and the old name ("delegates") was misleading. Now asserts the
retrieve, that the delegate is not invoked, and that the proxy is initiated
afterwards.
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/GroovyProxySpec.groovy:
##########
@@ -96,6 +96,29 @@ class GroovyProxySpec extends GrailsDataTckSpec {
useGroovyProxyFactory << [true, false]
}
+ void 'Test writing a property through a proxy updates the proxied
instance'() {
Review Comment:
End-to-end regression case for the dispatch fix, running for both
`GroovyProxyFactory` and the default Javassist factory. Against the unfixed
metaclass the `useGroovyProxyFactory: true` row fails at
`location.isInitialized()` (the write went to the shell). Runs under
`grails-datamapping-core-test` (SimpleMapDatastore) and the Neo4j suite; the
Hibernate suites skip this spec via the existing `@IgnoreIf`.
--
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]
