jdaugherty commented on PR #13863:
URL: https://github.com/apache/grails-core/pull/13863#issuecomment-4905621040
I had Fabled take a look at this PR now that we've merged the security
changes. I think the issue identified in the security PR is still a problem.
We're now registering beans that we don't want registered because using the MVC
annotation effectively prevented spring boot from registering from defaults
that Grails relied on being absent. Here are the details:
**Impacts beyond what the PR/docs cover**
This is the important part: activating WebMvcAutoConfiguration brings in
more than the two things the PR compensates for. These are behavior changes
every Grails 8 app will see:
- OrderedFormContentFilter is now registered (on by default via
spring.mvc.formcontent.filter.enabled). PUT/PATCH/DELETE form-encoded bodies
become visible as request parameters — previously they weren't. Arguably an
improvement, but it's a real behavioral difference for apps with PUT/PATCH
endpoints doing their own body parsing, and it's not mentioned in the upgrade
notes.
- spring.mvc.* and spring.web.* properties now take effect. Any app that had
these set (inertly, copied from Boot docs) will see them suddenly activate on
upgrade: spring.web.locale, spring.mvc.format.date, spring.web.resources.*, etc.
- Boot's static-resource handling and welcome-page mapping are now live.
Boot's WebMvcAutoConfigurationAdapter registers a /** resource handler
(classpath:/static, /public, …) alongside Grails' own GrailsWebMvcConfigurer
handler, plus WelcomePageHandlerMapping. URLs that previously 404'd through
Grails URL-mapping error handling may now be served as static resources or an
index page — this can change error-page behavior and even shadow expectations
in apps with catch-all URL mappings.
- viewResolver is now Boot's ContentNegotiatingViewResolver for non-GSP apps
(GSP apps alias it to gspViewResolver as before). Generally compatible since it
delegates, but it's a different resolution pipeline than before.
- mvcConversionService is now Boot's ApplicationConversionService (honoring
format properties) instead of Spring's plain one, and message converters get
the full Boot customization (context ObjectMapper, etc.) — subtle differences
for @RestController-style endpoints and MVC-layer binding.
I'd suggest a short bullet list of these in section 31 of the upgrade notes
("other Boot MVC features that become active"), since the current text says "No
action is required for most applications" and only documents the two
compensated differences.
**One real edge-case bug candidate**
If a user overrides only the grailsWebRequestFilter registration bean (which
the docs explicitly bless), the auto-configured grailsWebRequest plain Filter
bean is still created (@ConditionalOnMissingBean(GrailsWebRequestFilter.class)
doesn't back off), and since no registration bean references it anymore, Boot's
adaptable-beans mechanism will auto-register it on /* at
Ordered.LOWEST_PRECEDENCE — running in addition to the user's replacement
filter. Previously, overriding the registration bean fully removed the Grails
filter. The new spec's override test only asserts bean identity in the context,
so it wouldn't catch this. Fix options: gate grailsWebRequest on the
registration bean name too, or have the override test assert against the actual
servlet filter chain.
--
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]
