Skip to content

Keep BeanBuilder DSL independent of XML setup - #15824

Open
jamesfredley wants to merge 7 commits into
8.0.xfrom
fix/14915-beanbuilder-spring7
Open

Keep BeanBuilder DSL independent of XML setup#15824
jamesfredley wants to merge 7 commits into
8.0.xfrom
fix/14915-beanbuilder-spring7

Conversation

@jamesfredley

@jamesfredley jamesfredley commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Fixes #14915

Why this is needed

Grails 8 is moving onto Spring Framework 7, but existing applications still need their grails-app/conf/spring/resources.groovy BeanBuilder DSL files to keep working without application-side changes.

The problem with the previous implementation was that BeanBuilder eagerly initialized Spring XML reader/context infrastructure even for ordinary programmatic resources.groovy bean definitions. That made the standard DSL path more tightly coupled to Spring XML internals than it needs to be.

This PR keeps the public BeanBuilder DSL behavior backward compatible while avoiding that eager XML setup for the common resources.groovy path.

What changed

  • Moved Spring XML reader, namespace resolver, XmlReaderContext, and parser context setup into a lazy BeanBuilderXmlSupport helper.
  • Kept ordinary BeanBuilder DSL registration on the programmatic BeanDefinition path.
  • Preserved XML compatibility paths:
    • importBeans with .xml resources still loads XML bean definitions.
    • xmlns still resolves Spring XML namespace handlers on demand.
    • DynamicElementReader still receives the parser context it needs for namespace DSL usage.
  • Applied the shared test configuration to grails-spring so module-local JUnit Platform tests are actually discovered and run.
  • Added focused regressions for:
    • Plain BeanBuilder DSL does not initialize XML support.
    • XML importBeans still registers XML bean definitions.
    • xmlns namespace support still works through the BeanBuilder DSL.
  • Updated Grails 8 and Spring DSL documentation to explain the compatibility behavior and recommend resources.groovy for new custom bean definitions.

Compatibility behavior

No application changes are required for existing resources.groovy BeanBuilder DSL files.

Existing resources.xml, XML imports, and Spring XML namespace DSL usage remain supported as compatibility paths. They now initialize XML support only when those XML features are actually used.

Review feedback addressed

  • Copilot flagged the unknown-XML-namespace error path calling getXmlSupport().getReaderContext(...) (which would lazily initialize the XML reader/context just to build an error Location). This is already satisfied on the branch: the "No namespace handler found" error builds its Location directly from the already-available beanBuildResource, so it never forces XML reader/context initialization on the exception path, and testUnknownNamespaceDoesNotInitializeXmlReaderForErrorLocation locks that in. Merged the latest 8.0.x.
  • Follow-up (documented): the lazy refactor means setClassLoader no longer dispatches to the protected initializeBeanBuilderForClassLoader hook. No in-repo subclass overrides it, so this only affects hypothetical external subclasses; noted for a possible follow-up to preserve the hook dispatch.

Verification

Passed from the visible checkout on fix/14915-beanbuilder-spring7:

  • JAVA_HOME=/home/james/.jdks/corretto-21 PATH=/home/james/.jdks/corretto-21/bin:$PATH ./gradlew :grails-spring:test --rerun-tasks
  • JAVA_HOME=/home/james/.jdks/corretto-21 PATH=/home/james/.jdks/corretto-21/bin:$PATH ./gradlew :grails-spring:codeStyle
  • External smoke driver against the built grails-spring runtime classpath:
    • PASS plain BeanBuilder DSL did not initialize XML support

Review gate:

  • Oracle and Codex both reviewed staged hash f1e6f82bdd5a4035a8230306b3d519b03ac1a61a as GREEN.
  • Codex only flagged regenerated untracked .omo/run-continuation session metadata. That file was not staged or committed.

Full-suite notes

  • ./gradlew clean completed successfully.
  • ./gradlew aggregateViolations --continue was attempted, but the build is currently blocked by unrelated existing compile errors outside this change:
    • grails-validation/src/main/groovy/grails/validation/Validateable.groovy: unable to resolve groovy.transform.Virtual
    • grails-data-graphql/core/.../Arguable.groovy and ComplexTyped.groovy: static type checking cannot find withDelegate(Closure, Object)
  • ./gradlew :grails-test-report:check --continue was attempted and exceeded the 20 minute tool timeout after producing broad test output.

