Add unit tests and fix NPE bugs in grails-datamapping-core's jdbc package by borinquenkid · Pull Request #16153 · apache/grails-core

borinquenkid · 2026-08-15T19:02:59Z

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

./gradlew :grails-datamapping-core:test
./gradlew :grails-datamapping-core:codeStyle

Every previously-untested class under org.grails.datastore.gorm.jdbc (DatabaseDriver, MultiTenantConnection, MultiTenantDataSource, PropertyOrigin, OriginCapablePropertyValue, RelaxedNames, RelaxedConversionService, RelaxedDataBinder) and its connections subpackage (DataSourceSettings, DataSourceSettingsBuilder, DataSourceConnectionSource, DataSourceConnectionSourceFactory, CachedDataSourceConnectionSourceFactory, SpringDataSourceConnectionSourceFactory) now has a Spock spec. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ng a DelegatingDataSource DelegatingDataSource.getTargetDataSource() can return null when no target has been set, which previously caused an immediate NullPointerException on the following source.getClass() call. Break out of the unwrap loop instead and add a regression test covering the scenario. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@builder

The local variable was named 'properties', identical to the properties field and to the fluent properties(Map) setter that @builder(prefix = '') generates for it, which confused IntelliJ's definite-assignment analysis into flagging it as possibly unassigned. Renaming it to 'props' removes the shadowing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot

Pull request overview

This pull request extends automated coverage for grails-datamapping-core’s JDBC support by adding Spock unit tests across org.grails.datastore.gorm.jdbc and org.grails.datastore.gorm.jdbc.connections, and includes small correctness/clarity improvements in the connection-source implementation.

Changes:

Added new Spock specs covering relaxed name/binding/conversion utilities plus JDBC connection/data source wrappers and connection-source factories.
Fixed a potential NPE in DataSourceConnectionSource.close() when unwrapping a DelegatingDataSource with no target.
Refined small API/implementation details (DataSourceSettings.toProperties() local var rename; @NonNull on setApplicationContext parameter).

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/RelaxedNamesSpec.groovy	Adds unit coverage for `RelaxedNames` variation generation and iteration behavior.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/RelaxedDataBinderSpec.groovy	Adds unit coverage for relaxed property binding (dash/underscore aliases, nested binding, map binding).
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/RelaxedConversionServiceSpec.groovy	Adds unit coverage for relaxed enum conversion and fallback converters.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/PropertyOriginSpec.groovy	Adds unit coverage for `PropertyOrigin` construction and null-source behavior.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/OriginCapablePropertyValueSpec.groovy	Adds unit coverage for origin-aware property value behavior and origin extraction.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/MultiTenantDataSourceSpec.groovy	Adds unit coverage for tenant-aware `DataSource` wrapper delegation and equality/hashCode.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/MultiTenantConnectionSpec.groovy	Adds unit coverage for schema restore behavior on close and delegation to target connection.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/DatabaseDriverSpec.groovy	Adds unit coverage for driver detection from JDBC URLs/product names and constant metadata.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/connections/SpringDataSourceConnectionSourceFactorySpec.groovy	Adds unit coverage for Spring-backed lookup and fallback DataSource creation.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/connections/DataSourceSettingsSpec.groovy	Adds unit coverage for default settings plus property mapping helpers.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/connections/DataSourceSettingsBuilderSpec.groovy	Adds unit coverage for settings resolution from configuration prefixes.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/connections/DataSourceConnectionSourceSpec.groovy	Adds unit coverage for `close()` behavior (reflection close, unwrapping, exception swallowing).
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/connections/DataSourceConnectionSourceFactorySpec.groovy	Adds unit coverage for factory creation paths and proxy wrapping based on settings.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/jdbc/connections/CachedDataSourceConnectionSourceFactorySpec.groovy	Adds unit coverage for caching behavior by connection source name.
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/connections/SpringDataSourceConnectionSourceFactory.java	Annotates `setApplicationContext` parameter as non-null.
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/connections/DataSourceSettings.groovy	Renames local variable in `toProperties()` to avoid shadowing.
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/connections/DataSourceConnectionSource.java	Prevents NPE when unwrapping a `DelegatingDataSource` that has a null target.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

borinquenkid · 2026-08-15T21:12:11Z

+    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)
+


Fixed in 3470867: 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.

bito-code-review · 2026-08-15T19:07:35Z

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.

@SInCE

- Remove a blank Javadoc line ignored between the @author and @SInCE tags - Make the never-reassigned classLoader and properties fields final - Replace raw Map usage in coerceDbProperties/flattenMap with pattern-matching instanceof over wildcard-typed Maps, dropping the now-unneeded @SuppressWarnings("unchecked") on flattenMap - Annotate the ReadOnlyDriverManagerDataSource override of Spring's @NullMarked getConnectionFromDriverManager with matching @nonnull annotations Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@nonnull

- Remove namePrefix/ignoreNestedProperties: private fields with no setter that were always null/false, making getPropertyValuesForNamePrefix() and stripLastDot() dead code; deleted along with the now-pointless call. - Make the never-reassigned nameAliases field final. - Annotate the doBind(MutablePropertyValues) override's parameter @nonnull to match Spring's @NullMarked DataBinder.doBind. - Fix a malformed {@code[...]} Javadoc tag (missing space). - Guard against a null PropertyValue from getPropertyValue() with Objects.requireNonNull. - Drop a provably-always-true null check in isMapValueStringType. - Simplify isBlanked's nested if into a single pattern-matching return, dropping the now-unneeded @SuppressWarnings("rawtypes"). - Null-check propertyTypeDescriptor/elementTypeDescriptor in extendCollectionIfNecessary, which could NPE for raw/untyped collections; default a null element descriptor to Object, matching the existing behaviour in extendMapIfNecessary. - Drop a redundant regex escape on '.' inside a character class. - Replace candidate.length() > 0 with !candidate.isEmpty(). - Drop BeanPath.range()'s always-0 start parameter; its only caller always passed 0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

codecov · 2026-08-15T19:50:21Z

Codecov Report

❌ Patch coverage is 72.22222% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.2524%. Comparing base (0613772) to head (899db77).
⚠️ Report is 276 commits behind head on 8.1.x.

Files with missing lines	Patch %	Lines
.../grails/datastore/gorm/jdbc/RelaxedDataBinder.java	42.8571%	6 Missing and 2 partials ⚠️
.../grails/datastore/gorm/jdbc/DataSourceBuilder.java	80.0000%	0 Missing and 1 partial ⚠️
.../datastore/gorm/jdbc/RelaxedConversionService.java	50.0000%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.1.x     #16153        +/-   ##
==================================================
+ Coverage     52.8038%   53.2524%   +0.4486%     
- Complexity      18863      19423       +560     
==================================================
  Files            2079       2080         +1     
  Lines           97207      98974      +1767     
  Branches        16873      17357       +484     
==================================================
+ Hits            51329      52706      +1377     
- Misses          38468      38720       +252     
- Partials         7410       7548       +138

Files with missing lines	Coverage Δ
...atastore/gorm/jdbc/OriginCapablePropertyValue.java	`100.0000% <100.0000%> (+100.0000%)`	⬆️
...org/grails/datastore/gorm/jdbc/PropertyOrigin.java	`100.0000% <100.0000%> (+100.0000%)`	⬆️
...y/org/grails/datastore/gorm/jdbc/RelaxedNames.java	`96.8750% <100.0000%> (+3.1250%)`	⬆️
...m/jdbc/connections/DataSourceConnectionSource.java	`100.0000% <100.0000%> (+17.6471%)`	⬆️
...re/gorm/jdbc/connections/DataSourceSettings.groovy	`100.0000% <100.0000%> (+42.1053%)`	⬆️
...tions/SpringDataSourceConnectionSourceFactory.java	`88.8889% <ø> (+88.8889%)`	⬆️
.../grails/datastore/gorm/jdbc/DataSourceBuilder.java	`87.2093% <80.0000%> (-0.1470%)`	⬇️
.../datastore/gorm/jdbc/RelaxedConversionService.java	`86.0465% <50.0000%> (+64.3074%)`	⬆️
.../grails/datastore/gorm/jdbc/RelaxedDataBinder.java	`67.2131% <42.8571%> (+11.1525%)`	⬆️

... and 87 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@NullMarked

…te its @NullMarked override toString() dereferenced this.origin.getSource() without checking whether this.origin itself was null, which it legitimately can be (e.g. when constructed from a plain PropertyValue with no propertyOrigin attribute). Guard both null cases before falling back to "unknown". Also annotate the override with @nonnull to match Spring's @NullMarked PropertyValue.toString(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

It was a plain immutable data holder (two final fields, a constructor, two getters) with no behavior of its own — exactly what records exist for. Update its one caller (OriginCapablePropertyValue.toString()) to use the generated source()/name() accessors instead of getSource()/getName(). Groovy resolves record canonical accessors via the usual .source/.name property syntax, so the existing specs needed no changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@NullMarked

