Repository navigation
Fix private trait field access in TraitPropertyAccessStrategy - #16627
MartinJesusDev wants to merge 6 commits into
Conversation
Groovy implements trait properties with a private, name-mangled field on the implementing class. TraitPropertyAccessStrategy resolved the property to that field but never made it accessible, so the first read or write through the strategy failed with PropertyAccessException caused by IllegalAccessException. Make the generated field accessible before building the field-based Getter/Setter, matching Hibernate's field access and FieldEntityAccess' reader/writer setup. Covers grails-data-hibernate5 and grails-data-hibernate7. Refs apache#16615 Generated-by: OpenCode (DeepSeek V4.1 Flash)
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Scope check for 8.0.x: one line per module, no API change, and it is a real bug with a reachable path, not only a strategy-in-isolation issue. The normal startup flow is masked because FieldEntityAccess makes the same cached Field instance (Spring's ReflectionUtils field cache) accessible before Hibernate builds its accessors. Spring clears that cache at the end of every context refresh, so a SessionFactory built after startup gets a fresh, inaccessible Field: a connection source added at runtime fails on the first read of a trait property on both modules with IllegalAccessException from GetterFieldImpl. On Hibernate 5 every trait property is affected because the default access type is FIELD; on Hibernate 7 it takes accessType: AccessType.FIELD on the property.
One ask: add that path as a GORM-flow test next to the direct one, since the direct test bypasses every public API and the runtime path is what applications can hit. I verified it fails on the base commit and passes with this change on both modules; the spec is inline.
| class TraitPropertyFieldAccessSpec extends Specification { | ||
|
|
||
| @Issue('https://github.com/apache/grails-core/issues/16615') | ||
| void "field-based access to a trait property reads back what the setter wrote"() { |
There was a problem hiding this comment.
Calling the strategy directly is the only way to see the failure at startup, so this is fine as the unit-level check, but it bypasses every public API. There is a path applications can hit: Spring clears its reflection cache at the end of context refresh (AbstractApplicationContext.resetCommonCaches), and a SessionFactory built after that, such as for a connection source added at runtime, gets a fresh inaccessible Field. This fails on the base commit and passes with your change on both modules:
@Shared @AutoCleanup HibernateDatastore datastore = new HibernateDatastore(DatastoreUtils.createPropertyResolver([
'dataSource.url': 'jdbc:h2:mem:traitFieldRuntime;LOCK_TIMEOUT=10000',
'dataSource.dbCreate': 'create-drop',
'hibernate.hbm2ddl.auto': 'create-drop']), RuntimeTraitEntity)
void "a trait property is readable on a connection source added after Spring cleared its reflection cache"() {
given: 'what Spring does at the end of every context refresh'
ReflectionUtils.clearCache()
when: 'a data source added at runtime builds a new SessionFactory'
datastore.connectionSources.addConnectionSource('later', [pooled: true, dbCreate: 'create-drop',
url: 'jdbc:h2:mem:traitFieldLater;LOCK_TIMEOUT=10000'])
Long id = null
RuntimeTraitEntity.later.withTransaction {
def entity = new RuntimeTraitEntity(name: 'n', bar: 'b')
GormEnhancer.findInstanceApi(RuntimeTraitEntity, 'later').save(entity, [flush: true, failOnError: true])
id = entity.id
}
String bar = null
RuntimeTraitEntity.later.withNewSession { bar = RuntimeTraitEntity.later.get(id).bar }
then:
bar == 'b'
}
trait RuntimeTrait {
String bar
}
@Entity
class RuntimeTraitEntity implements RuntimeTrait {
String name
static mapping = {
datasource 'ALL'
bar accessType: AccessType.FIELD // needed on Hibernate 7, where the default access type is PROPERTY
}
}(imports: org.grails.datastore.gorm.GormEnhancer, org.grails.datastore.mapping.core.DatastoreUtils, org.springframework.util.ReflectionUtils, jakarta.persistence.AccessType.) Without the fix the first read fails with PropertyAccessException caused by IllegalAccessException: class org.hibernate.property.access.spi.GetterFieldImpl cannot access a member of class ...RuntimeTraitEntity with modifiers "private". Putting it under grails.gorm.tests.traits next to TraitPropertySpec keeps it with the other end-to-end trait coverage.
There was a problem hiding this comment.
Added in 46d9c54 as TraitPropertyRuntimeConnectionSourceSpec; verified on both modules that it fails with the fix reverted and passes with it.
…clear Spring clears its reflection cache at the end of every context refresh, so a SessionFactory built afterwards (a connection source added at runtime) gets a fresh, inaccessible generated trait field, and the first trait property read fails. Add an end-to-end GORM spec for that path on both modules, and fold the direct set/get check into the existing TraitPropertyAccessStrategySpec on Hibernate 7. Refs apache#16615 Generated-by: OpenCode (DeepSeek V4.1 Flash)
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Round 2 at 46d9c54: both asks from the first round are in. Verified locally on both modules at this head: grails.gorm.tests.traits.* and org.grails.orm.hibernate.access.* pass and codeStyle is clean. With the one-line makeAccessible change reverted, the two runtime connection-source specs and the two direct round-trip features fail with IllegalAccessException from GetterFieldImpl/SetterFieldImpl on both modules, so the coverage is tied to the fix. One optional note inline; nothing blocking.
| Long id = null | ||
| RuntimeTraitEntity.later.withTransaction { | ||
| def entity = new RuntimeTraitEntity(name: 'n', bar: 'b') | ||
| GormEnhancer.findInstanceApi(RuntimeTraitEntity, 'later').save(entity, [flush: true, failOnError: true]) |
There was a problem hiding this comment.
Optional, and this one is on me since the snippet I posted used it: GormEnhancer.findInstanceApi is not needed here. On 8.0.x an instance saved inside a named connection's own transaction is written to that connection (MultipleDataSourceConnectionsSpec covers it: "an instance saved inside a named connection's own transaction is written to that connection"), so this can be the plain end-user call:
entity.save(flush: true, failOnError: true)and the GormEnhancer import goes away. I ran both specs that way and they pass; the read-back through RuntimeTraitEntity.later.get(id) confirms the row lands on later. Same for the identical Hibernate 7 copy.
| class TraitPropertyFieldAccessSpec extends Specification { | ||
|
|
||
| @Issue('https://github.com/apache/grails-core/issues/16615') | ||
| void "field-based access to a trait property reads back what the setter wrote"() { |
There was a problem hiding this comment.
Added in 46d9c54 as TraitPropertyRuntimeConnectionSourceSpec; verified on both modules that it fails with the fix reverted and passes with it.
|
@MartinJesusDev if you want to do the optional changes, please proceed. Otherwie, I'll merge sometime tomorrow. |
GormEnhancer.findInstanceApi is not needed: an instance saved inside a named connection's own transaction is written to that connection, so the specs use entity.save(flush: true, failOnError: true) and drop the GormEnhancer import. Refs apache#16615 Generated-by: OpenCode (DeepSeek V4.1 Flash)
|
@jdaugherty Thanks for the review. Applied the optional change and updated the branch with the latest |
🚨 TestLens detected 3 failed tests 🚨Test SummaryTip Inspect all details with the TestLens CLI: CI / Build Grails-Core (Windows JDK 25 shard 2) > :grails-core-cli-legacy:test
CI / Build Grails-Core (Windows JDK 25 shard 2) > :grails-core:testCli
CI / Build Grails-Core (Windows JDK 25 shard 2) > :grails-shell-cli:test
🏷️ Commit: 2fecc7d Test FailuresFileSystemInteractionImplSpec > files that do not exist yet are resolved like existing ones (:grails-core-cli-legacy:test in CI / Build Grails-Core (Windows JDK 25 shard 2))
FileSystemInteractionImplSpec > files that do not exist yet are resolved like existing ones (:grails-core:testCli in CI / Build Grails-Core (Windows JDK 25 shard 2))
FileSystemInteractionImplSpec > files that do not exist yet are resolved like existing ones (:grails-shell-cli:test in CI / Build Grails-Core (Windows JDK 25 shard 2))
Rerun ControlsSelect tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app/docs. |
This branch has not been deployed
2 participants
Description
TraitPropertyAccessStrategy(grails-data-hibernate5andgrails-data-hibernate7) resolves atrait-backed property to the private, name-mangled field Groovy generates on the implementing
class and builds a field-based
Getter/Setterover it. The field is obtained with Spring'sReflectionUtils.findField, which does not make it accessible, so the first read or write throughthe strategy fails with
PropertyAccessExceptioncaused byIllegalAccessException.This change makes the generated field accessible before the accessors are built, matching what
Hibernate's own field access path (
ReflectHelper.findField) and Grails'FieldEntityAccess.ReaderAndWriterMakeralready do for the same field. Both modules are fixed inthis PR.
The GORM-flow spec covers the runtime path applications can hit: Spring clears its reflection cache
at the end of each context refresh, so a SessionFactory built afterwards (a connection source added
at runtime) sees a fresh, inaccessible generated field, and the first trait property read fails.
Fixes #16615
Contributor Checklist
Issue and Scope
8.0.x(bug fix only, no API changes).Code Quality
TraitPropertyFieldAccessSpeconHibernate 5, folded into
TraitPropertyAccessStrategySpecon Hibernate 7) plus a GORM-flowspec (
TraitPropertyRuntimeConnectionSourceSpecin both modules) for a connection sourceadded after Spring clears its reflection cache. Both fail with
PropertyAccessException/IllegalAccessExceptionbefore the fix and pass after it.8.0.xbase:grails-data-hibernate5full suite passes (1100tests / 0 failures), and the runtime connection-source spec plus the direct (Hibernate 5)
and folded (Hibernate 7) round-trip features pass on both modules. The full repository
check (
clean aggregateViolations :grails-test-report:check --continue) runs in CI on thishead.
authoring and verification; the output was reviewed and verified by the submitter.
Licensing and Attribution
Documentation
update is required.
Generated-by: OpenCode (DeepSeek V4.1 Flash)