jdaugherty commented on PR #15409: URL: https://github.com/apache/grails-core/pull/15409#issuecomment-3995121972
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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
