You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@grails.apache.org by "borinquenkid (via GitHub)" <gi...@apache.org> on 2026/08/15 23:54:11 UTC
[PR] Add proxy package test coverage and fix IntelliJ inspections in grails-datamapping-core [grails-core]
borinquenkid opened a new pull request, #16155:
URL: https://github.com/apache/grails-core/pull/16155
## Summary
- Adds unit test coverage for `GroovyProxyFactory` and `ProxyInstanceMetaClass` in `grails-datamapping-core`, which previously had none in this module despite implementing the core Groovy-based proxy contract used outside Hibernate/Neo4j.
- Fixes IntelliJ inspection warnings in both classes: methods with no instance-state dependency marked `static`, replaces deprecated `Class.newInstance()` with `getDeclaredConstructor().newInstance()`, makes never-reassigned fields `final`, suppresses the unchecked-generics warning inherent to `MetaClass.getTheClass()`'s raw return type, and fills in missing Javadoc tag descriptions.
## Test plan
- [x] `./gradlew :grails-datamapping-core:test --tests "org.grails.datastore.gorm.proxy.*"` — 29 tests pass
- [x] `./gradlew :grails-datamapping-core:test` — full module suite passes
- [x] `./gradlew :grails-datamapping-core:codeStyle` — no Checkstyle/CodeNarc violations
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add proxy package test coverage and fix IntelliJ inspections in grails-datamapping-core [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16155:
URL: https://github.com/apache/grails-core/pull/16155#issuecomment-5305294095
## 🚨 TestLens detected 1 failed test 🚨
Here is what you can do:
1) Inspect the test failures carefully.
2) If you are convinced that some of the tests are flaky, you can mute them below.
3) Finally, trigger a rerun by checking the rerun checkbox.
### Test Summary
#### [CI / Build Grails-Core \(macOS JDK 21\)](https://github.com/apache/grails-core/actions/runs/31918682230/job/95094848862?pr=16155) > :grails-wrapper:test
| Test | Runs | Flakiness |
|---|---|--:|
| GrailsUpdaterSpec > remote wrapper rejects HTTP redirect responses | ❌ | 0% 🟢 |
🏷️ Commit: 55d4b8116b770ab51736467ced8080fe89863422
▶️ Tests: 61168 executed
🟡 Checks: 68/73 completed
### Test Failures
<details>
<summary><strong>GrailsUpdaterSpec > remote wrapper rejects HTTP redirect responses</strong> (:grails-wrapper:test in <a href="https://github.com/apache/grails-core/actions/runs/31918682230/job/95094848862?pr=16155">CI / Build Grails-Core (macOS JDK 21)</a>)</summary>
```
Condition not satisfied:
server.verify()
| |
| false
<io.github.cjstehno.ersatz.GroovyErsatzServer@3ad49704 underlyingServer=inaccessible serverConfig=inaccessible>
at grails.init.GrailsUpdaterSpec.remote wrapper rejects HTTP redirect responses(GrailsUpdaterSpec.groovy:87)
```
</details>
### Muted Tests
> [!NOTE]
> Checks are currently running using the configuration below.
Select tests to mute in this pull request:
🔲 GrailsUpdaterSpec > remote wrapper rejects HTTP redirect responses <!-- uniqueId=[engine:spock]/[spec:grails.init.GrailsUpdaterSpec]/[feature:$spock_feature_0_3] -->
Reuse successful test results:
🔲 ♻️ Only rerun the tests that failed or were muted before
Click the checkbox to trigger a rerun:
🔲 **Rerun jobs**
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add proxy package test coverage and fix IntelliJ inspections in grails-datamapping-core [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty merged PR #16155:
URL: https://github.com/apache/grails-core/pull/16155
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add proxy package test coverage and fix IntelliJ inspections in grails-datamapping-core [grails-core]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #16155:
URL: https://github.com/apache/grails-core/pull/16155#issuecomment-5304958407
## [Codecov](https://app.codecov.io/gh/apache/grails-core/pull/16155?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `73.33333%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 53.1856%. Comparing base ([`df424da`](https://app.codecov.io/gh/apache/grails-core/commit/df424da847851e0c366a5ffe0c259f1ba22dfc5b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`06b346f`](https://app.codecov.io/gh/apache/grails-core/commit/06b346fcf7a18f0692fc4373a7e3914f1c824420?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/grails-core/pull/16155?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...s/datastore/gorm/proxy/ProxyInstanceMetaClass.java](https://app.codecov.io/gh/apache/grails-core/pull/16155?src=pr&el=tree&filepath=grails-datamapping-core%2Fsrc%2Fmain%2Fgroovy%2Forg%2Fgrails%2Fdatastore%2Fgorm%2Fproxy%2FProxyInstanceMetaClass.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JhaWxzLWRhdGFtYXBwaW5nLWNvcmUvc3JjL21haW4vZ3Jvb3Z5L29yZy9ncmFpbHMvZGF0YXN0b3JlL2dvcm0vcHJveHkvUHJveHlJbnN0YW5jZU1ldGFDbGFzcy5qYXZh) | 71.4286% | [1 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/grails-core/pull/16155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/grails-core/pull/16155?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## 8.1.x #16155 +/- ##
==================================================
+ Coverage 53.1444% 53.1856% +0.0412%
- Complexity 19371 19404 +33
==================================================
Files 2080 2080
Lines 99000 98991 -9
Branches 17363 17355 -8
==================================================
+ Hits 52613 52649 +36
+ Misses 38828 38788 -40
+ Partials 7559 7554 -5
```
| [Files with missing lines](https://app.codecov.io/gh/apache/grails-core/pull/16155?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ils/datastore/gorm/proxy/GroovyProxyFactory.groovy](https://app.codecov.io/gh/apache/grails-core/pull/16155?src=pr&el=tree&filepath=grails-datamapping-core%2Fsrc%2Fmain%2Fgroovy%2Forg%2Fgrails%2Fdatastore%2Fgorm%2Fproxy%2FGroovyProxyFactory.groovy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JhaWxzLWRhdGFtYXBwaW5nLWNvcmUvc3JjL21haW4vZ3Jvb3Z5L29yZy9ncmFpbHMvZGF0YXN0b3JlL2dvcm0vcHJveHkvR3Jvb3Z5UHJveHlGYWN0b3J5Lmdyb292eQ==) | `80.4348% <100.0000%> (+36.9565%)` | :arrow_up: |
| [...s/datastore/gorm/proxy/ProxyInstanceMetaClass.java](https://app.codecov.io/gh/apache/grails-core/pull/16155?src=pr&el=tree&filepath=grails-datamapping-core%2Fsrc%2Fmain%2Fgroovy%2Forg%2Fgrails%2Fdatastore%2Fgorm%2Fproxy%2FProxyInstanceMetaClass.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JhaWxzLWRhdGFtYXBwaW5nLWNvcmUvc3JjL21haW4vZ3Jvb3Z5L29yZy9ncmFpbHMvZGF0YXN0b3JlL2dvcm0vcHJveHkvUHJveHlJbnN0YW5jZU1ldGFDbGFzcy5qYXZh) | `89.1304% <71.4286%> (+41.8577%)` | :arrow_up: |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/grails-core/pull/16155/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add proxy package test coverage and fix IntelliJ inspections in grails-datamapping-core [grails-core]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #16155:
URL: https://github.com/apache/grails-core/pull/16155#discussion_r3790543072
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/proxy/GroovyProxyFactory.groovy:
##########
@@ -70,8 +71,8 @@ class GroovyProxyFactory implements ProxyFactory {
}
}
- @groovy.transform.CompileDynamic
- protected Serializable getIdDynamic(obj) {
+ @CompileDynamic
+ protected static Serializable getIdDynamic(obj) {
Review Comment:
Changing this `protected` helper from an instance method to `static` is a source/binary breaking change for any downstream subclasses that override it (static methods are not polymorphic). If this change was only to satisfy IntelliJ’s “method may be static” inspection, prefer keeping it as an instance method and suppressing the inspection (there is precedent in the repo, e.g. `grails-core/src/main/groovy/grails/boot/GrailsBanner.groovy:107`).
This issue also appears in the following locations of the same file:
- line 125
- line 165
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add proxy package test coverage and fix IntelliJ inspections in grails-datamapping-core [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on PR #16155:
URL: https://github.com/apache/grails-core/pull/16155#issuecomment-5924966623
per weekly, we've pulled this into the 8.0.x branch.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add proxy package test coverage and fix IntelliJ inspections in grails-datamapping-core [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on PR #16155:
URL: https://github.com/apache/grails-core/pull/16155#issuecomment-5919759250
I see a good case to move this from 8.1.x to 8.0.0.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add proxy package test coverage and fix IntelliJ inspections in grails-datamapping-core [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add proxy package test coverage and fix IntelliJ inspections in grails-datamapping-core [grails-core]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #16155:
URL: https://github.com/apache/grails-core/pull/16155#issuecomment-5304823724
<!-- Bito Reply -->
The concern regarding the conversion of `protected` instance methods to `static` is valid. In Groovy, changing an instance method to `static` breaks binary compatibility for any subclasses that override these methods, as static methods are not polymorphic and cannot be overridden.
If these changes were made solely to satisfy static analysis inspections, it is recommended to revert them to instance methods and suppress the inspection, consistent with existing patterns in the repository.
**grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/proxy/GroovyProxyFactory.groovy**
```
@CompileDynamic
protected Serializable getIdDynamic(obj) {
if (obj.respondsTo('getId')) {
return (Serializable)obj.invokeMethod('getId', null)
}
```
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add proxy package test coverage and fix IntelliJ inspections in grails-datamapping-core [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16155:
URL: https://github.com/apache/grails-core/pull/16155#issuecomment-5916220089
## 🚨 All tests passed but jobs failed 🚨
### Failed Jobs without Test Failures
❌ [Coverage / Coverage - grails-gradle \(ubuntu-24.04\)](https://github.com/apache/grails-core/actions/runs/36743994423/job/109985385252?pr=16155)
🏷️ Commit: 7b33002957bce1c674577316adaa3b9815e258a6
▶️ Tests: 20130 executed
🟡 Checks: 12/80 completed
### Rerun Controls
Click the checkbox to trigger a rerun:
🔲 **Rerun jobs**
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org