Skip to content

Test AbstractDatastoreInitializer and its production subclasses; fix bugs found along the way - #16147

Open
borinquenkid wants to merge 11 commits into
8.1.xfrom
test/abstract-datastore-initializer
Open

Test AbstractDatastoreInitializer and its production subclasses; fix bugs found along the way#16147
borinquenkid wants to merge 11 commits into
8.1.xfrom
test/abstract-datastore-initializer

Conversation

@borinquenkid

Copy link
Copy Markdown
Member

Summary

  • Adds unit coverage for AbstractDatastoreInitializer via a new shared TestDatastoreInitializer test double.
  • Closes test coverage gaps in the three production subclasses (Hibernate5, Hibernate7, MongoDB). grails-data-neo4j was excluded: it's not part of the root build, its README says it hasn't been updated for the current release, and its own standalone build is broken.
  • Fixes four real bugs found while writing that coverage, all following the same pattern - a customizable field silently ignored because a bean-registration/config-lookup site hardcoded a literal instead:
    • MongoDbDataStoreSpringInitializer.mongoBeanName ignored when building a new MongoClient from scratch.
    • MongoDbDataStoreSpringInitializer.databaseName ignored entirely - actively broken for real Grails+MongoDB apps via MongodbGrailsPlugin, which sets it expecting a default database name.
    • HibernateDatastoreSpringInitializer.defaultDataSourceBeanName hardcoded around instead of used, in both Hibernate5 and Hibernate7.
    • HibernateDatastoreSpringInitializer.enableReload ignored entirely - HibernateGrailsPlugin sets it to enable dev-mode reload by default, but it never reached HibernateConnectionSourceSettings.
  • Removes dead code identified via git archaeology (properties set by real plugin code but never read anywhere, with no surviving mechanism to restore): registerApplicationIfNotPresent (base class, dead since a 2018 simplification commit) and grailsPlugin (Hibernate initializers, dead since a January 2017 refactor). Also removes getTestDbUrl(), defaultSessionFactoryBeanName, and SESSION_FACTORY_BEAN_NAME from both Hibernate initializers - confirmed unused anywhere in the reachable codebase.

Test plan

  • grails-datamapping-core full test suite + codeStyle (Checkstyle/CodeNarc)
  • grails-data-hibernate7 full test suite + codeStyle
  • grails-data-hibernate5 full test suite + codeStyle
  • grails-data-mongodb-core full test suite (real MongoDB via Testcontainers) + codeStyle
  • grails-data-mongodb (grails-plugin) compiles clean
  • Every bug fix has a dedicated regression test, including two verified end-to-end against a live H2-backed HibernateDatastore/live MongoDB container, not just unit-level

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

borinquenkid and others added 10 commits August 13, 2026 13:34
…arnings

Introduces a shared TestDatastoreInitializer test double and a new
AbstractDatastoreInitializerSpec covering constructors, event/message
publisher resolution, mapped-class filtering, bean registration helpers,
data-service discovery and a full configure() round trip. The existing
web-application spec is refactored to reuse the shared double instead of
its own private copy.

Also resolves several IDE-flagged issues in AbstractDatastoreInitializer:
replaces the deprecated Class#newInstance() calls with
getDeclaredConstructor().newInstance(), names previously-unused catch
parameters, swaps an equals() call for ==, types the loadDataServices
closure parameters, and makes containsRegisteredBean/getGrailsValidatorClass
static since neither depends on instance state. getCommonConfiguration,
getGrailsApplicationClass and isGrailsPresent are left as instance methods
(with explanatory @SuppressWarnings) since they are genuine override hooks
for downstream datastore initializers.

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

Adds tests for branches of the three production AbstractDatastoreInitializer
subclasses that were previously unexercised: the OSIV interceptor
registration branch and configureForDataSource(DataSource) for both
Hibernate5 and Hibernate7, Hibernate7's IllegalStateException guard when
hibernateDatastore fails to register, and the Map/Collection<Class>
constructor form for both.

For MongoDB, covers the mongo != null branch (reusing a pre-existing
MongoClient) in both configure() and getBeanDefinitions(), the
package-scanning constructor, and adds a new Docker-free
MongoDbDataStoreSpringInitializerUnitSpec covering the isMappedClass/
collectMappedClasses mixed-entity filtering and the deprecated setters
that the Docker-backed spec never reaches.

grails-data-neo4j was excluded: it is not part of the root build, its
README states it hasn't been updated for the current release, and its
standalone build is broken, so no tests could be written or verified
there.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…esolver constructor coverage

