fix(grails-datamapping-core): GroovyProxyFactory.getProxiedClass returns Object instead of entity class #15650
Conversation
…s past entity to Object GroovyProxyFactory.createProxy() builds metaclass-only proxies: a real entity instance with a ProxyInstanceMetaClass attached. For those, proxy.getClass() is already the entity class. getProxiedClass() was copied from JavassistProxyFactory, whose proxies are runtime subclasses where getSuperclass() correctly recovers the entity. Applied to metaclass-only proxies, the same call walks one level past the entity — to java.lang.Object for any entity extending Object directly. Downstream callers that feed the result into MappingContext.getPersistentEntity (notably UniqueConstraint.processValidate) get null back and NPE on the next dereference. Surfaces on MongoDB / Neo4j (no Javassist on the classpath → GroovyProxyFactory wins) when cascade validation hits a not-yet-unwrapped to-one association. Drop the superclass walk; proxy.getClass() is the entity class for both the proxy and non-proxy paths. Adds GroovyProxyFactorySpec covering both paths to lock the contract.
✅ All tests passed ✅🏷️ Commit: ac24d22 Learn more about TestLens at testlens.app. |
|
this is way too risky of a change for 7.x, it should go in 8. proxies and the code around it have historically been a land mine field of problems and multiple regressions have occurred in this area on larger apps. |
To me this looks like a bug fix for GroovyProxyFactory:
|
|
This change should be made as a targeted fix for mongo instead of fixing it in core for 7.x. Proxies are one of the areas where my app has consistently broken over the years. |
@jdaugherty this isn't a fix for Mongo. it fixes everything that doesn't use Javassist. hibernate uses Javassist |
|
Why can't mongo have specific config in 7.x? How do you know it doesn't break graphql or the simple data store? I'm just saying this change has a lot of risk based on historical changes, and if we're going to pull this into 7.x, we should take a targeted approach. |
|
The root cause analysis here is correct — for My concern is the risk profile of this change landing in History of proxy regressions in this area This is the fourth patch to the
Two patterns from this history are directly relevant here:
The test doesn't cover the actual failure path The new Blast radius concern for 7.x
What would make me more comfortable merging this in 7.x Either of these would address the concern:
Can you confirm whether |
|
I used AI to post this last comment to show why I'm so concerned about this change. I used it to research the history of changes here and why I think this risk is too great for 7.x. |
but this is targeting 7.2.x and we can roll out a 7.2.0-M1 to start. |
|
You're assuming we can test all of this, see the above comment where we've tried this exact fix before and had to roll it back. |
Well I can state with certainty that it is clearly broken in 7.x. I have to run a custom built jar in my app as a workaround. This fix in in 7.2.x If we can't test it in 7.2.x, how are we going to test it in 8.0.x if 8.0.x is going to be released before 7.2.x ? |
|
@codeconsole I'm not proposing we don't fix it in 8.x. I'm first wanting a reproducer that isn't a unit test & an isolated (mongo) only fix in 7.x. This way the risk is low. |
|
@codeconsole |
@matrei good find, thanks Here was the issue configurations {
all {
exclude group: "org.apache.grails.data", module: "grails-datastore-core"
exclude group: "org.apache.grails.data", module: "grails-data-mongodb-bson"
exclude group: "org.apache.grails.data", module: "grails-data-mongodb-core"
}
}
List<String> extLibs = [
'lib/grails-datastore-core-7.1.1-SNAPSHOT.jar',
'lib/grails-data-mongodb-bson-7.1.1-SNAPSHOT.jar',
'lib/grails-data-mongodb-core-7.1.1-SNAPSHOT.jar'
}I wasn't resolving
if (ClassUtils.isPresent("javassist.util.proxy.ProxyFactory", classLoader)) {
proxyFactory = DefaultProxyFactoryCreator.create(); // → JavassistProxyFactory
}
else if (ClassUtils.isPresent("org.grails.datastore.gorm.proxy.GroovyProxyFactory", ...)) {
proxyFactory = new GroovyProxyFactory(); // ← fallback which results in failure
}So the question is do we want to address the edge case when javassist is not in the class path even though it is a transitive dependency of |
fix(grails-datamapping-core): GroovyProxyFactory.getProxiedClass walks past entity to Object
Summary
GroovyProxyFactory.getProxiedClass()returnsjava.lang.Object(or the entity's actual superclass, whatever that happens to be) for proxies that this same factory created, instead of the proxied entity class. Any caller that feeds the result back intoMappingContext.getPersistentEntity(name)then receivesnull— at best a confusing failure mode, at worst an NPE on the next dereference (UniqueConstraint.processValidateis one such caller — it goes on to invoketargetEntity.isRoot()without a null guard).This PR fixes the contract violation inside the factory itself; it does not depend on how any specific consumer is wired.
Root cause
GroovyProxyFactoryandJavassistProxyFactoryship a structurally identicalgetProxiedClass(), but the two factories produce different shapes of proxy:createProxy()returnsproxy.getClass()isproxy.getClass().getSuperclass()isJavassistProxyFactoryProxy_$$_javassist extends EntityGroovyProxyFactoryEntityinstance with aProxyInstanceMetaClassattachedjava.lang.Object) — wrongThe
getSuperclass()walk is correct for Javassist's runtime-subclass strategy. Copy-pasted intoGroovyProxyFactory, whose proxies are not subclasses of the entity (they are real entity instances with a custom metaclass), the walk steps one level past the entity. The post-walk null check at line 99 catches the inheritance-walk null but never the initial-lookup null, so callers that feed the result intoMappingContext.getPersistentEntity(name)get a silent null back and NPE further down the stack.The fix is to drop the walk: for the proxies this factory produces,
proxy.getClass()is already the entity class in both the proxy and non-proxy branches.(Also drops the duplicate
@Overrideannotation that was already on the method — pure cleanup, no behavior change.)Test coverage
Adds
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/proxy/GroovyProxyFactorySpec.groovywith two cases:getProxiedClassreturns the entity class for a proxy created by this factory — fails on the unmodified code (getProxiedClass(proxy)returnsjava.lang.Object), passes after the fix.getProxiedClassreturns the entity class for a non-proxy instance — passes both before and after; regression guard for the non-proxy branch.The test mirrors what
GroovyProxyFactory.createProxy()does internally (real entity instance +ProxyInstanceMetaClassattached), so it exercises the same proxy shape callers see at runtime.Verification
Test totals on the patched code (
groovy-proxy-getproxiedclass-fixbranch off7.2.x):grails-datamapping-core(incl. newGroovyProxyFactorySpec)grails-datamapping-core-test(TCK against the simple in-memory datastore — runsGroovyProxySpecfor bothuseGroovyProxyFactory: trueandfalse)grails-datamapping-validation,tck,support,asyncgrails-datastore-core(incl. existingJavassistProxyFactorySpec)grails-datastore-webgrails-data-mongodb-core(full cascade + proxy path against real Mongo)grails-data-mongodb-bsongrails-data-hibernate5/core(Javassist path — sanity check that the alternate proxy strategy is unaffected)Zero regressions.
Why this is worth fixing even though
AbstractMappingContextnormally picksJavassistProxyFactoryIn a normal Grails 7 application the
AbstractMappingContext.getProxyFactory()picker probes the classpath forjavassist.util.proxy.ProxyFactoryfirst and usesJavassistProxyFactorywhenever Javassist is present, so the buggyGroovyProxyFactory.getProxiedClass()path doesn't fire in most production setups. It does fire for any consumer that explicitly opts in toGroovyProxyFactory— directly (mappingContext.proxyFactory = new GroovyProxyFactory(), as the Neo4j and TCKGroovyProxySpectests do) or indirectly via dependency configurations that strip Javassist from the classpath (custom builds that excludegrails-datastore-core's transitive Javassist dep, etc.). For those consumers the bug is currently a latent NPE waiting on cascade validation; fixing the factory removes the trap regardless of how it ends up selected.