Skip to content

Add opt-in legacy Holder shim for early plugin registration - #16101

Open
jamesfredley wants to merge 7 commits into
8.0.xfrom
fix/legacy-plugin-holders-8.0.x
Open

Add opt-in legacy Holder shim for early plugin registration#16101
jamesfredley wants to merge 7 commits into
8.0.xfrom
fix/legacy-plugin-holders-8.0.x

Conversation

@jamesfredley

@jamesfredley jamesfredley commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR adds a narrow, default-off compatibility bridge for Grails 7-era plugins that read Holders.grailsApplication from doWithSpring.

  • Preserve the Grails 8 early plugin-registration lifecycle by default.
  • Allow legacy applications to opt into early Holder publication temporarily.
  • Prevent failed or concurrent application contexts from corrupting the process-global Holder fallback.
  • Document the compatibility setting and expose it through Spring configuration metadata.

What broke backward compatibility

The behavior changed in PR #15934, specifically commit a853dda.

PR #15934 intentionally moved plugin bean registration earlier so plugin-provided beans exist before Spring Boot evaluates auto-configuration such as @ConditionalOnMissingBean. This allows Boot to back off cleanly instead of creating a bean that a plugin must override later.

Before #15934, GrailsApplicationPostProcessor.postProcessBeanDefinitionRegistry performed this sequence:

  1. Publish the application with Holders.setGrailsApplication(application).
  2. Call pluginManager.doRuntimeConfiguration(springConfig), which executes plugin doWithSpring closures.
  3. Register the resulting bean definitions.

After #15934, GrailsEarlyPluginRegistrationPostProcessor performs the plugin phase before Boot auto-configuration:

  1. Create the promoted DefaultGrailsApplication and DefaultGrailsPluginManager.
  2. Load plugins and discover artefacts.
  3. Call pluginManager.doRuntimeConfiguration(springConfig) and run plugin beanRegistrar() implementations.
  4. Register those bean definitions.
  5. Promote the application and plugin-manager singletons and publish the application to Holders later in the lifecycle.

That new ordering solves the Boot conditional-bean problem, but it is backward-incompatible for older plugins whose doWithSpring closures read Holders.grailsApplication. Their code now executes before the application being initialized is published globally. In a normal single-application JVM with no prior Holder value, Holders.grailsApplication throws IllegalArgumentException: GrailsApplication not found. If another context previously published an application, the plugin can instead observe that unrelated application.

The concrete compatibility case is audit-logging 6.0.0, which accesses Holders.grailsApplication from doWithSpring to obtain the environment. The environment is available during this phase, but the application is not generally lifecycle-ready.

Why the bridge is default-off

Publishing the application early for every Grails 8 application would imply that the application is ready while Spring is still wiring bean definitions. That can mask other lifecycle assumptions and create difficult-to-debug behavior.

Following the default-off compatibility pattern used for legacyCommandSupport in PR #16011, this PR keeps the #15934 lifecycle as the default and provides an explicit migration bridge:

grails:
    legacy:
        holdersDuringDoWithSpring: true

This setting is transitional. Plugins should migrate to the configuration/environment available to the registration phase, injected dependencies, or beanRegistrar() rather than relying on process-global Holder state.

Behavior

Setting During plugin doWithSpring Warning Publication and failure behavior
Absent The application being initialized is not published. With no prior Holder, legacy access fails. No A preexisting Holder is left untouched. Successful late promotion cannot overwrite a newer publisher.
false Same as absent. No Same as absent.
true The promoted application is available to the legacy registration path. Yes Failure restores the first valid prior Holder only when ownership permits it. Failed applications are never restored, and newer publishers win.

The bridge only makes the promoted application discoverable through the Holder fallback during legacy registration. It does not make the application generally ready or restore every Grails 7 lifecycle assumption.

