codeconsole commented on PR #16237: URL: https://github.com/apache/grails-core/pull/16237#issuecomment-5704157169
@matrei Addressed the valid findings from [Review round 2](https://github.com/apache/grails-core/pull/16237#issuecomment-5696991670) in ab080d24ae. - **Web plugin graph:** web traits automatically include only `restResponder`. JSON and optional XML bean definitions are contributed without discovering their plugins' transitive dependencies. The regression spec overrides `getIncludePlugins()` and verifies that only `core` and `restResponder` load, no default URL mappings appear, and JSON/XML support remains available. Removed the `CompiledTagInvocationSpec` controller-name workaround; its tests pass without it. - **Duplicate definitions:** removed the interceptor's second converters registration and renderer registry definition; URL mappings, link generator and locale resolver definitions are conditional on their absence. A regression assertion verifies that the mapper customizer used while Boot constructs the mapper is the same instance retained in the context. - **Legacy strings:** both paths now use `new JSON(object)`. This also exposed a second underlying issue: `JSONWriter` rejected scalar roots. It now accepts one scalar root and rejects subsequent root values. Tests cover ordinary/named legacy string rendering, scalar values/escaping and existing enum rendering. - **Converter allocation:** cache the Grails Jackson converter per source converter, retaining the guards for custom converters and per-type mappers. Repeated-render coverage checks that distinct source mapper configurations remain distinct. - **Mapper lookup:** memoize the first successful supplier result; an absent mapper remains retryable. The regression test checks lookup counts and successful default/named output after an initial missing mapper. - **Documentation:** clarified automatic `restResponder` inclusion even with `getIncludePlugins()` overrides, optional XML bean contributions, and the absence of automatic controllers/URL mappings/i18n plugin loading. One refinement to the suggested XML approach: contributing XML beans in the interceptor after refresh broke existing nested XML binding tests because the binding registry had already captured its source creators. Both JSON and optional XML bean definitions therefore need to be contributed **before refresh**, while still avoiding plugin dependency expansion. Existing XML binding tests now pass. Validation was deliberately limited to the affected modules, with `--max-workers=2 -PmaxTestParallel=1 --no-parallel`; no full repository test sweep was run. The `check` tasks passed for `grails-test-suite-web`, `grails-testing-support-core`, `grails-testing-support-web`, `grails-web-common`, `grails-converters`, `grails-rest-transforms`, and `grails-gsp`. Their test reports total 1,480 tests, nine skipped, zero failures/errors (including the web suite's isolated tests). Checkstyle/CodeNarc XML reports contain zero violations. PMD/SpotBugs were requested, but this checkout does not expose `codeAnalysis` tasks in these modules, so those tools are not claimed as verified. -- 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]
