codeconsole commented on PR #13863: URL: https://github.com/apache/grails-core/pull/13863#issuecomment-4925520528
### Form-content parsing: consolidating on `FormContentFilter` Following up on @jdaugherty's first point (the newly-active `OrderedFormContentFilter`), I've made the filter the **single** mechanism for parsing non-POST form bodies, rather than having it coexist with `GrailsParameterMap`'s own PUT/PATCH parsing. **What changed** - `GrailsParameterMap` no longer parses `PUT`/`PATCH` bodies itself. At runtime the request has already passed through `OrderedFormContentFilter` (order `-9500`, ahead of the Grails and Spring Security filters), so its inline `if (requestMap.isEmpty()) …` parse never actually ran — it only fired in unit tests and filter-absent configs. It's removed; `GrailsMockHttpServletRequest` now simulates the filter so the unit-test ergonomic (set a form body, read `params`) is preserved. - New `GrailsFormContentFilterAutoConfiguration` guarantees a `FormContentFilter` for every Grails servlet app. Boot registers one for a default app; this only **fills the gap** when Boot's `WebMvcAutoConfiguration` backs off (i.e. an `@EnableWebMvc` app). It's ordered after `WebMvcAutoConfiguration` and gated on `@ConditionalOnMissingBean(FormContentFilter)`, so Boot's takes precedence by default and Grails only supplies one when it's absent. `spring.mvc.formcontent.filter.enabled=false` still disables it — that stays a real off-switch. Net: `PUT`/`PATCH`/`DELETE` form bodies parse uniformly in every Grails servlet app unless explicitly disabled. **On `DELETE` — and a heads-up on the `enable-mvc-check` test** The `enable-mvc-check` `Application` class **imported** `@EnableWebMvc` but never actually applied the annotation (the class only had `@CompileStatic`), so the app was silently running as a normal Grails app — which is why its `DELETE` and locale assertions were failing on the branch. I applied the annotation. With the annotation applied *and* the fallback filter in place, `DELETE` form bodies are now parsed even under `@EnableWebMvc`, so I flipped the `DELETE` spec from "not parsed" to "parsed." **This intentionally reverses the Grails-7 parity that test asserted, and I want to flag it explicitly.** The reasoning: parsing a `DELETE` form body is benign and consistent — Boot does it by default, `grails-spring-security` has registered a `FormContentFilter` (which handles DELETE) for years, and the old "`PUT`/`PATCH` yes, `DELETE` no" split was a historical accident in `GrailsParameterMap`, not a deliberate rule. The only argument for *not* parsing it is byte-for-byte Grails-7 parity for the `@EnableWebMvc` escape hatch — i.e. preserving a silently-ignored `DELETE` body that essentially no application relies on. The deliberate change here is specifically that form-content parsing (`PUT`/`PATCH`/`DELETE`) is always on — even under `@EnableWebMvc` — unless disabled via `spring.mvc.formcontent.filter.enabled=false`. If the consensus is that `@EnableWebMvc` should be exactly Grails 7 on this point too, I'm happy to switch the fallback to a `FormContentFilter` subclass that skips `DELETE` (via `shouldNotFilter`) — but I think uniform parsing is the better default. **Verification**: `enable-mvc-check` functional spec (7/7), new `GrailsFormContentFilterAutoConfigurationSpec` (5/5), plus `grails-controllers` / `grails-web-common` / `grails-test-suite-uber` / `grails-test-suite-web` green; Checkstyle / CodeNarc / PMD / SpotBugs clean. -- 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]
