jdaugherty commented on code in PR #16040:
URL: https://github.com/apache/grails-core/pull/16040#discussion_r3721764304
##########
grails-controllers/src/test/groovy/org/grails/plugins/web/controllers/ControllersAutoConfigurationSpec.groovy:
##########
@@ -54,6 +57,31 @@ class ControllersAutoConfigurationSpec extends Specification
{
def autoConfiguration = new ControllersAutoConfiguration()
+ def "legacy multipart configuration fails startup with migration
instructions"() {
+ given:
+ def applicationContext = new AnnotationConfigWebApplicationContext()
+ applicationContext.servletContext = new MockServletContext()
+ applicationContext.environment.propertySources.addFirst(new
MapPropertySource('test', [
+ 'grails.controllers.upload.maxFileSize': 20000000,
+ ]))
+ applicationContext.register(ControllersAutoConfiguration)
+
+ when:
+ applicationContext.refresh()
+
+ then:
+ def exception = thrown(BeanCreationException)
+ exception.rootCause instanceof IllegalStateException
+ exception.rootCause.message ==
+ "Configuration properties under 'grails.controllers.upload'
are no longer supported. " +
+ "Use Spring Boot's 'spring.servlet.multipart' configuration
instead. For example, set " +
+ "'spring.servlet.multipart.maxFileSize=200MB' and " +
+ "'spring.servlet.multipart.maxRequestSize=200MB'."
+
+ cleanup:
+ applicationContext.close()
+ }
Review Comment:
`dispatcherServletRegistration` changed signature to
`ObjectProvider<MultipartConfigElement>`, but nothing exercises it. Could this
spec add three cases using the `WebApplicationContextRunner` already used below?
- Boot supplies the element and it reaches the registration — the behavior
this PR is preserving
- `spring.servlet.multipart.enabled=false` leaves the registration without
multipart config
- no legacy property present → context starts cleanly (negative case for the
new guard)
Adding `MultipartAutoConfiguration` to `AutoConfigurations.of(...)` is
enough to get the element, and
`DispatcherServletRegistrationBean.getMultipartConfig()` exposes the result.
`grails-test-examples/app1/src/integration-test/groovy/functionaltests/fileupload/FileUploadSpec.groovy`
would also be a good home for an end-to-end case: a file over `maxFileSize` is
rejected, and raising `spring.servlet.multipart.maxFileSize` takes effect.
##########
grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/ControllersAutoConfiguration.java:
##########
@@ -150,12 +151,11 @@ public FilterRegistrationBean<GrailsWebRequestFilter>
grailsWebRequestFilter(Gra
return registrationBean;
}
- @Bean
- public MultipartConfigElement multipartConfigElement() {
- if (uploadTmpDir == null) {
- uploadTmpDir = System.getProperty("java.io.tmpdir");
+ @Override
+ public void setEnvironment(Environment environment) {
+ if (Binder.get(environment).bind(LEGACY_MULTIPART_CONFIGURATION,
Bindable.mapOf(String.class, Object.class)).isBound()) {
Review Comment:
Two things on the guard:
1. The message surfaces as a nested cause inside a `BeanCreationException`
stack trace — no `FailureAnalyzer` matches an `IllegalStateException` thrown
from `setEnvironment`, so the migration instructions never reach Boot's
`APPLICATION FAILED TO START` / `Action:` block where a user would look for
them. Either register a `FailureAnalyzer`, or move the check into an
`EnvironmentPostProcessor` (grails-core already registers those in
`META-INF/spring.factories`) so it fires before context creation. Secondary
point: `setEnvironment` is a wiring callback, and a validation that throws is
unexpected there.
2. The check is namespace-wide, so it can't tell the user which key to fix —
any property under `grails.controllers.upload` trips it, including keys that
were never Grails settings. The bound map already carries the names:
```java
BindResult<Map<String, Object>> legacy = Binder.get(environment)
.bind(LEGACY_MULTIPART_CONFIGURATION, Bindable.mapOf(String.class,
Object.class));
if (legacy.isBound()) {
throw new IllegalStateException(LEGACY_MULTIPART_CONFIGURATION_ERROR
+ " Found: " + legacy.get().keySet());
}
```
##########
grails-doc/src/en/guide/theWebLayer/controllers/uploadingFiles.adoc:
##########
@@ -103,16 +103,19 @@ You can configure the limit in your `application.yml` as
follows:
[source,yml]
.grails-app/conf/application.yml
----
-grails:
- controllers:
- upload:
- maxFileSize: 2000000
- maxRequestSize: 2000000
+spring:
+ servlet:
+ multipart:
+ maxFileSize: 200MB
+ maxRequestSize: 200MB
----
-`maxFileSize` = The maximum size allowed for uploaded files.
+`maxFileSize` = The maximum size allowed for an uploaded file.
-`maxRequestSize` = The maximum size allowed for multipart/form-data requests.
+`maxRequestSize` = The maximum size allowed for a multipart/form-data request.
+
+The `spring.servlet.multipart` namespace also supports Spring Boot's
`location`, `fileSizeThreshold`, and `enabled` properties.
Review Comment:
Boot 4.1 also exposes `spring.servlet.multipart.resolveLazily` and
`spring.servlet.multipart.strictServletCompliance` under this namespace. Either
add them, or link Boot's multipart configuration docs so this list doesn't need
maintaining.
##########
grails-doc/src/en/guide/upgrading/upgrading80x.adoc:
##########
@@ -136,6 +138,34 @@ spring:
database: mydb
----
+===== 4.2 Multipart Upload Configuration
+
+The `grails.controllers.upload.*` configuration properties are no longer
supported.
+Applications that still define any property under `grails.controllers.upload`
fail at startup with migration instructions.
+Move those settings to Spring Boot's `spring.servlet.multipart.*` namespace:
+
+[source,yaml]
+.application.yml - Before (Grails 7)
+----
+grails:
+ controllers:
+ upload:
+ maxFileSize: 200000
+ maxRequestSize: 200000
+----
+
+[source,yaml]
+.application.yml - After (Grails 8)
+----
+spring:
+ servlet:
+ multipart:
+ maxFileSize: 200MB
+ maxRequestSize: 200MB
+----
+
+Spring Boot also provides `spring.servlet.multipart.location`,
`spring.servlet.multipart.fileSizeThreshold`, and
`spring.servlet.multipart.enabled` as replacements for the corresponding legacy
settings.
Review Comment:
§4.2 covers the namespace move but not the two runtime changes an upgrading
operator would notice:
- **Temp directory.** `location` used to default to
`System.getProperty("java.io.tmpdir")`; Boot leaves it unset, so the servlet
container's temp directory is used instead (the resulting
`MultipartConfigElement` has `location=""`). Worth a sentence for anyone who
relied on uploads landing in `java.io.tmpdir`, or who runs with a constrained
container temp dir.
- **Effective limits when nothing is configured.** An app that never set an
upload limit now picks up Boot's `MultipartProperties` defaults — 1MB per file,
10MB per request. Stating that here, next to the advice to set an explicit
limit, saves the reader a hop to `uploadingFiles.adoc`.
--
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]
