Skip to content

Seed BeanRegistry adapter over BeanBuilder - #15957

Closed
jamesfredley wants to merge 6 commits into
8.0.xfrom
feat/beanbuilder-beanregistry-seed
Closed

Seed BeanRegistry adapter over BeanBuilder#15957
jamesfredley wants to merge 6 commits into
8.0.xfrom
feat/beanbuilder-beanregistry-seed

Conversation

@jamesfredley

@jamesfredley jamesfredley commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Description

What was found

Problem Impact
Spring 7 moves away from XML-centric registration Long-term BeanBuilder strategy is unclear
#15824 only keeps DSL independent of eager XML setup Compatibility fix, not the full programmatic future
Full rewrite is multi-week Needs a non-breaking adapter seed first

What changed

Area Change
Adapter SPI Add grails.spring.GrailsBeanRegistryAdapter (renamed to avoid colliding with Spring's org.springframework.beans.factory.support.BeanRegistryAdapter)
Implementation BeanBuilderGrailsBeanRegistryAdapter delegates to the existing BeanBuilder; constructor validated with Assert.notNull
Docs Upgrade note scoped to the remaining BeanBuilder DSL paths; a future implementation would use Spring's BeanRegistry registration APIs (BeanRegistrar itself is a callback already integrated in Grails 8)
Tests Adapter smoke coverage + null-argument coverage

Review feedback addressed (commits 0a1f09df85, a20c06fc82)

Source Item Resolution
Copilot Constructor did not validate its BeanBuilder Assert.notNull + test
Copilot BeanRegistryAdapter name collided with Spring's Renamed to GrailsBeanRegistryAdapter / BeanBuilderGrailsBeanRegistryAdapter
Copilot @since 8.1 vs 8.0.x target Aligned to 8.0.x / "Grails 8.0"
Copilot New .adoc missing ASF header + misleading #15824 wording Added header; softened to "See PR #15824 for related work"
Review pass Guide mislabeled a "BeanRegistrar-backed" backend Clarified BeanRegistrar vs Spring BeanRegistry

Out of scope / follow-up

Topic Status
Full Spring BeanRegistry programmatic rewrite 8.1/9.0
Breaking BeanBuilder removal Not this PR
Plugin doWithSpring migration tooling Follow-up

Contributor Checklist

Issue and Scope

  • Starter for strategic rewrite discussion.
  • Does not break existing BeanBuilder.
  • Single focused seed.
  • Targets 8.0.x.

Code Quality

  • Tests added (incl. null-argument coverage).
  • Focused spring module tests run.
  • No mass reformatting.
  • AI starting point labeled.

Licensing and Attribution

  • Apache License 2.0.
  • Contributor rights confirmed.
  • ai-generated-starting-point label applied.

Documentation

  • Upgrade note added and corrected.
  • Scope explained.

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

Add experimental BeanRegistryAdapter that still uses BeanBuilder while documenting Spring 7 BeanRegistrar path.

Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]

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 seeds a new adapter SPI in grails-spring to provide a non-breaking seam between the existing BeanBuilder DSL and Spring 7’s evolving registration APIs, while keeping current BeanBuilder-based behavior intact.

Changes:

  • Introduces a new grails.spring.BeanRegistryAdapter SPI plus a BeanBuilder-backed implementation.
  • Adds a JUnit smoke test validating that the adapter registers beans via the existing BeanBuilder path.
  • Updates the guide to document the intended migration strategy and wires grails-spring into the shared test Gradle config.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
grails-spring/src/main/groovy/grails/spring/BeanRegistryAdapter.java Adds the new adapter SPI surface for bean definition + registration.
grails-spring/src/main/groovy/grails/spring/BeanBuilderBeanRegistryAdapter.java Provides the initial adapter implementation by delegating to BeanBuilder.
grails-spring/src/test/groovy/grails/spring/BeanBuilderBeanRegistryAdapterTest.groovy Adds smoke coverage to ensure adapter-driven registration works end-to-end.
grails-spring/build.gradle Applies shared test configuration to ensure module-local JUnit Platform discovery/behavior.
grails-doc/src/en/guide/upgrading/beanRegistryAdapter.adoc Documents the new seam and the high-level migration approach.
grails-doc/src/en/guide/toc.yml Adds the new upgrading guide entry to the documentation TOC.

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