Fixes a real unchecked-cast issue in getPersistenceInterceptorClass() (it
was missing the cast that the Hibernate7 sibling already has), and removes
two genuinely dead members: the never-invoked getTestDbUrl() method and the
defaultSessionFactoryBeanName property, whose value was never actually read
by the sessionFactory bean registration it appeared to configure.

Rather than silence the "unused constructor" warnings on the
PropertyResolver-based constructors, adds direct tests for all three - they
are exercised in production only through a dynamically-typed call site in
HibernateGrailsPlugin, which static analysis can't resolve to a specific
overload. Also swaps a couple of ad-hoc H2 URLs (introduced in the prior
commit, one already merged for hibernate7) for the class's own TEST_DB_URL
constant.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Its only consumer, defaultSessionFactoryBeanName, was removed in the
prior commit for being unwired/no-op, leaving this constant unused too;
the actual sessionFactory bean registration already uses a hardcoded
literal rather than this constant.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Y_BEAN_NAME from H7 initializer

Mirrors the H5 cleanup: getTestDbUrl() was never invoked anywhere, and
defaultSessionFactoryBeanName's value was never read by the sessionFactory
bean registration it appeared to configure (which uses a hardcoded
literal). Removing both leaves SESSION_FACTORY_BEAN_NAME with no
remaining consumer, so it goes too.

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

The mongo == null branch of getBeanDefinitions() registered the built
MongoClient under a hardcoded 'mongo' bean name instead of the
customizable mongoBeanName field, unlike configure()'s mongo != null
branch which already honored it. Beyond fixing the inconsistency, this
also resolves an IDE warning where the literal mongo(...) DSL call was
being confused with the protected 'mongo' field of the same name.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same bug class as the mongoBeanName fix: configureDataSources() correctly
used defaultDataSourceBeanName in its null-config fallback, but hardcoded
the literal ConnectionSource.DEFAULT in the config-present branch (and, on
Hibernate7, in the per-datasource bean-registration loop guard too). No
current caller customizes this property, so today's behavior is
unaffected, but the property is now internally consistent wherever it's
referenced.

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

MongodbGrailsPlugin sets initializer.databaseName = applicationName to
default the Mongo database name to the Grails app's name, but that field
was never read anywhere - every Grails+MongoDB app not explicitly setting
grails.mongodb.databaseName silently got the wrong database.

Adds applyDatabaseNameFallback(), called at the top of getBeanDefinitions()
(the only method MongodbGrailsPlugin actually invokes - it doesn't go
through configure()), which injects databaseName as a
grails.mongodb.databaseName fallback property on `configuration` only when
it was customized away from the class default and the configuration
doesn't already specify one explicitly. Handles both configuration shapes
this class accepts: ConfigurableEnvironment (the default and the
Map-constructor path, both mutable via propertySources) and a generic Map
(covering Grails' own Config, which this module can't depend on directly
since it's designed to work standalone outside Grails - tested here via a
minimal Map+PropertyResolver double instead).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Git archaeology (3eb036d, "Simplify AbstractDatastoreInitializer",
Nov 2018) shows this flag gated registration of a legacy grailsApplication
Spring bean needed for pre-3.3 Grails compatibility - and that logic's own
first line already made it a no-op for Grails 3.3+ (released 2017). The
2018 commit correctly deleted the dead logic but left the now-meaningless
field behind, and the four plugins that set it were never updated. Removes
the field and all four call sites (Hibernate5, Hibernate7, MongoDB, and
the already-unbuildable Neo4j module, fixed for source-tree consistency).

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

Two different verdicts from git archaeology on these Hibernate5/7
initializer properties:

enableReload (real bug, same class as databaseName/defaultDataSourceBeanName):
HibernateGrailsPlugin sets springInitializer.enableReload =
Environment.isDevelopmentMode() to enable Hibernate's runtime-reload
support by default in dev mode, but the field was never read - it has a
genuine, still-live destination in HibernateConnectionSourceSettings.
enableReload, populated via the same generic PropertyResolver-binding
mechanism proven by the databaseName fix. Adds applyEnableReloadFallback(),
called at the top of getBeanDefinitions() for the same reason as the Mongo
fix, injecting an `enableReload` fallback property only when customized
away from its default and not already explicitly configured. Verified
end-to-end against HibernateDatastore's actual connection source settings,
which also confirms the unprefixed `enableReload` config key empirically
(not just by static tracing).