Implementation details

  • Settings.LEGACY_HOLDERS_DURING_DO_WITH_SPRING defines grails.legacy.holdersDuringDoWithSpring, with runtime and metadata defaults of false.
  • GrailsEarlyPluginRegistrationPostProcessor reads the setting from the application environment. When enabled, it publishes the exact promoted application before plugin runtime configuration and emits a migration warning.
  • With the bridge disabled, the early processor snapshots the Holder fallback without invoking discovery strategies and promotes the completed application with identity compare-and-set. A newer publisher is never overwritten.
  • GrailsApplicationPostProcessor recognizes the early-registration completion marker and preserves the early processor's ownership decision instead of publishing unconditionally during the downstream handoff.
  • Holders stores the fallback application in an AtomicReference and provides identity-based replacement and conditional restoration operations.
  • Failed publications are tracked with weak-identity tombstones. A later rollback cannot resurrect an application whose initialization failed, including stacked and temporarily hidden publisher sequences.
  • clear() and reset() clear failure history, while normal fallback reads drain collected weak tombstones so prior applications are not retained indefinitely.
  • Runtime exceptions and Error instances retain their identity after cleanup. Sneaky checked exceptions from Groovy closures are wrapped only after Holder and Environment.initializing state is restored.

The ownership hardening is split into commits 0c83760 and 787eb7d.

Documentation and configuration

  • The Grails 8 upgrade guide explains the Register plugin beans before Spring Boot auto-configuration by retiming doWithSpring #15934 ordering consequence, default-off behavior, preexisting Holder caveat, temporary opt-in, warning, and migration path.
  • spring-configuration-metadata.json exposes the property as a Boolean with default false under the grails.legacy group.
  • ConfigReportCommandSpec verifies that the metadata is visible through configuration-report tooling.

Verification

  • ./gradlew :grails-core:test --tests "grails.util.HoldersSpec" -x :grails-core:testCli
  • ./gradlew :grails-core:test --tests "grails.boot.config.EarlyPluginRegistrationOrderingSpec" -x :grails-core:testCli
  • ./gradlew :grails-core:testCli --tests "org.apache.grails.core.cli.ConfigReportCommandSpec"
  • ./gradlew :grails-core:checkstyleMain
  • ./gradlew :grails-doc:publishGuide -x aggregateGroovydoc

Focused coverage includes:

  • Absent, explicit false, and explicit true configuration.
  • Warning behavior and exact promoted-application identity.
  • Preexisting and newer competing publishers.
  • Runtime, checked, and Error failure paths.
  • Discovery-strategy isolation.
  • Stacked context failures and mixed success/failure orderings.
  • Temporary plain-publisher interleaving.
  • Weak-reference cleanup and clear()/reset() isolation.
  • Early-to-downstream application-postprocessor ownership handoff.

Publish the promoted GrailsApplication fallback before plugin doWithSpring callbacks run so Grails 7-era plugins can continue resolving it through Holders. Exchange the fallback without invoking discovery strategies and restore the exact prior value on every failure path, including sneaky checked exceptions from Groovy closures.

Add lifecycle regressions for audit-logging-style Holder access, runtime and checked failures, discovery-strategy isolation, and process-wide state cleanup.

Assisted-by: opencode:gpt-5.6-sol
Use atomic fallback publication and compare-and-restore semantics so a failed application context cannot overwrite a newer concurrent publisher. Cover both lifecycle replacement and a real two-thread race.

Assisted-by: opencode:gpt-5.6-sol
Copilot AI lite review requested due to automatic review settings August 6, 2026 03:28

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

Restores Grails 7-era lifecycle behavior during Grails 8 early plugin registration so legacy plugin doWithSpring callbacks can access the promoted GrailsApplication via Holders, while also making rollback of that global state safe in failure/concurrent-initialization scenarios.

Changes:

  • Publish the promoted DefaultGrailsApplication into Holders before pluginManager.doRuntimeConfiguration(...) executes (and restore prior state on failure).
  • Make Holders’ fallback GrailsApplication storage atomic and add ownership-aware replace/restore operations to prevent failed contexts from clobbering newer publications.
  • Expand ordering/rollback/concurrency regression tests for early plugin registration, including checked-exception wrapping and discovery-strategy isolation.

Reviewed changes

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

