codeconsole commented on code in PR #16019:
URL: https://github.com/apache/grails-core/pull/16019#discussion_r3686893207
##########
grails-cache/src/main/groovy/grails/plugin/cache/CacheGrailsPlugin.groovy:
##########
@@ -22,16 +22,25 @@ package grails.plugin.cache
import groovy.transform.CompileStatic
import groovy.util.logging.Slf4j
-import org.springframework.beans.factory.BeanRegistrar
-import org.springframework.beans.factory.BeanRegistry
+import org.springframework.boot.autoconfigure.AutoConfiguration
+import
org.springframework.boot.autoconfigure.condition.ConditionalOnBooleanProperty
import org.springframework.cache.Cache
-import org.springframework.core.env.Environment
import grails.plugins.Plugin
import org.grails.plugin.cache.GrailsCacheManager
+/**
+ * Configures the cache plugin.
+ *
+ * <p>Every bean is contributed as auto-configuration so that one supplied by
the application or
+ * another plugin — for example a cache-provider plugin's {@code
grailsCacheManager} — makes the
+ * default back off cleanly instead of triggering a bean-definition override.
The whole set is gated
+ * on {@code grails.cache.enabled}.</p>
+ */
@Slf4j
@CompileStatic
+@AutoConfiguration
+@ConditionalOnBooleanProperty(name = 'grails.cache.enabled', matchIfMissing =
true)
Review Comment:
Deliberate, and now stated in the class javadoc (`33c9ee771d`) rather than
left to omission: the jar being present is sufficient, as for an ordinary Boot
starter. The gate cannot be restated once `grailsCacheConfiguration` is
declared in this same block — it would condition on a bean the class
contributes — and the plugin declares no `profiles` or `environments`, so it is
active wherever its jar is. That is what makes it a change of mechanism rather
than of which applications get the beans. The javadoc says a descriptor that
later becomes conditionally inactive needs a new anchor.
On the property: real, but the direction is the other way round. Comparing
`@ConditionalOnBooleanProperty` against `config.getProperty(..., Boolean,
true)`:
| value | condition | old conversion |
| --- | --- | --- |
| `no` / `off` / `0` | disabled | disabled |
| `yes` / `on` / `1` | **disabled** | **enabled** |
So `=no` is unchanged; `=yes` is the regression. And it was worse than a
disagreement: `doWithApplicationContext` re-read the flag with the relaxed
conversion, so on `=yes` it believed caching was on and asked for
`grailsCacheConfiguration` — which the condition had just declined to register.
Startup failure.
That hook is now keyed on `applicationContext.containsBean(...)`, which
cannot drift from the condition that decided it. The `log.warn` is restored
with it.
Covered in `9f18a77067`: three unrolled cases for `yes`/`on`/`1` asserting
the beans are absent and the hook backs off. Against the previous
implementation all three fail with `NoSuchBeanDefinitionException`, and a
fourth case runs the hook on the enabled context so the back-off cannot pass by
being taken unconditionally.
--
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]