Comment thread grails-spring/src/main/groovy/grails/spring/BeanBuilderBeanRegistryAdapter.java Outdated
Comment thread grails-spring/src/main/groovy/grails/spring/BeanRegistryAdapter.java Outdated
Comment thread grails-doc/src/en/guide/upgrading/beanRegistryAdapter.adoc
@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.4958%. Comparing base (b980413) to head (16c1836).
⚠️ Report is 158 commits behind head on 8.0.x.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##             8.0.x     #15957         +/-   ##
================================================
+ Coverage         0   51.4958%   +51.4958%     
- Complexity       0      17785      +17785     
================================================
  Files            0       2040       +2040     
  Lines            0      95598      +95598     
  Branches         0      16591      +16591     
================================================
+ Hits             0      49229      +49229     
- Misses           0      39058      +39058     
- Partials         0       7311       +7311     
Files with missing lines Coverage Δ
...s/spring/BeanBuilderGrailsBeanRegistryAdapter.java 100.0000% <100.0000%> (ø)

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

- 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]
The upgrading page described a future "BeanRegistrar-backed
implementation", but BeanRegistrar is a Spring registration callback
(already integrated in Grails 8), not a registry backend. Reword to
scope the adapter to the remaining BeanBuilder DSL paths and note a
future implementation would use Spring's BeanRegistry registration APIs.

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

Copy link
Copy Markdown
Contributor Author

Review feedback addressed

Pushed 0a1f09df85 and a20c06fc82 after merging the latest 8.0.x.

Copilot review comments (all four resolved):

  1. Constructor does not validate its BeanBuilder - added Assert.notNull(beanBuilder, ...) plus a null-argument test.
  2. grails.spring.BeanRegistryAdapter collides with Spring's org.springframework.beans.factory.support.BeanRegistryAdapter - renamed the interface to GrailsBeanRegistryAdapter and the impl to BeanBuilderGrailsBeanRegistryAdapter (files git mvd; test, docs and TOC updated; repo grep confirms no dangling references to the old names).
  3. @since 8.1 vs an 8.0.x target - aligned @since to 8.0.x and the guide wording to "Grails 8.0".
  4. New .adoc missing the ASF license header + misleading "Keep BeanBuilder DSL independent of XML setup #15824" wording - added the standard ASF header block and softened the reference to "See PR Keep BeanBuilder DSL independent of XML setup #15824 for related work".

Additional review-pass fix (a20c06fc82): the guide previously described a future "BeanRegistrar-backed implementation". BeanRegistrar is a Spring registration callback (already integrated in Grails 8), not a registry backend, so the wording now scopes the adapter to the remaining BeanBuilder DSL paths and refers to Spring's BeanRegistry registration APIs as the future target.

Local verification: :grails-spring:test --tests grails.spring.BeanBuilderGrailsBeanRegistryAdapterTest (2 tests) and :grails-spring:compileTestGroovy pass.

@jamesfredley jamesfredley moved this to Todo in Apache Grails Jul 24, 2026
Codecov flagged 91.67% patch coverage with the getter as the only
uncovered line. Added a test asserting the adapter exposes the exact
BeanBuilder instance it was constructed with.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@borinquenkid borinquenkid added this to the grails:8.0.0-RC1 milestone Jul 27, 2026
@testlens-app

testlens-app Bot commented Jul 27, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 16c1836
▶️ Tests: 31561 executed
⚪️ Checks: 59/59 completed


Learn more about TestLens at testlens.app.

@jamesfredley

Copy link
Copy Markdown
Contributor Author

Closing as superseded by direction: #15824 for BeanBuilder Spring 7 compatibility; #16019 for the Spring-native / @GrailsBeans + beanRegistrar() path. An adapter that only wraps BeanBuilder is not on the roadmap unless we later need a shared SPI that both backends implement.

@github-project-automation github-project-automation Bot moved this from Todo to Done in Apache Grails Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants