jdaugherty commented on PR #15650:
URL: https://github.com/apache/grails-core/pull/15650#issuecomment-4423914813
The root cause analysis here is correct — for `GroovyProxyFactory`'s
metaclass-proxy strategy, `o.getClass()` is already the entity class, so the
`getSuperclass()` walk is wrong. I don't dispute the logic of the fix itself.
My concern is the risk profile of this change landing in `7.x`.
**History of proxy regressions in this area**
This is the fourth patch to the `UniqueConstraint` + proxy interaction alone
(apache/grails-data-mapping#1263, apache/grails-data-mapping#1287, `afd8bcd738`
Oct 2020, now this). Across Grails 2–7 I can trace roughly 23 distinct
proxy-related bugs and regressions:
| Issue/Commit | Grails Version | Date | Description |
|---|---|---|---|
| GRAILS-10162 | **Grails 2.x** | ~2013 | Proxy initialization NPE; poor
proxy init error messages |
| GRAILS-11614 | **Grails 2.x** | Oct–Dec 2014 | `GroovyProxyFactory` /
`JavassistProxyFactory` ClassCastExceptions between Integer/Long ids;
`setMetaClass` on proxy needed special handling |
| GRAILS-11792 | **Grails 2.x** | Oct 2014 | Hibernate objects required to
implement `EntityProxy` interface; extract `EntityProxyMethodHandler` from
`JavassistProxyFactory` |
| apache/grails-data-mapping#808 | **Grails 3.2** (GORM 6.0.1) | Oct 2016 |
`@Resource` fails with ClassCastException when proxy involved |
| apache/grails-data-mapping#813 | **Grails 3.2** (GORM 6.0.2) | Oct 2016 |
MongoDB proxy creator throws `NumberFormatException` on String ids |
| apache/grails-data-mapping#975 | **Grails 3.3** (GORM 6.x) | Jul 2017 |
Invoking `asBoolean` on entity proxy throws ClassCastException |
| apache/grails-data-mapping#1043 | **Grails 3.3** | Dec 2017 | `lock {}`
causes NPE via proxy path |
| apache/grails-data-mapping#1072 | **Grails 3.3** (GORM 6.1.3) | 2018 |
Lazy associations with inheritance stop unwrapping proxy — `handleLazyProxy`
line accidentally commented out |
| **apache/grails-data-mapping#1104** | **Grails 3.3** (GORM 6.1.10) | 2018
| **`getPropertyType` returns `Object` for to-one associations** — proxy
wrapping causes type resolution to return `Object` (direct parallel to this PR)
|
| **apache/grails-data-mapping#1112** | **Grails 3.3** (GORM 6.1.11) | 2018
| Embedded types also wrapped by proxy closures — ***regression introduced by
the #1072 fix, in the very next point release*** |
| apache/grails-data-mapping#1131 | **Grails 3.x** (GORM 6.0.12) | Aug 2018
| StackOverflowError during save (proxy-related) |
| apache/grails-data-mapping#1156 | **Grails 3.x** | Feb 2019 | Add mapping
attribute to skip proxy auto-unwrapping — workaround added because unwrapping
couldn't be made safe universally |
| apache/grails-data-mapping#938 | **Grails 4.0** (GORM 7.0.0) | 2019 |
Remove proxy meta-programming from `HibernateUtil` — startup perf and memory
problems caused by the proxy approach |
| **apache/grails-data-mapping#1263 / #1276** | **Grails 4.0** (GORM 7.0.3)
| 2019 | **First `UniqueConstraint` + proxy fix** |
| **apache/grails-data-mapping#1287** | **Grails 4.0** (GORM 7.0.4) | Apr
2020 | **`UniqueConstraint` failing with uninitialized proxies on Neo4j** —
same datastore category (non-Hibernate, `GroovyProxyFactory`) and same
constraint validation failure mode as this PR; patched at the
`UniqueConstraint` call site rather than in `GroovyProxyFactory` |
| apache/grails-data-mapping#1294 | **Grails 4.0** (GORM 7.0.5) | May 2020 |
OneToOne proxies incorrectly marked dirty |
| apache/grails-data-mapping#1307 | **Grails 4.0** (GORM 7.0.5) | May 2020 |
Domain classes marked dirty without changes — proxy `$changedProperties` sync
broken |
| `afd8bcd738` | **Grails 4.x** (GORM 7.x) | Oct 2020 | **Third
`UniqueConstraint` + proxy patch** — guard added to `processValidate` for proxy
`propertyValue` |
| **apache/grails-data-mapping#1468** | **Grails 4.0.10** (GORM 7.0.5) | Jul
2021 | **`handleLazyProxy` commented out again** — identical regression to
#1072 reintroduced in Grails 4, two major versions later |
| grails/grails-data-hibernate5#464 | **Grails 5.x** | 2022 |
`HibernateProxyHandler` relied on Javassist removed in Hibernate 5.6 — required
proxy handler rewrite |
| grails/grails-data-hibernate5#624 | **Grails 5.x / 6.x** | 2022 |
ByteBuddy proxy strategy introduced, requiring a new proxy handling layer |
| apache/grails-data-mapping#1830 | **Grails 6.x / 7.x** | Nov 2024 |
Evaluating whether to add automatic proxy unwrapping back for Grails 7 upgrade
path — closed as "not planned" |
| apache/grails-data-mapping#1949 | **Grails 7.x** | 2025 |
`NoClassDefFoundError: HibernateProxyHandler` — class moved, callers broken |
Two patterns from this history are directly relevant here:
- **The `Object`-instead-of-entity-class mistake has happened before.**
apache/grails-data-mapping#1104 (GORM 6.1.10 / Grails 3.3) was exactly this
class of bug: proxy handling caused `getPropertyType` to return `Object`
instead of the actual association type, breaking constraint processing. The fix
for that then caused apache/grails-data-mapping#1112 — embedded types
incorrectly caught by the same proxy logic — regressing data binding in the
very next point release (6.1.11).
- **Proxy regressions re-introduce themselves across versions.**
apache/grails-data-mapping#1072 fixed the `handleLazyProxy` call in Grails 3.3.
That exact same call was accidentally commented out again in the Grails 4 era
as apache/grails-data-mapping#1468. Same line, same regression, two major
versions apart.
**The test doesn't cover the actual failure path**
The new `GroovyProxyFactorySpec` tests `getProxiedClass()` in isolation,
which is a reasonable unit test. But the claimed bug is an NPE in
`UniqueConstraint.processValidate` during cascade validation on a non-request
thread. There is no test that exercises
`PersistentEntityValidator.cascadeValidationToOne` →
`UniqueConstraint.processValidate(target=proxy)` →
`mappingContext.getPersistentEntity(getProxiedClass(target).getName())`
end-to-end. This is exactly the pattern that produced the
apache/grails-data-mapping#1104 → #1112 chain: the unit-level fix was correct,
but a related code path was affected in a way the isolated test couldn't catch.
**Blast radius concern for 7.x**
`GroovyProxyFactory` is the proxy strategy for all non-Hibernate datastores
(MongoDB, Neo4j, simple datastore used in GORM unit tests). `getProxiedClass()`
is consumed from `UniqueConstraint.processValidate` line 81, but also
potentially other callers. The simple datastore underpins the entire GORM unit
test suite — subtle behavioral changes there won't be caught without running
integration tests against real datastores. apache/grails-data-mapping#938 is a
reminder that even removing proxy meta-programming — a clearly beneficial
cleanup — required its own dedicated milestone to land safely.
**What would make me more comfortable merging this in 7.x**
Either of these would address the concern:
1. An integration test that triggers cascade validation through
`PersistentEntityValidator` with a `GroovyProxyFactory` proxy as the `target`
entity — the simple datastore (no Docker required) would suffice — and confirms
the NPE no longer occurs. The TCK in `grails-datamapping-core-test` already
runs `GroovyProxySpec` with `useGroovyProxyFactory: true/false` per the PR
description; if it already covers this path, pointing to the specific test
would close the concern.
2. Alternatively, a targeted fix at the call site:
`UniqueConstraint.processValidate` line 81 calls `getProxiedClass(target)` —
unwrapping `target` there (mirroring what `afd8bcd738` did for `propertyValue`
at line 121 to address apache/grails-data-mapping#1287) fixes the specific
crash path without touching `GroovyProxyFactory` itself for 7.x. The core fix
can still go into 8.x where the broader blast radius is more acceptable.
Can you confirm whether `GroovyProxySpec` in `grails-datamapping-core-test`
actually exercises `UniqueConstraint.processValidate` with a proxy as `target`,
or whether the 568 MongoDB test runs include a cascade-validation scenario with
an unwrapped association proxy?
--
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]