File Description
grails-core/src/main/groovy/grails/boot/config/GrailsEarlyPluginRegistrationPostProcessor.java Publishes GrailsApplication to Holders before doWithSpring/runtime config, and restores global state safely on failure.
grails-core/src/main/groovy/grails/util/Holders.java Switches the fallback GrailsApplication to an AtomicReference and adds replace/conditional-restore helpers.
grails-core/src/test/groovy/grails/boot/config/EarlyPluginRegistrationOrderingSpec.groovy Adds regression coverage for legacy Holders access, failure rollback, discovery-strategy non-invocation, checked exception wrapping, and concurrent publisher safety.

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

@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.3589%. Comparing base (8c5b1cf) to head (1640000).
⚠️ Report is 9 commits behind head on 8.0.x.

Files with missing lines Patch % Lines
...ig/GrailsEarlyPluginRegistrationPostProcessor.java 78.5714% 1 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##                8.0.x     #16101        +/-   ##
==================================================
+ Coverage     52.3566%   52.3589%   +0.0023%     
- Complexity      18300      18302         +2     
==================================================
  Files            2036       2036                
  Lines           96347      96360        +13     
  Branches        16829      16832         +3     
==================================================
+ Hits            50444      50453         +9     
  Misses          38481      38481                
- Partials         7422       7426         +4     
Files with missing lines Coverage Δ
...ails-core/src/main/groovy/grails/util/Holders.java 74.7253% <100.0000%> (+0.5680%) ⬆️
...ig/GrailsEarlyPluginRegistrationPostProcessor.java 85.1852% <78.5714%> (-1.4127%) ⬇️

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

@jdaugherty jdaugherty 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.

A Grails application isn't mean to be available - beans have been processed. We are now wiring beans when spring wires them - which by definition is before the application can be made available. Allowing the application to be found is likely just going to mask other problems with the lifecycle not being ready. In this case it only would fix the audit logging plugin because it's trying to access the environment, which is ready at this point.

I'm a -1 on this change as it is - it's the default for all applications and the application should legitimately not be ready at this point. I'm okay adding a shim to add this behavior for legacy applications, but this shouldn't be the default for new applications as it creates a misunderstanding with the spring life cycle.

@testlens-app

testlens-app Bot commented Aug 6, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 1640000
▶️ Tests: 59011 executed
⚪️ Checks: 60/60 completed


Learn more about TestLens at testlens.app.

Track failed fallback publications by weak identity so concurrent rollback
cannot resurrect a failed GrailsApplication. Clear the failure history on
Holder reset and drain collected tombstones during fallback reads.

Assisted-by: opencode:gpt-5.6-sol
Keep the Grails 8 lifecycle unchanged by default and expose the promoted
application during doWithSpring only when the compatibility setting is enabled.
Preserve newer global publishers across both early and downstream promotion.

Assisted-by: opencode:gpt-5.6-sol
Expose the default-off setting through configuration metadata and explain its
limited migration purpose, preexisting Holder caveat, and recommended plugin
migration path in the Grails 8 upgrade guide.

Assisted-by: opencode:gpt-5.6-sol
@jamesfredley jamesfredley changed the title Restore Holders availability during early plugin registration Add opt-in legacy Holder shim for early plugin registration Aug 6, 2026
@jamesfredley

Copy link
Copy Markdown
Contributor Author

Updated this to follow the default-off compatibility-bridge pattern from #16011 rather than enabling the old behavior globally.

The Grails 8 lifecycle remains the default: the application being initialized is not published to Holders before plugin doWithSpring. Applications that still depend on the Grails 7 behavior can temporarily opt in with:

grails:
    legacy:
        holdersDuringDoWithSpring: true

With that property enabled, only the promoted application is exposed during legacy doWithSpring registration, a migration warning is logged, and failure rollback is ownership-aware. Failed contexts cannot be restored later, and neither early nor downstream publication overwrites a newer Holder owner.

The upgrade guide and configuration metadata now describe the shim as transitional support for plugins such as audit-logging 6.0.0, with migration toward configuration/injection or beanRegistrar(). This should address the concern about surprising default behavior while still giving legacy applications an explicit escape hatch.

@jamesfredley jamesfredley self-assigned this Aug 6, 2026
@jamesfredley jamesfredley moved this to In Progress in Apache Grails Aug 6, 2026
@jamesfredley jamesfredley added this to the grails:8.0.0-RC1 milestone Aug 6, 2026
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.

3 participants