jdaugherty opened a new pull request, #15841:
URL: https://github.com/apache/grails-core/pull/15841
# Groovy-Aware Hibernate Proxies for grails-data-hibernate5 (and handler
fixes for both Hibernate lines)
While working on the Spring Security branch merge to 8.x, I discovered
inconsistent property behavior with `.id` and `.getId` - the redis plugin
imported the Spring version of Rollback so we never noticed this before. This
PR ensures domains do not unwrap when calling either form in hibernate 5 or 7.
## The bug
Accessing the identifier of an uninitialized GORM/Hibernate proxy —
`proxy.id`, `proxy.getId()`,
`proxy['id']`, `proxy.ident()` — initialized the proxy on
`grails-data-hibernate5`. With an open
session that was a silent eager fetch (defeating the point of `load()`/lazy
associations); on a
detached proxy it threw `LazyInitializationException`. Identifier access
must never unwrap a
proxy.
First observed as two failures in `RedisIntegrationSpec`
(`testMemoizeDomainList`,
`testMemoizeDomainObject`) on this branch's CI. The redis plugin caches only
entity IDs in redis
and rehydrates with `domainClass.load(id)` inside short `@Transactional`
methods, so its
cache-hit results are exactly this shape: uninitialized proxies whose
transaction has ended.
## Root cause
Hibernate's stock ByteBuddy proxy intercepts **every** virtual method.
`BasicLazyInitializer`
has a tiny no-init allowlist — essentially the identifier getter (`getId()`)
and
`getHibernateLazyInitializer()` — and initializes on anything else. Dynamic
Groovy property
access never reaches `getId()` directly: the MOP resolves the receiver
first, calling
`getMetaClass()` (and `getProperty("id")`) on the instance. Those are
ordinary virtual methods
woven in by the Groovy compiler, so the proxy intercepts them, they miss the
allowlist, and the
proxy initializes before `getId()` is ever called.
Findings that scoped the fix:
- **Not a Groovy 5 regression.** A recording fake proxy run under Groovy
4.0.32 and 5.0.7 shows
byte-identical dispatch: `getMetaClass()` ×3 then `getProperty("id")` then
`getId()`. The same
redis tests also fail on a local run of `upstream/7.2.x` (Groovy 4) with a
real redis — they
had simply never executed in this repo's CI before the inverted `isRedis`
flag was fixed on
this branch.
- **hibernate7 already solved this in-repo** with `GrailsBytecodeProvider` →
`ByteBuddyGroovyProxyFactory` → `ByteBuddyGroovyInterceptor` +
`GroovyProxyInterceptorLogic`,
installed by default. hibernate5 had no equivalent — only an opt-in
third-party test dependency
(`org.yakworks:hibernate-groovy-proxy`) and `@PendingFeatureIf`-gated
tests in
`ByteBuddyProxySpec` documenting the gap.
- **A separate handler regression on this branch**: `HibernateProxyHandler`
(both modules) gained
a `getProxyInstanceMetaClass` probe that calls `getMetaClass()` on the
object *before* the
`instanceof HibernateProxy` checks. The probe only exists to detect the
simple/in-memory
datastore's metaclass-based proxies (`ProxyInstanceMetaClass`), which a
Hibernate proxy can
never be — so for Hibernate proxies it was pure downside:
`getIdentifier()`/`isProxy()`/
`unwrap()` initialized the proxy (or threw when detached) on
7.2.x-and-later code that used to
answer straight from the `LazyInitializer`.
## The fix
### 1. Port the hibernate7 proxy stack to grails-data-hibernate5 (new files)
Same classes, same names, same behavior — adapted only where the Hibernate
5.6 SPI differs
(`Serializable` ids, `ByteBuddyProxyFactory`/`ProxyFactoryFactory`
signatures):
| File (`grails-data-hibernate5/core/.../org/grails/orm/hibernate/proxy/`) |
Purpose |
|---|---|
| `GroovyProxyInterceptorLogic.java` | Verbatim copy of the h7 logic:
answers
`getMetaClass`/`getStaticMetaClass`/`getProperty("id"\|"metaClass")`/`ident`/`isDirty`/`hasChanged`/`toString`
for uninitialized proxies without initializing. |
| `ByteBuddyGroovyInterceptor.java` | Extends 5.6 `ByteBuddyInterceptor`.
Returns the identifier for `getId`/`getIdentifier`/the mapped id getter before
any other handling; routes uninitialized calls through the logic above; falls
back to stock interception otherwise. |
| `ByteBuddyGroovyProxyFactory.java` | Extends 5.6 `ByteBuddyProxyFactory`;
builds the proxy class via `ByteBuddyProxyHelper` and installs the Groovy-aware
interceptor via `ProxyConfiguration.$$_hibernate_set_interceptor`. |
| `GrailsProxyFactoryFactory.java` | `ProxyFactoryFactory` returning the
Groovy-aware factory for entities; basic (non-entity) proxies delegate to the
stock implementation. |
| `GrailsBytecodeProvider.java` | Extends stock `BytecodeProviderImpl` so
enhancement/reflection optimization are untouched; only
`getProxyFactoryFactory()` is overridden. |
**Wiring** (`HibernateMappingContextConfiguration.buildSessionFactory()`):
```java
standardServiceRegistryBuilder.addService(ProxyFactoryFactory.class,
new GrailsBytecodeProvider().getProxyFactoryFactory());
```
Hibernate 5.6's `PojoEntityTuplizer` resolves the `ProxyFactoryFactory` from
the service
registry, and a *provided* service beats any classpath-contributed initiator
— so this is
deterministic, default-on, and requires no third-party dependency.
(hibernate7 wires the same
provider through
`HibernateConnectionSourceFactory`/`hibernate.enhancer.bytecodeprovider.instance`;
the SPIs differ, the behavior does not.)
### 2. `HibernateProxyHandler` ordering fix (both modules)
- **h5**: `getProxyInstanceMetaClass()` returns `null` immediately for
`HibernateProxy`
instances, so `getIdentifier()`/`isProxy()`/`unwrap()`/`initialize()`
never dispatch
`getMetaClass()` through a Hibernate proxy. `getIdentifier()` is answered
session-free from
`getHibernateLazyInitializer().getIdentifier()` (the pre-branch behavior).
- **h7**: equivalent reorder in `getIdentifier()` and `isProxy()` —
`instanceof HibernateProxy`
before the `GroovyProxyInterceptorLogic` metaclass probe (the helper
itself stays
Hibernate-decoupled).
This matters independently of the interceptor: the handler must be safe for
any
`HibernateProxy` it is handed, including ones not produced by our factory.
### 3. Tests
- **`ByteBuddyProxySpec` (h5 + h7) un-gated**: the
`@PendingFeatureIf`/`runPending` machinery and
the yakworks `testImplementation` are removed from both modules — all
lazy-proxy requirements
are now *hard* tests proving the in-repo stacks. 7 features each,
identical scenarios:
- `getId()`/`.id`/`['id']` (dynamic and `@CompileStatic`) don't initialize
- `ident()` and id access on a **detached** proxy work with **no session**
and don't initialize
- association id checks (`team.club.id`, `team.clubId`) don't initialize
the association
- truthy checks don't initialize
- a clean `isDirty()` doesn't initialize (h5 expectation updated to match
h7 — previously
documented as initializing on h5)
- **new scenario spec**: `unwrap` of a detached uninitialized proxy throws
`LazyInitializationException` while `getIdentifier()`/`ident()` on the
same proxy succeed —
documents the one operation that legitimately needs a session (unwrap
must materialize the
entity; it exists only in the database).
- **`HibernateProxyHandlerMetaClassProbeSpec` (h5 + h7, new)**: DB-free unit
tripwire — a fake
`HibernateProxy` whose `getMetaClass()` throws proves the handler answers
`getIdentifier`/`isProxy`/`isInitialized` without dispatching through the
proxy. (All probe
interaction is `@CompileStatic`; the first version demonstrated live that
even *dynamic spec
code touching the object* trips the MOP interception.)
### 4. Redis example app
- `RedisIntegrationSpec`: **restored byte-for-byte to the original** (Spring
`@Rollback`,
original assertions). No session is bound during its assertions, and it
now passes because the
proxies answer id access lazily.
- `ProxyAwareSpec.getEntityId` (the only app change): was
`unwrapIfProxy(obj)?.ident()` — which
materializes the entire entity (session required, inherently) just to read
the id. Now
`proxyHandler.getProxyIdentifier(obj) ?: obj?.id` via the grails-core
`EntityProxyHandler`
API — session-free, and what the trait always meant.
## Verification (local)
| Suite | Result |
|---|---|
| h5 proxy suites (`ByteBuddyProxySpec`, `HibernateProxyHandler5Spec`,
`MetaClassProbeSpec`, `SimpleHibernateProxyHandlerSpec`, `ToOneProxySpec`) | 32
tests, 0 failures |
| h7 proxy suites (incl. `GrailsBytecodeProviderSpec`,
`ByteBuddyGroovyInterceptorSpec`, `HibernateProxyHandler7Spec`) | 85 tests, 0
failures |
| `:grails-test-examples-redis:integrationTest` (real redis 8 container) |
all specs green, incl. previously failing
`testMemoizeDomainList`/`testMemoizeDomainObject` |
| `:grails-redis:integrationTest` | 31/31 (`RedisServiceSpec`), taglib skips
are pre-existing |
## Behavior changes to watch in CI
- h5 proxies now behave like h7's everywhere: `toString()` on an
uninitialized proxy renders
`Entity:id` instead of initializing; a clean `isDirty()` returns `false`
without initializing;
`load()` results stay lazy. Any h5 test/app code that accidentally relied
on eager
initialization will surface in the full suites.
- The
`grails-test-examples/hibernate5|hibernate7/grails-hibernate-groovy-proxy`
example apps
still declare `org.yakworks:hibernate-groovy-proxy`; the provided service
now overrides its
contributor, so the dependency is redundant there and those apps/tests
should be reviewed
(likely: drop the dependency, keep the tests as extra coverage of the
in-repo stack).
- `unwrap()`/`unwrapIfProxy()` semantics are unchanged: they initialize by
contract. Code that
only needs the id must use `ident()`/`getProxyIdentifier()`/`proxy.id`.
## Related but separate changes in this working tree
- `UpdateRequestContextHolderExceptionTranslationFilter` (spring-security):
`setMultipartRequest`
and `addParametersFrom` added to the `@Delegate` excludes so multipart
state and params live on
the same instance the getters read — fixes all the
`request.getFile`/`params.myFile` upload
failures across CI jobs. Not proxy-related; listed here because it's
uncommitted alongside.
--
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]
