Load plugin.yml/plugin.groovy earlier so @Conditional* annotations can be used

[jdaugherty]

Fixes #15408

Assisted-by: OpenCode <opencode@opencode.ai>
Assisted-by: Claude <claude@anthropic.com>

[bito-code-review]

You're right—deprecation annotations should include a reason for clarity. For this method, the reason could be that plugin property sources are now handled early by GrailsPluginEnvironmentPostProcessor.

[jamesfredley]

If this does not require changes in end Grails apps I am OK with this going into 7.0.x, if we think the risk is too high, then 7.1.x is also OK.

[jamesfredley]

Looks solid. A few minor items to take a look at.

The GrailsPluginConfigurationClass test removed the "plugin with both yml and groovy throws exception" test. That validation still exists in GrailsPluginEnvironmentPostProcessor.readPluginConfiguration() (line 293), but there's no unit test covering it in the new code. The GrailsPluginEnvironmentPostProcessorSpec should add a test for this.

[jdaugherty]

This change is incomplete since the plugin configuration will be loaded potentially when the plugin is disabled. We may be able to post process already loaded configuration and remove them, but I'm not sure without further research.

Here's the loading order in Spring:

• SpringApplication.run(...)
• Prepare SpringApplicationRunListeners
• Prepare Environment
Create/attach ConfigurableEnvironment
Invoke all EnvironmentPostProcessors
• Create ApplicationContext
• Prepare context
• Refresh context (bean creation begins)

The problem is the plugin manager has different discovery mechanisms (but very similar). Worst, one of those mechanisms appears to require the application context, which won't exist when we need to load configuration. I think the need for the application context (or rather the parent application context) isn't actually used though.

I'm doing further research on this to see how best to unify the plugin manager mechanisms so we can share the logic in both places.

[jdaugherty]

I've been working on this and will likely push a significant update in the next day or so. The core issue is the plugin filtering needs shared between the environment & the application post processors. The problem: the plugin manager is what currently does this filtering & it's a bean. The environment post processor can't use a bean b/c the context doesn't exist yet. The only practical place the filtering is used is when in unit tests it filters the plugins. I think the right solution is either moving the filter further out & sharing it between the plugin manager / EPP, or just moving the plugin manager away from being a bean completely.

[jdaugherty]

This change was way larger than I wanted it to be. I ended up having to do this change manually because the complicated lifecycle made it so AI couldn't easily work in this area. As a summary: to determine what plugin configuration to load, you have to know the order & what plugins were loaded. This requires a lot of information, including:

• plugin version / name
• grails version (not supported => not loaded)
• loadbefore / loadafter / dependencies fields
• environment support (wrong environment => not loaded)
• evictions (if evicted, never loaded)

Worst, there isn't a stable interface for a Grails Plugin. Several properties are 'dynamic' in that they can support a single string, a list, or even a map. All of this is also determined at runtime via some rather complicated logic. I introduced a GrailsPluginDiscovery class that determines the order plugins should be loaded in (which is also different than the sorted order that later processing happens against). The majority of these changes are "lift" and shifting code to an earlier process & then referencing that bean to get the various orders so when the plugins are actually loaded they load the same way the configuration did. I very well could have made an error in the lift and shift, but I did it incrementally because it kept being so fragile. Most of the private methods on the various GrailsPluginManagers were moved to helpers on a GrailsPluginUtils class.

My other issue was I needed a way to "share" data between the bootstrap process & the actual bean loading processing (I did not want to load the data twice and assume it would load the same). Spring has a bootstrapContext that supports promoting shared classes to beans, and that's the approach I've taken in this PR. However, there is one spec test - EmbeddedContainerWithGrailsSpec - that explicitly tests an embedded context. Since this is a test, I'm assuming that bootstrap process works in an embedded container, but if we take this change we need to explicitly test the embedded tomcat server because I had to implement a crude bootstrap process to get the test to pass (are the functional tests enough for this? they start fine...). My guess is if anyone is programmatically starting a GrailsApplication, then they'll have to simulate this same process (unless they're invoking it via the spring mechanism).

I also split the upgrade documentation into a 7.1 & 7.0 section. I believe there's more work to do on documenting this PR, but I wanted the actual change reviewed before trying to finalize the documentation. The most noteable impacts to this change are: if any tests are "mocking" the plugin environment, and not using a standard mechanism, then the bootstrap process that populates the plugin order will now need mocked too. Note: if you're using the testing-support libraries, this is done for you.

Finally, as part of testing this, we need to make sure logging works when a bootstrap error occurs. In some of my tests, I didn't see logging on an exception from the GrailsPluginDiscovery class. I think it was because of the test logging configuration, but if we decide to move forward with this, it's one of those critical items we need to check.

This change is way too large for 7.0. Maybe even for 7.1 due to the scope of changes. I think if we leave this in 7.1, we should do a milestone release of 7.1 given the impact this could have. Otherwise, we need to wait until Grails 8. There are some class renames in this PR, that I can probably undo to limit the scope, but I think the renamed files are considered internal to Grails and it's best to probably leave them for clarity. I'd rather see this support added sooner than later given that a key feature of Spring can't be used without it. It's just unfortunate how much work had to be done to get this working.

Oh, and I also discovered there are several classes in core that don't appear to be used anymore. I'm assuming some of them were there for troubleshooting, but we should eventually cleanup the dead code in this area. There are other optimizations that can be done too - for example the plugin load order should probably be calculated before trying to load. Right now, it just iterates over and over against the delayed plugin list until all plugins are found in the expected order - effectively bruteforcing the load order.

@jamesfredley & @matrei take a look at this PR now and see what you think. All of the tests are passing locally for me now. I've also published this version locally and started my large Grails application without issue (which means it works with many, external & existing Grails plugins). I also confirmed the startup order was roughly the same in my application before/after this change (there is some randomness to this due to the classpath discovery order and not all plugins having explicit loadbefore/loadafter). My preference would be a 7.1.0-M1 and put this there.