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 19:03:02 UTC
[PR] Add unit tests and fix NPE bugs in grails-datamapping-core's jdbc package [grails-core]
borinquenkid opened a new pull request, #16153:
URL: https://github.com/apache/grails-core/pull/16153
## Summary
- Added Spock unit tests for every previously-untested class in `org.grails.datastore.gorm.jdbc` and its `connections` subpackage (DatabaseDriver, MultiTenantConnection, MultiTenantDataSource, PropertyOrigin, OriginCapablePropertyValue, RelaxedNames, RelaxedConversionService, RelaxedDataBinder, DataSourceSettings, DataSourceSettingsBuilder, DataSourceConnectionSource, DataSourceConnectionSourceFactory, CachedDataSourceConnectionSourceFactory, SpringDataSourceConnectionSourceFactory).
- Fixed a potential NPE in `DataSourceConnectionSource.close()`: `DelegatingDataSource.getTargetDataSource()` can return `null`, which previously caused an immediate NPE on the following `getClass()` call while unwrapping nested delegating data sources. Added a regression test.
- Renamed a shadowed local variable in `DataSourceSettings.toProperties()` (it collided with the field and the `@Builder`-generated fluent setter of the same name).
- Marked `SpringDataSourceConnectionSourceFactory.setApplicationContext`'s parameter `@NonNull`.
## Test plan
- [x] `./gradlew :grails-datamapping-core:test`
- [x] `./gradlew :grails-datamapping-core:codeStyle`
--
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 unit tests and fix NPE bugs in grails-datamapping-core's jdbc package [grails-core]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #16153:
URL: https://github.com/apache/grails-core/pull/16153#issuecomment-5303785734
<!-- Bito Reply -->
The requested file `OriginCapablePropertyValue` is not present in the provided pull request diff. As a result, I cannot analyze the `toString()` method or provide a regression test for the null-origin case.
--
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 unit tests and fix NPE bugs in grails-datamapping-core's jdbc package [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty merged PR #16153:
URL: https://github.com/apache/grails-core/pull/16153
--
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 unit tests and fix NPE bugs in grails-datamapping-core's jdbc package [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16153:
URL: https://github.com/apache/grails-core/pull/16153#issuecomment-5304579165
## β
All tests passed β
π·οΈ Commit: 899db777f8ea40c02bff7c7e58c7e60340e30ae9
βΆοΈ Tests: 63700 executed
βͺοΈ Checks: 77/77 completed
---
_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 unit tests and fix NPE bugs in grails-datamapping-core's jdbc package [grails-core]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #16153:
URL: https://github.com/apache/grails-core/pull/16153#issuecomment-5303948841
## [Codecov](https://app.codecov.io/gh/apache/grails-core/pull/16153?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 53.2424%. Comparing base ([`0613772`](https://app.codecov.io/gh/apache/grails-core/commit/0613772b365e00e19551542b43d958f04b4e7fc2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`73aac3d`](https://app.codecov.io/gh/apache/grails-core/commit/73aac3de406007fef406396bd5b012fa812bec56?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 276 commits behind head on 8.1.x.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/grails-core/pull/16153?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 #16153 +/- ##
==================================================
+ Coverage 52.8038% 53.2424% +0.4385%
- Complexity 18863 19427 +564
==================================================
Files 2079 2080 +1
Lines 97207 99002 +1795
Branches 16873 17364 +491
==================================================
+ Hits 51329 52711 +1382
- Misses 38468 38743 +275
- Partials 7410 7548 +138
```
| [Files with missing lines](https://app.codecov.io/gh/apache/grails-core/pull/16153?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [...m/jdbc/connections/DataSourceConnectionSource.java](https://app.codecov.io/gh/apache/grails-core/pull/16153?src=pr&el=tree&filepath=grails-datamapping-core%2Fsrc%2Fmain%2Fgroovy%2Forg%2Fgrails%2Fdatastore%2Fgorm%2Fjdbc%2Fconnections%2FDataSourceConnectionSource.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JhaWxzLWRhdGFtYXBwaW5nLWNvcmUvc3JjL21haW4vZ3Jvb3Z5L29yZy9ncmFpbHMvZGF0YXN0b3JlL2dvcm0vamRiYy9jb25uZWN0aW9ucy9EYXRhU291cmNlQ29ubmVjdGlvblNvdXJjZS5qYXZh) | `100.0000% <100.0000%> (+17.6471%)` | :arrow_up: |
| [...re/gorm/jdbc/connections/DataSourceSettings.groovy](https://app.codecov.io/gh/apache/grails-core/pull/16153?src=pr&el=tree&filepath=grails-datamapping-core%2Fsrc%2Fmain%2Fgroovy%2Forg%2Fgrails%2Fdatastore%2Fgorm%2Fjdbc%2Fconnections%2FDataSourceSettings.groovy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JhaWxzLWRhdGFtYXBwaW5nLWNvcmUvc3JjL21haW4vZ3Jvb3Z5L29yZy9ncmFpbHMvZGF0YXN0b3JlL2dvcm0vamRiYy9jb25uZWN0aW9ucy9EYXRhU291cmNlU2V0dGluZ3MuZ3Jvb3Z5) | `100.0000% <100.0000%> (+42.1053%)` | :arrow_up: |
| [...tions/SpringDataSourceConnectionSourceFactory.java](https://app.codecov.io/gh/apache/grails-core/pull/16153?src=pr&el=tree&filepath=grails-datamapping-core%2Fsrc%2Fmain%2Fgroovy%2Forg%2Fgrails%2Fdatastore%2Fgorm%2Fjdbc%2Fconnections%2FSpringDataSourceConnectionSourceFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JhaWxzLWRhdGFtYXBwaW5nLWNvcmUvc3JjL21haW4vZ3Jvb3Z5L29yZy9ncmFpbHMvZGF0YXN0b3JlL2dvcm0vamRiYy9jb25uZWN0aW9ucy9TcHJpbmdEYXRhU291cmNlQ29ubmVjdGlvblNvdXJjZUZhY3RvcnkuamF2YQ==) | `88.8889% <ΓΈ> (+88.8889%)` | :arrow_up: |
... and [92 files with indirect coverage changes](https://app.codecov.io/gh/apache/grails-core/pull/16153/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 unit tests and fix NPE bugs in grails-datamapping-core's jdbc package [grails-core]
Posted by "borinquenkid (via GitHub)" <gi...@apache.org>.
borinquenkid commented on code in PR #16153:
URL: https://github.com/apache/grails-core/pull/16153#discussion_r3790273989
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/OriginCapablePropertyValueSpec.groovy:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.jdbc
+
+import org.springframework.beans.PropertyValue
+import org.springframework.core.env.PropertySource
+import spock.lang.Specification
+
+class OriginCapablePropertyValueSpec extends Specification {
+
+ private static PropertySource<String> namedSource(String name) {
+ new PropertySource<String>(name, 'value') {
+ @Override
+ Object getProperty(String propertyName) { null }
+ }
+ }
+
+ void "getName and getValue expose the values passed to the constructor"() {
+ given:
+ def origin = new PropertyOrigin(namedSource('mySource'), 'original.name')
+
+ when:
+ def propertyValue = new OriginCapablePropertyValue('bound.name', 'boundValue', origin)
+
+ then:
+ propertyValue.name == 'bound.name'
+ propertyValue.value == 'boundValue'
+ }
+
+ void "toString includes the origin name and the source name"() {
+ given:
+ def origin = new PropertyOrigin(namedSource('myPropertySource'), 'original.name')
+ def propertyValue = new OriginCapablePropertyValue('bound.name', 'boundValue', origin)
+
+ expect:
+ propertyValue.toString() == "'original.name' from 'myPropertySource'"
+ }
+
+ void "toString reports an unknown source when the origin has no source"() {
+ given:
+ def origin = new PropertyOrigin(null, 'original.name')
+ def propertyValue = new OriginCapablePropertyValue('bound.name', 'boundValue', origin)
+
Review Comment:
Fixed in 3470867fbe: `toString()` now guards both `this.origin == null` and `this.origin.getSource() == null` before dereferencing, falling back to `getName()`/"unknown" respectively. Regression test added in the same commit: "toString does not fail when constructed with a null origin" in OriginCapablePropertyValueSpec.groovy.
--
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 unit tests and fix NPE bugs in grails-datamapping-core's jdbc package [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16153:
URL: https://github.com/apache/grails-core/pull/16153#issuecomment-5303815092
## π¨ 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 / Functional Tests \(Java 21, indy\=false, shard 1\)](https://github.com/apache/grails-core/actions/runs/31902823269/job/95056072319?pr=16153) > :grails-test-examples-app1:integrationTest
| Test | Runs | Flakiness |
|---|---|--:|
| RedirectWithAndWithoutParamsFunctionalSpec > Params are not added to the url after a redirect even if they are passed to the redirect | β | [1% π‘](https://github.com/apache/grails-core/issues/16030) |
π·οΈ Commit: 73aac3de406007fef406396bd5b012fa812bec56
βΆοΈ Tests: 8430 executed
π‘ Checks: 25/72 completed
### Test Failures
<details>
<summary><strong>RedirectWithAndWithoutParamsFunctionalSpec > Params are not added to the url after a redirect even if they are passed to the redirect</strong> (:grails-test-examples-app1:integrationTest in <a href="https://github.com/apache/grails-core/actions/runs/31902823269/job/95056072319?pr=16153">CI / Functional Tests (Java 21, indy=false, shard 1)</a>)</summary>
```
Condition not satisfied:
pageSource.contains('"id":')
| |
| false
<html><head></head><body><form action="save" method="post">
<input type="text" name="name">
<input type="submit">
</form>
</body></html>
at functionaltests.RedirectWithAndWithoutParamsFunctionalSpec.$tt__$spock_feature_1_0(RedirectWithAndWithoutParamsFunctionalSpec.groovy:41)
at functionaltests.RedirectWithAndWithoutParamsFunctionalSpec.Params are not added to the url after a redirect even if they are passed to the redirect_closure1(RedirectWithAndWithoutParamsFunctionalSpec.groovy)
at grails.gorm.transactions.GrailsTransactionTemplate$1.doInTransaction(GrailsTransactionTemplate.groovy:72)
at org.springframework.transaction.support.TransactionTemplate.execute(TransactionTemplate.java:137)
at grails.gorm.transactions.GrailsTransactionTemplate.executeAndRollback(GrailsTransactionTemplate.groovy:69)
at functionaltests.RedirectWithAndWithoutParamsFunctionalSpec.Params are not added to the url after a redirect even if they are passed to the redirect(RedirectWithAndWithoutParamsFunctionalSpec.groovy)
```
</details>
### Muted Tests
> [!NOTE]
> Checks are currently running using the configuration below.
Select tests to mute in this pull request:
π² RedirectWithAndWithoutParamsFunctionalSpec > Params are not added to the url after a redirect even if they are passed to the redirect <!-- uniqueId=[engine:spock]/[spec:functionaltests.RedirectWithAndWithoutParamsFunctionalSpec]/[feature:$spock_feature_1_0] -->
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 unit tests and fix NPE bugs in grails-datamapping-core's jdbc package [grails-core]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #16153:
URL: https://github.com/apache/grails-core/pull/16153#discussion_r3790052988
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/OriginCapablePropertyValueSpec.groovy:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.jdbc
+
+import org.springframework.beans.PropertyValue
+import org.springframework.core.env.PropertySource
+import spock.lang.Specification
+
+class OriginCapablePropertyValueSpec extends Specification {
+
+ private static PropertySource<String> namedSource(String name) {
+ new PropertySource<String>(name, 'value') {
+ @Override
+ Object getProperty(String propertyName) { null }
+ }
+ }
+
+ void "getName and getValue expose the values passed to the constructor"() {
+ given:
+ def origin = new PropertyOrigin(namedSource('mySource'), 'original.name')
+
+ when:
+ def propertyValue = new OriginCapablePropertyValue('bound.name', 'boundValue', origin)
+
+ then:
+ propertyValue.name == 'bound.name'
+ propertyValue.value == 'boundValue'
+ }
+
+ void "toString includes the origin name and the source name"() {
+ given:
+ def origin = new PropertyOrigin(namedSource('myPropertySource'), 'original.name')
+ def propertyValue = new OriginCapablePropertyValue('bound.name', 'boundValue', origin)
+
+ expect:
+ propertyValue.toString() == "'original.name' from 'myPropertySource'"
+ }
+
+ void "toString reports an unknown source when the origin has no source"() {
+ given:
+ def origin = new PropertyOrigin(null, 'original.name')
+ def propertyValue = new OriginCapablePropertyValue('bound.name', 'boundValue', origin)
+
Review Comment:
`OriginCapablePropertyValue.toString()` will throw an NPE when `origin` itself is `null` (it calls `this.origin.getSource()` without guarding `origin`). This can occur via the private wrapping constructor when a plain `PropertyValue` has no `propertyOrigin` attribute. Consider adding a regression test for the null-origin case here, and updating `OriginCapablePropertyValue.toString()` to handle `origin == null` safely (e.g., treat both name/source as unknown or fall back to `getName()`).
--
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