Avoid eager Spring XML reader setup when evaluating ordinary BeanBuilder DSLs so resources.groovy remains compatible with Spring Framework 7. Move XML reader and namespace support behind a lazy helper used only by XML import and xmlns paths.

Add grails-spring regression coverage for plain DSL, XML import, and namespace support, and wire the module into the shared test configuration so those tests execute. Document the compatibility behavior in the Grails 8 upgrade and Spring DSL guides.

Assisted-by: Hephaestus:openai/gpt-5.5 codex-review
Copilot AI review requested due to automatic review settings July 4, 2026 17:49

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 refactors grails.spring.BeanBuilder to keep the standard resources.groovy DSL path independent of Spring XML reader/context initialization, while preserving compatibility for XML-based imports (importBeans with XML resources) and Spring XML namespace DSL usage via xmlns. It also adds regression tests and documents the Grails 8 compatibility behavior in the guide and upgrade notes.

Changes:

  • Introduced lazy, encapsulated XML/namespace support (BeanBuilderXmlSupport) so XML infrastructure is only initialized when needed.
  • Updated BeanBuilder to delegate XML import and namespace parsing to the new support class.
  • Added grails-spring regression tests and updated documentation to clarify Grails 8 behavior.

Reviewed changes

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

Show a summary per file
File Description
grails-spring/src/test/groovy/grails/spring/BeanBuilderTests.groovy Adds regressions covering plain DSL (no XML init), XML import, and xmlns namespace usage.
grails-spring/src/main/groovy/grails/spring/BeanBuilderXmlSupport.java New helper to lazily manage Spring XML reader/context and namespace handler resolution.
grails-spring/src/main/groovy/grails/spring/BeanBuilder.java Removes eager XML initialization and routes XML-related behavior through BeanBuilderXmlSupport.
grails-spring/build.gradle Applies shared test-config.gradle for module test setup.
grails-doc/src/en/guide/upgrading/upgrading80x.adoc Documents Grails 8 BeanBuilder/XML compatibility and recommended path.
grails-doc/src/en/guide/spring/theBeanBuilderDSLExplained.adoc Clarifies that XML namespace support is compatibility-focused and initialized on-demand.
grails-doc/src/en/guide/spring/springdslAdditional.adoc Notes continued resources.xml support and encourages resources.groovy for new definitions.
grails-doc/src/en/guide/introduction/whatsNew.adoc Highlights the Grails 8 BeanBuilder behavior change at a high level.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread grails-spring/src/main/groovy/grails/spring/BeanBuilder.java Outdated
@bito-code-review

Copy link
Copy Markdown

The suggested change is correct and aligns with the goal of avoiding unnecessary initialization of Spring XML infrastructure in the error path. By using beanBuildResource directly when constructing the Location object, you bypass the need to call getXmlSupport().getReaderContext(beanBuildResource), which would otherwise trigger the lazy initialization of the XmlBeanDefinitionReader and the associated ApplicationContext.

This change effectively prevents the potential masking of the "No namespace handler found" error and improves performance in the exception path.

grails-spring/src/main/groovy/grails/spring/BeanBuilder.java

final NamespaceHandler namespaceHandler = getXmlSupport().resolveNamespaceHandler(uri);
            if (namespaceHandler == null) {
                throw new BeanDefinitionParsingException(
                      new Problem("No namespace handler found for URI: " + uri,
                            new Location(beanBuildResource)));
            }

@codecov

codecov Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.19048% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.1390%. Comparing base (b980413) to head (a1b7309).

Files with missing lines Patch % Lines
...in/groovy/grails/spring/BeanBuilderXmlSupport.java 80.6452% 4 Missing and 2 partials ⚠️
...ing/src/main/groovy/grails/spring/BeanBuilder.java 63.6364% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##             8.0.x     #15824         +/-   ##
================================================
+ Coverage         0   51.1390%   +51.1390%     
- Complexity       0      17615      +17615     
================================================
  Files            0       2042       +2042     
  Lines            0      95520      +95520     
  Branches         0      16587      +16587     
================================================
+ Hits             0      48848      +48848     
- Misses           0      39383      +39383     
- Partials         0       7289       +7289     
Files with missing lines Coverage Δ
...ing/src/main/groovy/grails/spring/BeanBuilder.java 54.2781% <63.6364%> (ø)
...in/groovy/grails/spring/BeanBuilderXmlSupport.java 80.6452% <80.6452%> (ø)

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

