codeconsole commented on PR #16292: URL: https://github.com/apache/grails-core/pull/16292#issuecomment-5572699397
Thanks — four of the five hold up, and I checked each against the code rather than taking them as read. Addressed in f7d22ce (code + spec) and cf68805 (docs). **[P1] `LinkageError` — valid, fixed.** `ClassNotFoundException` only covers the class being absent from the loader that was asked. A class present but unlinkable raises `NoClassDefFoundError`, and since `Class.forName` here initializes (reading `getCurrent` needs it), a static initializer that fails raises `ExceptionInInitializerError`. Both are `LinkageError`, so both were thrown straight out of `matches(...)` and failed the configuration this condition exists to skip quietly. `LinkageError` joins the catch; `VirtualMachineError` deliberately does not. **[P2] class loader — valid, fixed slightly differently.** Rather than adding TCCL as a third rung by hand, the null case now goes through Spring's own `ClassUtils.getDefaultClassLoader()`, which already tries the thread context loader first, then this class's, then the system loader. Same outcome for the case you name, one call, and it tracks Spring if that order ever changes. **[P3] `OnGrailsEnvCondition` spec — valid, added.** New `OnGrailsEnvConditionSpec` covers absent attributes, the `grails.env` fallback, case-insensitive matching across several environments, neither source answering, and both `LinkageError` paths. I reverted the catch and reran to confirm the last two genuinely fail without the fix rather than passing vacuously — 2 of 6 fail, exactly those. **[P2] behaviour change + [P3] docs — valid, and the more important of the two.** You are right that the guide is hand-written and was stale: `hookingIntoRuntimeConfiguration.adoc` enumerated the DSL as of #16019 and none of this PR. It now covers `bean(name, Interface, Implementation)`, what a construction settles about type arguments, `.typeArguments(...)`, constant member names, `group(...)`, the five remaining conditions, `.aliases(...)` and `.scope(...)`'s other attributes; corrects `.staticMethod()` (now required for a post-processor bean, not recommended) and `.annotate(...)` (not single-valued, and merges rather than collides); and adds two sections a reader cannot infer from the syntax — the three diagnostics, each with what used to happen instead, and the `-Dgrails.beans.dsl.dumpdir` property. The plugin-descriptor behaviour change is called out there, in the diagnostics section. One correction to the review's framing: the reflective catch and the loader fallback are the runtime half of `.conditionalOnGrailsEnv(...)`, not of the transform — worth saying only because the two failure modes look alike in a stack trace and land in different places. CI was also red on two of mine, fixed in c4cadbba: a checkstyle `ImportOrder` violation, and a `conditionalOnBean` test that asserted an ordering the annotation does not guarantee. It declared the supplier beside the consumer in one block, so it depended on `@Bean` method order within a single class — it passed locally and failed once the branch picked up the indy-off compile. The supplier now comes from a separate `@Configuration` registered first, which is the ordering `@ConditionalOnBean` actually promises, and the qualifier's javadoc says so now. -- 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]