…ing's @NullMarked contracts - RelaxedNames.iterator(): annotate the Iterable<String> override's return type @nonnull. - RelaxedConversionService: annotate canConvert/convert overrides to match ConversionService's exact JSpecify signatures (nullable sourceType/source parameters, nullable convert() return types). Also reindent the StringToEnum record's convert() method body, which was left misindented by an in-progress manual conversion from an inner class to a record. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@NullMarked

…ested classes StringToEnumIgnoringCaseConverterFactory.getConverter() and the StringToEnum record's convert() were still unannotated overrides of Spring's @NullMarked ConverterFactory/Converter contracts. StringToEnum.convert() legitimately returns null for an empty identifier (resets the enum value), so its return type and the Converter<String, T> type argument now carry @nullable to match; getConverter()'s return type is widened to Converter<String, ? extends @nullable T> to match ConverterFactory's exact signature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

testlens-app · 2026-08-15T22:42:23Z

✅ All tests passed ✅

🏷️ Commit: 899db77
▶️ Tests: 63700 executed
⚪️ Checks: 79/79 completed

Learn more about TestLens at testlens.app.

Copilot

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

Suppressed comments (2)

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/RelaxedConversionService.java:112

StringToEnumIgnoringCaseConverterFactory.getConverter changes the return type to Converter<String, ? extends @Nullable T>, which is not assignment-compatible with ConverterFactory's required Converter<String, T> and can break compilation/override compatibility. The nullability can be expressed via the converter implementation’s convert return annotation without changing the generic return type.

        public <T extends Enum> Converter<String, ? extends @Nullable T> getConverter(Class<T> targetType) {
            Class<?> enumType = targetType;
            while (enumType != null && !enumType.isEnum()) {
                enumType = enumType.getSuperclass();
            }

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/jdbc/DataSourceBuilder.java:143

properties is declared as Map<String, String>, but coerceDbProperties stores a Properties instance into it (propertiesMap.put(keyname, properties)). This makes the field’s generic type inaccurate and forces unchecked casts, and it risks runtime ClassCastException if future code assumes all values are String. Consider changing the field (and related setters/reads) to Map<String, Object> so both String and Properties values are represented safely.

    @SuppressWarnings("unchecked")
    private void coerceDbProperties(String keyname) {
        Map<String, Object> propertiesMap = (Map<String, Object>) (Map<?, ?>) this.properties;
        Object dbPropertiesObject = propertiesMap.get(keyname);
        if (dbPropertiesObject instanceof Map<?, ?> dbProperties) {

Copilot

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

borinquenkid and others added 3 commits August 15, 2026 11:35

Copilot AI lite review requested due to automatic review settings August 15, 2026 19:03

github-project-automation Bot added this to Apache Grails Aug 15, 2026

Copilot started reviewing on behalf of borinquenkid August 15, 2026 19:03 View session

borinquenkid added this to the grails:8.1.0-M1 milestone Aug 15, 2026

Copilot AI reviewed Aug 15, 2026

View reviewed changes

borinquenkid and others added 2 commits August 15, 2026 14:08

borinquenkid and others added 5 commits August 15, 2026 14:59

Remove blank Javadoc line in DataSourceBuilder class comment

899db77

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

borinquenkid requested review from jdaugherty and a lite review from Copilot August 16, 2026 01:08

Copilot started reviewing on behalf of borinquenkid August 16, 2026 01:09 View session

Copilot AI reviewed Aug 16, 2026

View reviewed changes

borinquenkid requested a lite review from Copilot August 16, 2026 15:27

borinquenkid moved this to In Progress in Apache Grails Aug 16, 2026

Copilot started reviewing on behalf of borinquenkid August 16, 2026 15:28 View session

Copilot AI reviewed Aug 16, 2026

View reviewed changes

Navigation Menu

Uh oh!

Uh oh!

Add unit tests and fix NPE bugs in grails-datamapping-core's jdbc package - #16153

Add unit tests and fix NPE bugs in grails-datamapping-core's jdbc package#16153
borinquenkid wants to merge 10 commits into
8.1.xfrom
chore/jdbc-package-cleanup

borinquenkid commented Aug 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

borinquenkid Aug 15, 2026

Uh oh!

bito-code-review Bot commented Aug 15, 2026

Uh oh!

codecov Bot commented Aug 15, 2026 •

edited

Loading

Uh oh!

testlens-app Bot commented Aug 15, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

borinquenkid commented Aug 15, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

borinquenkid Aug 15, 2026

Choose a reason for hiding this comment

Uh oh!

bito-code-review Bot commented Aug 15, 2026

Uh oh!

codecov Bot commented Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

testlens-app Bot commented Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ All tests passed ✅

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Aug 15, 2026 •

edited

Loading

testlens-app Bot commented Aug 15, 2026 •

edited

Loading