grailsPlugin (dead, same class as registerApplicationIfNotPresent):
git blame traces this to a 2016 fix (133ca44) that conditionally kept
GrailsHibernateTransactionManager alive for Grails-plugin bootstrapping
while a parallel refactor moved standalone usage to deriving the
transaction manager from the datastore instead. A follow-up commit five
months later (415d1c6, Jan 2017) dropped the conditional entirely in
favor of the datastore-derived approach unconditionally, but left the
grailsPlugin field and its two HibernateGrailsPlugin call sites behind.
Removed all four references.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 13, 2026 22:34
@borinquenkid borinquenkid added this to the grails:8.1.0-M1 milestone Aug 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR strengthens GORM datastore bootstrap reliability by adding focused test coverage for AbstractDatastoreInitializer (via a shared concrete test double) and by adding/expanding regression tests for the Hibernate 5/7 and MongoDB Spring initializers. Along the way, it fixes several initializer customization bugs where configurable fields were previously ignored, and removes now-dead initializer/plugin wiring.

Changes:

  • Add unit coverage for AbstractDatastoreInitializer using a reusable TestDatastoreInitializer test double.
  • Fix ignored customization points in MongoDB and Hibernate initializers (database name fallback, Mongo client bean name, default datasource bean name, enableReload fallback).
  • Remove dead initializer/plugin configuration flags and related unused constants/methods; expand regression tests across Hibernate 5/7 and MongoDB.

Reviewed changes

Copilot reviewed 15 out of 15 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/bootstrap/TestDatastoreInitializer.groovy New concrete test double for exercising AbstractDatastoreInitializer behavior in isolation.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/bootstrap/AbstractDatastoreInitializerWebApplicationSpec.groovy Switch to shared test double; keeps OSIV web-detection coverage in datamapping-core without spring-web.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/bootstrap/AbstractDatastoreInitializerSpec.groovy New unit spec covering constructors, service loading, bean registration helpers, and baseline behaviors.
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/bootstrap/AbstractDatastoreInitializer.groovy Small API/behavior refinements (static helpers, safer comparisons, reflective instantiation updates, dead flag removal).
grails-data-neo4j/grails-plugin/src/main/groovy/org/grails/datastore/gorm/neo4j/Neo4jGrailsPlugin.groovy Remove writes to deleted initializer flag.
grails-data-mongodb/grails-plugin/src/main/groovy/grails/plugins/mongodb/MongodbGrailsPlugin.groovy Remove writes to deleted initializer flag.
grails-data-mongodb/core/src/test/groovy/grails/mongodb/bootstrap/MongoDbDataStoreSpringInitializerUnitSpec.groovy New unit coverage for Mongo initializer mapping discrimination and deprecated setters, plus database-name fallback behavior.
grails-data-mongodb/core/src/test/groovy/grails/mongodb/bootstrap/MongoDbDataStoreSpringInitializerSpec.groovy New integration/regression tests for database-name fallback, custom Mongo bean name, pre-existing client reuse, and package scanning.
grails-data-mongodb/core/src/main/groovy/grails/mongodb/bootstrap/MongoDbDataStoreSpringInitializer.groovy Honor mongoBeanName when creating a client; implement database-name fallback injection.
grails-data-hibernate7/grails-plugin/src/test/groovy/grails/orm/bootstrap/HibernateDatastoreSpringInitializerSpec.groovy Add regression tests for default datasource bean naming, enableReload fallback, constructors, OSIV registration, and registry error cases.
grails-data-hibernate7/grails-plugin/src/main/groovy/grails/plugin/hibernate/HibernateGrailsPlugin.groovy Remove dead initializer flags (registerApplicationIfNotPresent, grailsPlugin) writes.
grails-data-hibernate7/grails-plugin/src/main/groovy/grails/orm/bootstrap/HibernateDatastoreSpringInitializer.groovy Fix default datasource bean-name usage; add enableReload fallback injection; remove unused constants/method.
grails-data-hibernate5/grails-plugin/src/test/groovy/grails/orm/bootstrap/HibernateDatastoreSpringInitializerSpec.groovy Add regression tests parallel to Hibernate7 for naming/fallback/constructors/OSIV/DataSource reuse.
grails-data-hibernate5/grails-plugin/src/main/groovy/grails/plugin/hibernate/HibernateGrailsPlugin.groovy Remove dead initializer flags (registerApplicationIfNotPresent, grailsPlugin) writes.
grails-data-hibernate5/grails-plugin/src/main/groovy/grails/orm/bootstrap/HibernateDatastoreSpringInitializer.groovy Fix default datasource bean-name usage; add enableReload fallback injection; tighten interceptor class typing; remove unused constants/method.

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