Use the existing BeanBuilder resource when reporting missing XML namespace handlers so the error path does not create a reader context. Add coverage that fails if the XML reader path initializes while building the missing namespace error.

Assisted-by: opencode:gpt-5.5 codex-review
@jdaugherty

Copy link
Copy Markdown
Contributor

FYI: spring started enforcing certain configuration like AMPQ / rabbit to be associated to an application context. Why are we maintaining the link to the bean builder dsl that Spring has stated will no longer be supported? Can't we adjust the beanDsl to use a BeanRegistrar instead which is the programmatic way Spring suggests using now?

Assisted-by: opencode:gpt-5.5
jamesfredley added a commit that referenced this pull request Jul 15, 2026
- Rename the new SPI to GrailsBeanRegistryAdapter (impl
  BeanBuilderGrailsBeanRegistryAdapter) to avoid colliding with Spring's
  org.springframework.beans.factory.support.BeanRegistryAdapter
- Validate the BeanBuilder constructor argument with Assert.notNull
- Align @SInCE and the guide wording to 8.0.x and soften the #15824 reference
- Add the ASF license header to the new upgrading guide page

Assisted-by: Sisyphus:openai/gpt-5.6-terra [gpt-coding]
@jamesfredley

Copy link
Copy Markdown
Contributor Author

Review feedback addressed

Merged the latest 8.0.x and pushed.

Copilot review comment (unknown-namespace error path initializing XML reader/context): already satisfied on this branch. The "No namespace handler found for URI: ..." error builds its Location directly from the already-available beanBuildResource (new Location(beanBuildResource)), so it does not call getXmlSupport().getReaderContext(...) and never forces XmlBeanDefinitionReader / unrefreshed-context initialization in the exception path. The regression test testUnknownNamespaceDoesNotInitializeXmlReaderForErrorLocation locks this in by making context initialization throw and asserting the namespace-handler error still surfaces.

Follow-up (documented, not changed here): the lazy XML refactor means setClassLoader no longer dispatches to the protected initializeBeanBuilderForClassLoader hook. No in-repo BeanBuilder subclass overrides that hook, so this only affects hypothetical external subclasses; noting it here in case we want to preserve the hook dispatch in a follow-up.

Local verification: :grails-spring:test --tests BeanBuilderTests passes.

@jamesfredley

Copy link
Copy Markdown
Contributor Author

How this relates to #16019

These two PRs are complementary, not alternatives.

This PR (#15824) keeps the existing BeanBuilder path working cleanly on Spring 7. It moves Spring XML reader / namespace / parser setup behind a lazy helper so ordinary resources.groovy and programmatic BeanBuilder DSL registration no longer eagerly initialize XML infrastructure. XML importBeans, xmlns, and related paths still work, but only initialize XML support when those features are actually used. No application rewrite is required; public BeanBuilder behavior stays compatible.

#16019 is the forward registration model. It adds @GrailsBeans, which compiles a beans = { bean(...) ... } DSL into real @AutoConfiguration / @Bean methods (with Boot ordering and @Conditional* support), and moves several framework plugins onto that path plus beanRegistrar() where plugin runtime state is needed. It does not refactor BeanBuilder or fix the eager-XML issue this PR addresses, and it is not a drop-in replacement for evaluating existing resources.groovy / doWithSpring BeanBuilder DSLs.

#15824 (this PR) #16019
Goal Compatibility fix for current BeanBuilder New Spring-native bean authoring path
Touches BeanBuilder Yes (lazy XML) No
App resources.groovy Keeps working unchanged Not a substitute
Plugin bean registration Unchanged public DSL @GrailsBeans / beanRegistrar() / generated autoconfig
Scope Small, focused Large feature + framework conversions

Recommendation: land this PR for Spring 7 BeanBuilder compatibility; continue #16019 as the longer-term way to declare beans. Merging #16019 does not make this PR unnecessary while BeanBuilder / resources.groovy / doWithSpring still exist

@testlens-app

testlens-app Bot commented Jul 29, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: c47fb1e
▶️ Tests: 33193 executed
⚪️ Checks: 59/59 completed


Learn more about TestLens at testlens.app.

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

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

Spring 7 - XML configuration no longer supported

4 participants