@bito-code-review

Copy link
Copy Markdown

The user's concern regarding test flakiness due to external environment variables or system properties is valid. To make the test deterministic, you should ensure the PropertyResolver or Environment used in the test is isolated from the system environment.

Instead of relying on a default StandardEnvironment, you can use a MutablePropertySources instance with only the necessary properties, or a MapPropertySource that does not delegate to system properties. For example, when initializing the HibernateDatastoreSpringInitializer in your test, pass a PropertySourcesPropertyResolver configured with an empty or minimal MutablePropertySources instead of the default environment.

@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.9084%. Comparing base (506228b) to head (5e6cc3b).

Additional details and impacted files

Impacted file tree graph

@@                         Coverage Diff                          @@
##             feat/enable-datamapping-rx     #16147        +/-   ##
====================================================================
+ Coverage                       52.8254%   52.9084%   +0.0830%     
- Complexity                        18871      18910        +39     
====================================================================
  Files                              2079       2079                
  Lines                             97207      97217        +10     
  Branches                          16873      16875         +2     
====================================================================
+ Hits                              51350      51436        +86     
+ Misses                            38446      38374        -72     
+ Partials                           7411       7407         -4     
Files with missing lines Coverage Δ
...bootstrap/MongoDbDataStoreSpringInitializer.groovy 83.0769% <100.0000%> (+31.2912%) ⬆️
.../grails/plugins/mongodb/MongodbGrailsPlugin.groovy 0.0000% <ø> (ø)
...gorm/bootstrap/AbstractDatastoreInitializer.groovy 82.9268% <100.0000%> (+38.4824%) ⬆️

... and 3 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.

Fixes a real flakiness risk Copilot flagged: two applyDatabaseNameFallback
unit tests relied on the default StandardEnvironment not already
containing grails.mongodb.databaseName, but StandardEnvironment reads
system properties and environment variables, so an externally-set value
could make either test fail unpredictably. Strips those property sources
before asserting.

Closes the coverage gaps Codecov flagged on this PR's diff:
- Adds a PlainPropertyResolver test double (neither ConfigurableEnvironment
  nor Map) to cover applyDatabaseNameFallback's fallthrough branch, which
  was previously only exercised on the true side.
- Adds a direct test for GrailsBeanBuilderInit.registerBeans(), the
  Groovy-BeanBuilder-based fallback path in AbstractDatastoreInitializer
  that was completely untested (GroovyBeanReaderInit always wins in this
  environment since spring-beans is always present, so this path is only
  reachable by calling it directly, as this test now does - the same
  pattern already used for GroovyBeanReaderInit.registerBeans()).

The remaining uncovered lines (the catch(ignored){return false} branches
in both GroovyBeanReaderInit.isAvailable() and GrailsBeanBuilderInit.
isAvailable()) are left as acknowledged gaps: both guarded classes are
always present in this test environment, so triggering the
ClassNotFoundException path would require classloader-hiding tricks with
more fragility than the coverage is worth.

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

testlens-app Bot commented Aug 14, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 5e6cc3b
▶️ Tests: 44509 executed
⚪️ Checks: 56/56 completed


Learn more about TestLens at testlens.app.

Base automatically changed from feat/enable-datamapping-rx to 8.1.x August 15, 2026 02:34
@borinquenkid borinquenkid moved this to In Progress in Apache Grails Aug 16, 2026
@borinquenkid
borinquenkid requested review from codeconsole and jdaugherty and a lite review from Copilot August 16, 2026 15:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (1)

grails-data-mongodb/core/src/main/groovy/grails/mongodb/bootstrap/MongoDbDataStoreSpringInitializer.groovy:97

  • applyDatabaseNameFallback() will inject the value of databaseName into the configuration whenever it differs from the default. Since setDatabaseName(String) accepts null/blank values, this method can end up writing a null/empty grails.mongodb.databaseName into the PropertyResolver, which can break downstream Mongo settings resolution. Guard against null/blank and treat it as “not customized” (or reset to DEFAULT_DATABASE_NAME).
    protected void applyDatabaseNameFallback() {
        if (databaseName == DEFAULT_DATABASE_NAME || configuration.containsProperty(MongoSettings.SETTING_DATABASE_NAME)) {
            return
        }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants