diff --git a/grails-async/plugin/src/main/groovy/grails/async/web/AsyncController.groovy b/grails-async/plugin/src/main/groovy/grails/async/web/AsyncController.groovy index 71d4892d526..afcb28f58a4 100644 --- a/grails-async/plugin/src/main/groovy/grails/async/web/AsyncController.groovy +++ b/grails-async/plugin/src/main/groovy/grails/async/web/AsyncController.groovy @@ -50,7 +50,7 @@ trait AsyncController { AsyncContext startAsync() { GrailsWebRequest webRequest = (GrailsWebRequest) RequestContextHolder.currentRequestAttributes() - HttpServletRequest request = webRequest.currentRequest + HttpServletRequest request = webRequest.request WebAsyncManager asyncManager = WebAsyncUtils.getAsyncManager(request) AsyncWebRequest asyncWebRequest = new AsyncGrailsWebRequest(request, webRequest.currentResponse, webRequest.servletContext) diff --git a/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/AsyncWebRequestPromiseDecorator.groovy b/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/AsyncWebRequestPromiseDecorator.groovy index 28bc9f1b050..3edd96a28b0 100644 --- a/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/AsyncWebRequestPromiseDecorator.groovy +++ b/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/AsyncWebRequestPromiseDecorator.groovy @@ -53,7 +53,7 @@ class AsyncWebRequestPromiseDecorator implements PromiseDecorator { AsyncWebRequestPromiseDecorator(GrailsWebRequest webRequest) { this.webRequest = webRequest - HttpServletRequest currentServletRequest = webRequest.currentRequest + HttpServletRequest currentServletRequest = webRequest.request WebAsyncManager asyncManager = WebAsyncUtils.getAsyncManager(currentServletRequest) AsyncGrailsWebRequest newWebRequest if (asyncManager.isConcurrentHandlingStarted()) { diff --git a/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/mvc/AsyncActionResultTransformer.groovy b/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/mvc/AsyncActionResultTransformer.groovy index bfbb5eda24c..ada5ec1d090 100644 --- a/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/mvc/AsyncActionResultTransformer.groovy +++ b/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/mvc/AsyncActionResultTransformer.groovy @@ -52,7 +52,7 @@ class AsyncActionResultTransformer implements ActionResultTransformer { if (actionResult instanceof Promise) { - final request = webRequest.getCurrentRequest() + final request = webRequest.getRequest() WebAsyncManager asyncManager = WebAsyncUtils.getAsyncManager(request) final response = webRequest.getResponse() diff --git a/grails-benchmarks/README.adoc b/grails-benchmarks/README.adoc index fed304575d1..d600ef421cf 100644 --- a/grails-benchmarks/README.adoc +++ b/grails-benchmarks/README.adoc @@ -88,12 +88,17 @@ Benchmarks are Java, under `src/jmh/java`. Setup fixtures are Groovy, under `src The comparison tool is Groovy in the `report` source set, under `src/report/groovy`. That split is intentional. A benchmark written in Groovy measures Groovy's dynamic call-site -machinery as much as the Grails API under test, so the measured method stays in Java. Fixtures -are Groovy only where the Grails API genuinely requires it - the URL mappings DSL and view -templates are closure-based, and `grails.artefact.Interceptor` is a Groovy trait. Fixtures live in `main` rather -than `src/jmh/groovy` because the `me.champeau.jmh` plugin puts the main source set's output on -the jmh compile classpath, which keeps the benchmark source set pure Java and preserves normal -JMH annotation processing. +machinery as much as the Grails API under test, so the measured method stays in Java. Fixtures live +in `main` rather than `src/jmh/groovy` because the `me.champeau.jmh` plugin puts the main source +set's output on the jmh compile classpath, which keeps the benchmark source set pure Java and +preserves normal JMH annotation processing. + +Setup code goes in a Groovy fixture whether or not the Grails API forces it to - the URL mappings +DSL and view templates are closure-based and `grails.artefact.Interceptor` is a Groovy trait, but +building a mock servlet context or compiling a controller does not need Groovy and lives there +anyway. Nothing a fixture does is inside the timed region, so the only fixture that is Groovy *for +the measurement* is `web/RequestPropertyFixture`, whose reader is deliberately not statically +compiled because the dynamic call site is the thing being measured. The `report` source set depends on Groovy alone and on no Grails project. The CI reporting job runs it after a bare checkout, so it must render Markdown without compiling the framework. @@ -103,10 +108,12 @@ Benchmarks are grouped by package, and the CI report aggregates per group: |=== | Package | Covers -| `urlmappings` | Request URI matching (warm and cold cache) and reverse URL creation +| `urlmappings` | Request URI matching (warm cache, cold cache, catch-all fall-through) and reverse URL creation +| `controllers` | Controller action invocation, the `redirect` and `render(template:)` response paths, and the per-request collection of controller URL mappings +| `web` | Binding a `GrailsWebRequest`, building `params`, multipart discovery, Groovy request property access | `databinding` | Binding a map onto an object, with and without type conversion | `gsp` | GSP parsing (template text to generated Groovy source) -| `interceptors` | Interceptor URI match decisions +| `interceptors` | Interceptor URI match decisions, and the interceptor handler chain per request | `views` | JSON and markup view rendering | `ruler` | Pure-JDK probes used to detect an unstable CI runner |=== diff --git a/grails-benchmarks/build.gradle b/grails-benchmarks/build.gradle index be247d4ca1e..0e00de69ec0 100644 --- a/grails-benchmarks/build.gradle +++ b/grails-benchmarks/build.gradle @@ -71,14 +71,26 @@ dependencies { implementation project(':grails-views-gson') implementation project(':grails-views-markup') implementation project(':grails-core') + // The controller AST transformer, so that benchmarked controllers are compiled by the same + // injector a real application's controllers go through. + implementation project(':grails-controllers') + // The data binding collaborators an application registers as beans, so that a command object + // action binds the way it does in a running application rather than building a binder per call. + implementation project(':grails-web-databinding') + implementation project(':grails-mimetypes') implementation 'org.apache.groovy:groovy' + // ObservationRegistry, which GrailsInterceptorHandlerInterceptorAdapter branches on + implementation 'io.micrometer:micrometer-observation' + // The framework modules declare these as compileOnly, so they are absent from the runtime // classpath a benchmark actually executes on. The modules' own test suites add them back // the same way. Without them, URL mapping, interceptor and view benchmarks fail at @Setup // with NoClassDefFoundError: jakarta/servlet/ServletContext. implementation 'jakarta.servlet:jakarta.servlet-api' implementation 'org.springframework:spring-test' + // ModelAndView, which the interceptor adapter's postHandle takes + implementation 'org.springframework:spring-webmvc' reportImplementation platform(project(':grails-bom')) reportImplementation 'org.apache.groovy:groovy' diff --git a/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/controllers/ControllerActionBenchmark.java b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/controllers/ControllerActionBenchmark.java new file mode 100644 index 00000000000..23fcd5bce2f --- /dev/null +++ b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/controllers/ControllerActionBenchmark.java @@ -0,0 +1,252 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.benchmarks.controllers; + +import java.util.concurrent.TimeUnit; + +import groovy.lang.GroovyClassLoader; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; +import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.mock.web.MockServletContext; +import org.springframework.web.context.WebApplicationContext; + +import grails.core.GrailsApplication; +import grails.core.GrailsControllerClass; +import grails.util.Environment; +import grails.util.GrailsWebMockUtil; +import grails.web.databinding.DataBindingUtils; +import grails.web.databinding.GrailsWebDataBinder; +import grails.web.mime.MimeTypeResolver; +import org.apache.grails.benchmarks.web.WebContextFixture; +import org.grails.core.DefaultGrailsControllerClass; +import org.grails.web.databinding.bindingsource.DataBindingSourceRegistry; +import org.grails.web.databinding.bindingsource.DefaultDataBindingSourceRegistry; +import org.grails.web.mime.DefaultMimeTypeResolver; + +/** + * Measures one controller action invocation through {@code GrailsControllerClass.invoke}, which is + * the call {@code UrlMappingsInfoHandlerAdapter} makes for every request that reaches a controller. + * + *

Three shapes are measured, because the generated wrapper differs between them:

+ * + * + *

The controllers are compiled at setup by a {@code GrailsAwareClassLoader} running the real + * {@code ControllerActionTransformer}, so the bytecode invoked is the bytecode a Grails application + * would run. Setup also prints, once per fork, how many request-attribute operations one invocation + * of each action performs, measured against a counting request outside the timed region. That count + * - not the timing - is the direct evidence of what the generated code does.

+ */ +@State(Scope.Benchmark) +@Threads(1) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Warmup(iterations = 3, time = 1, timeUnit = TimeUnit.SECONDS) +@Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) +@Fork(value = 2, jvmArgsAppend = {"-Xms1g", "-Xmx1g", "-XX:+UseG1GC"}) +public class ControllerActionBenchmark { + + private static final String PLAIN_CONTROLLER_SOURCE = """ + @grails.artefact.Artefact('Controller') + class BenchmarkPlainController { + def index() { + 'plain' + } + } + """; + + private static final String RESTRICTED_CONTROLLER_SOURCE = """ + @grails.artefact.Artefact('Controller') + class BenchmarkRestrictedController { + static allowedMethods = [index: 'GET'] + def index() { + 'restricted' + } + } + """; + + private static final String COMMAND_CONTROLLER_SOURCE = """ + class BenchmarkBookCommand { + String title + Integer pages + } + + @grails.artefact.Artefact('Controller') + class BenchmarkCommandController { + def save(BenchmarkBookCommand command) { + 'saved' + } + } + """; + + private GrailsControllerClass plainControllerClass; + + private Object plainController; + + private GrailsControllerClass restrictedControllerClass; + + private Object restrictedController; + + private GrailsControllerClass commandControllerClass; + + private Object commandController; + + @Setup + public void setup() throws Throwable { + MockServletContext servletContext = WebContextFixture.createServletContext(); + WebApplicationContext applicationContext = WebContextFixture.applicationContext(servletContext); + registerDataBindingBeans(applicationContext); + + GroovyClassLoader classLoader = ControllerFixture.createTransformingClassLoader(); + + classLoader.parseClass(PLAIN_CONTROLLER_SOURCE, "BenchmarkPlainController.groovy"); + Class plainClass = classLoader.loadClass("BenchmarkPlainController"); + plainControllerClass = new DefaultGrailsControllerClass(plainClass); + plainController = plainClass.getDeclaredConstructor().newInstance(); + + classLoader.parseClass(RESTRICTED_CONTROLLER_SOURCE, "BenchmarkRestrictedController.groovy"); + Class restrictedClass = classLoader.loadClass("BenchmarkRestrictedController"); + restrictedControllerClass = new DefaultGrailsControllerClass(restrictedClass); + restrictedController = restrictedClass.getDeclaredConstructor().newInstance(); + + classLoader.parseClass(COMMAND_CONTROLLER_SOURCE, "BenchmarkCommandController.groovy"); + Class commandClass = classLoader.loadClass("BenchmarkCommandController"); + commandControllerClass = new DefaultGrailsControllerClass(commandClass); + commandController = commandClass.getDeclaredConstructor().newInstance(); + + reportAttributeCounts(servletContext, applicationContext); + + // The request all three measured invocations run against. GET, which is what the restricted + // controller's allowedMethods declares, so that benchmark measures the check passing rather + // than the 405 error path. Two request parameters are present so the command object action + // has something to bind. + MockHttpServletRequest request = new MockHttpServletRequest(servletContext, "GET", "/benchmark/index"); + request.setParameter("title", "Groovy in Action"); + request.setParameter("pages", "912"); + GrailsWebMockUtil.bindMockWebRequest(applicationContext, request, new MockHttpServletResponse()); + + assertFixtureInvokes(); + + System.out.println("[fixture] Environment.isDevelopmentMode()=" + Environment.isDevelopmentMode() + + " (false means GrailsControllerClass.invoke dispatches through a MethodHandle, as in production)"); + } + + /** + * Registers the data binding collaborators an application context normally holds as singletons, + * and points the {@code GrailsApplication} at that context so they are found. + * + *

Without this, {@code DataBindingUtils} cannot find a {@code grailsWebDataBinder} bean and + * builds a whole {@code GrailsWebDataBinder} - conversion service and all - on every single + * command object binding, which costs tens of microseconds and is nothing like what a running + * application does.

+ */ + private static void registerDataBindingBeans(WebApplicationContext applicationContext) { + GrailsApplication grailsApplication = applicationContext.getBean(GrailsApplication.APPLICATION_ID, GrailsApplication.class); + grailsApplication.setApplicationContext(applicationContext); + + DefaultDataBindingSourceRegistry dataBindingSourceRegistry = new DefaultDataBindingSourceRegistry(); + dataBindingSourceRegistry.initialize(); + + ConfigurableListableBeanFactory beanFactory = ((ConfigurableApplicationContext) applicationContext).getBeanFactory(); + beanFactory.registerSingleton(DataBindingSourceRegistry.BEAN_NAME, dataBindingSourceRegistry); + beanFactory.registerSingleton(MimeTypeResolver.BEAN_NAME, new DefaultMimeTypeResolver()); + beanFactory.registerSingleton(DataBindingUtils.DATA_BINDER_BEAN_NAME, new GrailsWebDataBinder(grailsApplication)); + } + + /** + * Invokes each action once against a request that counts attribute operations, and prints the + * counts. Runs before the measured request is bound, and never inside the timed region. + */ + private void reportAttributeCounts(MockServletContext servletContext, WebApplicationContext applicationContext) throws Throwable { + AttributeCountingRequest countingRequest = ControllerFixture.createCountingRequest(servletContext, "GET", "/benchmark/index"); + countingRequest.setParameter("title", "Groovy in Action"); + countingRequest.setParameter("pages", "912"); + GrailsWebMockUtil.bindMockWebRequest(applicationContext, countingRequest, new MockHttpServletResponse()); + + // The first invocation of an action initialises metaclasses and caches, which does its own + // attribute traffic; the reported counts are from the second, steady state, invocation. + plainControllerClass.invoke(plainController, "index"); + countingRequest.resetCounts(); + plainControllerClass.invoke(plainController, "index"); + System.out.println("[fixture] plainAction request attribute ops: " + countingRequest.describeCounts()); + + restrictedControllerClass.invoke(restrictedController, "index"); + countingRequest.resetCounts(); + restrictedControllerClass.invoke(restrictedController, "index"); + System.out.println("[fixture] restrictedAction request attribute ops: " + countingRequest.describeCounts()); + + commandControllerClass.invoke(commandController, "save"); + countingRequest.resetCounts(); + commandControllerClass.invoke(commandController, "save"); + System.out.println("[fixture] commandObjectAction request attribute ops: " + countingRequest.describeCounts()); + } + + // An action that fails - a rejected HTTP method, say - still returns, so check the return value + // rather than publishing a number measured on an error path. + private void assertFixtureInvokes() throws Throwable { + require(plainControllerClass.invoke(plainController, "index"), "plain"); + require(restrictedControllerClass.invoke(restrictedController, "index"), "restricted"); + require(commandControllerClass.invoke(commandController, "save"), "saved"); + } + + private static void require(Object actual, String expected) { + if (!expected.equals(actual)) { + throw new IllegalStateException("Expected the action to return '" + expected + "' but it returned '" + actual + "'"); + } + } + + /** An action on a controller declaring no {@code allowedMethods} - the common case. */ + @Benchmark + public Object plainAction() throws Throwable { + return plainControllerClass.invoke(plainController, "index"); + } + + /** An action on a controller declaring {@code allowedMethods}, where the check has to run. */ + @Benchmark + public Object restrictedAction() throws Throwable { + return restrictedControllerClass.invoke(restrictedController, "index"); + } + + /** An action taking a command object, so the generated wrapper binds one per invocation. */ + @Benchmark + public Object commandObjectAction() throws Throwable { + return commandControllerClass.invoke(commandController, "save"); + } +} diff --git a/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/controllers/ControllerMappingCollectionBenchmark.java b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/controllers/ControllerMappingCollectionBenchmark.java new file mode 100644 index 00000000000..28c777d1fa3 --- /dev/null +++ b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/controllers/ControllerMappingCollectionBenchmark.java @@ -0,0 +1,195 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.benchmarks.controllers; + +import java.util.concurrent.TimeUnit; + +import groovy.lang.GroovyClassLoader; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.mock.web.MockServletContext; +import org.springframework.web.context.WebApplicationContext; + +import grails.core.DefaultGrailsApplication; +import grails.util.GrailsWebMockUtil; +import grails.web.mapping.UrlMapping; +import grails.web.mapping.UrlMappingInfo; +import org.apache.grails.benchmarks.web.WebContextFixture; +import org.grails.web.mapping.mvc.GrailsControllerUrlMappings; + +/** + * Measures {@code GrailsControllerUrlMappings.matchAll}, which is what + * {@code UrlMappingsHandlerMapping.getHandlerInternal} calls on every request. + * + *

The delegate's own {@code matchAll} memoises its result in a Caffeine cache, but the wrapper + * around it - {@code AbstractGrailsControllerUrlMappings.collectControllerMappings} - is not cached + * and runs in full every time. For each candidate it calls {@code webRequest.resetParams()} (which + * clones the parameter map), {@code info.configure(webRequest)}, and allocates a controller key. + * Each benchmark here calls a single URI repeatedly, so the delegate cache always hits and what is + * measured is the uncached wrapper.

+ * + *

Where this is not faithful to production

+ * + */ +@State(Scope.Benchmark) +@Threads(1) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Warmup(iterations = 3, time = 1, timeUnit = TimeUnit.SECONDS) +@Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) +@Fork(value = 2, jvmArgsAppend = {"-Xms1g", "-Xmx1g", "-XX:+UseG1GC"}) +public class ControllerMappingCollectionBenchmark { + + private static final String CONTROLLER_SOURCES = """ + @grails.artefact.Artefact('Controller') + class BenchmarkBookController { + def index() { null } + def show() { null } + def create() { null } + def save() { null } + def edit() { null } + def update() { null } + def patch() { null } + def delete() { null } + } + + @grails.artefact.Artefact('Controller') + class BenchmarkBlogController { + def show() { null } + } + + @grails.artefact.Artefact('Controller') + class BenchmarkStoreController { + def browse() { null } + } + + @grails.artefact.Artefact('Controller') + class BenchmarkHealthController { + def index() { null } + } + """; + + /** Matched by the {@code resources: 'book'} show mapping, and by the catch-all default mapping. */ + private static final String TWO_CANDIDATE_URI = "/api/books/42"; + + /** Five segments, so only the multi-token blog mapping can serve it. */ + private static final String ONE_CANDIDATE_URI = "/blog/2026/08/14/grails-8-is-fast"; + + /** Served by several deliberately overlapping mappings. */ + private static final String FOUR_CANDIDATE_URI = "/api/books/42"; + + private GrailsControllerUrlMappings applicationMappings; + + private GrailsControllerUrlMappings overlappingMappings; + + @Setup + public void setup() throws Exception { + MockServletContext servletContext = WebContextFixture.createServletContext(); + WebApplicationContext applicationContext = WebContextFixture.applicationContext(servletContext); + + GroovyClassLoader classLoader = ControllerFixture.createTransformingClassLoader(); + classLoader.parseClass(CONTROLLER_SOURCES, "BenchmarkMappingControllers.groovy"); + Class[] controllers = new Class[] { + classLoader.loadClass("BenchmarkBookController"), + classLoader.loadClass("BenchmarkBlogController"), + classLoader.loadClass("BenchmarkStoreController"), + classLoader.loadClass("BenchmarkHealthController") + }; + + DefaultGrailsApplication grailsApplication = new DefaultGrailsApplication(controllers); + grailsApplication.setApplicationContext(applicationContext); + grailsApplication.initialise(); + + applicationMappings = ControllerMappingsFixture.createApplicationMappings(grailsApplication, applicationContext); + overlappingMappings = ControllerMappingsFixture.createOverlappingMappings(grailsApplication, applicationContext); + + // The bound request is what makes collectControllerMappings take its expensive branch: + // GrailsWebRequest.lookup() has to return non-null for resetParams()/configure() to run, + // which is always the case in production. + MockHttpServletRequest request = new MockHttpServletRequest(servletContext, "GET", TWO_CANDIDATE_URI); + GrailsWebMockUtil.bindMockWebRequest(applicationContext, request, new MockHttpServletResponse()); + + report("oneCandidate", applicationMappings, ONE_CANDIDATE_URI, 1); + report("twoCandidates", applicationMappings, TWO_CANDIDATE_URI, 2); + report("fourCandidates", overlappingMappings, FOUR_CANDIDATE_URI, 4); + } + + /** + * Prints how many candidates the delegate produces for a URI and how many survive collection, + * and fails if the delegate count is not the number the benchmark claims to measure. + */ + private static void report(String name, GrailsControllerUrlMappings mappings, String uri, int expectedCandidates) { + UrlMappingInfo[] rawCandidates = mappings.getUrlMappingsHolderDelegate() + .matchAll(uri, "GET", UrlMapping.ANY_VERSION); + UrlMappingInfo[] collected = mappings.matchAll(uri, "GET", UrlMapping.ANY_VERSION); + System.out.println("[fixture] " + name + " uri=" + uri + + " candidates=" + rawCandidates.length + " collected=" + collected.length); + if (rawCandidates.length != expectedCandidates) { + throw new IllegalStateException(name + " expected " + expectedCandidates + + " candidates for " + uri + " but the mappings produced " + rawCandidates.length); + } + } + + /** A URI only one mapping can serve, so the wrapper does one candidate's worth of work. */ + @Benchmark + public UrlMappingInfo[] oneCandidate() { + return applicationMappings.matchAll(ONE_CANDIDATE_URI, "GET", UrlMapping.ANY_VERSION); + } + + /** A REST URI served by a resources mapping and by the catch-all default mapping. */ + @Benchmark + public UrlMappingInfo[] twoCandidates() { + return applicationMappings.matchAll(TWO_CANDIDATE_URI, "GET", UrlMapping.ANY_VERSION); + } + + /** Four overlapping candidates, to show how the uncached per-candidate work scales. */ + @Benchmark + public UrlMappingInfo[] fourCandidates() { + return overlappingMappings.matchAll(FOUR_CANDIDATE_URI, "GET", UrlMapping.ANY_VERSION); + } +} diff --git a/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/controllers/ControllerResponseBenchmark.java b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/controllers/ControllerResponseBenchmark.java new file mode 100644 index 00000000000..e3fcc7c571a --- /dev/null +++ b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/controllers/ControllerResponseBenchmark.java @@ -0,0 +1,248 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.benchmarks.controllers; + +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.concurrent.TimeUnit; + +import groovy.lang.GroovyClassLoader; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; +import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.mock.web.MockServletContext; +import org.springframework.web.context.WebApplicationContext; + +import grails.artefact.Controller; +import grails.core.GrailsApplication; +import grails.util.GrailsWebMockUtil; +import grails.web.mapping.LinkGenerator; +import org.apache.grails.benchmarks.web.WebContextFixture; +import org.grails.core.artefact.ControllerArtefactHandler; +import org.grails.web.servlet.view.CompositeViewResolver; +import org.grails.web.util.GrailsApplicationAttributes; + +/** + * Measures the two controller response paths that resolve an application-scoped collaborator on + * every call: {@code redirect(...)}, which needs the controller's declared namespace and a + * {@link grails.web.mapping.ResponseRedirector}, and {@code render(template: ...)}, which needs the + * {@link CompositeViewResolver}. + * + *

Three shapes are measured:

+ * + * + *

Both redirect benchmarks allocate a fresh argument map per invocation, exactly as a controller + * writing {@code redirect(action: 'show')} does. That is not cosmetic: {@code redirect} writes the + * resolved namespace back into the map it is given, so a shared map would skip namespace resolution + * on every invocation after the first and measure nothing. They also clear the + * "redirect already issued" request attribute, since a second redirect on one request is an error. + * Both costs are constant across the states being compared.

+ */ +@State(Scope.Benchmark) +@Threads(1) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Warmup(iterations = 3, time = 1, timeUnit = TimeUnit.SECONDS) +@Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) +@Fork(value = 2, jvmArgsAppend = {"-Xms1g", "-Xmx1g", "-XX:+UseG1GC"}) +public class ControllerResponseBenchmark { + + private static final String PLAIN_CONTROLLER_SOURCE = """ + @grails.artefact.Artefact('Controller') + class BenchmarkPlainRedirectController { + def index() { + 'plain' + } + } + """; + + private static final String NAMESPACED_CONTROLLER_SOURCE = """ + @grails.artefact.Artefact('Controller') + class BenchmarkNamespacedRedirectController { + static namespace = 'admin' + def index() { + 'namespaced' + } + } + """; + + private static final String TEMPLATE_CONTROLLER_SOURCE = """ + @grails.artefact.Artefact('Controller') + class BenchmarkTemplateController { + def index() { + 'template' + } + } + """; + + private Controller plainController; + + private Controller namespacedController; + + private Controller templateController; + + private MockHttpServletRequest request; + + private CountingView countingView; + + @Setup + public void setup() throws Exception { + MockServletContext servletContext = WebContextFixture.createServletContext(); + WebApplicationContext applicationContext = WebContextFixture.applicationContext(servletContext); + + this.countingView = ControllerResponseFixture.createCountingView(); + ConfigurableListableBeanFactory beanFactory = ((ConfigurableApplicationContext) applicationContext).getBeanFactory(); + beanFactory.registerSingleton(LinkGenerator.BEAN_NAME, ControllerResponseFixture.createLinkGenerator()); + beanFactory.registerSingleton(CompositeViewResolver.BEAN_NAME, ControllerResponseFixture.createViewResolver(this.countingView)); + + // The controllers have to be registered as artefacts, exactly as an application's controllers are. A + // controller that is not in the registry is a shape only a hand-built unit test produces, and measuring it + // would measure the framework's fallback rather than a request. + GrailsApplication grailsApplication = applicationContext.getBean(GrailsApplication.APPLICATION_ID, GrailsApplication.class); + grailsApplication.setApplicationContext(applicationContext); + grailsApplication.initialise(); + + GroovyClassLoader classLoader = ControllerFixture.createTransformingClassLoader(); + this.plainController = newController(grailsApplication, classLoader, PLAIN_CONTROLLER_SOURCE, "BenchmarkPlainRedirectController"); + this.namespacedController = newController(grailsApplication, classLoader, NAMESPACED_CONTROLLER_SOURCE, "BenchmarkNamespacedRedirectController"); + this.templateController = newController(grailsApplication, classLoader, TEMPLATE_CONTROLLER_SOURCE, "BenchmarkTemplateController"); + + this.request = new MockHttpServletRequest(servletContext, "GET", "/benchmark/index"); + GrailsWebMockUtil.bindMockWebRequest(applicationContext, this.request, new MockHttpServletResponse()); + + assertFixtureRedirects(); + assertFixtureRenders(); + } + + private static Controller newController(GrailsApplication grailsApplication, GroovyClassLoader classLoader, + String source, String className) throws Exception { + classLoader.parseClass(source, className + ".groovy"); + Class controllerClass = classLoader.loadClass(className); + grailsApplication.addArtefact(ControllerArtefactHandler.TYPE, controllerClass); + if (grailsApplication.getArtefact(ControllerArtefactHandler.TYPE, controllerClass.getName()) == null) { + throw new IllegalStateException("Expected " + className + " to be retrievable from the artefact registry"); + } + return (Controller) controllerClass.getDeclaredConstructor().newInstance(); + } + + private static Map redirectArguments() { + Map arguments = new LinkedHashMap<>(4); + arguments.put("controller", "catalog"); + arguments.put("action", "show"); + return arguments; + } + + private static Map renderArguments() { + Map arguments = new LinkedHashMap<>(2); + arguments.put("template", "summary"); + return arguments; + } + + /** + * A redirect is only allowed once per request, so the flag the redirector sets is cleared before + * every measured redirect. Kept out of the benchmark methods' documentation of what they measure + * because it is the same map removal in every state being compared. + */ + private void clearRedirectIssued() { + this.request.removeAttribute(GrailsApplicationAttributes.REDIRECT_ISSUED); + } + + /** + * A redirect that resolves no link still returns normally, so setup checks the location header + * and the resolved namespace rather than publishing a number measured on a path that did nothing. + */ + private void assertFixtureRedirects() { + clearRedirectIssued(); + Map plainArguments = redirectArguments(); + this.plainController.redirect(plainArguments); + require(plainArguments.get("namespace"), null, "plain controller namespace"); + if (this.request.getAttribute(GrailsApplicationAttributes.REDIRECT_ISSUED) == null) { + throw new IllegalStateException("Expected the redirect to generate a link and mark the request as redirected"); + } + + clearRedirectIssued(); + Map namespacedArguments = redirectArguments(); + this.namespacedController.redirect(namespacedArguments); + require(namespacedArguments.get("namespace"), "admin", "namespaced controller namespace"); + + clearRedirectIssued(); + } + + private void assertFixtureRenders() { + long before = this.countingView.getRenderCount(); + this.templateController.render(renderArguments()); + if (this.countingView.getRenderCount() != before + 1) { + throw new IllegalStateException("Expected render(template:) to resolve and render the template view"); + } + } + + private static void require(Object actual, Object expected, String description) { + if (expected == null ? actual != null : !expected.equals(actual)) { + throw new IllegalStateException("Expected " + description + " to be [" + expected + "] but it was [" + actual + "]"); + } + } + + /** A redirect issued by a controller that declares no namespace - the common shape. */ + @Benchmark + public Object redirectWithoutNamespace() { + clearRedirectIssued(); + Map arguments = redirectArguments(); + this.plainController.redirect(arguments); + return arguments; + } + + /** A redirect issued by a controller that declares a namespace. */ + @Benchmark + public Object redirectWithNamespace() { + clearRedirectIssued(); + Map arguments = redirectArguments(); + this.namespacedController.redirect(arguments); + return arguments; + } + + /** {@code render(template: ...)}, which has to reach the composite view resolver. */ + @Benchmark + public long renderTemplate() { + this.templateController.render(renderArguments()); + return this.countingView.getRenderCount(); + } +} diff --git a/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/interceptors/InterceptorChainBenchmark.java b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/interceptors/InterceptorChainBenchmark.java new file mode 100644 index 00000000000..6c5e51f0d66 --- /dev/null +++ b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/interceptors/InterceptorChainBenchmark.java @@ -0,0 +1,183 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.benchmarks.interceptors; + +import java.util.List; +import java.util.concurrent.TimeUnit; + +import jakarta.servlet.ServletContext; + +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationHandler; +import io.micrometer.observation.ObservationRegistry; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.web.servlet.ModelAndView; + +import grails.artefact.Interceptor; +import org.apache.grails.benchmarks.web.WebContextFixture; +import org.grails.plugins.web.interceptors.GrailsInterceptorHandlerInterceptorAdapter; + +/** + * Measures the {@code preHandle} + {@code postHandle} pair of + * {@code GrailsInterceptorHandlerInterceptorAdapter}, which every request with at least one + * interceptor bean runs through twice - once before the handler and once after. + * + *

The dominant production configuration is a no-op {@code ObservationRegistry}: an application + * that has not enabled metrics or tracing gets {@code ObservationRegistry.NOOP}, and even one that + * has, gets a registry that is no-op until a handler is registered. The observing variants are + * measured too, but the no-op numbers are the ones that describe most applications.

+ * + *

The interceptors leave {@code before()} and {@code after()} at their trait defaults, so the + * score is the adapter's own per-interceptor per-phase cost - matcher evaluation, list bookkeeping, + * and whatever the adapter allocates to dispatch the callback - and not the cost of anybody's + * interceptor body.

+ */ +@State(Scope.Benchmark) +@Threads(1) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Warmup(iterations = 3, time = 1, timeUnit = TimeUnit.SECONDS) +@Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) +@Fork(value = 2, jvmArgsAppend = {"-Xms1g", "-Xmx1g", "-XX:+UseG1GC"}) +public class InterceptorChainBenchmark { + + private static final String MATCHED_INTERCEPTORS = "org.grails.web.MATCHED_INTERCEPTORS"; + + private MockHttpServletRequest request; + + private MockHttpServletResponse response; + + private ModelAndView modelAndView; + + private Object handler; + + private GrailsInterceptorHandlerInterceptorAdapter oneNoOp; + + private GrailsInterceptorHandlerInterceptorAdapter threeNoOp; + + private GrailsInterceptorHandlerInterceptorAdapter oneObserving; + + private GrailsInterceptorHandlerInterceptorAdapter threeObserving; + + @Setup + public void setup() { + ServletContext servletContext = WebContextFixture.createServletContext(); + request = new MockHttpServletRequest(servletContext, "GET", "/book/show/42"); + response = new MockHttpServletResponse(); + modelAndView = new ModelAndView("/book/show"); + handler = new Object(); + + oneNoOp = createAdapter(1, ObservationRegistry.NOOP); + threeNoOp = createAdapter(3, ObservationRegistry.NOOP); + oneObserving = createAdapter(1, createObservingRegistry()); + threeObserving = createAdapter(3, createObservingRegistry()); + + assertFixtureMatches(oneNoOp, 1); + assertFixtureMatches(threeNoOp, 3); + assertFixtureMatches(oneObserving, 1); + assertFixtureMatches(threeObserving, 3); + } + + private static GrailsInterceptorHandlerInterceptorAdapter createAdapter(int count, ObservationRegistry registry) { + GrailsInterceptorHandlerInterceptorAdapter adapter = new GrailsInterceptorHandlerInterceptorAdapter(); + Interceptor[] interceptors = InterceptorChainFixture.createMatchingInterceptors(count); + adapter.setInterceptors(interceptors); + adapter.setObservationRegistry(registry); + return adapter; + } + + /** + * @return a registry that is not no-op, because a handler is registered on it - the shape an + * application running Micrometer tracing or metrics has + */ + private static ObservationRegistry createObservingRegistry() { + ObservationRegistry registry = ObservationRegistry.create(); + registry.observationConfig().observationHandler(new ObservationHandler() { + @Override + public boolean supportsContext(Observation.Context context) { + return true; + } + }); + if (registry.isNoop()) { + throw new IllegalStateException("The observing registry reports itself as no-op"); + } + return registry; + } + + // An interceptor whose matcher rejects the request is skipped silently, which would leave the + // benchmark timing an empty chain under a name that claims one, three interceptors. + private void assertFixtureMatches(GrailsInterceptorHandlerInterceptorAdapter adapter, int expected) { + try { + adapter.preHandle(request, response, handler); + } + catch (Exception e) { + throw new IllegalStateException("preHandle threw during setup", e); + } + Object matched = request.getAttribute(MATCHED_INTERCEPTORS); + int size = matched instanceof List ? ((List) matched).size() : -1; + if (size != expected) { + throw new IllegalStateException("Expected " + expected + " matched interceptors but got " + size); + } + } + + /** One matched interceptor, no-op observation registry - the dominant production shape. */ + @Benchmark + public boolean oneInterceptorNoOpRegistry() throws Exception { + boolean proceed = oneNoOp.preHandle(request, response, handler); + oneNoOp.postHandle(request, response, handler, modelAndView); + return proceed; + } + + /** Three matched interceptors, no-op observation registry. */ + @Benchmark + public boolean threeInterceptorsNoOpRegistry() throws Exception { + boolean proceed = threeNoOp.preHandle(request, response, handler); + threeNoOp.postHandle(request, response, handler, modelAndView); + return proceed; + } + + /** One matched interceptor, with a registry that actually records observations. */ + @Benchmark + public boolean oneInterceptorObservingRegistry() throws Exception { + boolean proceed = oneObserving.preHandle(request, response, handler); + oneObserving.postHandle(request, response, handler, modelAndView); + return proceed; + } + + /** Three matched interceptors, with a registry that actually records observations. */ + @Benchmark + public boolean threeInterceptorsObservingRegistry() throws Exception { + boolean proceed = threeObserving.preHandle(request, response, handler); + threeObserving.postHandle(request, response, handler, modelAndView); + return proceed; + } +} diff --git a/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/urlmappings/UrlMappingsBenchmark.java b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/urlmappings/UrlMappingsBenchmark.java index 5e71aab913d..65cd14f83b5 100644 --- a/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/urlmappings/UrlMappingsBenchmark.java +++ b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/urlmappings/UrlMappingsBenchmark.java @@ -53,19 +53,24 @@ public class UrlMappingsBenchmark { private static final String WARM_URI = "/catalog/books/42"; private static final String EXPECTED_REVERSE_URL = "/catalog/books/42"; + private static final String FALL_THROUGH_URI = "/widget/show/42"; private static final int COLD_URI_COUNT = 16_384; private DefaultUrlMappingsHolder holder; private String[] coldUris; + private String[] coldFallThroughUris; private Map reverseParameters; private int coldUriIndex; + private int coldFallThroughUriIndex; @Setup public void setup() { holder = UrlMappingsFixture.createHolder(); coldUris = new String[COLD_URI_COUNT]; + coldFallThroughUris = new String[COLD_URI_COUNT]; for (int index = 0; index < COLD_URI_COUNT; index++) { coldUris[index] = "/catalog/books/" + index; + coldFallThroughUris[index] = "/widget/show/" + index; } reverseParameters = new HashMap<>(); reverseParameters.put("category", "books"); @@ -84,6 +89,14 @@ private void assertFixtureMatches() { if (!"catalog".equals(info.getControllerName()) || !"show".equals(info.getActionName())) { throw new IllegalStateException("Unexpected mapping for " + WARM_URI + ": " + info); } + // Only the catch-all mapping can serve this URI - the other three all begin with a literal + // token - so a match here is proof the fall-through benchmark reaches the end of the list. + // The matched controller name is captured from the URI rather than declared, and reading it + // needs a bound request, so the check stops at the match itself. + if (holder.match(FALL_THROUGH_URI) == null) { + throw new IllegalStateException( + "URL mappings fixture does not fall through to the catch-all for " + FALL_THROUGH_URI); + } String reverse = holder.getReverseMapping("catalog", "show", reverseParameters) .createRelativeURL("catalog", "show", reverseParameters, "UTF-8"); if (!EXPECTED_REVERSE_URL.equals(reverse)) { @@ -102,6 +115,18 @@ public UrlMappingInfo matchColdVariedKeys() { return holder.match(uri); } + /** + * The same cold-cache shape as {@link #matchColdVariedKeys()}, but for URIs that only the + * catch-all {@code "/$controller/$action?/$id?"} mapping can serve, so every earlier mapping is + * considered and rejected before the match succeeds. That fall-through, not the first-mapping + * hit, is what an application pays for any URI its explicit mappings do not cover. + */ + @Benchmark + public UrlMappingInfo matchColdCatchAllFallThrough() { + String uri = coldFallThroughUris[coldFallThroughUriIndex++ & (COLD_URI_COUNT - 1)]; + return holder.match(uri); + } + @Benchmark public String reverseMappingAndCreateRelativeUrl() { UrlCreator creator = holder.getReverseMapping("catalog", "show", reverseParameters); diff --git a/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/web/GrailsWebRequestBenchmark.java b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/web/GrailsWebRequestBenchmark.java new file mode 100644 index 00000000000..45a999e84bd --- /dev/null +++ b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/web/GrailsWebRequestBenchmark.java @@ -0,0 +1,132 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.benchmarks.web; + +import java.util.concurrent.TimeUnit; + +import jakarta.servlet.ServletContext; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.TearDown; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.web.context.request.RequestContextHolder; + +import grails.web.servlet.mvc.GrailsParameterMap; +import org.grails.web.servlet.mvc.GrailsWebRequest; + +/** + * Measures the per-request cost of binding a Grails request, which every request pays before any + * application code runs. + * + *
    + *
  • {@link #construct()} - {@code new GrailsWebRequest(request, response, servletContext)}, + * the whole per-request bind including however the application attributes are obtained.
  • + *
  • {@link #paramsOnFreshRequest()} - construction plus the first {@code getParams()}, i.e. + * what a controller action actually pays the first time it touches {@code params}.
  • + *
  • {@link #paramsCached()} - the memoised {@code getParams()} fast path.
  • + *
  • {@link #paramsRebuilt()} - {@code resetParams()} plus {@code getParams()}, which isolates + * the deep clone of the already-built {@code GrailsParameterMap}.
  • + *
+ */ +@State(Scope.Benchmark) +@Threads(1) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Warmup(iterations = 3, time = 1, timeUnit = TimeUnit.SECONDS) +@Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) +@Fork(value = 2, jvmArgsAppend = {"-Xms1g", "-Xmx1g", "-XX:+UseG1GC"}) +public class GrailsWebRequestBenchmark { + + private ServletContext servletContext; + + private MockHttpServletRequest request; + + private MockHttpServletResponse response; + + private GrailsWebRequest webRequest; + + @Setup + public void setup() { + servletContext = WebContextFixture.createServletContext(); + request = new MockHttpServletRequest(servletContext, "GET", "/book/show/42"); + request.setContextPath(""); + // A query string representative of a real controller request: a handful of flat parameters + // plus a nested one, so that GrailsParameterMap builds (and later deep clones) a nested map + // rather than a flat one. + request.addParameter("q", "groovy"); + request.addParameter("format", "json"); + request.addParameter("offset", "0"); + request.addParameter("max", "20"); + request.addParameter("sort", "dateCreated"); + request.addParameter("order", "desc"); + request.addParameter("author.name", "Rocher"); + request.addParameter("author.email", "rocher@example.com"); + response = new MockHttpServletResponse(); + + webRequest = new GrailsWebRequest(request, response, servletContext); + assertFixtureBinds(); + } + + // A missing application context makes GrailsWebRequest take an error path rather than the + // normal one, which would leave these benchmarks timing the wrong code. + private void assertFixtureBinds() { + GrailsParameterMap params = webRequest.getParams(); + if (!"groovy".equals(params.get("q"))) { + throw new IllegalStateException("Request parameters did not bind: " + params); + } + } + + @TearDown + public void tearDown() { + RequestContextHolder.resetRequestAttributes(); + } + + @Benchmark + public GrailsWebRequest construct() { + return new GrailsWebRequest(request, response, servletContext); + } + + @Benchmark + public GrailsParameterMap paramsOnFreshRequest() { + return new GrailsWebRequest(request, response, servletContext).getParams(); + } + + @Benchmark + public GrailsParameterMap paramsCached() { + return webRequest.getParams(); + } + + @Benchmark + public GrailsParameterMap paramsRebuilt() { + webRequest.resetParams(); + return webRequest.getParams(); + } +} diff --git a/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/web/MultipartResolutionBenchmark.java b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/web/MultipartResolutionBenchmark.java new file mode 100644 index 00000000000..1bcb27f7ca4 --- /dev/null +++ b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/web/MultipartResolutionBenchmark.java @@ -0,0 +1,135 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.benchmarks.web; + +import java.nio.charset.StandardCharsets; +import java.util.concurrent.TimeUnit; + +import jakarta.servlet.ServletContext; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletRequestWrapper; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockMultipartFile; +import org.springframework.mock.web.MockMultipartHttpServletRequest; +import org.springframework.web.multipart.MultipartHttpServletRequest; + +import org.grails.web.util.WebUtils; + +/** + * Measures {@code WebUtils.resolveMultipartRequest(request)}, which every + * {@code GrailsParameterMap} construction calls to discover uploaded files. + * + *

The overwhelmingly common case is a request that is not multipart, so the miss path + * matters more than the hit path. Both the flat and the wrapped shapes are measured: in a real + * application the Grails request filter sits under several servlet filters, so the request handed + * to Grails is normally a couple of {@code HttpServletRequestWrapper}s deep.

+ */ +@State(Scope.Benchmark) +@Threads(1) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Warmup(iterations = 3, time = 1, timeUnit = TimeUnit.SECONDS) +@Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) +@Fork(value = 2, jvmArgsAppend = {"-Xms1g", "-Xmx1g", "-XX:+UseG1GC"}) +public class MultipartResolutionBenchmark { + + private HttpServletRequest plain; + + private HttpServletRequest plainBehindTwoWrappers; + + private HttpServletRequest multipartBehindTwoWrappers; + + private HttpServletRequest multipartByAttribute; + + @Setup + public void setup() { + ServletContext servletContext = WebContextFixture.createServletContext(); + + MockHttpServletRequest plainRequest = new MockHttpServletRequest(servletContext, "POST", "/book/save"); + plainRequest.setContentType("application/x-www-form-urlencoded"); + plain = plainRequest; + plainBehindTwoWrappers = wrapTwice(plainRequest); + + MockMultipartHttpServletRequest multipartRequest = new MockMultipartHttpServletRequest(servletContext); + multipartRequest.setMethod("POST"); + multipartRequest.setRequestURI("/book/save"); + multipartRequest.addFile(new MockMultipartFile("cover", "cover.png", "image/png", + "not-really-a-png".getBytes(StandardCharsets.UTF_8))); + multipartBehindTwoWrappers = wrapTwice(multipartRequest); + + // Spring's StandardServletMultipartResolver publishes the resolved multipart request as a + // request attribute rather than as a wrapper, so the attribute fallback is a real path too. + MockHttpServletRequest attributeCarrier = new MockHttpServletRequest(servletContext, "POST", "/book/save"); + attributeCarrier.setAttribute(MultipartHttpServletRequest.class.getName(), multipartRequest); + multipartByAttribute = attributeCarrier; + + assertFixtureResolves(); + } + + // A resolution that silently found nothing would leave the multipart benchmarks timing the + // miss path under a name that claims a hit. + private void assertFixtureResolves() { + if (WebUtils.resolveMultipartRequest(plain) != null) { + throw new IllegalStateException("A plain request must not resolve to a multipart request"); + } + if (WebUtils.resolveMultipartRequest(multipartBehindTwoWrappers) == null) { + throw new IllegalStateException("A wrapped multipart request must resolve through the wrappers"); + } + if (WebUtils.resolveMultipartRequest(multipartByAttribute) == null) { + throw new IllegalStateException("A multipart request published as an attribute must resolve"); + } + } + + private static HttpServletRequest wrapTwice(HttpServletRequest request) { + return new HttpServletRequestWrapper(new HttpServletRequestWrapper(request)); + } + + @Benchmark + public MultipartHttpServletRequest resolvePlain() { + return WebUtils.resolveMultipartRequest(plain); + } + + @Benchmark + public MultipartHttpServletRequest resolvePlainBehindTwoWrappers() { + return WebUtils.resolveMultipartRequest(plainBehindTwoWrappers); + } + + @Benchmark + public MultipartHttpServletRequest resolveMultipartBehindTwoWrappers() { + return WebUtils.resolveMultipartRequest(multipartBehindTwoWrappers); + } + + @Benchmark + public MultipartHttpServletRequest resolveMultipartByAttribute() { + return WebUtils.resolveMultipartRequest(multipartByAttribute); + } +} diff --git a/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/web/RequestPropertyAccessBenchmark.java b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/web/RequestPropertyAccessBenchmark.java new file mode 100644 index 00000000000..ddf3e790607 --- /dev/null +++ b/grails-benchmarks/src/jmh/java/org/apache/grails/benchmarks/web/RequestPropertyAccessBenchmark.java @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.benchmarks.web; + +import java.util.concurrent.TimeUnit; + +import jakarta.servlet.ServletContext; +import jakarta.servlet.http.HttpServletRequest; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +import org.springframework.mock.web.MockHttpServletRequest; + +/** + * Measures Groovy property access on an {@code HttpServletRequest}, which application code performs + * on every request that reads a request attribute. + * + *

{@code request.someAttribute} in application code is not a field read: the property is unknown + * to the request class, so it goes through the metaclass, falls through to the {@code getProperty} + * / {@code propertyMissing} methods contributed by {@code HttpServletRequestExtension} (registered + * as a Groovy extension module by {@code grails-web-core}), and that implementation performs a + * further {@code metaClass.getMetaProperty(name)} lookup before reading the request attribute.

+ * + *

{@link #groovyAttributeCall()} and {@link #javaGetAttribute()} bracket that cost with the + * explicit {@code getAttribute} call from Groovy and from Java respectively.

+ */ +@State(Scope.Benchmark) +@Threads(1) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Warmup(iterations = 3, time = 1, timeUnit = TimeUnit.SECONDS) +@Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) +@Fork(value = 2, jvmArgsAppend = {"-Xms1g", "-Xmx1g", "-XX:+UseG1GC"}) +public class RequestPropertyAccessBenchmark { + + private HttpServletRequest request; + + private DynamicRequestPropertyReader reader; + + @Setup + public void setup() { + ServletContext servletContext = WebContextFixture.createServletContext(); + MockHttpServletRequest mockRequest = new MockHttpServletRequest(servletContext, "GET", "/book/show/42"); + mockRequest.setAttribute("someAttribute", "someValue"); + request = mockRequest; + reader = RequestPropertyFixture.createReader(); + assertFixtureResolves(); + } + + // Without the extension module on the classpath the unknown property raises rather than + // resolving, so check it here instead of publishing a number for the wrong path. + private void assertFixtureResolves() { + if (!"someValue".equals(reader.readUnknownProperty(request))) { + throw new IllegalStateException( + "request.someAttribute did not resolve through HttpServletRequestExtension"); + } + } + + /** {@code request.someAttribute} - metaclass miss, extension {@code getProperty}, attribute read. */ + @Benchmark + public Object groovyUnknownProperty() { + return reader.readUnknownProperty(request); + } + + /** {@code request.method} - metaclass hit on a real getter. */ + @Benchmark + public Object groovyGetterBackedProperty() { + return reader.readGetterBackedProperty(request); + } + + /** {@code request.getAttribute('someAttribute')} from Groovy - a dynamic call, not a property. */ + @Benchmark + public Object groovyAttributeCall() { + return reader.readAttributeDirectly(request); + } + + /** The floor: the same attribute read straight from Java. */ + @Benchmark + public Object javaGetAttribute() { + return request.getAttribute("someAttribute"); + } +} diff --git a/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/controllers/ControllerFixture.groovy b/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/controllers/ControllerFixture.groovy new file mode 100644 index 00000000000..46253d22342 --- /dev/null +++ b/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/controllers/ControllerFixture.groovy @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.benchmarks.controllers + +import org.codehaus.groovy.control.CompilationUnit + +import jakarta.servlet.ServletContext + +import org.springframework.mock.web.MockHttpServletRequest + +import grails.compiler.ast.ClassInjector +import org.grails.compiler.injection.GrailsAwareClassLoader +import org.grails.compiler.web.ControllerActionTransformer + +/** + * Compiles controller sources through the real {@code ControllerActionTransformer}, so that the + * bytecode a benchmark invokes is the bytecode a Grails application would run. + * + * This is the same arrangement {@code ControllerActionTransformerSpec} uses: a + * {@code GrailsAwareClassLoader} whose only URL-driven injector is the controller action + * transformer, with {@code shouldInject} forced on because sources compiled from a String have no + * {@code grails-app/controllers} URL to recognise. The {@code @Artefact('Controller')} annotation in + * each source still drives the normal annotation-driven injection, which is what applies the + * {@code Controller} trait. + */ +class ControllerFixture { + + static GroovyClassLoader createTransformingClassLoader() { + ControllerActionTransformer transformer = new ControllerActionTransformer() { + @Override + boolean shouldInject(URL url) { + true + } + } + transformer.compilationUnit = new CompilationUnit() + new GrailsAwareClassLoader().tap { + classInjectors = [transformer] as ClassInjector[] + } + } + + static AttributeCountingRequest createCountingRequest(ServletContext servletContext, String method, String requestUri) { + new AttributeCountingRequest(servletContext, method, requestUri) + } +} + +/** + * A mock request that counts attribute operations, used once outside the measured region to report + * how much request-attribute bookkeeping a generated controller action actually performs. + * + * This exists so a benchmark can state what it measures rather than assert it: the count printed + * during setup is the difference the generated code makes, independent of the timing numbers. + */ +class AttributeCountingRequest extends MockHttpServletRequest { + + private int getAttributeCount + + private int setAttributeCount + + private int removeAttributeCount + + AttributeCountingRequest(ServletContext servletContext, String method, String requestUri) { + super(servletContext, method, requestUri) + } + + @Override + Object getAttribute(String name) { + this.getAttributeCount++ + super.getAttribute(name) + } + + @Override + void setAttribute(String name, Object value) { + this.setAttributeCount++ + super.setAttribute(name, value) + } + + @Override + void removeAttribute(String name) { + this.removeAttributeCount++ + super.removeAttribute(name) + } + + void resetCounts() { + this.getAttributeCount = 0 + this.setAttributeCount = 0 + this.removeAttributeCount = 0 + } + + String describeCounts() { + "getAttribute=${this.getAttributeCount} setAttribute=${this.setAttributeCount} removeAttribute=${this.removeAttributeCount}" + } +} diff --git a/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/controllers/ControllerMappingsFixture.groovy b/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/controllers/ControllerMappingsFixture.groovy new file mode 100644 index 00000000000..2ccf2c127d3 --- /dev/null +++ b/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/controllers/ControllerMappingsFixture.groovy @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.benchmarks.controllers + +import org.springframework.context.ApplicationContext + +import grails.core.GrailsApplication +import grails.web.mapping.UrlMapping +import org.grails.web.mapping.DefaultUrlMappingEvaluator +import org.grails.web.mapping.DefaultUrlMappingsHolder +import org.grails.web.mapping.mvc.GrailsControllerUrlMappings + +/** + * Mapping sets for {@code ControllerMappingCollectionBenchmark}, wrapped in the + * {@code GrailsControllerUrlMappings} that {@code UrlMappingsHandlerMapping} consults per request. + */ +class ControllerMappingsFixture { + + /** + * A mapping set sized and shaped like a mid-size application's {@code UrlMappings.groovy}: a + * handful of static URLs, five REST resource blocks, some multi-token dynamic URLs, a couple of + * method-scoped mappings, the catch-all default mapping, and the error mappings. + */ + static GrailsControllerUrlMappings createApplicationMappings(GrailsApplication grailsApplication, + ApplicationContext applicationContext) { + // These MUST be double-quoted GStrings, exactly as a real UrlMappings.groovy is written. + // The DSL captures variables by letting its delegate resolve $category and friends; with + // single quotes they stay literal text, every mapping becomes a fixed path, and match() + // silently returns null so the benchmark measures failed lookups. + createMappings(grailsApplication, applicationContext) { + '/'(controller: 'application', action: 'index') + '/login'(controller: 'auth', action: 'login') + '/logout'(controller: 'auth', action: 'logout') + '/health'(controller: 'health', action: 'index') + + '/api/books'(resources: 'book') + '/api/authors'(resources: 'author') + '/api/publishers'(resources: 'publisher') + '/api/orders'(resources: 'order') + '/api/customers'(resources: 'customer') + + "/store/$category/$subcategory?"(controller: 'store', action: 'browse') + "/blog/$year/$month?/$day?/$slug?"(controller: 'blog', action: 'show') + "/report/$id/download"(controller: 'report', action: 'download') + + get '/search'(controller: 'search', action: 'index') + post '/feedback'(controller: 'feedback', action: 'save') + + "/$controller/$action?/$id?(.$format)?"() + + '500'(view: '/error') + '404'(view: '/notFound') + } + } + + /** + * A mapping set in which several patterns deliberately overlap on the same URI, so that + * {@code matchAll} returns a multi-element candidate array. The application set above produces + * one or two candidates for a typical URI; this one shows how the per-candidate work in + * {@code collectControllerMappings} scales, which a two-candidate measurement alone cannot. + */ + static GrailsControllerUrlMappings createOverlappingMappings(GrailsApplication grailsApplication, + ApplicationContext applicationContext) { + createMappings(grailsApplication, applicationContext) { + '/api/books'(resources: 'book') + + "/api/books/$id"(controller: 'book', action: 'show') + "/api/$section/$id"(controller: 'book', action: 'show') + "/$controller/$action?/$id?(.$format)?"() + } + } + + private static GrailsControllerUrlMappings createMappings(GrailsApplication grailsApplication, + ApplicationContext applicationContext, Closure mappings) { + DefaultUrlMappingEvaluator evaluator = new DefaultUrlMappingEvaluator(applicationContext) + List evaluated = evaluator.evaluateMappings(mappings) + new GrailsControllerUrlMappings(grailsApplication, new DefaultUrlMappingsHolder(evaluated)) + } +} diff --git a/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/controllers/ControllerResponseFixture.groovy b/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/controllers/ControllerResponseFixture.groovy new file mode 100644 index 00000000000..97d26671c2f --- /dev/null +++ b/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/controllers/ControllerResponseFixture.groovy @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.benchmarks.controllers + +import groovy.transform.CompileStatic + +import jakarta.servlet.http.HttpServletRequest +import jakarta.servlet.http.HttpServletResponse + +import org.springframework.web.servlet.View +import org.springframework.web.servlet.ViewResolver + +import grails.web.mapping.LinkGenerator +import org.apache.grails.benchmarks.urlmappings.UrlMappingsFixture +import org.grails.web.mapping.DefaultLinkGenerator +import org.grails.web.servlet.view.CompositeViewResolver + +/** + * Builds the collaborators the {@code redirect} and {@code render(template:)} controller paths + * resolve from the application context. + * + * The link generator is the real {@link DefaultLinkGenerator} over the shared URL mappings fixture, + * so a benchmarked redirect performs the same reverse URL creation a running application performs. + * The view resolver, by contrast, resolves to a view that renders nothing: template rendering is + * already covered by the {@code views} and {@code gsp} benchmarks, and leaving it out here makes the + * measurement as sensitive as it can be to the framework work {@code render(template:)} itself does. + */ +@CompileStatic +class ControllerResponseFixture { + + static LinkGenerator createLinkGenerator() { + DefaultLinkGenerator linkGenerator = new DefaultLinkGenerator('http://localhost:8080') + linkGenerator.urlMappingsHolder = UrlMappingsFixture.createHolder() + linkGenerator + } + + static CompositeViewResolver createViewResolver(View view) { + CompositeViewResolver viewResolver = new CompositeViewResolver() + viewResolver.viewResolvers = [new FixedViewResolver(view)] as List + viewResolver + } + + static CountingView createCountingView() { + new CountingView() + } +} + +/** + * Resolves every view name to the same view, so that view lookup contributes a constant to the + * measurement rather than a resolver-specific cost. + */ +@CompileStatic +class FixedViewResolver implements ViewResolver { + + private final View view + + FixedViewResolver(View view) { + this.view = view + } + + @Override + View resolveViewName(String viewName, Locale locale) { + this.view + } +} + +/** + * A view that writes nothing and only counts the calls, so the measured region contains the + * framework's path to the view but not the cost of producing markup. The count is returned from the + * benchmark method so the call cannot be optimised away. + */ +@CompileStatic +class CountingView implements View { + + private long renderCount + + @Override + String getContentType() { + 'text/html;charset=UTF-8' + } + + @Override + void render(Map model, HttpServletRequest request, HttpServletResponse response) { + this.renderCount++ + } + + long getRenderCount() { + this.renderCount + } +} diff --git a/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/interceptors/InterceptorChainFixture.groovy b/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/interceptors/InterceptorChainFixture.groovy new file mode 100644 index 00000000000..e421e1bc176 --- /dev/null +++ b/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/interceptors/InterceptorChainFixture.groovy @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.benchmarks.interceptors + +import grails.artefact.Interceptor + +/** + * Interceptor chains for {@code InterceptorChainBenchmark}. + * + * Three distinct classes, so that a chain of three is as polymorphic at the adapter's call sites as + * a real application's chain is. Each matches every request and leaves {@code before()} and + * {@code after()} at the trait defaults, because what is measured is the adapter's per-interceptor + * per-phase overhead, not the body of anybody's interceptor. + */ +class InterceptorChainFixture { + + static Interceptor[] createMatchingInterceptors(int count) { + List created = [new ChainAuditInterceptor(), new ChainSecurityInterceptor(), new ChainTimingInterceptor()] + if (count > created.size()) { + throw new IllegalArgumentException("The fixture only defines ${created.size()} interceptor classes, asked for ${count}") + } + created = created.take(count) + created.each { Interceptor interceptor -> interceptor.matchAll() } + created as Interceptor[] + } +} + +class ChainAuditInterceptor implements Interceptor { + +} + +class ChainSecurityInterceptor implements Interceptor { + +} + +class ChainTimingInterceptor implements Interceptor { + +} diff --git a/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/web/RequestPropertyFixture.groovy b/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/web/RequestPropertyFixture.groovy new file mode 100644 index 00000000000..e7cd4621e88 --- /dev/null +++ b/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/web/RequestPropertyFixture.groovy @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.benchmarks.web + +import jakarta.servlet.http.HttpServletRequest + +class RequestPropertyFixture { + + static DynamicRequestPropertyReader createReader() { + new DynamicRequestPropertyReader() + } +} + +/** + * Reads properties off an {@code HttpServletRequest} the way application Groovy code does. + * + * Deliberately not statically compiled: the point of the benchmark is the dynamic call + * site and the metaclass lookup behind {@code HttpServletRequestExtension}. + */ +class DynamicRequestPropertyReader { + + /** + * {@code request.someAttribute} - an unknown property, which falls through the metaclass to + * {@code HttpServletRequestExtension} and ends up as an attribute read. + */ + Object readUnknownProperty(HttpServletRequest request) { + request.someAttribute + } + + /** {@code request.method} - a property backed by a real getter on the request. */ + Object readGetterBackedProperty(HttpServletRequest request) { + request.method + } + + /** The explicit, non-dynamic equivalent of {@link #readUnknownProperty}, called from Groovy. */ + Object readAttributeDirectly(HttpServletRequest request) { + request.getAttribute('someAttribute') + } +} diff --git a/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/web/WebContextFixture.groovy b/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/web/WebContextFixture.groovy new file mode 100644 index 00000000000..175c752f712 --- /dev/null +++ b/grails-benchmarks/src/main/groovy/org/apache/grails/benchmarks/web/WebContextFixture.groovy @@ -0,0 +1,57 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.benchmarks.web + +import jakarta.servlet.ServletContext + +import org.springframework.mock.web.MockServletContext +import org.springframework.web.context.WebApplicationContext +import org.springframework.web.context.support.StaticWebApplicationContext + +import grails.core.DefaultGrailsApplication +import grails.core.GrailsApplication +import org.grails.web.util.GrailsApplicationAttributes + +/** + * Builds the servlet and application context every request-path benchmark runs against. + * + * A refreshed {@code StaticWebApplicationContext} is registered into the {@code MockServletContext} + * under the attribute Grails looks the context up by, so that + * {@code DefaultGrailsApplicationAttributes} resolves a real {@code ApplicationContext} and the + * benchmarks exercise the normal code path instead of the "no application context" error path. + */ +class WebContextFixture { + + static MockServletContext createServletContext() { + MockServletContext servletContext = new MockServletContext() + + StaticWebApplicationContext applicationContext = new StaticWebApplicationContext() + applicationContext.servletContext = servletContext + applicationContext.refresh() + applicationContext.beanFactory.registerSingleton(GrailsApplication.APPLICATION_ID, new DefaultGrailsApplication()) + + servletContext.setAttribute(GrailsApplicationAttributes.APPLICATION_CONTEXT, applicationContext) + servletContext.setAttribute(WebApplicationContext.ROOT_WEB_APPLICATION_CONTEXT_ATTRIBUTE, applicationContext) + servletContext + } + + static WebApplicationContext applicationContext(ServletContext servletContext) { + (WebApplicationContext) servletContext.getAttribute(WebApplicationContext.ROOT_WEB_APPLICATION_CONTEXT_ATTRIBUTE) + } +} diff --git a/grails-controllers/src/main/groovy/grails/artefact/Controller.groovy b/grails-controllers/src/main/groovy/grails/artefact/Controller.groovy index c82be4a50b8..2c96b5add5f 100644 --- a/grails-controllers/src/main/groovy/grails/artefact/Controller.groovy +++ b/grails-controllers/src/main/groovy/grails/artefact/Controller.groovy @@ -42,6 +42,8 @@ import org.springframework.web.servlet.ModelAndView import grails.artefact.controller.support.RequestForwarder import grails.artefact.controller.support.ResponseRedirector import grails.artefact.controller.support.ResponseRenderer +import grails.core.GrailsApplication +import grails.core.GrailsClass import grails.core.GrailsControllerClass import grails.databinding.DataBindingSource import grails.databinding.SimpleMapDataBindingSource @@ -52,6 +54,7 @@ import grails.web.api.WebAttributes import grails.web.databinding.DataBinder import grails.web.databinding.DataBindingUtils import org.grails.compiler.web.ControllerActionTransformer +import org.grails.core.artefact.ControllerArtefactHandler import org.grails.core.artefact.DomainClassArtefactHandler import org.grails.datastore.mapping.model.config.GormProperties import org.grails.plugins.web.api.MimeTypesApiSupport @@ -249,14 +252,38 @@ trait Controller implements ResponseRenderer, ResponseRedirector, RequestForward argMap.put(GrailsControllerClass.ACTION, action.toString()) } if (!argMap.containsKey(GrailsControllerClass.NAMESPACE_PROPERTY)) { - // this could be made more efficient if we had a reference to the GrailsControllerClass object, which - // has the namespace property accessible without needing reflection - argMap.put(GrailsControllerClass.NAMESPACE_PROPERTY, GrailsClassUtils.getStaticFieldValue(controller.getClass(), GrailsControllerClass.NAMESPACE_PROPERTY)) + argMap.put(GrailsControllerClass.NAMESPACE_PROPERTY, resolveNamespace(controller.getClass())) } } super.redirect(argMap) } + + /** + * Resolves the namespace declared by the given controller class. {@link GrailsControllerClass} already reads the + * static namespace property from the class when the artefact is created, so the value is taken from + * the artefact registry rather than read from the class again. + * + *

The class passed in is the class of the controller issuing the redirect, which is not necessarily + * the controller currently executing - one controller may redirect on behalf of another - so the registry is + * looked up by that class and never by the executing controller.

+ * + * @param controllerClass The class of the controller issuing the redirect + * @return The declared namespace, or null if the class declares none + */ + private Object resolveNamespace(Class controllerClass) { + GrailsApplication application = getGrailsApplication() + if (application != null) { + GrailsClass controllerArtefact = application.getArtefact(ControllerArtefactHandler.TYPE, controllerClass.getName()) + if (controllerArtefact instanceof GrailsControllerClass) { + return ((GrailsControllerClass) controllerArtefact).getNamespace() + } + } + // A controller that was never registered as an artefact - one constructed directly, as a unit test may do - + // has no GrailsControllerClass to read, so fall back to the class itself. + GrailsClassUtils.getStaticFieldValue(controllerClass, GrailsControllerClass.NAMESPACE_PROPERTY) + } + /** * Used the synchronizer token pattern to avoid duplicate form submissions * @@ -303,7 +330,7 @@ trait Controller implements ResponseRenderer, ResponseRedirector, RequestForward * @param request The servlet request */ private boolean consumeToken(GrailsWebRequest webRequest) { - final request = webRequest.getCurrentRequest() + final request = webRequest.getRequest() SynchronizerTokensHolder tokensHolderInSession = (SynchronizerTokensHolder) request.getSession(false)?.getAttribute(SynchronizerTokensHolder.HOLDER) if (!tokensHolderInSession) return false diff --git a/grails-controllers/src/main/groovy/grails/artefact/controller/support/RequestForwarder.groovy b/grails-controllers/src/main/groovy/grails/artefact/controller/support/RequestForwarder.groovy index 1dcbde36b61..5e502ce3298 100644 --- a/grails-controllers/src/main/groovy/grails/artefact/controller/support/RequestForwarder.groovy +++ b/grails-controllers/src/main/groovy/grails/artefact/controller/support/RequestForwarder.groovy @@ -110,7 +110,7 @@ trait RequestForwarder implements WebAttributes { Map model = params.model instanceof Map ? (Map) params.model : Collections.EMPTY_MAP - HttpServletRequest request = webRequest.currentRequest + HttpServletRequest request = webRequest.request HttpServletResponse response = webRequest.currentResponse for (Map.Entry entry : model.entrySet()) { diff --git a/grails-controllers/src/main/groovy/grails/artefact/controller/support/ResponseRedirector.groovy b/grails-controllers/src/main/groovy/grails/artefact/controller/support/ResponseRedirector.groovy index 2166c431e8b..9f5781d30f8 100644 --- a/grails-controllers/src/main/groovy/grails/artefact/controller/support/ResponseRedirector.groovy +++ b/grails-controllers/src/main/groovy/grails/artefact/controller/support/ResponseRedirector.groovy @@ -122,7 +122,7 @@ trait ResponseRedirector implements WebAttributes { throw new IllegalArgumentException("Invalid arguments for method 'redirect': $argMap") } - grails.web.mapping.ResponseRedirector redirector = new grails.web.mapping.ResponseRedirector(grailsLinkGenerator) + grails.web.mapping.ResponseRedirector redirector = new grails.web.mapping.ResponseRedirector(getGrailsLinkGenerator()) redirector.setRedirectListeners(redirectListeners) redirector.setRequestDataValueProcessor(requestDataValueProcessor) redirector.setUseJessionId(useJsessionId) @@ -184,7 +184,7 @@ trait ResponseRedirector implements WebAttributes { String url = creator.createURL(controller, action, namespace, plugin, params, 'utf-8') if (requestDataValueProcessor) { - HttpServletRequest request = currentWebRequest.getCurrentRequest() + HttpServletRequest request = currentWebRequest.getRequest() url = response.encodeRedirectURL(requestDataValueProcessor.processUrl(request, url)) } else { url = response.encodeRedirectURL(url) diff --git a/grails-controllers/src/main/groovy/grails/artefact/controller/support/ResponseRenderer.groovy b/grails-controllers/src/main/groovy/grails/artefact/controller/support/ResponseRenderer.groovy index 59206c1ba89..60b2c25aac2 100644 --- a/grails-controllers/src/main/groovy/grails/artefact/controller/support/ResponseRenderer.groovy +++ b/grails-controllers/src/main/groovy/grails/artefact/controller/support/ResponseRenderer.groovy @@ -180,7 +180,7 @@ trait ResponseRenderer extends WebAttributes { } else { renderMarkupInternal(webRequest, closure, response) } - setLayout(webRequest.currentRequest, false, layoutArg) + setLayout(webRequest.request, false, layoutArg) } private void renderJsonInternal(HttpServletResponse response, @DelegatesTo(value = StreamingJsonBuilder.StreamingJsonDelegate, strategy = Closure.DELEGATE_FIRST) Closure callable) { @@ -204,7 +204,7 @@ trait ResponseRenderer extends WebAttributes { applyContentType(response, argMap, body) handleStatusArgument(argMap, webRequest, response) render(body) - setLayout(webRequest.currentRequest, false, layoutArg) + setLayout(webRequest.request, false, layoutArg) } /** @@ -247,7 +247,7 @@ trait ResponseRenderer extends WebAttributes { handleStatusArgument(argMap, webRequest, response) applyContentType(response, argMap, writable) renderWritable(writable, response) - setLayout(webRequest.currentRequest, false, layoutArg) + setLayout(webRequest.request, false, layoutArg) webRequest.renderView = false } @@ -274,7 +274,7 @@ trait ResponseRenderer extends WebAttributes { CharSequence text = (textArg instanceof CharSequence) ? ((CharSequence) textArg) : textArg.toString() render(text) } - setLayout(webRequest.currentRequest, false, layoutArg) + setLayout(webRequest.request, false, layoutArg) } else if (argMap.containsKey(ARGUMENT_VIEW)) { String viewName = argMap[ARGUMENT_VIEW].toString() String viewUri = applicationAttributes.getNoSuffixViewURI((GroovyObject) this, viewName) @@ -300,7 +300,7 @@ trait ResponseRenderer extends WebAttributes { } ((GroovyObject) this).setProperty('modelAndView', new ModelAndView(viewUri, model)) - setLayout(webRequest.currentRequest, true, layoutArg) + setLayout(webRequest.request, true, layoutArg) } else if (argMap.containsKey(ARGUMENT_TEMPLATE)) { applyContentType(response, argMap, null, false) webRequest.renderView = false @@ -319,8 +319,7 @@ trait ResponseRenderer extends WebAttributes { String templateUri = applicationAttributes.getTemplateURI((GroovyObject) this, templateName, false) // retrieve view resolver - def applicationContext = applicationAttributes.getApplicationContext() - def viewResolver = applicationContext.getBean(CompositeViewResolver.BEAN_NAME, CompositeViewResolver) + CompositeViewResolver viewResolver = applicationAttributes.getCompositeViewResolver() try { View view = viewResolver.resolveView(templateUri, webRequest.locale) @@ -331,10 +330,10 @@ trait ResponseRenderer extends WebAttributes { throw new ControllerExecutionException("Unable to load template for uri [$templateUri]. Template not found.") } - boolean renderWithLayout = (layoutArg || webRequest.getCurrentRequest().getAttribute(WebUtils.LAYOUT_ATTRIBUTE)) + boolean renderWithLayout = (layoutArg || webRequest.getRequest().getAttribute(WebUtils.LAYOUT_ATTRIBUTE)) // if automatic decoration occurred unwrap, since this is a partial if (renderWithLayout) { - setLayout(webRequest.currentRequest, false, layoutArg) + setLayout(webRequest.request, false, layoutArg) } if (grailsRenderViewMutator) { @@ -615,7 +614,7 @@ trait ResponseRenderer extends WebAttributes { private void renderViewForTemplate(GrailsWebRequest webRequest, View view, Map binding) { try { - view.render(binding, webRequest.getCurrentRequest(), webRequest.getResponse()) + view.render(binding, webRequest.getRequest(), webRequest.getResponse()) } catch (Exception e) { throw new ControllerExecutionException(e.getMessage(), e) diff --git a/grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java b/grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java index ed43cec8526..553ef53fb07 100644 --- a/grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java +++ b/grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java @@ -404,11 +404,9 @@ private MethodNode convertToMethodAction(ClassNode classNode, MethodNode methodN if (methodNode.getParameters().length > 0) { final BlockStatement methodCode = new BlockStatement(); - final BlockStatement codeToHandleAllowedMethods = getCodeToHandleAllowedMethods(classNode, methodNode.getName()); final Statement codeToCallOriginalMethod = addOriginalMethodCall(methodNode, initializeActionParameters( classNode, methodNode, methodNode.getName(), parameters, source, context)); - methodCode.addStatement(codeToHandleAllowedMethods); methodCode.addStatement(codeToCallOriginalMethod); method = new MethodNode( @@ -528,48 +526,39 @@ protected void annotateActionMethod(ClassNode controllerClassNode, final Paramet protected BlockStatement getCodeToHandleAllowedMethods(ClassNode controllerClass, String methodName) { GrailsASTUtils.addEnhancedAnnotation(controllerClass, DefaultGrailsControllerClass.ALLOWED_HTTP_METHODS_PROPERTY); + + final BlockStatement code = new BlockStatement(); + + // The ALLOWED_METHODS_HANDLED request attribute records that an action of this controller has already + // begun handling the request, so that an action invoked programmatically from another action does not + // repeat the check. A controller which restricts no action at all never reads the attribute, so neither + // it nor the code which cleans it up is worth generating. + final MapExpression allowedMethodsMapExpression = getAllowedMethodsMapExpression(controllerClass); + if (allowedMethodsMapExpression == null) { + return code; + } + final BlockStatement checkAllowedMethodsBlock = new BlockStatement(); final PropertyExpression requestPropertyExpression = new PropertyExpression(new VariableExpression("this"), "request"); - final FieldNode allowedMethodsField = controllerClass.getField(DefaultGrailsControllerClass.ALLOWED_HTTP_METHODS_PROPERTY); - - if (allowedMethodsField != null) { - final Expression initialAllowedMethodsExpression = allowedMethodsField.getInitialExpression(); - if (initialAllowedMethodsExpression instanceof MapExpression) { - boolean actionIsRestricted = false; - final MapExpression allowedMethodsMapExpression = (MapExpression) initialAllowedMethodsExpression; - final List allowedMethodsMapEntryExpressions = allowedMethodsMapExpression.getMapEntryExpressions(); - for (MapEntryExpression allowedMethodsMapEntryExpression : allowedMethodsMapEntryExpressions) { - final Expression allowedMethodsMapEntryKeyExpression = allowedMethodsMapEntryExpression.getKeyExpression(); - if (allowedMethodsMapEntryKeyExpression instanceof ConstantExpression) { - final ConstantExpression allowedMethodsMapKeyConstantExpression = (ConstantExpression) allowedMethodsMapEntryKeyExpression; - final Object allowedMethodsMapKeyValue = allowedMethodsMapKeyConstantExpression.getValue(); - if (methodName.equals(allowedMethodsMapKeyValue)) { - actionIsRestricted = true; - break; - } - } - } - if (actionIsRestricted) { - final PropertyExpression responsePropertyExpression = new PropertyExpression(new VariableExpression("this"), "response"); - - final ArgumentListExpression isAllowedArgumentList = new ArgumentListExpression(); - isAllowedArgumentList.addExpression(new ConstantExpression(methodName)); - isAllowedArgumentList.addExpression(new PropertyExpression(new VariableExpression("this"), "request")); - isAllowedArgumentList.addExpression(new PropertyExpression(new VariableExpression("this"), DefaultGrailsControllerClass.ALLOWED_HTTP_METHODS_PROPERTY)); - final Expression isAllowedMethodCall = new StaticMethodCallExpression(ClassHelper.make(AllowedMethodsHelper.class), "isAllowed", isAllowedArgumentList); - final BooleanExpression isValidRequestMethod = new BooleanExpression(isAllowedMethodCall); - final MethodCallExpression sendErrorMethodCall = new MethodCallExpression(responsePropertyExpression, "sendError", new ConstantExpression(WebUtils.SC_METHOD_NOT_ALLOWED)); - final ReturnStatement returnStatement = new ReturnStatement(new ConstantExpression(null)); - final BlockStatement blockToSendError = new BlockStatement(); - blockToSendError.addStatement(new ExpressionStatement(sendErrorMethodCall)); - blockToSendError.addStatement(returnStatement); - final IfStatement ifIsValidRequestMethodStatement = new IfStatement(isValidRequestMethod, new ExpressionStatement(new EmptyExpression()), blockToSendError); - - checkAllowedMethodsBlock.addStatement(ifIsValidRequestMethodStatement); - } - } + if (isActionRestricted(allowedMethodsMapExpression, methodName)) { + final PropertyExpression responsePropertyExpression = new PropertyExpression(new VariableExpression("this"), "response"); + + final ArgumentListExpression isAllowedArgumentList = new ArgumentListExpression(); + isAllowedArgumentList.addExpression(new ConstantExpression(methodName)); + isAllowedArgumentList.addExpression(new PropertyExpression(new VariableExpression("this"), "request")); + isAllowedArgumentList.addExpression(new PropertyExpression(new VariableExpression("this"), DefaultGrailsControllerClass.ALLOWED_HTTP_METHODS_PROPERTY)); + final Expression isAllowedMethodCall = new StaticMethodCallExpression(ClassHelper.make(AllowedMethodsHelper.class), "isAllowed", isAllowedArgumentList); + final BooleanExpression isValidRequestMethod = new BooleanExpression(isAllowedMethodCall); + final MethodCallExpression sendErrorMethodCall = new MethodCallExpression(responsePropertyExpression, "sendError", new ConstantExpression(WebUtils.SC_METHOD_NOT_ALLOWED)); + final ReturnStatement returnStatement = new ReturnStatement(new ConstantExpression(null)); + final BlockStatement blockToSendError = new BlockStatement(); + blockToSendError.addStatement(new ExpressionStatement(sendErrorMethodCall)); + blockToSendError.addStatement(returnStatement); + final IfStatement ifIsValidRequestMethodStatement = new IfStatement(isValidRequestMethod, new ExpressionStatement(new EmptyExpression()), blockToSendError); + + checkAllowedMethodsBlock.addStatement(ifIsValidRequestMethodStatement); } final ArgumentListExpression argumentListExpression = new ArgumentListExpression(); @@ -585,12 +574,48 @@ protected BlockStatement getCodeToHandleAllowedMethods(ClassNode controllerClass final BooleanExpression attributeIsSetBooleanExpression = new BooleanExpression(new MethodCallExpression(requestPropertyExpression, "getAttribute", new ArgumentListExpression(new ConstantExpression(ALLOWED_METHODS_HANDLED_ATTRIBUTE_NAME)))); final Statement ifAttributeIsAlreadySetStatement = new IfStatement(attributeIsSetBooleanExpression, new EmptyStatement(), codeToExecuteIfAttributeIsNotSet); - final BlockStatement code = new BlockStatement(); code.addStatement(ifAttributeIsAlreadySetStatement); return code; } + /** + * Retrieves the map literal assigned to the controller's allowedMethods property. + * + * @param controllerClass the controller class + * @return the map literal, or null if the controller does not declare a non empty map literal + */ + private MapExpression getAllowedMethodsMapExpression(final ClassNode controllerClass) { + final FieldNode allowedMethodsField = controllerClass.getField(DefaultGrailsControllerClass.ALLOWED_HTTP_METHODS_PROPERTY); + if (allowedMethodsField == null) { + return null; + } + final Expression initialAllowedMethodsExpression = allowedMethodsField.getInitialExpression(); + if (!(initialAllowedMethodsExpression instanceof MapExpression)) { + return null; + } + final MapExpression allowedMethodsMapExpression = (MapExpression) initialAllowedMethodsExpression; + return allowedMethodsMapExpression.getMapEntryExpressions().isEmpty() ? null : allowedMethodsMapExpression; + } + + /** + * @param allowedMethodsMapExpression the map literal assigned to the controller's allowedMethods property + * @param methodName the name of an action + * @return true if the action is a key in the allowedMethods map and is therefore restricted to specific request methods + */ + private boolean isActionRestricted(final MapExpression allowedMethodsMapExpression, final String methodName) { + for (MapEntryExpression allowedMethodsMapEntryExpression : allowedMethodsMapExpression.getMapEntryExpressions()) { + final Expression allowedMethodsMapEntryKeyExpression = allowedMethodsMapEntryExpression.getKeyExpression(); + if (allowedMethodsMapEntryKeyExpression instanceof ConstantExpression) { + final ConstantExpression allowedMethodsMapKeyConstantExpression = (ConstantExpression) allowedMethodsMapEntryKeyExpression; + if (methodName.equals(allowedMethodsMapKeyConstantExpression.getValue())) { + return true; + } + } + } + return false; + } + /** * This will wrap the method body in a try catch block which does something * like this: @@ -637,14 +662,23 @@ protected void wrapMethodBodyWithExceptionHandling(final ClassNode controllerCla final CatchStatement catchStatement = new CatchStatement(new Parameter(new ClassNode(Exception.class), caughtExceptionArgumentName), catchBlockCode); final Statement methodBody = methodNode.getCode(); + final BlockStatement codeToHandleAllowedMethods = getCodeToHandleAllowedMethods(controllerClassNode, methodNode.getName()); + BlockStatement tryBlock = new BlockStatement(); - BlockStatement codeToHandleAllowedMethods = getCodeToHandleAllowedMethods(controllerClassNode, methodNode.getName()); - tryBlock.addStatement(codeToHandleAllowedMethods); + if (!codeToHandleAllowedMethods.isEmpty()) { + tryBlock.addStatement(codeToHandleAllowedMethods); + } tryBlock.addStatement(methodBody); final TryCatchStatement tryCatchStatement = new TryCatchStatement(tryBlock, new EmptyStatement()); tryCatchStatement.addCatch(catchStatement); + if (codeToHandleAllowedMethods.isEmpty()) { + // Nothing wrote the ALLOWED_METHODS_HANDLED request attribute, so there is nothing to clean up. + methodNode.setCode(tryCatchStatement); + return; + } + final ArgumentListExpression argumentListExpression = new ArgumentListExpression(); argumentListExpression.addExpression(new ConstantExpression(ALLOWED_METHODS_HANDLED_ATTRIBUTE_NAME)); diff --git a/grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/api/ControllersDomainBindingApi.java b/grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/api/ControllersDomainBindingApi.java index 9aad4693e15..aa884611eb5 100644 --- a/grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/api/ControllersDomainBindingApi.java +++ b/grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/api/ControllersDomainBindingApi.java @@ -47,7 +47,8 @@ public class ControllersDomainBindingApi { * @param instance The target instance */ public static void initialize(Object instance) { - autowire(instance); + GrailsApplication application = findApplication(); + autowire(instance, getDomainClass(instance, application)); } /** @@ -57,7 +58,8 @@ public static void initialize(Object instance) { * @param namedArgs The named arguments */ public static void initialize(Object instance, Map namedArgs) { - PersistentEntity dc = getDomainClass(instance); + GrailsApplication application = findApplication(); + PersistentEntity dc = getDomainClass(instance, application); if (dc == null) { DataBindingUtils.bindObjectToInstance(instance, namedArgs); } @@ -65,49 +67,44 @@ public static void initialize(Object instance, Map namedArgs) { DataBindingUtils.bindObjectToDomainInstance(dc, instance, namedArgs); DataBindingUtils.assignBidirectionalAssociations(instance, namedArgs, dc); } - autowire(instance); + autowire(instance, dc); } - private static PersistentEntity getDomainClass(Object instance) { - PersistentEntity domainClass = null; - if (!Environment.isInitializing()) { - final GrailsApplication grailsApplication = Holders.findApplication(); - if (grailsApplication != null) { - try { - domainClass = grailsApplication.getMappingContext().getPersistentEntity(instance.getClass().getName()); - } catch (GrailsConfigurationException e) { - //no-op - } - } - } - - return domainClass; + /** + * @return The current application, or null while the environment is still initializing or before an + * application has been bound + */ + private static GrailsApplication findApplication() { + return Environment.isInitializing() ? null : Holders.findApplication(); } - private static void autowire(Object instance) { - if (!Environment.isInitializing()) { - - GrailsApplication application = Holders.findApplication(); - if (application != null) { - - try { - PersistentEntity domainClass = application.getMappingContext().getPersistentEntity(instance.getClass().getName()); - if (domainClass != null) { + private static PersistentEntity getDomainClass(Object instance, GrailsApplication application) { + if (application == null) { + return null; + } + try { + return application.getMappingContext().getPersistentEntity(instance.getClass().getName()); + } catch (GrailsConfigurationException e) { + // ignore, Mapping Context not initialized yet + return null; + } + } - if (domainClass.getMapping().getMappedForm().isAutowire()) { - final ApplicationContext applicationContext = Holders.findApplicationContext(); - if (applicationContext != null) { - applicationContext - .getAutowireCapableBeanFactory() - .autowireBeanProperties(instance, AutowireCapableBeanFactory.AUTOWIRE_BY_NAME, false); - } - } - } - } catch (GrailsConfigurationException e) { - // ignore, Mapping Context not initialized yet + private static void autowire(Object instance, PersistentEntity domainClass) { + if (domainClass == null) { + return; + } + try { + if (domainClass.getMapping().getMappedForm().isAutowire()) { + final ApplicationContext applicationContext = Holders.findApplicationContext(); + if (applicationContext != null) { + applicationContext + .getAutowireCapableBeanFactory() + .autowireBeanProperties(instance, AutowireCapableBeanFactory.AUTOWIRE_BY_NAME, false); } - } + } catch (GrailsConfigurationException e) { + // ignore, Mapping Context not initialized yet } } } diff --git a/grails-controllers/src/test/groovy/grails/artefact/ControllerRedirectSpec.groovy b/grails-controllers/src/test/groovy/grails/artefact/ControllerRedirectSpec.groovy new file mode 100644 index 00000000000..073518e14fa --- /dev/null +++ b/grails-controllers/src/test/groovy/grails/artefact/ControllerRedirectSpec.groovy @@ -0,0 +1,230 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package grails.artefact + +import org.springframework.mock.web.MockHttpServletRequest +import org.springframework.mock.web.MockHttpServletResponse +import org.springframework.mock.web.MockServletContext +import org.springframework.web.context.WebApplicationContext +import org.springframework.web.context.request.RequestContextHolder +import spock.lang.Specification + +import grails.core.DefaultGrailsApplication +import grails.core.GrailsApplication +import grails.core.GrailsControllerClass +import grails.util.GrailsWebMockUtil +import grails.web.mapping.LinkGenerator +import grails.web.mapping.mvc.RedirectEventListener +import org.grails.core.artefact.ControllerArtefactHandler +import org.grails.web.servlet.mvc.ParameterCreationListener +import org.grails.web.util.GrailsApplicationAttributes + +class ControllerRedirectSpec extends Specification { + + MockServletContext servletContext = new MockServletContext() + + List linkArguments = [] + + LinkGenerator linkGenerator = Stub(LinkGenerator) { + getServerBaseURL() >> 'http://localhost:8080' + link(_) >> { Map arguments -> + linkArguments << arguments + "/${arguments.action}".toString() + } + } + + WebApplicationContext applicationContext = Mock(WebApplicationContext) + + DefaultGrailsApplication grailsApplication = new DefaultGrailsApplication(PlainRedirectController, NamespacedRedirectController) + + void setup() { + grailsApplication.initialise() + applicationContext.getBean(LinkGenerator) >> linkGenerator + applicationContext.getBeansOfType(ParameterCreationListener) >> [:] + applicationContext.containsBean(GrailsApplication.APPLICATION_ID) >> true + applicationContext.getBean(GrailsApplication.APPLICATION_ID) >> grailsApplication + servletContext.setAttribute(GrailsApplicationAttributes.APPLICATION_CONTEXT, applicationContext) + } + + void cleanup() { + RequestContextHolder.setRequestAttributes(null) + } + + private MockHttpServletRequest bindRequest() { + MockHttpServletRequest request = new MockHttpServletRequest(servletContext) + GrailsWebMockUtil.bindMockWebRequest(applicationContext, request, new MockHttpServletResponse()) + request + } + + private MockHttpServletResponse currentResponse() { + RequestContextHolder.currentRequestAttributes().currentResponse as MockHttpServletResponse + } + + void 'redirect resolves the namespace declared by the controller class issuing it'() { + given: 'a controller without a namespace and one declaring a namespace' + def plain = new PlainRedirectController() + def namespaced = new NamespacedRedirectController() + + when: 'each controller redirects' + bindRequest() + plain.redirectToIndex() + bindRequest() + namespaced.redirectToIndex() + + then: 'each redirect carries the namespace of its own class' + linkArguments[0].namespace == null + linkArguments[1].namespace == 'admin' + + when: 'the same two controller classes redirect again' + bindRequest() + namespaced.redirectToIndex() + bindRequest() + plain.redirectToIndex() + bindRequest() + new NamespacedRedirectController().redirectToIndex() + + then: 'the value is still the one declared by each class, never shared between them' + linkArguments[2].namespace == 'admin' + linkArguments[3].namespace == null + linkArguments[4].namespace == 'admin' + } + + void 'the namespace is taken from the artefact registered for the controller issuing the redirect'() { + given: 'the namespace the artefact reports for each controller class' + GrailsControllerClass plainArtefact = grailsApplication.getArtefact(ControllerArtefactHandler.TYPE, + PlainRedirectController.name) as GrailsControllerClass + GrailsControllerClass namespacedArtefact = grailsApplication.getArtefact(ControllerArtefactHandler.TYPE, + NamespacedRedirectController.name) as GrailsControllerClass + + expect: 'the registry is what declares them' + plainArtefact.namespace == null + namespacedArtefact.namespace == 'admin' + + when: 'a namespaced controller redirects while a different controller is the one executing' + MockHttpServletRequest request = bindRequest() + request.setAttribute(GrailsApplicationAttributes.GRAILS_CONTROLLER_CLASS, plainArtefact) + request.setAttribute(GrailsApplicationAttributes.CONTROLLER_NAME_ATTRIBUTE, 'plainRedirect') + new NamespacedRedirectController().redirectToIndex() + + then: 'the namespace is the one declared by the redirecting controller, not the executing one' + linkArguments[0].namespace == 'admin' + } + + void 'a controller that is not a registered artefact still resolves its declared namespace'() { + given: 'a controller class the application knows nothing about' + expect: + grailsApplication.getArtefact(ControllerArtefactHandler.TYPE, UnregisteredRedirectController.name) == null + + when: + bindRequest() + new UnregisteredRedirectController().redirectToIndex() + + then: 'the namespace declared on the class is used' + linkArguments[0].namespace == 'reporting' + } + + void 'an explicit namespace argument is never overwritten by the declared one'() { + given: + def namespaced = new NamespacedRedirectController() + + when: + bindRequest() + namespaced.redirectToIndexInNamespace('reporting') + + then: + linkArguments[0].namespace == 'reporting' + currentResponse().redirectedUrl == 'http://localhost:8080/index' + } + + void 'a link generator set after the first redirect replaces the one already in use'() { + given: + def controller = new PlainRedirectController() + List replacementArguments = [] + LinkGenerator replacement = Stub(LinkGenerator) { + getServerBaseURL() >> 'http://replacement:9090' + link(_) >> { Map arguments -> + replacementArguments << arguments + '/replaced' + } + } + + when: 'the controller redirects once, then is given a different link generator' + bindRequest() + controller.redirectToIndex() + controller.setGrailsLinkGenerator(replacement) + bindRequest() + controller.redirectToIndex() + + then: 'the second redirect is generated by the replacement' + linkArguments.size() == 1 + replacementArguments.size() == 1 + currentResponse().redirectedUrl == 'http://replacement:9090/replaced' + } + + void 'redirect listeners registered after the first redirect are notified'() { + given: + def controller = new PlainRedirectController() + List notified = [] + RedirectEventListener listener = { String url -> notified << url } as RedirectEventListener + + when: 'the controller redirects once before any listener is registered' + bindRequest() + controller.redirectToIndex() + + then: + notified.isEmpty() + + when: 'a listener is registered and a further redirect is issued' + controller.setRedirectListeners([listener]) + bindRequest() + controller.redirectToIndex() + + then: + notified == ['http://localhost:8080/index'] + } +} + +class PlainRedirectController implements Controller { + + void redirectToIndex() { + redirect(action: 'index') + } +} + +class NamespacedRedirectController implements Controller { + + static namespace = 'admin' + + void redirectToIndex() { + redirect(action: 'index') + } + + void redirectToIndexInNamespace(String explicitNamespace) { + redirect(action: 'index', namespace: explicitNamespace) + } +} + +class UnregisteredRedirectController implements Controller { + + static namespace = 'reporting' + + void redirectToIndex() { + redirect(action: 'index') + } +} diff --git a/grails-controllers/src/test/groovy/grails/artefact/controller/support/ResponseRendererSpec.groovy b/grails-controllers/src/test/groovy/grails/artefact/controller/support/ResponseRendererSpec.groovy new file mode 100644 index 00000000000..1a46eb282c2 --- /dev/null +++ b/grails-controllers/src/test/groovy/grails/artefact/controller/support/ResponseRendererSpec.groovy @@ -0,0 +1,107 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package grails.artefact.controller.support + +import org.springframework.mock.web.MockHttpServletRequest +import org.springframework.mock.web.MockHttpServletResponse +import org.springframework.mock.web.MockServletContext +import org.springframework.web.context.WebApplicationContext +import org.springframework.web.context.request.RequestContextHolder +import org.springframework.web.servlet.View +import spock.lang.Specification + +import grails.util.GrailsWebMockUtil +import org.grails.web.servlet.mvc.ParameterCreationListener +import org.grails.web.servlet.view.CompositeViewResolver +import org.grails.web.util.GrailsApplicationAttributes + +class ResponseRendererSpec extends Specification { + + MockServletContext servletContext = new MockServletContext() + WebApplicationContext applicationContext = Mock(WebApplicationContext) + + void setup() { + applicationContext.getBeansOfType(ParameterCreationListener) >> [:] + servletContext.setAttribute(GrailsApplicationAttributes.APPLICATION_CONTEXT, applicationContext) + } + + void cleanup() { + RequestContextHolder.setRequestAttributes(null) + } + + private void bindRequest() { + GrailsWebMockUtil.bindMockWebRequest(applicationContext, new MockHttpServletRequest(servletContext), + new MockHttpServletResponse()) + } + + void 'the composite view resolver is looked up once and reused for every template rendered'() { + given: + def view = Mock(View) + def viewResolver = Mock(CompositeViewResolver) + def controller = new TemplateRenderingController() + + when: 'the same controller renders two templates' + bindRequest() + controller.renderTemplate('first') + bindRequest() + controller.renderTemplate('second') + + then: 'the view resolver bean is only resolved from the context once' + 1 * applicationContext.getBean(CompositeViewResolver.BEAN_NAME, CompositeViewResolver) >> viewResolver + + and: 'but each template is still resolved and rendered' + 1 * viewResolver.resolveView('/templateRendering/_first', _) >> view + 1 * viewResolver.resolveView('/templateRendering/_second', _) >> view + 2 * view.render(_, _, _) + } + + void 'different controllers in the same servlet context share one view resolver lookup'() { + given: + def view = Mock(View) + def viewResolver = Mock(CompositeViewResolver) + + when: 'two different controller classes each render a template' + bindRequest() + new TemplateRenderingController().renderTemplate('first') + bindRequest() + new OtherTemplateRenderingController().renderTemplate('second') + + then: 'the bean is still resolved only once, because the lookup is not per controller' + 1 * applicationContext.getBean(CompositeViewResolver.BEAN_NAME, CompositeViewResolver) >> viewResolver + + and: 'each controller resolves its own template uri' + 1 * viewResolver.resolveView('/templateRendering/_first', _) >> view + 1 * viewResolver.resolveView('/otherTemplateRendering/_second', _) >> view + 2 * view.render(_, _, _) + } +} + +class TemplateRenderingController implements ResponseRenderer { + + void renderTemplate(String templateName) { + render(template: templateName) + } +} + +class OtherTemplateRenderingController implements ResponseRenderer { + + void renderTemplate(String templateName) { + render(template: templateName) + } +} diff --git a/grails-controllers/src/test/groovy/org/grails/plugins/web/controllers/api/ControllersDomainBindingApiSpec.groovy b/grails-controllers/src/test/groovy/org/grails/plugins/web/controllers/api/ControllersDomainBindingApiSpec.groovy new file mode 100644 index 00000000000..e70b095dfff --- /dev/null +++ b/grails-controllers/src/test/groovy/org/grails/plugins/web/controllers/api/ControllersDomainBindingApiSpec.groovy @@ -0,0 +1,147 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.plugins.web.controllers.api + +import org.springframework.beans.factory.config.AutowireCapableBeanFactory +import org.springframework.context.ApplicationContext +import spock.lang.Specification + +import grails.core.DefaultGrailsApplication +import grails.core.GrailsApplication +import grails.util.Holders +import org.grails.core.support.GrailsApplicationDiscoveryStrategy +import org.grails.datastore.mapping.keyvalue.mapping.config.KeyValueMappingContext + +class ControllersDomainBindingApiSpec extends Specification { + + KeyValueMappingContext mappingContext = new KeyValueMappingContext('test') + + AutowireCapableBeanFactory beanFactory = Mock(AutowireCapableBeanFactory) + + ApplicationContext applicationContext = Stub(ApplicationContext) { + getAutowireCapableBeanFactory() >> beanFactory + } + + GrailsApplication grailsApplication = new DefaultGrailsApplication() + + void setup() { + // Discovery strategies are static and consulted in registration order, and tests share a fork, so a + // strategy left behind by an earlier test - pointing at an application context since closed - would be + // asked first and throw before the one registered below is ever reached. + Holders.clear() + mappingContext.addPersistentEntity(Widget) + grailsApplication.mappingContext = mappingContext + Holders.addApplicationDiscoveryStrategy(new GrailsApplicationDiscoveryStrategy() { + + @Override + GrailsApplication findGrailsApplication() { + grailsApplication + } + + @Override + ApplicationContext findApplicationContext() { + applicationContext + } + }) + } + + void cleanup() { + Holders.clear() + } + + private void setAutowire(boolean autowire) { + mappingContext.getPersistentEntity(Widget.name).mapping.mappedForm.autowire = autowire + } + + void 'a map constructor binds the named arguments and autowires the instance when its mapping asks for it'() { + given: + setAutowire(true) + def widget = new Widget() + + when: + ControllersDomainBindingApi.initialize(widget, [name: 'spanner']) + + then: + widget.name == 'spanner' + 1 * beanFactory.autowireBeanProperties(widget, AutowireCapableBeanFactory.AUTOWIRE_BY_NAME, false) + } + + void 'a map constructor binds the named arguments without autowiring when the mapping does not ask for it'() { + given: + setAutowire(false) + def widget = new Widget() + + when: + ControllersDomainBindingApi.initialize(widget, [name: 'spanner']) + + then: + widget.name == 'spanner' + 0 * beanFactory.autowireBeanProperties(_, _, _) + } + + void 'an instance of a class that is not a persistent entity is bound but never autowired'() { + given: + setAutowire(true) + def gadget = new Gadget() + + when: + ControllersDomainBindingApi.initialize(gadget, [name: 'spanner']) + + then: + gadget.name == 'spanner' + 0 * beanFactory.autowireBeanProperties(_, _, _) + } + + void 'the no argument initializer autowires the instance when its mapping asks for it'() { + given: + setAutowire(true) + def widget = new Widget() + + when: + ControllersDomainBindingApi.initialize(widget) + + then: + 1 * beanFactory.autowireBeanProperties(widget, AutowireCapableBeanFactory.AUTOWIRE_BY_NAME, false) + } + + void 'named arguments are still bound when no application has been bound yet'() { + given: + Holders.clear() + def widget = new Widget() + + when: + ControllersDomainBindingApi.initialize(widget, [name: 'spanner']) + + then: + widget.name == 'spanner' + 0 * beanFactory.autowireBeanProperties(_, _, _) + } +} + +class Widget { + + Long id + Long version + String name +} + +class Gadget { + + String name +} diff --git a/grails-databinding/src/main/groovy/org/grails/databinding/converters/web/LocaleAwareNumberConverter.groovy b/grails-databinding/src/main/groovy/org/grails/databinding/converters/web/LocaleAwareNumberConverter.groovy index f41494f2305..9bf7b47467e 100644 --- a/grails-databinding/src/main/groovy/org/grails/databinding/converters/web/LocaleAwareNumberConverter.groovy +++ b/grails-databinding/src/main/groovy/org/grails/databinding/converters/web/LocaleAwareNumberConverter.groovy @@ -83,7 +83,7 @@ class LocaleAwareNumberConverter implements ValueConverter { protected Locale getLocale() { def locale - def request = GrailsWebRequest.lookup()?.currentRequest + def request = GrailsWebRequest.lookup()?.request if (request instanceof HttpServletRequest) { locale = localeResolver?.resolveLocale(request) } diff --git a/grails-doc/src/en/guide/theWebLayer/controllers/uploadingFiles.adoc b/grails-doc/src/en/guide/theWebLayer/controllers/uploadingFiles.adoc index 606232098e7..ab4d786b1f0 100644 --- a/grails-doc/src/en/guide/theWebLayer/controllers/uploadingFiles.adoc +++ b/grails-doc/src/en/guide/theWebLayer/controllers/uploadingFiles.adoc @@ -21,7 +21,7 @@ under the License. ==== Programmatic File Uploads -Grails supports file uploads using Spring's {springapi}org/springframework/web/multipart/MultipartHttpServletRequest.html[MultipartHttpServletRequest] interface. The first step for file uploading is to create a multipart form like this: +Grails supports file uploads on top of Spring's multipart handling, exposing the uploaded parts as {springapi}org/springframework/web/multipart/MultipartFile.html[MultipartFile] instances. The first step for file uploading is to create a multipart form like this: [source,xml] ---- diff --git a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc index bd34bb094ab..bfdcd8807d2 100644 --- a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc +++ b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc @@ -2357,3 +2357,114 @@ resolved unambiguously before can become ambiguous. Removing the packages from `grails.spring.bean.packages`, or the stray classes from those packages, restores the previous set of beans. + +==== 45. `request` Is No Longer a `MultipartHttpServletRequest` + +The `request` object Grails exposes to controllers, tag libraries and GSPs is now always the outermost +servlet request. Previously, for a file upload, Grails replaced it with the resolved +`MultipartHttpServletRequest`, which discarded the request wrappers contributed by other filters — the +hidden HTTP method filter, Spring Security, and any application filter. + +All file upload methods work exactly as before: + +[source,groovy] +---- +def upload() { + def file = request.getFile('myFile') + if (file.empty) { + flash.message = 'file cannot be empty' + render(view: 'uploadForm') + return + } + file.transferTo(new File('/some/local/dir/myfile.txt')) +} +---- + +`request.getFiles(name)`, `request.getFileNames()`, `request.getFileMap()`, `request.getMultiFileMap()` +and `request.getMultipartContentType(name)` are likewise unchanged, as is binding a file through +`params`: + +[source,groovy] +---- +def img = new Image(params) +---- + +What no longer works is treating `request` as a `MultipartHttpServletRequest` by type: + +[source,groovy] +---- +// Before +if (request instanceof MultipartHttpServletRequest) { + def file = ((MultipartHttpServletRequest) request).getFile('myFile') +} + +// After +def file = request.getFile('myFile') +---- + +Calling one of the file methods on a request that is not a file upload throws `IllegalStateException` +with a diagnostic message, the same way it previously failed with `MissingMethodException`. + +==== 46. Request Processing Behaviour Changes + +Four changes fall out of Grails 8 delegating more of the request path to Spring. + +===== 46.1 API versioning headers are now emitted for Grails-mapped requests + +`UrlMappingsHandlerMapping` previously assembled its own handler execution chain rather than using +Spring's. As a result, Grails-mapped requests skipped the interceptor Spring adds for API versioning, +so the `Deprecation`, `Sunset` and `Link` headers configured through `spring.mvc.apiversion.*` were +never sent. The chain is now assembled by Spring, and those headers are emitted as configured. + +If an application relied on those headers being absent, unset the corresponding +`spring.mvc.apiversion.*` configuration. + +===== 46.2 The `LocaleContext` is restored rather than cleared + +At the end of a request Grails previously called `LocaleContextHolder.setLocale(null)`, which discarded +any `LocaleContext` established earlier in the filter chain by non-Grails code. Grails now saves and +restores the previous `LocaleContext`, matching Spring's own `RequestContextFilter`. + +===== 46.3 The application attributes object is shared per servlet context + +`GrailsApplicationAttributes` was constructed once per request. It is now created once per servlet +context and reused, so the beans it caches are resolved once rather than on every request. It is +rebuilt automatically if the `ApplicationContext` is replaced. + +Applications that implemented `GrailsApplicationAttributes` themselves must ensure their +implementation holds no request-scoped state and is safe to use from multiple request threads. + +===== 46.4 `GrailsWebRequest.getCurrentRequest()` is deprecated + +`getCurrentRequest()` existed to hand back the resolved multipart request in place of the request Grails +was bound to. That substitution is gone (see _45. `request` Is No Longer a `MultipartHttpServletRequest`_), +so the method now returns exactly what `getRequest()` returns. It is deprecated; call `getRequest()` +instead. + +[source,groovy] +---- +// Before +def uri = webRequest.currentRequest.requestURI + +// After +def uri = webRequest.request.requestURI +---- + +Nothing about the value returned has changed, so this is a rename, not a behaviour change. Code that +reaches for the uploaded files of a multipart request should use the `request.getFile(..)` methods +described in section 45. + +Tests that mock a `GrailsWebRequest` and stub `getCurrentRequest()` need updating. `getRequest()` is +`final` on Spring's `ServletRequestAttributes`, so `getCurrentRequest()` was the only stubbable request +accessor, and framework code now calls `getRequest()` directly. Drive a real `GrailsWebRequest` over a +`MockHttpServletRequest` rather than stubbing the accessor: + +[source,groovy] +---- +// Before +def webRequest = Mock(GrailsWebRequest) +webRequest.getCurrentRequest() >> request + +// After +def webRequest = new GrailsWebRequest(request, response, servletContext) +---- diff --git a/grails-fields/grails-app/taglib/grails/plugin/formfields/FormFieldsTagLib.groovy b/grails-fields/grails-app/taglib/grails/plugin/formfields/FormFieldsTagLib.groovy index 7eafc020c64..2c7633d0b47 100644 --- a/grails-fields/grails-app/taglib/grails/plugin/formfields/FormFieldsTagLib.groovy +++ b/grails-fields/grails-app/taglib/grails/plugin/formfields/FormFieldsTagLib.groovy @@ -856,7 +856,7 @@ class FormFieldsTagLib { @CompileStatic private Locale getLocale() { def locale - def request = GrailsWebRequest.lookup()?.currentRequest + def request = GrailsWebRequest.lookup()?.request if (request instanceof HttpServletRequest) { locale = localeResolver?.resolveLocale(request) } diff --git a/grails-gsp/grails-web-gsp/src/main/groovy/org/grails/web/gsp/io/GrailsConventionGroovyPageLocator.java b/grails-gsp/grails-web-gsp/src/main/groovy/org/grails/web/gsp/io/GrailsConventionGroovyPageLocator.java index b1bf943be23..477d5c72e55 100644 --- a/grails-gsp/grails-web-gsp/src/main/groovy/org/grails/web/gsp/io/GrailsConventionGroovyPageLocator.java +++ b/grails-gsp/grails-web-gsp/src/main/groovy/org/grails/web/gsp/io/GrailsConventionGroovyPageLocator.java @@ -334,7 +334,7 @@ protected String lookupRequestFormat() { GrailsWebRequest.lookup(); if (webRequest != null) { - HttpServletRequest request = webRequest.getCurrentRequest(); + HttpServletRequest request = webRequest.getRequest(); Object format = request.getAttribute(GrailsApplicationAttributes.RESPONSE_FORMAT); return format == null ? null : format.toString(); } diff --git a/grails-gsp/grails-web-gsp/src/main/groovy/org/grails/web/pages/GSPResponseWriter.java b/grails-gsp/grails-web-gsp/src/main/groovy/org/grails/web/pages/GSPResponseWriter.java index 5b9ca8e1540..e2999834e0d 100644 --- a/grails-gsp/grails-web-gsp/src/main/groovy/org/grails/web/pages/GSPResponseWriter.java +++ b/grails-gsp/grails-web-gsp/src/main/groovy/org/grails/web/pages/GSPResponseWriter.java @@ -217,7 +217,7 @@ public void close() { flushResponse(); } else if (!isTrouble()) { GrailsWebRequest webRequest = GrailsWebRequest.lookup(); - if (webRequest != null && webRequest.getCurrentRequest().getAttribute(WebUtils.SITEMESH2_PAGE_ATTRIBUTE) != null) { + if (webRequest != null && webRequest.getRequest().getAttribute(WebUtils.SITEMESH2_PAGE_ATTRIBUTE) != null) { // flush the response if its a layout flushResponse(); } diff --git a/grails-gsp/grails-web-gsp/src/main/groovy/org/grails/web/servlet/view/GroovyPageViewResolver.java b/grails-gsp/grails-web-gsp/src/main/groovy/org/grails/web/servlet/view/GroovyPageViewResolver.java index a97352c765e..c7a4ce5027e 100644 --- a/grails-gsp/grails-web-gsp/src/main/groovy/org/grails/web/servlet/view/GroovyPageViewResolver.java +++ b/grails-gsp/grails-web-gsp/src/main/groovy/org/grails/web/servlet/view/GroovyPageViewResolver.java @@ -152,7 +152,7 @@ protected String resolveCurrentControllerKeyPrefixes(boolean absolute) { StringBuilder stringBuilder = new StringBuilder(); namespace = webRequest.getControllerNamespace(); controller = webRequest.getControllerName(); - pluginContextPath = (webRequest.getAttributes() != null && webRequest.getCurrentRequest() != null) ? webRequest.getAttributes().getPluginContextPath(webRequest.getCurrentRequest()) : null; + pluginContextPath = (webRequest.getAttributes() != null && webRequest.getRequest() != null) ? webRequest.getAttributes().getPluginContextPath(webRequest.getRequest()) : null; stringBuilder.append(GrailsStringUtils.isNotEmpty(pluginContextPath) ? pluginContextPath : "-"); stringBuilder.append(','); @@ -190,7 +190,7 @@ protected View createGrailsView(String viewName) throws Exception { GrailsWebRequest webRequest = GrailsWebRequest.lookup(); if (webRequest != null) { - HttpServletRequest request = webRequest.getCurrentRequest(); + HttpServletRequest request = webRequest.getRequest(); controller = webRequest.getAttributes().getController(request); } diff --git a/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/GroovyPagesPageContext.java b/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/GroovyPagesPageContext.java index 9169326955b..7231024bcea 100644 --- a/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/GroovyPagesPageContext.java +++ b/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/GroovyPagesPageContext.java @@ -90,7 +90,7 @@ public GroovyPagesPageContext(Servlet pagesServlet, Binding pageScope) { webRequest = (GrailsWebRequest) RequestContextHolder.currentRequestAttributes(); servletContext = webRequest.getServletContext(); - request = webRequest.getCurrentRequest(); + request = webRequest.getRequest(); response = webRequest.getCurrentResponse(); servlet = pagesServlet; servletConfig = pagesServlet.getServletConfig(); diff --git a/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/PageContextFactory.groovy b/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/PageContextFactory.groovy index 4892fc669d1..e05e701d8da 100644 --- a/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/PageContextFactory.groovy +++ b/grails-gsp/grails-web-jsp/src/main/groovy/org/grails/gsp/jsp/PageContextFactory.groovy @@ -39,7 +39,7 @@ class PageContextFactory { static GroovyPagesPageContext getCurrent() { GrailsWebRequest webRequest = RequestContextHolder.currentRequestAttributes() - def request = webRequest.getCurrentRequest() + def request = webRequest.getRequest() def pageContext = request.getAttribute(PC.PAGECONTEXT) if (pageContext instanceof GroovyPagesPageContext) return pageContext diff --git a/grails-gsp/grails-web-taglib/src/main/groovy/org/grails/web/taglib/WebRequestTemplateVariableBinding.java b/grails-gsp/grails-web-taglib/src/main/groovy/org/grails/web/taglib/WebRequestTemplateVariableBinding.java index cf20c4d087c..bf06f2bea3a 100644 --- a/grails-gsp/grails-web-taglib/src/main/groovy/org/grails/web/taglib/WebRequestTemplateVariableBinding.java +++ b/grails-gsp/grails-web-taglib/src/main/groovy/org/grails/web/taglib/WebRequestTemplateVariableBinding.java @@ -54,7 +54,7 @@ public Object evaluate(GrailsWebRequest webRequest) { }); m.put("request", new LazyRequestBasedValue() { public Object evaluate(GrailsWebRequest webRequest) { - return webRequest.getCurrentRequest(); + return webRequest.getRequest(); } }); m.put("response", new LazyRequestBasedValue() { @@ -116,7 +116,7 @@ public WebRequestTemplateVariableBinding(GrailsWebRequest webRequest) { public Binding findBindingForVariable(String name) { Binding binding = super.findBindingForVariable(name); if (binding == null) { - if (webRequest.getCurrentRequest().getAttribute(name) != null) { + if (webRequest.getRequest().getAttribute(name) != null) { requestAttributeVariables.add(name); binding = this; } @@ -139,7 +139,7 @@ public boolean isVariableCachingAllowed(String name) { public Object getVariable(String name) { Object val = getVariablesMap().get(name); if (val == null && !getVariablesMap().containsKey(name) && webRequest != null) { - val = webRequest.getCurrentRequest().getAttribute(name); + val = webRequest.getRequest().getAttribute(name); if (val != null) { requestAttributeVariables.add(name); } else { diff --git a/grails-interceptors/src/main/groovy/org/grails/plugins/web/interceptors/GrailsInterceptorHandlerInterceptorAdapter.groovy b/grails-interceptors/src/main/groovy/org/grails/plugins/web/interceptors/GrailsInterceptorHandlerInterceptorAdapter.groovy index e1f7707add8..58f14114063 100644 --- a/grails-interceptors/src/main/groovy/org/grails/plugins/web/interceptors/GrailsInterceptorHandlerInterceptorAdapter.groovy +++ b/grails-interceptors/src/main/groovy/org/grails/plugins/web/interceptors/GrailsInterceptorHandlerInterceptorAdapter.groovy @@ -18,7 +18,7 @@ */ package org.grails.plugins.web.interceptors -import java.util.function.BooleanSupplier +import java.util.concurrent.ConcurrentHashMap import groovy.transform.CompileDynamic import groovy.transform.CompileStatic @@ -54,12 +54,23 @@ class GrailsInterceptorHandlerInterceptorAdapter implements HandlerInterceptor { private static final Log LOG = LogFactory.getLog(Interceptor) private static final String ATTRIBUTE_MATCHED_INTERCEPTORS = 'org.grails.web.MATCHED_INTERCEPTORS' + private static final String OBSERVATION_NAME = 'grails.interceptor' + private static final String OBSERVATION_CONTEXTUAL_NAME_PREFIX = 'grails.interceptor ' + private static final String OBSERVATION_PHASE_KEY = 'grails.interceptor.phase' + private static final String UNKNOWN_INTERCEPTOR_NAME = 'unknown' static final String INTERCEPTOR_RENDERED_VIEW = 'interceptor_rendered_view' protected List interceptors = [] protected List reverseInterceptors = [] + /** + * Caches the logical interceptor name per interceptor class, so that it is derived once rather + * than on every observed callback. Held per adapter instance (the adapter is an application + * singleton) so the cached class references die with the application context. + */ + private final Map, String> logicalInterceptorNames = new ConcurrentHashMap<>() + @Autowired(required = false) ServiceRegistry[] serviceRegistry // inject the service registry to ensure data services are wired up @@ -84,10 +95,10 @@ class GrailsInterceptorHandlerInterceptorAdapter implements HandlerInterceptor { if (!interceptors.isEmpty()) { List matchInterceptors = [] request.setAttribute(ATTRIBUTE_MATCHED_INTERCEPTORS, matchInterceptors) - for (i in interceptors) { + for (Interceptor i in interceptors) { if (i.doesMatch(request)) { matchInterceptors.add(i) - if (!observe(i, 'before', { -> i.before() } as BooleanSupplier)) { + if (!observe(i, Phase.BEFORE)) { return false } } @@ -104,10 +115,11 @@ class GrailsInterceptorHandlerInterceptorAdapter implements HandlerInterceptor { request.setAttribute(GrailsApplicationAttributes.MODEL_AND_VIEW, modelAndView) } - List reversedInterceptors = ((List) matchedInterceptorsObject).reverse() + List reversedInterceptors = (List) matchedInterceptorsObject + Collections.reverse(reversedInterceptors) request.setAttribute(ATTRIBUTE_MATCHED_INTERCEPTORS, reversedInterceptors) - for (i in reversedInterceptors) { - if (!observe(i, 'after', { -> i.after() } as BooleanSupplier)) { + for (Interceptor i in reversedInterceptors) { + if (!observe(i, Phase.AFTER)) { if (request.getAttribute(INTERCEPTOR_RENDERED_VIEW)) { ModelAndView interceptorsModelAndView = i.modelAndView modelAndView.viewName = interceptorsModelAndView.viewName @@ -131,7 +143,7 @@ class GrailsInterceptorHandlerInterceptorAdapter implements HandlerInterceptor { request.setAttribute(Matcher.THROWABLE, ex) Object matchedInterceptorsObject = request.getAttribute(ATTRIBUTE_MATCHED_INTERCEPTORS) if (matchedInterceptorsObject) { - for (i in ((List) matchedInterceptorsObject)) { + for (Interceptor i in ((List) matchedInterceptorsObject)) { i.afterView() } } @@ -139,22 +151,22 @@ class GrailsInterceptorHandlerInterceptorAdapter implements HandlerInterceptor { /** * Records a {@code grails.interceptor} span around a single interceptor callback, preserving its - * boolean result and control flow. No-op (runs the callback directly) when observation is disabled. + * boolean result and control flow. No-op (invokes the callback directly) when observation is disabled. */ - private boolean observe(Interceptor interceptor, String phase, BooleanSupplier action) { + private boolean observe(Interceptor interceptor, Phase phase) { var registry = this.observationRegistry if (registry == null || registry.isNoop()) { - return action.getAsBoolean() + return phase.invoke(interceptor) } - var name = GrailsNameUtils.getLogicalPropertyName(interceptor.getClass().name, 'Interceptor') ?: 'unknown' - var observation = Observation.createNotStarted('grails.interceptor', registry) - .contextualName('grails.interceptor ' + name) - .lowCardinalityKeyValue('grails.interceptor', name) - .lowCardinalityKeyValue('grails.interceptor.phase', phase ?: 'unknown') + var name = logicalNameOf(interceptor) + var observation = Observation.createNotStarted(OBSERVATION_NAME, registry) + .contextualName(OBSERVATION_CONTEXTUAL_NAME_PREFIX + name) + .lowCardinalityKeyValue(OBSERVATION_NAME, name) + .lowCardinalityKeyValue(OBSERVATION_PHASE_KEY, phase.tag) .start() var scope = observation.openScope() try { - return action.getAsBoolean() + return phase.invoke(interceptor) } catch (Throwable t) { observation.error(t) @@ -165,4 +177,39 @@ class GrailsInterceptorHandlerInterceptorAdapter implements HandlerInterceptor { observation.stop() } } + + /** + * Returns the logical name reported for the given interceptor, deriving it at most once per + * interceptor class. + */ + private String logicalNameOf(Interceptor interceptor) { + Class interceptorClass = interceptor.getClass() + String name = this.logicalInterceptorNames.get(interceptorClass) + if (name == null) { + name = GrailsNameUtils.getLogicalPropertyName(interceptorClass.name, 'Interceptor') ?: UNKNOWN_INTERCEPTOR_NAME + this.logicalInterceptorNames.put(interceptorClass, name) + } + name + } + + /** + * The interceptor callback being invoked. Dispatching through this enum keeps the observation + * plumbing shared between the two phases without a per-call callback object having to be + * allocated for every matched interceptor on every request. + */ + private enum Phase { + + BEFORE('before'), + AFTER('after') + + final String tag + + Phase(String tag) { + this.tag = tag + } + + boolean invoke(Interceptor interceptor) { + this.is(BEFORE) ? interceptor.before() : interceptor.after() + } + } } diff --git a/grails-interceptors/src/test/groovy/grails/artefact/GrailsInterceptorHandlerInterceptorAdapterSpec.groovy b/grails-interceptors/src/test/groovy/grails/artefact/GrailsInterceptorHandlerInterceptorAdapterSpec.groovy index e22b0a12270..95d43340a80 100644 --- a/grails-interceptors/src/test/groovy/grails/artefact/GrailsInterceptorHandlerInterceptorAdapterSpec.groovy +++ b/grails-interceptors/src/test/groovy/grails/artefact/GrailsInterceptorHandlerInterceptorAdapterSpec.groovy @@ -20,6 +20,9 @@ package grails.artefact import grails.interceptors.Matcher import grails.util.GrailsWebMockUtil +import io.micrometer.observation.Observation +import io.micrometer.observation.ObservationHandler +import io.micrometer.observation.ObservationRegistry import org.grails.plugins.web.interceptors.GrailsInterceptorHandlerInterceptorAdapter import org.grails.web.servlet.mvc.GrailsWebRequest import org.springframework.web.context.request.RequestContextHolder @@ -125,6 +128,305 @@ class GrailsInterceptorHandlerInterceptorAdapterSpec extends Specification{ then: webRequest.request.getAttribute(Matcher.THROWABLE) instanceof Exception } + + void "Test observation is disabled by default and the registry is left untouched"() { + given: "An adapter with the default observation registry" + def adapter = new GrailsInterceptorHandlerInterceptorAdapter() + adapter.setInterceptors([new HighestInterceptor(), new LowestInterceptor()] as Interceptor[]) + + expect: "The default registry is a no-op" + adapter.observationRegistry.isNoop() + + when: "A request is handled" + def webRequest = GrailsWebMockUtil.bindMockWebRequest() + def modelAndView = new ModelAndView() + adapter.preHandle(webRequest.request, webRequest.response, this) + + then: "The interceptors still run in order" + webRequest.request.getAttribute('executed') == ['highest before', 'lowest before'] + + when: "The remaining phases run" + webRequest.request.setAttribute('executed', null) + adapter.postHandle(webRequest.request, webRequest.response, this, modelAndView) + adapter.afterCompletion(webRequest.request, webRequest.response, this, null) + + then: "They still run in reverse order" + webRequest.request.getAttribute('executed') == ['lowest after', 'highest after', 'lowest afterView', 'highest afterView'] + } + + void "Test a no-op registry is never asked to create an observation"() { + given: "An adapter wired to a registry that reports itself as a no-op" + def registry = Mock(ObservationRegistry) + registry.isNoop() >> true + def adapter = new GrailsInterceptorHandlerInterceptorAdapter() + adapter.observationRegistry = registry + adapter.setInterceptors([new MyInterceptor()] as Interceptor[]) + def webRequest = GrailsWebMockUtil.bindMockWebRequest() + + when: "A request is handled" + def proceed = adapter.preHandle(webRequest.request, webRequest.response, this) + adapter.postHandle(webRequest.request, webRequest.response, this, new ModelAndView()) + + then: "The interceptor result is honoured" + proceed + + and: "No observation state is created from the registry" + 0 * registry.observationConfig() + 0 * registry.getCurrentObservationScope() + } + + void "Test a null observation registry falls back to invoking the interceptor directly"() { + given: "An adapter with no observation registry at all" + def adapter = new GrailsInterceptorHandlerInterceptorAdapter() + adapter.observationRegistry = null + adapter.setInterceptors([new MyInterceptor()] as Interceptor[]) + def webRequest = GrailsWebMockUtil.bindMockWebRequest() + def modelAndView = new ModelAndView() + + when: "A request is handled" + def proceed = adapter.preHandle(webRequest.request, webRequest.response, this) + adapter.postHandle(webRequest.request, webRequest.response, this, modelAndView) + + then: "The interceptor callbacks still drive the result" + proceed + modelAndView.model.foo == 'bar' + modelAndView.viewName == 'foo' + } + + void "Test an observation is recorded for each interceptor phase when observation is enabled"() { + given: "An adapter wired to a registry with a recording handler" + def handler = new RecordingObservationHandler() + def adapter = observingAdapter(handler, new HighestInterceptor(), new LowestInterceptor()) + def webRequest = GrailsWebMockUtil.bindMockWebRequest() + def modelAndView = new ModelAndView() + + when: "The before phase runs" + adapter.preHandle(webRequest.request, webRequest.response, this) + + then: "One observation per matched interceptor is recorded, in execution order" + handler.started.size() == 2 + handler.stopped.size() == 2 + handler.errored.isEmpty() + handler.scopesOpened == 2 + handler.scopesClosed == 2 + + and: "Each observation carries the interceptor name and phase" + handler.started.every { it.name == 'grails.interceptor' } + handler.names == ['highest', 'lowest'] + handler.phases == ['before', 'before'] + handler.contextualNames == ['grails.interceptor highest', 'grails.interceptor lowest'] + + when: "The after phase runs" + handler.reset() + adapter.postHandle(webRequest.request, webRequest.response, this, modelAndView) + + then: "Observations are recorded in reverse order and tagged as the after phase" + handler.started.size() == 2 + handler.names == ['lowest', 'highest'] + handler.phases == ['after', 'after'] + handler.contextualNames == ['grails.interceptor lowest', 'grails.interceptor highest'] + + when: "The afterView phase runs" + handler.reset() + adapter.afterCompletion(webRequest.request, webRequest.response, this, null) + + then: "It is not observed" + handler.started.isEmpty() + } + + void "Test an observed interceptor that vetoes the request still cancels processing"() { + given: "An observed interceptor that vetoes" + def handler = new RecordingObservationHandler() + def adapter = observingAdapter(handler, new MyInterceptor()) + def webRequest = GrailsWebMockUtil.bindMockWebRequest() + webRequest.request.setAttribute('something', 'test') + + when: "The before phase runs" + def proceed = adapter.preHandle(webRequest.request, webRequest.response, this) + + then: "Processing is cancelled and the observation is still completed cleanly" + !proceed + handler.started.size() == 1 + handler.stopped.size() == 1 + handler.errored.isEmpty() + handler.scopesClosed == 1 + } + + void "Test an observed interceptor that vetoes view rendering still clears the model and view"() { + given: "An observed interceptor that vetoes the after phase" + def handler = new RecordingObservationHandler() + def adapter = observingAdapter(handler, new MyInterceptor()) + def webRequest = GrailsWebMockUtil.bindMockWebRequest() + def modelAndView = new ModelAndView() + adapter.preHandle(webRequest.request, webRequest.response, this) + adapter.postHandle(webRequest.request, webRequest.response, this, modelAndView) + + when: "A condition is met for exclusion" + webRequest.request.setAttribute('bar', 'test') + adapter.postHandle(webRequest.request, webRequest.response, this, modelAndView) + + then: "The view is cleared" + modelAndView.viewName == null + + and: "Every observation was stopped" + handler.started.size() == handler.stopped.size() + handler.errored.isEmpty() + } + + void "Test an exception thrown by an observed interceptor is recorded and rethrown"() { + given: "An observed interceptor that throws" + def handler = new RecordingObservationHandler() + def failure = new IllegalStateException('boom') + def adapter = observingAdapter(handler, new ExplodingInterceptor(failure: failure)) + def webRequest = GrailsWebMockUtil.bindMockWebRequest() + + when: "The before phase runs" + adapter.preHandle(webRequest.request, webRequest.response, this) + + then: "The original exception propagates" + def e = thrown(IllegalStateException) + e.is(failure) + + and: "The observation recorded the error and was still stopped" + handler.errored.size() == 1 + handler.errored.first().error.is(failure) + handler.stopped.size() == 1 + handler.scopesOpened == 1 + handler.scopesClosed == 1 + } + + void "Test interceptor names are resolved per interceptor class across repeated requests"() { + given: "An adapter observing two different interceptor classes" + def handler = new RecordingObservationHandler() + def adapter = observingAdapter(handler, new HighestInterceptor(), new LowestInterceptor()) + + when: "Two requests are handled by the same adapter" + def firstRequest = GrailsWebMockUtil.bindMockWebRequest() + adapter.preHandle(firstRequest.request, firstRequest.response, this) + def firstNames = handler.names + + handler.reset() + RequestContextHolder.setRequestAttributes(null) + def secondRequest = GrailsWebMockUtil.bindMockWebRequest() + adapter.preHandle(secondRequest.request, secondRequest.response, this) + + then: "The cached names are per class, not shared between classes or requests" + firstNames == ['highest', 'lowest'] + handler.names == ['highest', 'lowest'] + } + + void "Test the matched interceptor list is not carried over between requests"() { + given: "An adapter reused across requests" + def adapter = new GrailsInterceptorHandlerInterceptorAdapter() + adapter.setInterceptors([new HighestInterceptor(), new LowestInterceptor()] as Interceptor[]) + + when: "A first request runs through every phase" + def firstRequest = GrailsWebMockUtil.bindMockWebRequest() + adapter.preHandle(firstRequest.request, firstRequest.response, this) + firstRequest.request.setAttribute('executed', null) + adapter.postHandle(firstRequest.request, firstRequest.response, this, new ModelAndView()) + adapter.afterCompletion(firstRequest.request, firstRequest.response, this, null) + + then: "The order is reversed exactly once" + firstRequest.request.getAttribute('executed') == ['lowest after', 'highest after', 'lowest afterView', 'highest afterView'] + + when: "A second request runs through the same adapter" + RequestContextHolder.setRequestAttributes(null) + def secondRequest = GrailsWebMockUtil.bindMockWebRequest() + adapter.preHandle(secondRequest.request, secondRequest.response, this) + + then: "The before phase still runs in the original order" + secondRequest.request.getAttribute('executed') == ['highest before', 'lowest before'] + + when: "The remaining phases run" + secondRequest.request.setAttribute('executed', null) + adapter.postHandle(secondRequest.request, secondRequest.response, this, new ModelAndView()) + adapter.afterCompletion(secondRequest.request, secondRequest.response, this, null) + + then: "The order is reversed again, independently of the first request" + secondRequest.request.getAttribute('executed') == ['lowest after', 'highest after', 'lowest afterView', 'highest afterView'] + } + + private static GrailsInterceptorHandlerInterceptorAdapter observingAdapter(RecordingObservationHandler handler, + Interceptor... interceptors) { + def registry = ObservationRegistry.create() + registry.observationConfig().observationHandler(handler) + def adapter = new GrailsInterceptorHandlerInterceptorAdapter() + adapter.observationRegistry = registry + adapter.setInterceptors(interceptors) + adapter + } +} +class RecordingObservationHandler implements ObservationHandler { + + final List started = [] + final List stopped = [] + final List errored = [] + int scopesOpened + int scopesClosed + + @Override + void onStart(Observation.Context context) { + started << context + } + + @Override + void onStop(Observation.Context context) { + stopped << context + } + + @Override + void onError(Observation.Context context) { + errored << context + } + + @Override + void onScopeOpened(Observation.Context context) { + scopesOpened++ + } + + @Override + void onScopeClosed(Observation.Context context) { + scopesClosed++ + } + + @Override + boolean supportsContext(Observation.Context context) { + true + } + + List getNames() { + started.collect { it.getLowCardinalityKeyValue('grails.interceptor')?.value } + } + + List getPhases() { + started.collect { it.getLowCardinalityKeyValue('grails.interceptor.phase')?.value } + } + + List getContextualNames() { + started.collect { it.contextualName } + } + + void reset() { + started.clear() + stopped.clear() + errored.clear() + scopesOpened = 0 + scopesClosed = 0 + } +} +class ExplodingInterceptor implements Interceptor { + + RuntimeException failure + + ExplodingInterceptor() { + matchAll() + } + + @Override + boolean before() { + throw failure + } } class MyInterceptor implements Interceptor { diff --git a/grails-mimetypes/src/main/groovy/org/grails/plugins/web/api/MimeTypesApiSupport.groovy b/grails-mimetypes/src/main/groovy/org/grails/plugins/web/api/MimeTypesApiSupport.groovy index 36e4eaa1927..86622bfcb9e 100644 --- a/grails-mimetypes/src/main/groovy/org/grails/plugins/web/api/MimeTypesApiSupport.groovy +++ b/grails-mimetypes/src/main/groovy/org/grails/plugins/web/api/MimeTypesApiSupport.groovy @@ -138,7 +138,7 @@ class MimeTypesApiSupport { formatProvider.setAttribute(GrailsApplicationAttributes.CONTENT_FORMAT, format) } else { - GrailsWebRequest.lookup().currentRequest.setAttribute(GrailsApplicationAttributes.RESPONSE_FORMAT, format) + GrailsWebRequest.lookup().request.setAttribute(GrailsApplicationAttributes.RESPONSE_FORMAT, format) } if (formatResponse instanceof Closure) { diff --git a/grails-mimetypes/src/main/groovy/org/grails/web/mime/HttpServletResponseExtension.groovy b/grails-mimetypes/src/main/groovy/org/grails/web/mime/HttpServletResponseExtension.groovy index 90f6acf341b..145471ae943 100755 --- a/grails-mimetypes/src/main/groovy/org/grails/web/mime/HttpServletResponseExtension.groovy +++ b/grails-mimetypes/src/main/groovy/org/grails/web/mime/HttpServletResponseExtension.groovy @@ -110,7 +110,7 @@ class HttpServletResponseExtension { static String getFormat(HttpServletResponse response) { final webRequest = GrailsWebRequest.lookup() - HttpServletRequest request = webRequest.getCurrentRequest() + HttpServletRequest request = webRequest.getRequest() def result = request.getAttribute(GrailsApplicationAttributes.RESPONSE_FORMAT) if (!result) { final mimeType = getMimeType(response) @@ -135,7 +135,7 @@ class HttpServletResponseExtension { } private static MimeType getMimeTypeForRequest(GrailsWebRequest webRequest) { - HttpServletRequest request = webRequest.getCurrentRequest() + HttpServletRequest request = webRequest.getRequest() MimeType result = (MimeType) request.getAttribute(GrailsApplicationAttributes.RESPONSE_MIME_TYPE) if (!result) { def formatOverride = webRequest?.params?.format @@ -174,7 +174,7 @@ class HttpServletResponseExtension { * @return The configured mime types */ static MimeType[] getMimeTypes(HttpServletResponse response) { - return getMimeTypesInternal(GrailsWebRequest.lookup().currentRequest) + return getMimeTypesInternal(GrailsWebRequest.lookup().request) } /** @@ -185,7 +185,7 @@ class HttpServletResponseExtension { */ static MimeType[] getMimeTypesFormatAware(HttpServletResponse response) { GrailsWebRequest webRequest = GrailsWebRequest.lookup() - HttpServletRequest request = webRequest.getCurrentRequest() + HttpServletRequest request = webRequest.getRequest() MimeType[] result = (MimeType[]) request.getAttribute(GrailsApplicationAttributes.RESPONSE_MIME_TYPES) if (!result) { def formatOverride = webRequest?.params?.format diff --git a/grails-rest-transforms/src/main/groovy/grails/artefact/controller/RestResponder.groovy b/grails-rest-transforms/src/main/groovy/grails/artefact/controller/RestResponder.groovy index 710228de1a5..56568c9acc1 100644 --- a/grails-rest-transforms/src/main/groovy/grails/artefact/controller/RestResponder.groovy +++ b/grails-rest-transforms/src/main/groovy/grails/artefact/controller/RestResponder.groovy @@ -165,7 +165,7 @@ trait RestResponder { final firstFormat = formats[0] mimeType = allMimeTypes.find { MimeType mt -> mt.extension == firstFormat } if (mimeType) { - webRequest.currentRequest.setAttribute(GrailsApplicationAttributes.RESPONSE_MIME_TYPE, mimeType) + webRequest.request.setAttribute(GrailsApplicationAttributes.RESPONSE_MIME_TYPE, mimeType) } } diff --git a/grails-rest-transforms/src/main/groovy/org/grails/plugins/web/rest/render/ServletRenderContext.groovy b/grails-rest-transforms/src/main/groovy/org/grails/plugins/web/rest/render/ServletRenderContext.groovy index 9ac6e861b65..6dfb2cba4ef 100644 --- a/grails-rest-transforms/src/main/groovy/org/grails/plugins/web/rest/render/ServletRenderContext.groovy +++ b/grails-rest-transforms/src/main/groovy/org/grails/plugins/web/rest/render/ServletRenderContext.groovy @@ -105,7 +105,7 @@ class ServletRenderContext extends AbstractRenderContext { @Override HttpMethod getHttpMethod() { - HttpMethod.valueOf(webRequest.currentRequest.method) + HttpMethod.valueOf(webRequest.request.method) } @Override @@ -126,7 +126,7 @@ class ServletRenderContext extends AbstractRenderContext { @Override String getViewName() { - final request = webRequest.currentRequest + final request = webRequest.request ModelAndView modelAndView = (ModelAndView) request.getAttribute(GrailsApplicationAttributes.MODEL_AND_VIEW) if (modelAndView) { return modelAndView.viewName @@ -135,7 +135,7 @@ class ServletRenderContext extends AbstractRenderContext { } protected ModelAndView getModelAndView() { - final request = webRequest.currentRequest + final request = webRequest.request ModelAndView modelAndView = (ModelAndView) request.getAttribute(GrailsApplicationAttributes.MODEL_AND_VIEW) if (modelAndView == null) { modelAndView = new ModelAndView() diff --git a/grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/ReflectionUtils.groovy b/grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/ReflectionUtils.groovy index ad3838f0278..bac028245cb 100644 --- a/grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/ReflectionUtils.groovy +++ b/grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/ReflectionUtils.groovy @@ -206,7 +206,7 @@ class ReflectionUtils { static UrlMappingInfo[] matchAllUrlMappings(UrlMappingsHolder urlMappingsHolder, String requestUrl, GrailsWebRequest grailsRequest, HttpServletResponseExtension extension) { - String method = grailsRequest.currentRequest.method + String method = grailsRequest.request.method String version = grailsRequest.getHeader(ACCEPT_VERSION) ?: extension.getMimeTypeForRequest(grailsRequest).version urlMappingsHolder.matchAll requestUrl, method, version == null ? UrlMapping.ANY_VERSION : version } diff --git a/grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/SpringSecurityUtils.groovy b/grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/SpringSecurityUtils.groovy index 3e70ec97d1b..a1cfdd3d558 100644 --- a/grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/SpringSecurityUtils.groovy +++ b/grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/SpringSecurityUtils.groovy @@ -51,6 +51,7 @@ import grails.core.GrailsApplication import grails.plugin.springsecurity.web.GrailsSecurityFilterChain import grails.plugin.springsecurity.web.SecurityRequestHolder import grails.util.Environment +import org.grails.web.util.WebUtils import static org.springframework.security.web.context.HttpSessionSecurityContextRepository.SPRING_SECURITY_CONTEXT_KEY @@ -63,6 +64,8 @@ import static org.springframework.security.web.context.HttpSessionSecurityContex @Slf4j final class SpringSecurityUtils { + // TODO gh-16145 follow-up: the SavedRequest branch of isAjax() compares Ajax header values against + // this class name, which looks unintended - it is most likely meant to be XML_HTTP_REQUEST. private static final String MULTIPART_HTTP_SERVLET_REQUEST_KEY = MultipartHttpServletRequest.name private static ConfigObject _securityConfig @@ -303,7 +306,7 @@ final class SpringSecurityUtils { } // process multipart requests - MultipartHttpServletRequest multipart = (MultipartHttpServletRequest) request.getAttribute(MULTIPART_HTTP_SERVLET_REQUEST_KEY) + MultipartHttpServletRequest multipart = WebUtils.resolveMultipartRequest(request) if ('true' == multipart?.getParameter('ajax')) { return true } diff --git a/grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/web/UpdateRequestContextHolderExceptionTranslationFilter.groovy b/grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/web/UpdateRequestContextHolderExceptionTranslationFilter.groovy index 1f69bc80ce7..5bde1ea0d23 100644 --- a/grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/web/UpdateRequestContextHolderExceptionTranslationFilter.groovy +++ b/grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/web/UpdateRequestContextHolderExceptionTranslationFilter.groovy @@ -73,8 +73,11 @@ class UpdateRequestContextHolderExceptionTranslationFilter extends ExceptionTran @CompileStatic class DelegatingGrailsWebRequest extends GrailsWebRequest { - // GROOVY-12134 - Groovy 5 workaround not ignoring final methods for the `@Delegate` - @Delegate(excludes = ['getRequest', 'getResponse', 'getCurrentRequest', 'getCurrentResponse', 'setMultipartRequest', + // getRequest and getResponse are final in Spring's ServletRequestAttributes and have to be excluded + // because Groovy 5 no longer skips final methods for `@Delegate` (GROOVY-12134). The rest are excluded + // so they answer for the filter chain's request rather than the superseded one - including the + // deprecated getCurrentRequest, which must stay excluded for as long as it exists. + @Delegate(excludes = ['getRequest', 'getResponse', 'getCurrentRequest', 'getCurrentResponse', 'multipartRequestResolved', 'getParams', 'getParameterMap', 'getOriginalParams', 'resetParams', 'addParametersFrom']) GrailsWebRequest current @@ -87,8 +90,11 @@ class DelegatingGrailsWebRequest extends GrailsWebRequest { @CompileStatic class DelegatingAsyncGrailsWebRequest extends AsyncGrailsWebRequest { - // GROOVY-12134 - Groovy 5 workaround not ignoring final methods for the `@Delegate` - @Delegate(excludes = ['getRequest', 'getResponse', 'getCurrentRequest', 'getCurrentResponse', 'setMultipartRequest', + // getRequest and getResponse are final in Spring's ServletRequestAttributes and have to be excluded + // because Groovy 5 no longer skips final methods for `@Delegate` (GROOVY-12134). The rest are excluded + // so they answer for the filter chain's request rather than the superseded one - including the + // deprecated getCurrentRequest, which must stay excluded for as long as it exists. + @Delegate(excludes = ['getRequest', 'getResponse', 'getCurrentRequest', 'getCurrentResponse', 'multipartRequestResolved', 'getParams', 'getParameterMap', 'getOriginalParams', 'resetParams', 'addParametersFrom']) AsyncGrailsWebRequest current diff --git a/grails-spring-security/plugin/src/test/groovy/grails/plugin/springsecurity/web/UpdateRequestContextHolderExceptionTranslationFilterSpec.groovy b/grails-spring-security/plugin/src/test/groovy/grails/plugin/springsecurity/web/UpdateRequestContextHolderExceptionTranslationFilterSpec.groovy new file mode 100644 index 00000000000..48539d34001 --- /dev/null +++ b/grails-spring-security/plugin/src/test/groovy/grails/plugin/springsecurity/web/UpdateRequestContextHolderExceptionTranslationFilterSpec.groovy @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package grails.plugin.springsecurity.web + +import org.springframework.mock.web.MockFilterChain +import org.springframework.mock.web.MockHttpServletRequest +import org.springframework.mock.web.MockHttpServletResponse +import org.springframework.mock.web.MockServletContext +import org.springframework.security.web.AuthenticationEntryPoint +import org.springframework.web.context.request.RequestContextHolder + +import org.grails.web.servlet.mvc.GrailsWebRequest +import org.grails.web.util.WebUtils + +import spock.lang.Specification + +class UpdateRequestContextHolderExceptionTranslationFilterSpec extends Specification { + + MockServletContext servletContext = new MockServletContext() + + void cleanup() { + RequestContextHolder.resetRequestAttributes() + } + + void 'the rebound web request answers for the filter chain request, not the superseded one'() { + given: 'a web request bound earlier in the chain, carrying state of its own' + GrailsWebRequest superseded = webRequest() + superseded.controllerName = 'book' + WebUtils.storeGrailsWebRequest(superseded) + + and: 'a later filter carrying a different request and response' + def chainRequest = new MockHttpServletRequest(servletContext) + def chainResponse = new MockHttpServletResponse() + + when: 'the filter runs' + filter().doFilter(chainRequest, chainResponse, new MockFilterChain()) + GrailsWebRequest rebound = GrailsWebRequest.lookup() + + then: 'it replaced the bound web request with a delegating one' + rebound instanceof DelegatingGrailsWebRequest + !rebound.is(superseded) + + and: 'both request accessors report the request the chain is carrying' + rebound.getRequest().is(chainRequest) + rebound.getCurrentRequest().is(chainRequest) + rebound.getResponse().is(chainResponse) + + and: 'it holds on to the superseded instance without disturbing it' + rebound.current.is(superseded) + !superseded.getRequest().is(chainRequest) + + and: 'request-derived state is read from the chain request rather than carried over' + superseded.controllerName == 'book' + rebound.controllerName == null + } + + void 'a web request that is already delegating is left alone'() { + given: 'a delegating web request already bound' + GrailsWebRequest already = new DelegatingGrailsWebRequest(new MockHttpServletRequest(servletContext), + new MockHttpServletResponse(), webRequest()) + WebUtils.storeGrailsWebRequest(already) + + when: 'the filter runs again, as it does for a nested chain' + filter().doFilter(new MockHttpServletRequest(servletContext), new MockHttpServletResponse(), new MockFilterChain()) + + then: 'it is not wrapped a second time' + GrailsWebRequest.lookup().is(already) + } + + private GrailsWebRequest webRequest() { + new GrailsWebRequest(new MockHttpServletRequest(servletContext), new MockHttpServletResponse(), servletContext) + } + + private UpdateRequestContextHolderExceptionTranslationFilter filter() { + new UpdateRequestContextHolderExceptionTranslationFilter(Mock(AuthenticationEntryPoint)) + } +} diff --git a/grails-test-core/src/main/groovy/org/grails/plugins/testing/AbstractGrailsMockHttpServletResponse.groovy b/grails-test-core/src/main/groovy/org/grails/plugins/testing/AbstractGrailsMockHttpServletResponse.groovy index 5c6fef0f88a..df7ebf0835a 100644 --- a/grails-test-core/src/main/groovy/org/grails/plugins/testing/AbstractGrailsMockHttpServletResponse.groovy +++ b/grails-test-core/src/main/groovy/org/grails/plugins/testing/AbstractGrailsMockHttpServletResponse.groovy @@ -42,7 +42,7 @@ abstract class AbstractGrailsMockHttpServletResponse extends MockHttpServletResp * @param format The format of the response */ void setFormat(String format) { - HttpServletRequest request = GrailsWebRequest.lookup().getCurrentRequest() + HttpServletRequest request = GrailsWebRequest.lookup().getRequest() request.setAttribute(GrailsApplicationAttributes.RESPONSE_FORMAT, format) // remove so that is can be repopulated @@ -106,7 +106,7 @@ abstract class AbstractGrailsMockHttpServletResponse extends MockHttpServletResp @Override void reset() { final webRequest = GrailsWebRequest.lookup() - webRequest?.currentRequest?.removeAttribute(GrailsApplicationAttributes.REDIRECT_ISSUED) + webRequest?.request?.removeAttribute(GrailsApplicationAttributes.REDIRECT_ISSUED) setCommitted(false) super.reset() webRequest?.setOut(getWriter()) @@ -119,7 +119,7 @@ abstract class AbstractGrailsMockHttpServletResponse extends MockHttpServletResp @Override String getRedirectedUrl() { final webRequest = GrailsWebRequest.lookup() - final redirectURI = webRequest?.currentRequest?.getAttribute(GrailsApplicationAttributes.REDIRECT_ISSUED) + final redirectURI = webRequest?.request?.getAttribute(GrailsApplicationAttributes.REDIRECT_ISSUED) if (redirectURI != null) { return redirectURI diff --git a/grails-test-suite-uber/src/test/groovy/org/grails/web/servlet/DefaultGrailsApplicationAttributesSpec.groovy b/grails-test-suite-uber/src/test/groovy/org/grails/web/servlet/DefaultGrailsApplicationAttributesSpec.groovy new file mode 100644 index 00000000000..ec0fae44ce1 --- /dev/null +++ b/grails-test-suite-uber/src/test/groovy/org/grails/web/servlet/DefaultGrailsApplicationAttributesSpec.groovy @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.web.servlet + +import org.springframework.beans.factory.NoSuchBeanDefinitionException +import org.springframework.mock.web.MockServletContext +import org.springframework.web.context.WebApplicationContext +import spock.lang.Specification + +import org.grails.web.servlet.view.CompositeViewResolver +import org.grails.web.util.GrailsApplicationAttributes + +class DefaultGrailsApplicationAttributesSpec extends Specification { + + MockServletContext servletContext = new MockServletContext() + + WebApplicationContext applicationContext = Mock(WebApplicationContext) + + private DefaultGrailsApplicationAttributes createAttributes() { + servletContext.setAttribute(GrailsApplicationAttributes.APPLICATION_CONTEXT, applicationContext) + new DefaultGrailsApplicationAttributes(servletContext) + } + + void 'the composite view resolver is resolved from the application context only once'() { + given: + CompositeViewResolver viewResolver = new CompositeViewResolver() + DefaultGrailsApplicationAttributes attributes = createAttributes() + + when: 'the resolver is asked for repeatedly, as it is once per rendered template' + List resolved = [attributes.compositeViewResolver, + attributes.compositeViewResolver, + attributes.compositeViewResolver] + + then: 'the bean is only looked up on the first call' + 1 * applicationContext.getBean(CompositeViewResolver.BEAN_NAME, CompositeViewResolver) >> viewResolver + + and: 'and every call returns it' + resolved.every { it.is(viewResolver) } + } + + void 'a missing composite view resolver bean is reported rather than returned as null'() { + given: + DefaultGrailsApplicationAttributes attributes = createAttributes() + applicationContext.getBean(CompositeViewResolver.BEAN_NAME, CompositeViewResolver) >> { + throw new NoSuchBeanDefinitionException(CompositeViewResolver.BEAN_NAME) + } + + when: + attributes.getCompositeViewResolver() + + then: + thrown(NoSuchBeanDefinitionException) + } + + void 'a separate attributes instance for another servlet context resolves its own view resolver'() { + given: 'two servlet contexts, each with its own application context and view resolver' + CompositeViewResolver firstViewResolver = new CompositeViewResolver() + CompositeViewResolver secondViewResolver = new CompositeViewResolver() + WebApplicationContext secondApplicationContext = Mock(WebApplicationContext) + secondApplicationContext.getBean(CompositeViewResolver.BEAN_NAME, CompositeViewResolver) >> secondViewResolver + applicationContext.getBean(CompositeViewResolver.BEAN_NAME, CompositeViewResolver) >> firstViewResolver + + MockServletContext secondServletContext = new MockServletContext() + secondServletContext.setAttribute(GrailsApplicationAttributes.APPLICATION_CONTEXT, secondApplicationContext) + + when: + DefaultGrailsApplicationAttributes first = createAttributes() + DefaultGrailsApplicationAttributes second = new DefaultGrailsApplicationAttributes(secondServletContext) + + then: 'neither is served the other context bean' + first.compositeViewResolver.is(firstViewResolver) + second.compositeViewResolver.is(secondViewResolver) + } +} diff --git a/grails-test-suite-web/src/test/groovy/org/grails/compiler/web/ControllerActionTransformerAllowedMethodsSpec.groovy b/grails-test-suite-web/src/test/groovy/org/grails/compiler/web/ControllerActionTransformerAllowedMethodsSpec.groovy index 54cc1a23691..ebe6ff4ae85 100644 --- a/grails-test-suite-web/src/test/groovy/org/grails/compiler/web/ControllerActionTransformerAllowedMethodsSpec.groovy +++ b/grails-test-suite-web/src/test/groovy/org/grails/compiler/web/ControllerActionTransformerAllowedMethodsSpec.groovy @@ -22,6 +22,7 @@ package org.grails.compiler.web import grails.artefact.Artefact import grails.artefact.Enhanced import grails.testing.web.controllers.ControllerUnitTest +import grails.validation.Validateable import jakarta.servlet.http.HttpServletResponse import spock.lang.Issue @@ -183,22 +184,100 @@ class ControllerActionTransformerAllowedMethodsSpec extends Specification implem response.reset() request.method = 'PUT' controller.callPostMethodFromPutMethod() - + then: 'the allowedMethods should not be checked by the second method' response.status == HttpServletResponse.SC_OK } + + void 'a restricted action which accepts a command object rejects an invalid request method'() { + when: 'the no-arg action wrapper is invoked with a request method which is not allowed' + request.method = 'GET' + controller.onlyPostAllowedWithCommand() + + then: 'the wrapper imposes the allowedMethods check' + response.status == HttpServletResponse.SC_METHOD_NOT_ALLOWED + } + + void 'a restricted action which accepts a command object binds and runs for a valid request method'() { + when: 'the no-arg action wrapper is invoked with an allowed request method' + request.method = 'POST' + params.name = 'Jeff' + controller.onlyPostAllowedWithCommand() + + then: 'the check passes, the command object is bound and the delegate runs' + response.status == HttpServletResponse.SC_OK + response.contentAsString == 'Success Jeff' + } + + void 'a restricted action which accepts a command object imposes the check when it is the first action of the request'() { + when: 'the action which accepts the command object is invoked directly with a request method which is not allowed' + request.method = 'GET' + controller.onlyPostAllowedWithCommand(new SomeAllowedMethodsCommand(name: 'Jeff')) + + then: 'the allowedMethods check is imposed' + response.status == HttpServletResponse.SC_METHOD_NOT_ALLOWED + } + + @Issue('GRAILS-11444') + void 'a restricted action which accepts a command object does not re-check when invoked from another action'() { + when: 'an unrestricted action invokes the restricted action which accepts a command object' + request.method = 'GET' + controller.callPostMethodWithCommand() + + then: 'the allowedMethods should not be checked by the restricted method' + response.status == HttpServletResponse.SC_OK + response.contentAsString == 'Success Jeff' + } +} + +class ControllerActionTransformerWithoutAllowedMethodsSpec extends Specification implements ControllerUnitTest { + + void 'a controller which declares no allowedMethods runs its actions for any request method'() { + when: + request.method = requestMethod + controller.index() + + then: + response.status == HttpServletResponse.SC_OK + response.contentAsString == 'Success' + + where: + requestMethod << ['GET', 'POST', 'PUT', 'DELETE'] + } + + void 'a controller which declares no allowedMethods binds command objects'() { + when: + request.method = 'GET' + params.name = 'Jeff' + controller.withCommand() + + then: + response.status == HttpServletResponse.SC_OK + response.contentAsString == 'Success Jeff' + } + + void 'a controller which declares no allowedMethods may invoke one action from another'() { + when: + request.method = 'DELETE' + controller.callIndex() + + then: + response.status == HttpServletResponse.SC_OK + response.contentAsString == 'Success' + } } @Artefact('Controller') class SomeAllowedMethodsController { - static allowedMethods = [callPostMethodFromPutMethod: 'PUT', - onlyPostAllowed: 'POST', - postOrPutAllowed: ['POST', 'PUT'], + static allowedMethods = [callPostMethodFromPutMethod: 'PUT', + onlyPostAllowed: 'POST', + onlyPostAllowedWithCommand: 'POST', + postOrPutAllowed: ['POST', 'PUT'], mixedCasePost: 'pOsT', postOne: 'POST', postTwo: 'POST'] - + def anyMethodAllowed() { render 'Success' } @@ -231,4 +310,32 @@ class SomeAllowedMethodsController { def postOne() {} def postTwo() {} + + def onlyPostAllowedWithCommand(SomeAllowedMethodsCommand cmd) { + render "Success ${cmd.name}" + } + + def callPostMethodWithCommand() { + onlyPostAllowedWithCommand(new SomeAllowedMethodsCommand(name: 'Jeff')) + } +} + +class SomeAllowedMethodsCommand implements Validateable { + String name +} + +@Artefact('Controller') +class NoAllowedMethodsController { + + def index() { + render 'Success' + } + + def withCommand(SomeAllowedMethodsCommand cmd) { + render "Success ${cmd.name}" + } + + def callIndex() { + index() + } } diff --git a/grails-test-suite-web/src/test/groovy/org/grails/web/servlet/mvc/GrailsWebRequestSpec.groovy b/grails-test-suite-web/src/test/groovy/org/grails/web/servlet/mvc/GrailsWebRequestSpec.groovy new file mode 100644 index 00000000000..923e7d9f35b --- /dev/null +++ b/grails-test-suite-web/src/test/groovy/org/grails/web/servlet/mvc/GrailsWebRequestSpec.groovy @@ -0,0 +1,117 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.web.servlet.mvc + +import jakarta.servlet.ServletContext +import jakarta.servlet.http.HttpServletRequestWrapper + +import org.springframework.mock.web.MockHttpServletRequest +import org.springframework.mock.web.MockHttpServletResponse +import org.springframework.mock.web.MockServletContext +import org.springframework.web.context.support.StaticWebApplicationContext + +import org.grails.web.util.GrailsApplicationAttributes + +import spock.lang.Specification + +class GrailsWebRequestSpec extends Specification { + + MockServletContext servletContext = new MockServletContext() + + void 'the application attributes are resolved once per servlet context, not once per request'() { + given: 'a servlet context bound to an application context' + def applicationContext = applicationContext() + + when: 'several requests are handled, as they would be at runtime' + def first = newWebRequest() + def second = newWebRequest() + def third = newWebRequest() + + then: 'they share one attributes instance, so the beans it caches survive between requests' + first.attributes.is(second.attributes) + second.attributes.is(third.attributes) + + and: 'and it is the one resolved against the current application context' + first.attributes.applicationContext.is(applicationContext) + } + + void 'the application attributes are rebuilt when the application context is replaced'() { + given: 'a request handled against one application context' + applicationContext() + def before = newWebRequest().attributes + + when: 'the context is replaced, as it is between tests or after a restart' + def replacement = applicationContext() + def after = newWebRequest().attributes + + then: 'a stale attributes instance is not served' + !after.is(before) + after.applicationContext.is(replacement) + } + + void 'a null servlet context still yields usable attributes'() { + expect: 'no caching is attempted, and construction does not blow up' + new GrailsWebRequest(new MockHttpServletRequest(), new MockHttpServletResponse(), + (ServletContext) null).attributes != null + } + + void 'the deprecated current request is an alias for the request the web request was built with'() { + given: 'a request already wrapped by a filter, as the outermost request usually is' + def wrapped = new HttpServletRequestWrapper(new MockHttpServletRequest(servletContext)) + + when: 'a web request is bound to it' + def webRequest = new GrailsWebRequest(wrapped, new MockHttpServletResponse(), servletContext) + + then: 'the wrapper is what both accessors hand back' + webRequest.getRequest().is(wrapped) + webRequest.getCurrentRequest().is(wrapped) + } + + void 'resolving a multipart request no longer substitutes the request Grails exposes'() { + given: 'a web request bound to an ordinary request' + def request = new MockHttpServletRequest(servletContext) + def webRequest = new GrailsWebRequest(request, new MockHttpServletResponse(), servletContext) + webRequest.params.put('name', 'unresolved') + + when: 'the dispatcher reports that multipart resolution has happened' + webRequest.multipartRequestResolved() + + then: 'only the cached params are discarded - the request itself is untouched' + webRequest.getRequest().is(request) + webRequest.getCurrentRequest().is(request) + !webRequest.params.containsKey('name') + } + + void 'the current request accessor is deprecated in favour of getRequest'() { + expect: 'plugins still compile against it, but are told where to go instead' + GrailsWebRequest.getMethod('getCurrentRequest').isAnnotationPresent(Deprecated) + } + + private GrailsWebRequest newWebRequest() { + new GrailsWebRequest(new MockHttpServletRequest(servletContext), new MockHttpServletResponse(), servletContext) + } + + private StaticWebApplicationContext applicationContext() { + def applicationContext = new StaticWebApplicationContext() + applicationContext.servletContext = servletContext + applicationContext.refresh() + servletContext.setAttribute(GrailsApplicationAttributes.APPLICATION_CONTEXT, applicationContext) + applicationContext + } +} diff --git a/grails-testing-support-web/src/main/groovy/grails/testing/web/GrailsWebUnitTest.groovy b/grails-testing-support-web/src/main/groovy/grails/testing/web/GrailsWebUnitTest.groovy index f4ae0940c20..887b15fb88a 100644 --- a/grails-testing-support-web/src/main/groovy/grails/testing/web/GrailsWebUnitTest.groovy +++ b/grails-testing-support-web/src/main/groovy/grails/testing/web/GrailsWebUnitTest.groovy @@ -55,7 +55,7 @@ trait GrailsWebUnitTest implements GrailsUnitTest { GrailsWebRequest webRequest GrailsMockHttpServletRequest getRequest() { - return (GrailsMockHttpServletRequest) getWebRequest().getCurrentRequest() + return (GrailsMockHttpServletRequest) getWebRequest().getRequest() } GrailsMockHttpServletResponse getResponse() { diff --git a/grails-views-core/src/main/groovy/grails/views/mvc/GenericGroovyTemplateViewResolver.groovy b/grails-views-core/src/main/groovy/grails/views/mvc/GenericGroovyTemplateViewResolver.groovy index 8078983a610..ea19ad2e7f8 100644 --- a/grails-views-core/src/main/groovy/grails/views/mvc/GenericGroovyTemplateViewResolver.groovy +++ b/grails-views-core/src/main/groovy/grails/views/mvc/GenericGroovyTemplateViewResolver.groovy @@ -48,7 +48,7 @@ class GenericGroovyTemplateViewResolver implements ViewResolver { View resolveViewName(String viewName, Locale locale) throws Exception { def webRequest = GrailsWebRequest.lookup() if (webRequest != null) { - def currentRequest = webRequest?.currentRequest + def currentRequest = webRequest?.request if (viewName.startsWith('/')) { def controller = webRequest.controllerClass View view @@ -96,7 +96,7 @@ class GenericGroovyTemplateViewResolver implements ViewResolver { } private View resolveViewWithController(String controllerUri, String viewName, GrailsWebRequest webRequest) { - HttpServletRequest currentRequest = webRequest?.currentRequest + HttpServletRequest currentRequest = webRequest?.request HttpServletResponse currentResponse = webRequest?.currentResponse if (controllerUri) { diff --git a/grails-views-core/src/main/groovy/grails/views/mvc/renderer/DefaultViewRenderer.groovy b/grails-views-core/src/main/groovy/grails/views/mvc/renderer/DefaultViewRenderer.groovy index 5a7bf9951aa..af1be21b4de 100644 --- a/grails-views-core/src/main/groovy/grails/views/mvc/renderer/DefaultViewRenderer.groovy +++ b/grails-views-core/src/main/groovy/grails/views/mvc/renderer/DefaultViewRenderer.groovy @@ -94,7 +94,7 @@ abstract class DefaultViewRenderer extends DefaultHtmlRenderer { } def webRequest = ((ServletRenderContext) context).getWebRequest() - def request = webRequest.currentRequest + def request = webRequest.request def response = webRequest.currentResponse AbstractUrlBasedView view diff --git a/grails-views-gson/src/main/groovy/grails/plugin/json/renderer/AbstractJsonViewContainerRenderer.groovy b/grails-views-gson/src/main/groovy/grails/plugin/json/renderer/AbstractJsonViewContainerRenderer.groovy index 384cf1c9a7e..9c3b86ea161 100644 --- a/grails-views-gson/src/main/groovy/grails/plugin/json/renderer/AbstractJsonViewContainerRenderer.groovy +++ b/grails-views-gson/src/main/groovy/grails/plugin/json/renderer/AbstractJsonViewContainerRenderer.groovy @@ -66,7 +66,7 @@ abstract class AbstractJsonViewContainerRenderer extends DefaultJsonRendere model.putAll((Map) contextModel) } - def request = webRequest.currentRequest + def request = webRequest.request def response = webRequest.currentResponse view.render(model, request, response) } else { diff --git a/grails-views-gson/src/test/groovy/grails/plugin/json/view/JsonViewTemplateResolverSpec.groovy b/grails-views-gson/src/test/groovy/grails/plugin/json/view/JsonViewTemplateResolverSpec.groovy index d217ac71abf..320efaaab81 100644 --- a/grails-views-gson/src/test/groovy/grails/plugin/json/view/JsonViewTemplateResolverSpec.groovy +++ b/grails-views-gson/src/test/groovy/grails/plugin/json/view/JsonViewTemplateResolverSpec.groovy @@ -29,13 +29,11 @@ import grails.web.http.HttpHeaders import org.grails.web.servlet.mvc.GrailsWebRequest import org.grails.web.util.GrailsApplicationAttributes import org.springframework.mock.web.MockHttpServletRequest +import org.springframework.mock.web.MockHttpServletResponse import org.springframework.web.context.request.RequestContextHolder import spock.lang.Issue import spock.lang.Specification -import jakarta.servlet.http.HttpServletRequest -import jakarta.servlet.http.HttpServletResponse - /** * Created by graemerocher on 24/08/15. */ @@ -70,17 +68,13 @@ class JsonViewTemplateResolverSpec extends Specification { def applicationAttributes = Mock(GrailsApplicationAttributes) applicationAttributes.getControllerUri(_) >> "/test" - def webRequest = Mock(GrailsWebRequest) - webRequest.getAttributes() >> applicationAttributes + def request = new MockHttpServletRequest() + request.addHeader(HttpHeaders.ACCEPT_VERSION, "1.1") + request.addHeader(HttpHeaders.ACCEPT, "text/html") + request.preferredLocales = [Locale.ENGLISH] + def response = new MockHttpServletResponse() + def webRequest = new GrailsWebRequest(request, response, applicationAttributes) RequestContextHolder.setRequestAttributes(webRequest) - def request = Mock(HttpServletRequest) - def response = Mock(HttpServletResponse) - request.getHeader(HttpHeaders.ACCEPT_VERSION) >> "1.1" - request.getHeader(HttpHeaders.ACCEPT) >> "text/html" - request.getLocale() >> Locale.ENGLISH - // resolveView(viewName, request, response) and buildQualifiers use the request/response passed - // directly, so there is no need to stub GrailsWebRequest's final getRequest()/getResponse(). - webRequest.getCurrentRequest() >> request def templateResolver = Mock(TemplateResolver) viewResolver.templateResolver = templateResolver @@ -139,14 +133,11 @@ class JsonViewTemplateResolverSpec extends Specification { def viewResolver = new GenericGroovyTemplateViewResolver(smartResolver) - def webRequest = Mock(GrailsWebRequest) - def applicationAttributes = Mock(GrailsApplicationAttributes) applicationAttributes.getControllerUri(_) >> "/test" - webRequest.getAttributes() >> applicationAttributes def currentRequest = new MockHttpServletRequest() currentRequest.addHeader('Accept', 'text/html') - webRequest.getCurrentRequest() >> currentRequest + def webRequest = new GrailsWebRequest(currentRequest, new MockHttpServletResponse(), applicationAttributes) RequestContextHolder.setRequestAttributes(webRequest) def templateResolver = Mock(TemplateResolver) @@ -175,17 +166,14 @@ class JsonViewTemplateResolverSpec extends Specification { def smartResolver = new JsonViewResolver() def viewResolver = new GenericGroovyTemplateViewResolver(smartResolver) - def webRequest = Mock(GrailsWebRequest) - and: 'the default controller URI' def applicationAttributes = Mock(GrailsApplicationAttributes) applicationAttributes.getControllerUri(_) >> "/test" - webRequest.getAttributes() >> applicationAttributes and: 'the actual URI because of a redirect' def currentRequest = new MockHttpServletRequest("", "/foo") currentRequest.addHeader('Accept', 'text/html') - webRequest.getCurrentRequest() >> currentRequest + def webRequest = new GrailsWebRequest(currentRequest, new MockHttpServletResponse(), applicationAttributes) RequestContextHolder.setRequestAttributes(webRequest) def templateResolver = Mock(TemplateResolver) diff --git a/grails-web-common/src/main/groovy/grails/web/api/ServletAttributes.groovy b/grails-web-common/src/main/groovy/grails/web/api/ServletAttributes.groovy index c8ba48ec95f..77b330f7796 100644 --- a/grails-web-common/src/main/groovy/grails/web/api/ServletAttributes.groovy +++ b/grails-web-common/src/main/groovy/grails/web/api/ServletAttributes.groovy @@ -45,7 +45,7 @@ trait ServletAttributes implements WebAttributes { @Generated HttpServletRequest getRequest() { - currentRequestAttributes().getCurrentRequest() + currentRequestAttributes().getRequest() } @Generated diff --git a/grails-web-common/src/main/groovy/grails/web/servlet/mvc/GrailsParameterMap.java b/grails-web-common/src/main/groovy/grails/web/servlet/mvc/GrailsParameterMap.java index ea762ec71f2..c42468fe7bc 100644 --- a/grails-web-common/src/main/groovy/grails/web/servlet/mvc/GrailsParameterMap.java +++ b/grails-web-common/src/main/groovy/grails/web/servlet/mvc/GrailsParameterMap.java @@ -92,17 +92,25 @@ public GrailsParameterMap(HttpServletRequest request) { this.request = request; // Request parameters (including form-encoded PUT/PATCH/DELETE bodies parsed at the servlet // layer by Spring's FormContentFilter) are read straight from the request parameter map. - final Map requestMap = new LinkedHashMap(request.getParameterMap()); - - if (request instanceof MultipartHttpServletRequest) { - MultiValueMap fileMap = ((MultipartHttpServletRequest) request).getMultiFileMap(); - for (Entry> entry : fileMap.entrySet()) { - List value = entry.getValue(); - if (value.size() == 1) { - requestMap.put(entry.getKey(), value.get(0)); - } - else { - requestMap.put(entry.getKey(), value); + // updateNestedKeys only reads this map - everything it builds goes into wrappedMap - so the + // servlet's own map is used directly, and copied only when uploaded files have to be merged in. + Map requestMap = request.getParameterMap(); + + // The request is the outermost request, so the multipart request is discovered from its wrapper + // chain rather than being the request itself. + MultipartHttpServletRequest multipartRequest = WebUtils.resolveMultipartRequest(request); + if (multipartRequest != null) { + MultiValueMap fileMap = multipartRequest.getMultiFileMap(); + if (!fileMap.isEmpty()) { + requestMap = new LinkedHashMap(requestMap); + for (Entry> entry : fileMap.entrySet()) { + List value = entry.getValue(); + if (value.size() == 1) { + requestMap.put(entry.getKey(), value.get(0)); + } + else { + requestMap.put(entry.getKey(), value); + } } } } diff --git a/grails-web-common/src/main/groovy/org/grails/web/errors/GrailsWrappedRuntimeException.java b/grails-web-common/src/main/groovy/org/grails/web/errors/GrailsWrappedRuntimeException.java index 3927fef2117..49ca2ddb02b 100644 --- a/grails-web-common/src/main/groovy/org/grails/web/errors/GrailsWrappedRuntimeException.java +++ b/grails-web-common/src/main/groovy/org/grails/web/errors/GrailsWrappedRuntimeException.java @@ -31,8 +31,8 @@ import jakarta.servlet.ServletContext; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.core.io.Resource; import org.springframework.core.io.support.PathMatchingResourcePatternResolver; @@ -69,7 +69,7 @@ public class GrailsWrappedRuntimeException extends GrailsException { private static final Pattern PARSE_DETAILS_STEP2 = Pattern.compile("at\\s{1}(\\w+)\\$_closure\\d+\\.doCall\\(\\1:(\\d+)\\)"); private static final Pattern PARSE_GSP_DETAILS_STEP1 = Pattern.compile("_gsp\\.run\\(((\\w+?)_.*?):(\\d+)\\)"); public static final String URL_PREFIX = "/WEB-INF/grails-app/"; - private static final Log LOG = LogFactory.getLog(GrailsWrappedRuntimeException.class); + private static final Logger LOG = LoggerFactory.getLogger(GrailsWrappedRuntimeException.class); private String className = UNKNOWN; private int lineNumber = -1; private String stackTrace; @@ -193,7 +193,7 @@ else if (application.isArtefactOfType(ServiceArtefactHandler.TYPE, className)) { InputStream in = null; if (!GrailsStringUtils.isBlank(url)) { in = servletContext.getResourceAsStream(url); - LOG.debug("Attempting to display code snippet found in url " + url); + LOG.debug("Attempting to display code snippet found in url {}", url); } if (in == null) { Resource r = null; @@ -240,7 +240,7 @@ else if (currentLineNumber == lineNumber + 1) { } } catch (IOException e) { - LOG.warn("[GrailsWrappedRuntimeException] I/O error reading line diagnostics: " + e.getMessage(), e); + LOG.warn("[GrailsWrappedRuntimeException] I/O error reading line diagnostics: {}", e.getMessage(), e); } finally { if (reader != null) { diff --git a/grails-web-common/src/main/groovy/org/grails/web/json/PathCapturingJSONWriterWrapper.java b/grails-web-common/src/main/groovy/org/grails/web/json/PathCapturingJSONWriterWrapper.java index 5d7d36e062a..2d7e99619a4 100644 --- a/grails-web-common/src/main/groovy/org/grails/web/json/PathCapturingJSONWriterWrapper.java +++ b/grails-web-common/src/main/groovy/org/grails/web/json/PathCapturingJSONWriterWrapper.java @@ -20,8 +20,8 @@ import java.util.Stack; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * TODO Proof of concept @@ -31,7 +31,7 @@ */ public class PathCapturingJSONWriterWrapper extends JSONWriter { - private final Log log = LogFactory.getLog(getClass()); + private final Logger log = LoggerFactory.getLogger(getClass()); private final boolean debugCurrentStack = true; private JSONWriter delegate; @@ -45,8 +45,8 @@ public PathCapturingJSONWriterWrapper(JSONWriter delegate) { @Override public JSONWriter append(String s) { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("append(%s)", s)); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > append({})", delegate.mode.name(), s); } delegate.append(s); return this; @@ -55,7 +55,7 @@ public JSONWriter append(String s) { @Override public void comma() { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); log.debug("comma()"); } delegate.comma(); @@ -64,8 +64,8 @@ public void comma() { @Override public JSONWriter array() { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("array()")); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > array()", delegate.mode.name()); } pathStack.push(new IndexElement(-1)); delegate.array(); @@ -75,8 +75,8 @@ public JSONWriter array() { @Override public JSONWriter end(Mode m, char c) { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("end(%s, %s)", m, c)); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > end({}, {})", delegate.mode.name(), m, c); } delegate.end(m, c); return this; @@ -85,8 +85,8 @@ public JSONWriter end(Mode m, char c) { @Override public JSONWriter endArray() { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("endArray()")); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > endArray()", delegate.mode.name()); } pathStack.pop(); delegate.endArray(); @@ -99,8 +99,8 @@ public JSONWriter endArray() { @Override public JSONWriter endObject() { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("endObject()")); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > endObject()", delegate.mode.name()); } delegate.endObject(); if (delegate.mode != Mode.ARRAY && pathStack.size() > 0) { @@ -112,8 +112,8 @@ public JSONWriter endObject() { @Override public JSONWriter key(String s) { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("key(%s)", s)); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > key({})", delegate.mode.name(), s); } pathStack.push(new PropertyElement(s)); delegate.key(s); @@ -123,8 +123,8 @@ public JSONWriter key(String s) { @Override public JSONWriter object() { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("object()")); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > object()", delegate.mode.name()); } if (delegate.mode == Mode.ARRAY) { pushNextIndex(); @@ -136,8 +136,8 @@ public JSONWriter object() { @Override public void pop(Mode c) { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("pop(%s)", c)); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > pop({})", delegate.mode.name(), c); } delegate.pop(c); } @@ -145,8 +145,8 @@ public void pop(Mode c) { @Override public void push(Mode c) { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("push(%s)", c)); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > push({})", delegate.mode.name(), c); } delegate.push(c); } @@ -160,8 +160,8 @@ private void pushNextIndex() { private int nextIndex() { int x = ((IndexElement) pathStack.peek()).index + 1; if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("Next index: " + x)); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > Next index: {}", delegate.mode.name(), x); } return x; } @@ -169,8 +169,8 @@ private int nextIndex() { @Override public JSONWriter value(boolean b) { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("value(boolean %b)", b)); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > value(boolean {})", delegate.mode.name(), b); } if (delegate.mode == Mode.ARRAY) { pushNextIndex(); @@ -185,8 +185,8 @@ public JSONWriter value(boolean b) { @Override public JSONWriter value(double d) { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("value(double %s)", d)); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > value(double {})", delegate.mode.name(), d); } if (delegate.mode == Mode.ARRAY) { pushNextIndex(); @@ -201,8 +201,8 @@ public JSONWriter value(double d) { @Override public JSONWriter value(long l) { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("value(long %s)", l)); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > value(long {})", delegate.mode.name(), l); } if (delegate.mode == Mode.ARRAY) { pushNextIndex(); @@ -217,8 +217,8 @@ public JSONWriter value(long l) { @Override public JSONWriter value(Object o) { if (log.isDebugEnabled()) { - if (debugCurrentStack) log.debug(delegate.mode.name() + " > " + String.format(">> " + getCurrentStrackReference())); - log.debug(delegate.mode.name() + " > " + String.format("value(Object %s)", o)); + if (debugCurrentStack) log.debug("{} > >> {}", delegate.mode.name(), getCurrentStrackReference()); + log.debug("{} > value(Object {})", delegate.mode.name(), o); } if (delegate.mode == Mode.ARRAY) { diff --git a/grails-web-common/src/main/groovy/org/grails/web/servlet/DefaultGrailsApplicationAttributes.java b/grails-web-common/src/main/groovy/org/grails/web/servlet/DefaultGrailsApplicationAttributes.java index 49c187cc023..743d9d0395d 100644 --- a/grails-web-common/src/main/groovy/org/grails/web/servlet/DefaultGrailsApplicationAttributes.java +++ b/grails-web-common/src/main/groovy/org/grails/web/servlet/DefaultGrailsApplicationAttributes.java @@ -27,8 +27,8 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpSession; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.beans.BeansException; import org.springframework.context.ApplicationContext; @@ -45,6 +45,7 @@ import grails.web.pages.GroovyPagesUriService; import org.grails.gsp.ResourceAwareTemplateEngine; import org.grails.web.pages.DefaultGroovyPagesUriService; +import org.grails.web.servlet.view.CompositeViewResolver; import org.grails.web.util.GrailsApplicationAttributes; /** @@ -59,28 +60,31 @@ public class DefaultGrailsApplicationAttributes implements GrailsApplicationAttributes { protected static final String DEFAULT_NAMESPACE = "g"; - private static Log LOG = LogFactory.getLog(DefaultGrailsApplicationAttributes.class); + private static final Logger LOG = LoggerFactory.getLogger(DefaultGrailsApplicationAttributes.class); - private UrlPathHelper urlHelper = new UrlPathHelper(); + private final UrlPathHelper urlHelper = UrlPathHelper.defaultInstance; - private ServletContext context; - private ApplicationContext appContext; + private final ServletContext context; + private final ApplicationContext appContext; // Beans used very often - private ResourceAwareTemplateEngine pagesTemplateEngine; - private GrailsApplication grailsApplication; - private GroovyPagesUriService groovyPagesUriService; - private MessageSource messageSource; - private GrailsPluginManager pluginManager; + private volatile ResourceAwareTemplateEngine pagesTemplateEngine; + private volatile GrailsApplication grailsApplication; + private volatile GroovyPagesUriService groovyPagesUriService; + private volatile MessageSource messageSource; + private volatile GrailsPluginManager pluginManager; + private volatile CompositeViewResolver compositeViewResolver; public DefaultGrailsApplicationAttributes(ServletContext context) { this.context = context; + ApplicationContext resolved = null; if (context != null) { - appContext = (ApplicationContext) context.getAttribute(APPLICATION_CONTEXT); - if (appContext == null) { - appContext = Holders.findApplicationContext(); + resolved = (ApplicationContext) context.getAttribute(APPLICATION_CONTEXT); + if (resolved == null) { + resolved = Holders.findApplicationContext(); } } + this.appContext = resolved; } public ApplicationContext getApplicationContext() { @@ -107,7 +111,7 @@ private T fetchBeanFromAppCtx(String name) { } } catch (BeansException e) { - LOG.warn("Bean named '" + name + "' is missing."); + LOG.warn("Bean named '{}' is missing.", name); return null; } } @@ -216,8 +220,8 @@ public ResourceAwareTemplateEngine getPagesTemplateEngine() { if (pagesTemplateEngine == null) { pagesTemplateEngine = fetchBeanFromAppCtx(ResourceAwareTemplateEngine.BEAN_ID); } - if (pagesTemplateEngine == null && LOG.isWarnEnabled()) { - LOG.warn("No bean named [" + ResourceAwareTemplateEngine.BEAN_ID + "] defined in Spring application context!"); + if (pagesTemplateEngine == null) { + LOG.warn("No bean named [{}] defined in Spring application context!", ResourceAwareTemplateEngine.BEAN_ID); } return pagesTemplateEngine; } @@ -269,4 +273,21 @@ public MessageSource getMessageSource() { } return messageSource; } + + /** + * {@inheritDoc} + * + *

Resolved from the application context on first use and held afterwards. Unlike the beans above this one is + * required rather than optional - a template cannot be rendered without it - so a missing bean is left to raise + * the same {@code NoSuchBeanDefinitionException} it always has rather than being turned into a null.

+ */ + @Override + public CompositeViewResolver getCompositeViewResolver() { + CompositeViewResolver resolver = compositeViewResolver; + if (resolver == null) { + resolver = getApplicationContext().getBean(CompositeViewResolver.BEAN_NAME, CompositeViewResolver.class); + compositeViewResolver = resolver; + } + return resolver; + } } diff --git a/grails-web-common/src/main/groovy/org/grails/web/servlet/GrailsFlashScope.java b/grails-web-common/src/main/groovy/org/grails/web/servlet/GrailsFlashScope.java index ed8cdab4b5b..6785191e730 100644 --- a/grails-web-common/src/main/groovy/org/grails/web/servlet/GrailsFlashScope.java +++ b/grails-web-common/src/main/groovy/org/grails/web/servlet/GrailsFlashScope.java @@ -216,7 +216,7 @@ else if (value instanceof Map) { private void registerWithSessionIfNecessary() { if (registerWithSession) { GrailsWebRequest webRequest = (GrailsWebRequest) RequestContextHolder.currentRequestAttributes(); - HttpSession session = webRequest.getCurrentRequest().getSession(true); + HttpSession session = webRequest.getRequest().getSession(true); if (session.getAttribute(GrailsApplicationAttributes.FLASH_SCOPE) == null) { session.setAttribute(GrailsApplicationAttributes.FLASH_SCOPE, this); } diff --git a/grails-web-common/src/main/groovy/org/grails/web/servlet/WebRequestDelegatingRequestContext.java b/grails-web-common/src/main/groovy/org/grails/web/servlet/WebRequestDelegatingRequestContext.java index 27de350a7b2..46f92e036d9 100644 --- a/grails-web-common/src/main/groovy/org/grails/web/servlet/WebRequestDelegatingRequestContext.java +++ b/grails-web-common/src/main/groovy/org/grails/web/servlet/WebRequestDelegatingRequestContext.java @@ -56,7 +56,7 @@ public GrailsWebRequest getWebRequest() { } public HttpServletRequest getRequest() { - return webRequest.getCurrentRequest(); + return webRequest.getRequest(); } public HttpServletResponse getResponse() { diff --git a/grails-web-common/src/main/groovy/org/grails/web/servlet/mvc/DefaultRequestStateLookupStrategy.java b/grails-web-common/src/main/groovy/org/grails/web/servlet/mvc/DefaultRequestStateLookupStrategy.java index b3ac020a382..2f73e09b300 100644 --- a/grails-web-common/src/main/groovy/org/grails/web/servlet/mvc/DefaultRequestStateLookupStrategy.java +++ b/grails-web-common/src/main/groovy/org/grails/web/servlet/mvc/DefaultRequestStateLookupStrategy.java @@ -61,7 +61,7 @@ public String getContextPath() { public String getCharacterEncoding() { final GrailsWebRequest req = getWebRequest(); if (req != null) { - return req.getCurrentRequest().getCharacterEncoding(); + return req.getRequest().getCharacterEncoding(); } return DEFAULT_REQUEST_ENCODING; } @@ -70,7 +70,7 @@ public String getCharacterEncoding() { public String getHttpMethod() { final GrailsWebRequest req = getWebRequest(); if (req != null) { - return req.getCurrentRequest().getMethod(); + return req.getRequest().getMethod(); } return null; } diff --git a/grails-web-common/src/main/groovy/org/grails/web/servlet/mvc/GrailsWebRequest.java b/grails-web-common/src/main/groovy/org/grails/web/servlet/mvc/GrailsWebRequest.java index 27421eb267e..1e22d03b273 100644 --- a/grails-web-common/src/main/groovy/org/grails/web/servlet/mvc/GrailsWebRequest.java +++ b/grails-web-common/src/main/groovy/org/grails/web/servlet/mvc/GrailsWebRequest.java @@ -44,6 +44,7 @@ import grails.core.GrailsApplication; import grails.core.GrailsControllerClass; +import grails.util.Holders; import grails.validation.DeferredBindingActions; import grails.web.mvc.FlashScope; import grails.web.servlet.mvc.GrailsHttpSession; @@ -67,6 +68,11 @@ * * def webRequest = RequestContextHolder.currentRequestAttributes() * + *

The request this exposes through {@link #getRequest()} is always the outermost request, so wrappers + * contributed by other filters keep working. Multipart capabilities are discovered from its wrapper chain + * rather than by substituting the request - see + * {@link org.grails.web.util.WebUtils#resolveMultipartRequest(HttpServletRequest)}. + * * @author Graeme Rocher * @since 3.0 */ @@ -74,6 +80,9 @@ public class GrailsWebRequest extends DispatcherServletWebRequest { private static final String REDIRECT_CALLED = GrailsApplicationAttributes.REDIRECT_ISSUED; + /** Servlet context attribute caching the {@link GrailsApplicationAttributes} for that context. */ + private static final String GRAILS_APPLICATION_ATTRIBUTES = GrailsWebRequest.class.getName() + ".ATTRIBUTES"; + private static final Class grailsApplicationAttributesClass = GrailsFactoriesLoader.loadFactoryClasses(GrailsApplicationAttributes.class, GrailsWebRequest.class.getClassLoader()).get(0); private static final Constructor grailsApplicationAttributesConstructor = ClassUtils.getConstructorIfAvailable(grailsApplicationAttributesClass, ServletContext.class); private GrailsApplicationAttributes attributes; @@ -85,13 +94,12 @@ public class GrailsWebRequest extends DispatcherServletWebRequest { private Encoder filteringEncoder; public static final String ID_PARAMETER = "id"; private final List parameterCreationListeners = new ArrayList<>(); - private final UrlPathHelper urlHelper = new UrlPathHelper(); + private final UrlPathHelper urlHelper = UrlPathHelper.defaultInstance; private ApplicationContext applicationContext; private String baseUrl; private HttpServletResponse wrappedResponse; private EncodingStateRegistry encodingStateRegistry; - private HttpServletRequest multipartRequest; public GrailsWebRequest(HttpServletRequest request, HttpServletResponse response, GrailsApplicationAttributes attributes) { super(request, response); @@ -102,14 +110,50 @@ public GrailsWebRequest(HttpServletRequest request, HttpServletResponse response public GrailsWebRequest(HttpServletRequest request, HttpServletResponse response, ServletContext servletContext) { super(request, response); + attributes = resolveAttributes(servletContext); + this.applicationContext = attributes.getApplicationContext(); + inheritEncodingStateRegistry(); + } + + /** + * Returns the {@link GrailsApplicationAttributes} for the given servlet context, creating it on first + * use and caching it in the servlet context afterwards. + *

+ * The attributes object holds no request state - it caches the beans it describes as "used very often" - + * so building one per request both paid for a reflective construction and discarded those caches every + * request. It is rebuilt if the {@code ApplicationContext} it was resolved against is no longer current, + * which keeps a restarted or re-created context (as happens between tests) from being served a stale one. + */ + private static GrailsApplicationAttributes resolveAttributes(ServletContext servletContext) { + if (servletContext == null) { + return createAttributes(null); + } + Object cached = servletContext.getAttribute(GRAILS_APPLICATION_ATTRIBUTES); + if (cached instanceof GrailsApplicationAttributes grailsApplicationAttributes && + grailsApplicationAttributes.getApplicationContext() == currentApplicationContext(servletContext)) { + return grailsApplicationAttributes; + } + GrailsApplicationAttributes attributes = createAttributes(servletContext); + servletContext.setAttribute(GRAILS_APPLICATION_ATTRIBUTES, attributes); + return attributes; + } + + private static ApplicationContext currentApplicationContext(ServletContext servletContext) { + Object applicationContext = servletContext.getAttribute(GrailsApplicationAttributes.APPLICATION_CONTEXT); + if (applicationContext instanceof ApplicationContext context) { + return context; + } + return Holders.findApplicationContext(); + } + + private static GrailsApplicationAttributes createAttributes(ServletContext servletContext) { try { - attributes = grailsApplicationAttributesConstructor.newInstance(servletContext); - this.applicationContext = attributes.getApplicationContext(); + return grailsApplicationAttributesConstructor.newInstance(servletContext); } catch (Exception e) { ReflectionUtils.rethrowRuntimeException(e); + return null; } - inheritEncodingStateRegistry(); } public GrailsWebRequest(HttpServletRequest request, HttpServletResponse response, ServletContext servletContext, ApplicationContext applicationContext) { @@ -118,13 +162,15 @@ public GrailsWebRequest(HttpServletRequest request, HttpServletResponse response } /** - * Holds a reference to the {@link org.springframework.web.multipart.MultipartRequest} + * Discards the cached params so they are rebuilt and pick up uploaded files, for when multipart + * resolution happens after params were already read. + * See gh-13837. * - * @param multipartRequest The multipart request + * @since 8.0 */ - public void setMultipartRequest(HttpServletRequest multipartRequest) { - this.multipartRequest = multipartRequest; - this.originalParams = null; // originalParams will need to be re-initialized. See https://github.com/apache/grails-core/issues/13837 + public void multipartRequestResolved() { + this.originalParams = null; + this.params = null; } private void inheritEncodingStateRegistry() { @@ -158,7 +204,7 @@ public void requestCompleted() { * @return the out */ public Writer getOut() { - Writer out = attributes.getOut(getCurrentRequest()); + Writer out = attributes.getOut(getRequest()); if (out == null) { try { return getCurrentResponse().getWriter(); @@ -181,7 +227,7 @@ public boolean isActive() { * @param out the out to set */ public void setOut(Writer out) { - attributes.setOut(getCurrentRequest(), out); + attributes.setOut(getRequest(), out); } /** @@ -197,7 +243,7 @@ public ServletContext getServletContext() { */ @Override public String getContextPath() { - final HttpServletRequest request = getCurrentRequest(); + final HttpServletRequest request = getRequest(); String appUri = (String) request.getAttribute(GrailsApplicationAttributes.APP_URI_ATTRIBUTE); if (appUri == null) { appUri = urlHelper.getContextPath(request); @@ -214,14 +260,14 @@ public FlashScope getFlashScope() { /** * @return The currently executing request + * + * @deprecated as of 8.0, use {@link #getRequest()} instead. This used to return the resolved + * multipart request in place of the request Grails was bound to; that substitution is + * gone, so the two are now the same object. */ + @Deprecated(since = "8.0") public HttpServletRequest getCurrentRequest() { - if (multipartRequest != null) { - return multipartRequest; - } - else { - return getRequest(); - } + return getRequest(); } public HttpServletResponse getCurrentResponse() { @@ -255,7 +301,7 @@ public GrailsParameterMap getParams() { */ public GrailsParameterMap getOriginalParams() { if (originalParams == null) { - originalParams = new GrailsParameterMap(getCurrentRequest()); + originalParams = new GrailsParameterMap(getRequest()); } return originalParams; } @@ -293,7 +339,7 @@ public void informParameterCreationListeners() { */ public GrailsHttpSession getSession() { if (session == null) { - session = new GrailsHttpSession(getCurrentRequest()); + session = new GrailsHttpSession(getRequest()); } return session; @@ -307,36 +353,36 @@ public GrailsApplicationAttributes getAttributes() { } public void setActionName(String actionName) { - getCurrentRequest().setAttribute(GrailsApplicationAttributes.ACTION_NAME_ATTRIBUTE, actionName); + getRequest().setAttribute(GrailsApplicationAttributes.ACTION_NAME_ATTRIBUTE, actionName); } public void setControllerName(String controllerName) { - getCurrentRequest().setAttribute(GrailsApplicationAttributes.CONTROLLER_NAME_ATTRIBUTE, controllerName); + getRequest().setAttribute(GrailsApplicationAttributes.CONTROLLER_NAME_ATTRIBUTE, controllerName); } public void setControllerNamespace(String controllerNamespace) { - getCurrentRequest().setAttribute(GrailsApplicationAttributes.CONTROLLER_NAMESPACE_ATTRIBUTE, controllerNamespace); + getRequest().setAttribute(GrailsApplicationAttributes.CONTROLLER_NAMESPACE_ATTRIBUTE, controllerNamespace); } /** * @return the actionName */ public String getActionName() { - return (String) getCurrentRequest().getAttribute(GrailsApplicationAttributes.ACTION_NAME_ATTRIBUTE); + return (String) getRequest().getAttribute(GrailsApplicationAttributes.ACTION_NAME_ATTRIBUTE); } /** * @return the controllerName */ public String getControllerName() { - return (String) getCurrentRequest().getAttribute(GrailsApplicationAttributes.CONTROLLER_NAME_ATTRIBUTE); + return (String) getRequest().getAttribute(GrailsApplicationAttributes.CONTROLLER_NAME_ATTRIBUTE); } /** * @return the controllerClass */ public GrailsControllerClass getControllerClass() { - HttpServletRequest currentRequest = getCurrentRequest(); + HttpServletRequest currentRequest = getRequest(); GrailsControllerClass controllerClass = (GrailsControllerClass) currentRequest.getAttribute(GrailsApplicationAttributes.GRAILS_CONTROLLER_CLASS); if (controllerClass == null) { Object controllerNameObject = currentRequest.getAttribute(GrailsApplicationAttributes.CONTROLLER_NAME_ATTRIBUTE); @@ -356,7 +402,7 @@ public GrailsControllerClass getControllerClass() { * @return the controllerNamespace */ public String getControllerNamespace() { - return (String) getCurrentRequest().getAttribute(GrailsApplicationAttributes.CONTROLLER_NAMESPACE_ATTRIBUTE); + return (String) getRequest().getAttribute(GrailsApplicationAttributes.CONTROLLER_NAMESPACE_ATTRIBUTE); } public void setRenderView(boolean renderView) { @@ -367,7 +413,7 @@ public void setRenderView(boolean renderView) { * @return true if the view for this GrailsWebRequest should be rendered */ public boolean isRenderView() { - final HttpServletRequest currentRequest = getCurrentRequest(); + final HttpServletRequest currentRequest = getRequest(); HttpServletResponse currentResponse = getCurrentResponse(); return renderView && !currentResponse.isCommitted() && @@ -420,7 +466,7 @@ public ApplicationContext getApplicationContext() { * @return The PropertyEditorRegistry */ public PropertyEditorRegistry getPropertyEditorRegistry() { - final HttpServletRequest servletRequest = getCurrentRequest(); + final HttpServletRequest servletRequest = getRequest(); PropertyEditorRegistry registry = (PropertyEditorRegistry) servletRequest.getAttribute(GrailsApplicationAttributes.PROPERTY_REGISTRY); if (registry == null) { registry = new PropertyEditorRegistrySupport(); @@ -463,7 +509,7 @@ public void setId(Object id) { public String getBaseUrl() { if (baseUrl == null) { - HttpServletRequest request = getCurrentRequest(); + HttpServletRequest request = getRequest(); String scheme = request.getScheme(); String forwardedScheme = request.getHeader("X-Forwarded-Proto"); StringBuilder sb = new StringBuilder(); diff --git a/grails-web-common/src/main/groovy/org/grails/web/util/GrailsApplicationAttributes.java b/grails-web-common/src/main/groovy/org/grails/web/util/GrailsApplicationAttributes.java index e4407f02fc9..bbf981da05d 100644 --- a/grails-web-common/src/main/groovy/org/grails/web/util/GrailsApplicationAttributes.java +++ b/grails-web-common/src/main/groovy/org/grails/web/util/GrailsApplicationAttributes.java @@ -33,6 +33,7 @@ import grails.web.mvc.FlashScope; import grails.web.pages.GroovyPagesUriService; import org.grails.gsp.ResourceAwareTemplateEngine; +import org.grails.web.servlet.view.CompositeViewResolver; /** * Defines the names of and methods to retrieve Grails specific request and servlet attributes. @@ -196,4 +197,18 @@ public interface GrailsApplicationAttributes extends ApplicationAttributes { * @return The MessageSource instance */ MessageSource getMessageSource(); + + /** + * Obtains the {@link CompositeViewResolver} used to resolve the views and templates a controller renders. + * + *

Declared as a default method so that implementations outside the framework keep compiling; the default + * resolves the bean on every call, which is what callers did for themselves before this method existed. + * {@link org.grails.web.servlet.DefaultGrailsApplicationAttributes} overrides it to resolve the bean once.

+ * + * @return The CompositeViewResolver instance + * @since 8.0 + */ + default CompositeViewResolver getCompositeViewResolver() { + return getApplicationContext().getBean(CompositeViewResolver.BEAN_NAME, CompositeViewResolver.class); + } } diff --git a/grails-web-common/src/main/groovy/org/grails/web/util/WebUtils.java b/grails-web-common/src/main/groovy/org/grails/web/util/WebUtils.java index 453f0018e73..75b9b32216c 100644 --- a/grails-web-common/src/main/groovy/org/grails/web/util/WebUtils.java +++ b/grails-web-common/src/main/groovy/org/grails/web/util/WebUtils.java @@ -44,6 +44,7 @@ import org.springframework.web.context.request.RequestContextHolder; import org.springframework.web.context.request.WebRequestInterceptor; import org.springframework.web.context.support.WebApplicationContextUtils; +import org.springframework.web.multipart.MultipartHttpServletRequest; import org.springframework.web.servlet.HandlerInterceptor; import org.springframework.web.servlet.View; import org.springframework.web.servlet.ViewResolver; @@ -82,6 +83,9 @@ public class WebUtils extends org.springframework.web.util.WebUtils { public static final String ASYNC_REQUEST_URI_ATTRIBUTE = "jakarta.servlet.async.request_uri"; public static final String SITEMESH2_PAGE_ATTRIBUTE = "__sitemesh__page"; + /** Published when a resolved multipart request cannot be reached by unwrapping. */ + public static final String MULTIPART_HTTP_SERVLET_REQUEST_ATTRIBUTE = MultipartHttpServletRequest.class.getName(); + public static final int SC_METHOD_NOT_ALLOWED = HttpServletResponse.SC_METHOD_NOT_ALLOWED; public static ViewResolver lookupViewResolver(ServletContext servletContext) { @@ -142,7 +146,7 @@ public static WebRequestInterceptor[] lookupWebRequestInterceptors(ServletContex * @param request The request */ public static String getRequestURIForGrailsDispatchURI(HttpServletRequest request) { - UrlPathHelper pathHelper = new UrlPathHelper(); + UrlPathHelper pathHelper = UrlPathHelper.defaultInstance; if (request.getRequestURI().endsWith(GRAILS_DISPATCH_EXTENSION)) { String path = pathHelper.getPathWithinApplication(request); if (path.startsWith(GRAILS_SERVLET_PATH)) { @@ -545,4 +549,21 @@ public static boolean isForwardOrInclude(HttpServletRequest request) { return isForward(request) || isInclude(request); } + /** + * Locate the resolved multipart request for the given request, if there is one. Normally found by + * unwrapping; when the {@code DispatcherServlet} resolved a request Grails had already bound, the + * wrapper sits above it instead, so {@link #MULTIPART_HTTP_SERVLET_REQUEST_ATTRIBUTE} is consulted too. + * + * @param request The request + * @return The resolved multipart request, or {@code null} when the request is not multipart + */ + public static MultipartHttpServletRequest resolveMultipartRequest(HttpServletRequest request) { + MultipartHttpServletRequest resolved = getNativeRequest(request, MultipartHttpServletRequest.class); + if (resolved != null) { + return resolved; + } + Object attribute = request.getAttribute(MULTIPART_HTTP_SERVLET_REQUEST_ATTRIBUTE); + return attribute instanceof MultipartHttpServletRequest multipartRequest ? multipartRequest : null; + } + } diff --git a/grails-web-common/src/test/groovy/grails/web/servlet/mvc/GrailsParameterMapTests.groovy b/grails-web-common/src/test/groovy/grails/web/servlet/mvc/GrailsParameterMapTests.groovy index a4340a841c2..9e5aeda69ac 100644 --- a/grails-web-common/src/test/groovy/grails/web/servlet/mvc/GrailsParameterMapTests.groovy +++ b/grails-web-common/src/test/groovy/grails/web/servlet/mvc/GrailsParameterMapTests.groovy @@ -20,15 +20,20 @@ package grails.web.servlet.mvc import jakarta.servlet.FilterChain import jakarta.servlet.http.HttpServletRequest +import jakarta.servlet.http.HttpServletRequestWrapper import org.junit.jupiter.api.Test import org.springframework.context.support.StaticMessageSource import org.springframework.mock.web.MockHttpServletRequest import org.springframework.mock.web.MockHttpServletResponse +import org.springframework.mock.web.MockMultipartFile +import org.springframework.mock.web.MockMultipartHttpServletRequest import org.springframework.mock.web.MockServletContext import org.springframework.web.context.request.RequestContextHolder import org.springframework.web.context.support.GenericWebApplicationContext import org.springframework.web.filter.FormContentFilter +import org.grails.web.util.WebUtils + import spock.lang.Issue import static org.junit.jupiter.api.Assertions.* @@ -38,6 +43,52 @@ class GrailsParameterMapTests { GrailsParameterMap theMap MockHttpServletRequest mockRequest = new MockHttpServletRequest() + @Test + void testMultipartFilesArePopulatedFromTheRequestItself() { + def request = multipartRequest() + request.addParameter('name', 'Dierk Koenig') + + theMap = new GrailsParameterMap(request) + + assertEquals 'Dierk Koenig', theMap.name + assertEquals 'test.txt', theMap.file.originalFilename + } + + @Test + void testMultipartFilesArePopulatedThroughLaterRequestWrappers() { + // The request Grails exposes is the outermost one, so files have to be discovered by unwrapping + // rather than by the request itself being a MultipartHttpServletRequest. + def request = new HttpServletRequestWrapper(new HttpServletRequestWrapper(multipartRequest())) + + theMap = new GrailsParameterMap(request) + + assertEquals 'test.txt', theMap.file.originalFilename + } + + @Test + void testMultipartFilesArePopulatedFromThePublishedAttribute() { + // The shape produced when the DispatcherServlet resolves a request Grails had already bound, + // leaving the multipart wrapper above it rather than below. + def request = new MockHttpServletRequest() + request.setAttribute(WebUtils.MULTIPART_HTTP_SERVLET_REQUEST_ATTRIBUTE, multipartRequest()) + + theMap = new GrailsParameterMap(request) + + assertEquals 'test.txt', theMap.file.originalFilename + } + + @Test + void testMultipleFilesUnderOneNameArePopulatedAsAList() { + def request = new MockMultipartHttpServletRequest() + request.contentType = 'multipart/form-data; boundary=test' + request.addFile(new MockMultipartFile('file', 'one.txt', 'text/plain', 'one'.bytes)) + request.addFile(new MockMultipartFile('file', 'two.txt', 'text/plain', 'two'.bytes)) + + theMap = new GrailsParameterMap(request) + + assertEquals(['one.txt', 'two.txt'], theMap.file*.originalFilename) + } + @Test void testSubmapViaArraySubscript() { mockRequest.addParameter("name", "Dierk Koenig") @@ -128,6 +179,13 @@ class GrailsParameterMapTests { assert 'two' == params.one } + private static MockMultipartHttpServletRequest multipartRequest() { + def request = new MockMultipartHttpServletRequest() + request.contentType = 'multipart/form-data; boundary=test' + request.addFile(new MockMultipartFile('file', 'test.txt', 'text/plain', 'content'.bytes)) + request + } + // Runs the request through Spring's FormContentFilter — the same filter Boot registers at runtime — // and returns the wrapped request, so the test exercises the real parsing path instead of a fallback. private static HttpServletRequest formFilteredRequest(String method, String body, String contentType) { diff --git a/grails-web-common/src/test/groovy/org/grails/web/util/WebUtilsSpec.groovy b/grails-web-common/src/test/groovy/org/grails/web/util/WebUtilsSpec.groovy index 28f717268c9..b261d6ca4d1 100644 --- a/grails-web-common/src/test/groovy/org/grails/web/util/WebUtilsSpec.groovy +++ b/grails-web-common/src/test/groovy/org/grails/web/util/WebUtilsSpec.groovy @@ -18,6 +18,12 @@ */ package org.grails.web.util +import jakarta.servlet.http.HttpServletRequestWrapper + +import org.springframework.mock.web.MockHttpServletRequest +import org.springframework.mock.web.MockMultipartFile +import org.springframework.mock.web.MockMultipartHttpServletRequest + import spock.lang.Issue import spock.lang.Specification @@ -40,4 +46,47 @@ class WebUtilsSpec extends Specification { tokens.find({ it == "name=sudhir-nimavat"}) != null tokens.find({ it == "address.zip=12345"}) != null } + + void 'resolveMultipartRequest returns the request itself when it is already a multipart request'() { + given: + def request = multipartRequest() + + expect: + WebUtils.resolveMultipartRequest(request).is(request) + } + + void 'resolveMultipartRequest unwraps a multipart request nested inside later request wrappers'() { + given: 'the wrapper chain a request picks up from filters running after multipart resolution' + def multipartRequest = multipartRequest() + def outerRequest = new HttpServletRequestWrapper(new HttpServletRequestWrapper(multipartRequest)) + + when: + def resolved = WebUtils.resolveMultipartRequest(outerRequest) + + then: 'the multipart request is found without the outer wrappers being discarded' + resolved.is(multipartRequest) + resolved.getFile('file').originalFilename == 'test.txt' + } + + void 'resolveMultipartRequest falls back to the published attribute when the multipart request cannot be unwrapped'() { + given: 'the shape produced when the DispatcherServlet resolves a request Grails already bound' + def request = new MockHttpServletRequest() + def multipartRequest = multipartRequest() + request.setAttribute(WebUtils.MULTIPART_HTTP_SERVLET_REQUEST_ATTRIBUTE, multipartRequest) + + expect: + WebUtils.resolveMultipartRequest(request).is(multipartRequest) + } + + void 'resolveMultipartRequest returns null for an ordinary request'() { + expect: + WebUtils.resolveMultipartRequest(new MockHttpServletRequest()) == null + } + + private static MockMultipartHttpServletRequest multipartRequest() { + def request = new MockMultipartHttpServletRequest() + request.contentType = 'multipart/form-data; boundary=test' + request.addFile(new MockMultipartFile('file', 'test.txt', 'text/plain', 'content'.bytes)) + request + } } diff --git a/grails-web-core/src/main/groovy/org/grails/web/servlet/HttpServletRequestExtension.groovy b/grails-web-core/src/main/groovy/org/grails/web/servlet/HttpServletRequestExtension.groovy index 347172dc7e4..d817361e029 100644 --- a/grails-web-core/src/main/groovy/org/grails/web/servlet/HttpServletRequestExtension.groovy +++ b/grails-web-core/src/main/groovy/org/grails/web/servlet/HttpServletRequestExtension.groovy @@ -22,6 +22,9 @@ import groovy.transform.CompileStatic import org.apache.grails.core.internal.util.TypeConverters import org.springframework.util.ClassUtils +import org.springframework.util.MultiValueMap +import org.springframework.web.multipart.MultipartFile +import org.springframework.web.multipart.MultipartHttpServletRequest import jakarta.servlet.http.HttpServletRequest @@ -43,6 +46,47 @@ class HttpServletRequestExtension { WebUtils.getForwardURI(request) } + /** + * File upload accessors, delegating to the resolved multipart request found in this request's wrapper + * chain. Each throws {@link IllegalStateException} when the request is not a resolved multipart request, + * matching how these methods previously failed when the request was not a {@code MultipartHttpServletRequest}. + */ + static MultipartFile getFile(HttpServletRequest request, String name) { + multipartRequest(request).getFile(name) + } + + static List getFiles(HttpServletRequest request, String name) { + multipartRequest(request).getFiles(name) + } + + static Iterator getFileNames(HttpServletRequest request) { + multipartRequest(request).fileNames + } + + static Map getFileMap(HttpServletRequest request) { + multipartRequest(request).fileMap + } + + static MultiValueMap getMultiFileMap(HttpServletRequest request) { + multipartRequest(request).multiFileMap + } + + static String getMultipartContentType(HttpServletRequest request, String name) { + multipartRequest(request).getMultipartContentType(name) + } + + private static MultipartHttpServletRequest multipartRequest(HttpServletRequest request) { + MultipartHttpServletRequest multipartRequest = WebUtils.resolveMultipartRequest(request) + if (multipartRequest == null) { + throw new IllegalStateException( + "Not a resolved multipart request. Content-Type is [${request.contentType}]. " + + 'If this is a file upload, check that multipart support is enabled ' + + '(spring.servlet.multipart.enabled) and that any application-supplied MultipartFilter ' + + 'is ordered before the Grails request filter.') + } + multipartRequest + } + static getProperty(HttpServletRequest request, String name) { def mp = request.getClass().metaClass.getMetaProperty(name) mp ? mp.getProperty(request) : request.getAttribute(name) diff --git a/grails-web-core/src/test/groovy/org/grails/web/servlet/HttpServletRequestExtensionSpec.groovy b/grails-web-core/src/test/groovy/org/grails/web/servlet/HttpServletRequestExtensionSpec.groovy index 80f84ea8158..6ca24d79eae 100644 --- a/grails-web-core/src/test/groovy/org/grails/web/servlet/HttpServletRequestExtensionSpec.groovy +++ b/grails-web-core/src/test/groovy/org/grails/web/servlet/HttpServletRequestExtensionSpec.groovy @@ -21,8 +21,12 @@ package org.grails.web.servlet import groovy.transform.CompileStatic import jakarta.servlet.http.HttpServletRequest +import jakarta.servlet.http.HttpServletRequestWrapper import org.springframework.mock.web.MockHttpServletRequest +import org.springframework.mock.web.MockMultipartFile +import org.springframework.mock.web.MockMultipartHttpServletRequest +import org.springframework.web.multipart.MultipartFile import net.bytebuddy.ByteBuddy @@ -149,5 +153,57 @@ class HttpServletRequestExtensionSpec extends Specification { static String readName(HttpServletRequest request) { request.string('user', 'anonymous') } + + static MultipartFile readFile(HttpServletRequest request, String name) { + request.getFile(name) + } + } + + void "Test multipart accessors read through the request wrapper chain"() { + given: 'a multipart request behind the wrappers later filters contribute' + HttpServletRequest request = new HttpServletRequestWrapper(multipartRequest()) + + expect: + request.getFile('file').originalFilename == 'test.txt' + request.getFiles('file').size() == 1 + request.getFileMap().keySet() == ['file'] as Set + request.getMultiFileMap().get('file').size() == 1 + request.getFileNames().toList() == ['file'] + request.getMultipartContentType('file') == 'text/plain' + } + + void "Test getFile is resolved by the static compiler"() { + given: + HttpServletRequest request = new HttpServletRequestWrapper(multipartRequest()) + + expect: 'statically compiled callers get the extension method, not a dynamic dispatch failure' + StaticCaller.readFile(request, 'file').originalFilename == 'test.txt' + } + + void "Test getFile returns null for a part that was not submitted"() { + given: + HttpServletRequest request = multipartRequest() + + expect: 'matching Spring, an absent part is null rather than an error' + request.getFile('missing') == null + } + + void "Test multipart accessors fail loudly when the request is not a resolved multipart request"() { + given: + HttpServletRequest request = new MockHttpServletRequest() + + when: + request.getFile('file') + + then: 'a diagnostic is raised rather than a silent null' + IllegalStateException e = thrown() + e.message.contains('Not a resolved multipart request') + } + + private static MockMultipartHttpServletRequest multipartRequest() { + def request = new MockMultipartHttpServletRequest() + request.contentType = 'multipart/form-data; boundary=test' + request.addFile(new MockMultipartFile('file', 'test.txt', 'text/plain', 'content'.bytes)) + request } } diff --git a/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java b/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java index e5adb5d30f8..2f773b2e864 100644 --- a/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java +++ b/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java @@ -88,6 +88,21 @@ public class DataBindingUtils { private static final class NoBindingIncludeList extends ArrayList { } + /** + * The beans used by data binding for the most recently seen {@link ApplicationContext}. Data binding happens on + * every request and each bind previously repeated the same {@code containsBean}/{@code getBean} lookups for + * singletons which never change for the life of a context. + *

+ * The cache holds a single entry on purpose. An application has one main context, so a single entry is enough to + * remove the repeated lookups, while a map keyed by context would retain every context ever seen - which would + * leak whole bean factories in test runs and in development where contexts are replaced. Whenever the context + * differs from the cached one, the entry is replaced and the previous context becomes eligible for collection. + *

+ * The field is volatile so that request threads see a consistent, fully constructed entry. A race merely results + * in two threads resolving the same singletons, which is harmless. + */ + private static volatile ContextBoundBeans contextBoundBeans; + /** * Associations both sides of any bidirectional relationships found in the object and source map to bind * @@ -471,15 +486,12 @@ public static BindingResult bindObjectToDomainInstance(PersistentEntity entity, * @since 2.3 */ public static void bindToCollection(final Class targetType, final Collection collectionToPopulate, final CollectionDataBindingSource collectionBindingSource) throws InstantiationException, IllegalAccessException { - final GrailsApplication application = Holders.findApplication(); - PersistentEntity entity = null; - if (application != null) { - try { - entity = application.getMappingContext().getPersistentEntity(targetType.getName()); - } catch (GrailsConfigurationException e) { - //no-op - } - } + bindToCollection(targetType, collectionToPopulate, collectionBindingSource, Holders.findApplication()); + } + + private static void bindToCollection(final Class targetType, final Collection collectionToPopulate, + final CollectionDataBindingSource collectionBindingSource, final GrailsApplication application) throws InstantiationException, IllegalAccessException { + final PersistentEntity entity = findPersistentEntity(application, targetType.getName()); final List dataBindingSources = collectionBindingSource.getDataBindingSources(); for (final DataBindingSource dataBindingSource : dataBindingSources) { final T newObject; @@ -491,7 +503,7 @@ public static void bindToCollection(final Class targetType, final Collect ); } - bindObjectToDomainInstance(entity, newObject, dataBindingSource, getBindingIncludeList(newObject), Collections.emptyList(), null); + bindObjectToDomainInstance(entity, newObject, dataBindingSource, getBindingIncludeList(newObject), Collections.emptyList(), null, false, application); collectionToPopulate.add(newObject); } } @@ -499,7 +511,18 @@ public static void bindToCollection(final Class targetType, final Collect public static void bindToCollection(final Class targetType, final Collection collectionToPopulate, final ServletRequest request) throws InstantiationException, IllegalAccessException { final GrailsApplication grailsApplication = Holders.findApplication(); final CollectionDataBindingSource collectionDataBindingSource = createCollectionDataBindingSource(grailsApplication, targetType, request); - bindToCollection(targetType, collectionToPopulate, collectionDataBindingSource); + bindToCollection(targetType, collectionToPopulate, collectionDataBindingSource, grailsApplication); + } + + private static PersistentEntity findPersistentEntity(final GrailsApplication application, final String className) { + if (application != null) { + try { + return application.getMappingContext().getPersistentEntity(className); + } catch (GrailsConfigurationException e) { + //no-op + } + } + return null; } /** @@ -543,16 +566,10 @@ public static BindingResult bindObjectToInstance(Object object, Object source, L else if (include.isEmpty() && !GrailsWebDataBinder.isBindAllIncludeList(include)) { include = Collections.singletonList(DefaultASTDatabindingHelper.NO_BINDABLE_PROPERTIES); } - GrailsApplication application = Holders.findApplication(); - PersistentEntity entity = null; - if (application != null) { - try { - entity = application.getMappingContext().getPersistentEntity(object.getClass().getName()); - } catch (GrailsConfigurationException e) { - //no-op - } - } - return bindObjectToDomainInstance(entity, object, source, include, exclude, filter, clearMissing && explicitInclude); + final GrailsApplication application = Holders.findApplication(); + final PersistentEntity entity = findPersistentEntity(application, object.getClass().getName()); + return bindObjectToDomainInstance(entity, object, source, include, exclude, filter, + clearMissing && explicitInclude, application); } /** @@ -599,9 +616,19 @@ else if (include.isEmpty() && !GrailsWebDataBinder.isBindAllIncludeList(include) * binding errors already stored on the target and null-clearing failures, or null when no such errors occur. * Clearing runs after normal binder listeners have completed and does not emit listener callbacks. */ - @SuppressWarnings("unchecked") public static BindingResult bindObjectToDomainInstance(PersistentEntity entity, Object object, Object source, List include, List exclude, String filter, boolean clearMissing) { + return bindObjectToDomainInstance(entity, object, source, include, exclude, filter, clearMissing, Holders.findApplication()); + } + + /** + * Binds using an already resolved {@link GrailsApplication}. Resolving the application is not a simple field + * read, it walks the registered discovery strategies and ends in a bean lookup, so callers which have already + * resolved it pass it down instead of resolving it again for the same bind. + */ + @SuppressWarnings("unchecked") + private static BindingResult bindObjectToDomainInstance(PersistentEntity entity, Object object, + Object source, List include, List exclude, String filter, boolean clearMissing, GrailsApplication grailsApplication) { boolean explicitInclude = include != null; if (include == null) { if (exclude == null || isDenyByDefaultEnabled()) { @@ -614,7 +641,6 @@ else if (include.isEmpty() && !GrailsWebDataBinder.isBindAllIncludeList(include) include = Collections.singletonList(DefaultASTDatabindingHelper.NO_BINDABLE_PROPERTIES); } BindingResult bindingResult = null; - GrailsApplication grailsApplication = Holders.findApplication(); try { final DataBindingSource bindingSource = createDataBindingSource(grailsApplication, object.getClass(), source); @@ -691,15 +717,8 @@ protected static String[] getMessageCodes(String messageCode, } public static DataBindingSourceRegistry getDataBindingSourceRegistry(GrailsApplication grailsApplication) { - DataBindingSourceRegistry registry = null; - if (grailsApplication != null) { - ApplicationContext context = grailsApplication.getMainContext(); - if (context != null) { - if (context.containsBean(DataBindingSourceRegistry.BEAN_NAME)) { - registry = context.getBean(DataBindingSourceRegistry.BEAN_NAME, DataBindingSourceRegistry.class); - } - } - } + final ContextBoundBeans beans = getContextBoundBeans(grailsApplication); + DataBindingSourceRegistry registry = beans == null ? null : beans.getDataBindingSourceRegistry(); if (registry == null) { registry = new DefaultDataBindingSourceRegistry(); } @@ -727,16 +746,8 @@ public static MimeType getMimeType(GrailsApplication grailsApplication, public static MimeTypeResolver getMimeTypeResolver( GrailsApplication grailsApplication) { - MimeTypeResolver mimeTypeResolver = null; - if (grailsApplication != null) { - ApplicationContext context = grailsApplication.getMainContext(); - if (context != null) { - if (context.containsBean(MimeTypeResolver.BEAN_NAME)) { - mimeTypeResolver = context.getBean(MimeTypeResolver.BEAN_NAME, MimeTypeResolver.class); - } - } - } - return mimeTypeResolver; + final ContextBoundBeans beans = getContextBoundBeans(grailsApplication); + return beans == null ? null : beans.getMimeTypeResolver(); } public static MimeType resolveMimeType(Object bindingSource, MimeTypeResolver mimeTypeResolver) { @@ -744,13 +755,8 @@ public static MimeType resolveMimeType(Object bindingSource, MimeTypeResolver mi } private static DataBinder getGrailsWebDataBinder(final GrailsApplication grailsApplication) { - DataBinder dataBinder = null; - if (grailsApplication != null) { - final ApplicationContext mainContext = grailsApplication.getMainContext(); - if (mainContext != null && mainContext.containsBean(DATA_BINDER_BEAN_NAME)) { - dataBinder = mainContext.getBean(DATA_BINDER_BEAN_NAME, DataBinder.class); - } - } + final ContextBoundBeans beans = getContextBoundBeans(grailsApplication); + DataBinder dataBinder = beans == null ? null : beans.getDataBinder(); if (dataBinder == null) { // this should really never happen in the running app as the binder // should always be found in the context @@ -759,6 +765,26 @@ private static DataBinder getGrailsWebDataBinder(final GrailsApplication grailsA return dataBinder; } + /** + * @param grailsApplication the application to resolve the main context from, may be null + * @return the cached beans of the application's main context or null if there is no context to look beans up in + */ + private static ContextBoundBeans getContextBoundBeans(final GrailsApplication grailsApplication) { + if (grailsApplication == null) { + return null; + } + final ApplicationContext context = grailsApplication.getMainContext(); + if (context == null) { + return null; + } + ContextBoundBeans beans = contextBoundBeans; + if (beans == null || beans.applicationContext != context) { + beans = new ContextBoundBeans(context); + contextBoundBeans = beans; + } + return beans; + } + @SuppressWarnings("unchecked") public static Map convertPotentialGStrings(Map args) { Map newArgs = new HashMap(args.size()); @@ -774,4 +800,65 @@ private static Object unwrapGString(Object value) { } return value; } + + /** + * The data binding beans of a single {@link ApplicationContext}, resolved on first use and kept for as long as + * that context is the one being bound against. + */ + private static final class ContextBoundBeans { + + private final ApplicationContext applicationContext; + private final CachedBean dataBindingSourceRegistry = + new CachedBean<>(DataBindingSourceRegistry.BEAN_NAME, DataBindingSourceRegistry.class); + private final CachedBean mimeTypeResolver = + new CachedBean<>(MimeTypeResolver.BEAN_NAME, MimeTypeResolver.class); + private final CachedBean dataBinder = + new CachedBean<>(DATA_BINDER_BEAN_NAME, DataBinder.class); + + private ContextBoundBeans(ApplicationContext applicationContext) { + this.applicationContext = applicationContext; + } + + private DataBindingSourceRegistry getDataBindingSourceRegistry() { + return dataBindingSourceRegistry.get(applicationContext); + } + + private MimeTypeResolver getMimeTypeResolver() { + return mimeTypeResolver.get(applicationContext); + } + + private DataBinder getDataBinder() { + return dataBinder.get(applicationContext); + } + } + + /** + * A singleton bean looked up at most once per {@link ApplicationContext}. Each bean is resolved lazily so that + * asking for one of them never triggers the creation of another. + *

+ * Both fields are volatile and {@code resolved} is written last, so a thread which sees {@code resolved} also + * sees the bean it was resolved to. Two threads racing simply look the same singleton up twice. + * + * @param the type of the bean + */ + private static final class CachedBean { + + private final String beanName; + private final Class beanType; + private volatile T bean; + private volatile boolean resolved; + + private CachedBean(String beanName, Class beanType) { + this.beanName = beanName; + this.beanType = beanType; + } + + private T get(ApplicationContext applicationContext) { + if (!resolved) { + bean = applicationContext.containsBean(beanName) ? applicationContext.getBean(beanName, beanType) : null; + resolved = true; + } + return bean; + } + } } diff --git a/grails-web-databinding/src/test/groovy/grails/web/databinding/DataBindingUtilsSpec.groovy b/grails-web-databinding/src/test/groovy/grails/web/databinding/DataBindingUtilsSpec.groovy new file mode 100644 index 00000000000..a812426fc40 --- /dev/null +++ b/grails-web-databinding/src/test/groovy/grails/web/databinding/DataBindingUtilsSpec.groovy @@ -0,0 +1,248 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package grails.web.databinding + +import spock.lang.Specification + +import org.springframework.context.ApplicationContext +import org.springframework.context.support.StaticApplicationContext + +import grails.core.GrailsApplication +import grails.databinding.CollectionDataBindingSource +import grails.databinding.DataBinder +import grails.databinding.SimpleMapDataBindingSource +import grails.util.Holders +import grails.web.mime.MimeTypeResolver +import org.grails.datastore.mapping.model.MappingContext +import org.grails.web.databinding.bindingsource.DataBindingSourceRegistry +import org.grails.web.databinding.bindingsource.DefaultDataBindingSourceRegistry + +class DataBindingUtilsSpec extends Specification { + + private GrailsApplication previousApplication + + void setup() { + previousApplication = Holders.findApplication() + } + + void cleanup() { + Holders.setGrailsApplication(previousApplication) + } + + void 'test binding a class which does not declare a data binding whitelist'() { + given: + def command = new NoWhitelistCommand() + + when: + def bindingResult = DataBindingUtils.bindObjectToInstance(command, [name: 'Grails', version: '8']) + + then: 'the absence of a whitelist places no restriction on the binding' + bindingResult == null + command.name == 'Grails' + command.version == '8' + + when: 'the same class is bound again' + def secondCommand = new NoWhitelistCommand() + DataBindingUtils.bindObjectToInstance(secondCommand, [name: 'Apache Grails', version: '8.0']) + + then: + secondCommand.name == 'Apache Grails' + secondCommand.version == '8.0' + } + + void 'test the whitelist field is read only once per class'() { + when: 'a class whose whitelist field does not hold a list is bound' + def command = new MutableWhitelistCommand() + DataBindingUtils.bindObjectToInstance(command, [name: 'Grails', version: '8']) + + then: 'there is no usable include list, so every property is bound' + command.name == 'Grails' + command.version == '8' + + when: 'the whitelist field is given a usable value and another instance is bound' + MutableWhitelistCommand.$defaultDatabindingWhiteList = ['name'] + def secondCommand = new MutableWhitelistCommand() + DataBindingUtils.bindObjectToInstance(secondCommand, [name: 'Grails', version: '8']) + + then: 'the cached include list is used, the field is not read a second time' + secondCommand.name == 'Grails' + secondCommand.version == '8' + } + + void 'test a declared whitelist restricts the bound properties'() { + given: + def command = new WhitelistedCommand() + + when: + DataBindingUtils.bindObjectToInstance(command, [name: 'Grails', version: '8']) + + then: + command.name == 'Grails' + command.version == null + } + + void 'test a whitelist declared by a super class also restricts a sub class'() { + given: + def command = new SubclassOfWhitelistedCommand() + + when: + DataBindingUtils.bindObjectToInstance(command, [name: 'Grails', version: '8']) + + then: 'the inherited whitelist applies to the sub class as well' + command.name == 'Grails' + command.version == null + } + + void 'test binding a collection'() { + given: + def collectionBindingSource = Stub(CollectionDataBindingSource) { + getDataBindingSources() >> [ + new SimpleMapDataBindingSource([name: 'Grails', version: '8']), + new SimpleMapDataBindingSource([name: 'Groovy', version: '5']) + ] + } + def commands = [] + + when: + DataBindingUtils.bindToCollection(NoWhitelistCommand, commands, collectionBindingSource) + + then: + commands.size() == 2 + commands[0].name == 'Grails' + commands[0].version == '8' + commands[1].name == 'Groovy' + commands[1].version == '5' + } + + void 'test the beans of the current application context are used after the context is replaced'() { + given: + def firstRegistry = new DefaultDataBindingSourceRegistry() + def firstResolver = Stub(MimeTypeResolver) + def firstContext = createContext([ + (DataBindingSourceRegistry.BEAN_NAME): firstRegistry, + (MimeTypeResolver.BEAN_NAME): firstResolver + ]) + + and: + def secondRegistry = new DefaultDataBindingSourceRegistry() + def secondResolver = Stub(MimeTypeResolver) + def secondContext = createContext([ + (DataBindingSourceRegistry.BEAN_NAME): secondRegistry, + (MimeTypeResolver.BEAN_NAME): secondResolver + ]) + + and: + ApplicationContext mainContext = firstContext + def application = Stub(GrailsApplication) { + getMainContext() >> { mainContext } + } + + expect: 'the beans of the first context are used' + DataBindingUtils.getDataBindingSourceRegistry(application).is(firstRegistry) + DataBindingUtils.getDataBindingSourceRegistry(application).is(firstRegistry) + DataBindingUtils.getMimeTypeResolver(application).is(firstResolver) + + when: 'the application context is replaced' + mainContext = secondContext + + then: 'the beans of the new context are used' + DataBindingUtils.getDataBindingSourceRegistry(application).is(secondRegistry) + DataBindingUtils.getMimeTypeResolver(application).is(secondResolver) + + when: 'the original context is used again' + mainContext = firstContext + + then: + DataBindingUtils.getDataBindingSourceRegistry(application).is(firstRegistry) + DataBindingUtils.getMimeTypeResolver(application).is(firstResolver) + + cleanup: + firstContext.close() + secondContext.close() + } + + void 'test the data binder of the current application context is used after the context is replaced'() { + given: + def firstBinder = Mock(DataBinder) + def firstContext = createContext([(DataBindingUtils.DATA_BINDER_BEAN_NAME): firstBinder]) + def secondBinder = Mock(DataBinder) + def secondContext = createContext([(DataBindingUtils.DATA_BINDER_BEAN_NAME): secondBinder]) + + and: + ApplicationContext mainContext = firstContext + def application = Stub(GrailsApplication) { + getMainContext() >> { mainContext } + getMappingContext() >> Stub(MappingContext) + } + Holders.setGrailsApplication(application) + + when: + DataBindingUtils.bindObjectToInstance(new NoWhitelistCommand(), [name: 'Grails']) + + then: + 1 * firstBinder.bind(_, _, _, _, _) + 0 * secondBinder.bind(_, _, _, _, _) + + when: 'the application context is replaced' + mainContext = secondContext + DataBindingUtils.bindObjectToInstance(new NoWhitelistCommand(), [name: 'Grails']) + + then: 'the binder of the new context is used' + 1 * secondBinder.bind(_, _, _, _, _) + 0 * firstBinder.bind(_, _, _, _, _) + + cleanup: + firstContext.close() + secondContext.close() + } + + private static StaticApplicationContext createContext(Map beans) { + def context = new StaticApplicationContext() + context.refresh() + beans.each { String beanName, Object bean -> + context.beanFactory.registerSingleton(beanName, bean) + } + return context + } +} + +class NoWhitelistCommand { + + String name + String version +} + +class MutableWhitelistCommand { + + public static Object $defaultDatabindingWhiteList = 'not a list' + + String name + String version +} + +class WhitelistedCommand { + + public static final List $defaultDatabindingWhiteList = ['name'] + + String name + String version +} + +class SubclassOfWhitelistedCommand extends WhitelistedCommand { +} diff --git a/grails-web-mvc/src/main/groovy/org/grails/web/errors/GrailsExceptionResolver.java b/grails-web-mvc/src/main/groovy/org/grails/web/errors/GrailsExceptionResolver.java index a6189afa4d2..d8dc878a390 100644 --- a/grails-web-mvc/src/main/groovy/org/grails/web/errors/GrailsExceptionResolver.java +++ b/grails-web-mvc/src/main/groovy/org/grails/web/errors/GrailsExceptionResolver.java @@ -34,8 +34,8 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.beans.BeanUtils; import org.springframework.beans.BeansException; @@ -75,7 +75,7 @@ public class GrailsExceptionResolver extends SimpleMappingExceptionResolver impl public static final String EXCEPTION_ATTRIBUTE = WebUtils.EXCEPTION_ATTRIBUTE; - protected static final Log LOG = LogFactory.getLog(GrailsExceptionResolver.class); + protected static final Logger LOG = LoggerFactory.getLogger(GrailsExceptionResolver.class); protected static final String LINE_SEPARATOR = System.getProperty("line.separator"); protected ServletContext servletContext; @@ -225,7 +225,7 @@ else if (info != null && info.getControllerName() != null) { return mv; } catch (Exception e) { - LOG.error("Unable to render errors view: " + e.getMessage(), e); + LOG.error("Unable to render errors view: {}", e.getMessage(), e); throw new GrailsRuntimeException(e); } } @@ -235,10 +235,8 @@ protected void forwardRequest(UrlMappingInfo info, HttpServletRequest request, H info.configure(WebUtils.retrieveGrailsWebRequest()); String forwardUrl = UrlMappingUtils.forwardRequestForUrlMappingInfo( request, response, info, mv.getModel(), true); - if (LOG.isDebugEnabled()) { - LOG.debug("Matched URI [" + uri + "] to URL mapping [" + info + - "], forwarding to [" + forwardUrl + "] with response [" + response.getClass() + "]"); - } + LOG.debug("Matched URI [{}] to URL mapping [{}], forwarding to [{}] with response [{}]", + uri, info, forwardUrl, response.getClass()); } protected String determineUri(HttpServletRequest request) { diff --git a/grails-web-mvc/src/main/groovy/org/grails/web/servlet/mvc/GrailsDispatcherServlet.groovy b/grails-web-mvc/src/main/groovy/org/grails/web/servlet/mvc/GrailsDispatcherServlet.groovy index 5c5331e1bf4..2021d955338 100644 --- a/grails-web-mvc/src/main/groovy/org/grails/web/servlet/mvc/GrailsDispatcherServlet.groovy +++ b/grails-web-mvc/src/main/groovy/org/grails/web/servlet/mvc/GrailsDispatcherServlet.groovy @@ -85,10 +85,12 @@ class GrailsDispatcherServlet extends DispatcherServlet implements ServletContex if (shouldProcessMultiPart) { HttpServletRequest processedRequest = super.checkMultipart(request) if (!processedRequest.is(request)) { - def webRequest = GrailsWebRequest.lookup(request) - if (webRequest != null) { - webRequest.multipartRequest = processedRequest - } + // The GrailsWebRequest was bound by GrailsWebRequestFilter, so the request it holds sits + // below this wrapper and cannot reach it by unwrapping. Publish it so params and + // request.getFile(..) can find it, then hand it to the dispatch like Spring MVC expects. + request.setAttribute(WebUtils.MULTIPART_HTTP_SERVLET_REQUEST_ATTRIBUTE, processedRequest) + GrailsWebRequest.lookup(request)?.multipartRequestResolved() + return processedRequest } } return request diff --git a/grails-web-mvc/src/main/groovy/org/grails/web/servlet/mvc/GrailsWebRequestFilter.java b/grails-web-mvc/src/main/groovy/org/grails/web/servlet/mvc/GrailsWebRequestFilter.java index 820ee1a00bc..11b23e6fde6 100644 --- a/grails-web-mvc/src/main/groovy/org/grails/web/servlet/mvc/GrailsWebRequestFilter.java +++ b/grails-web-mvc/src/main/groovy/org/grails/web/servlet/mvc/GrailsWebRequestFilter.java @@ -29,6 +29,7 @@ import org.springframework.beans.BeansException; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; +import org.springframework.context.i18n.LocaleContext; import org.springframework.context.i18n.LocaleContextHolder; import org.springframework.web.filter.RequestContextFilter; @@ -52,6 +53,9 @@ public class GrailsWebRequestFilter extends RequestContextFilter implements Appl protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { + // Save and restore rather than clear, so a LocaleContext established by a filter outside Grails + // survives this filter, the way Spring's own RequestContextFilter behaves. + LocaleContext previousLocaleContext = LocaleContextHolder.getLocaleContext(); LocaleContextHolder.setLocale(request.getLocale()); response = new OutputAwareHttpServletResponse(response); @@ -88,10 +92,14 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse } } else { - WebUtils.clearGrailsWebRequest(); - LocaleContextHolder.setLocale(null); } + + // Restored on every invocation, not just the outermost one: the locale is set + // unconditionally above, so an include or forward would otherwise leave the enclosing + // request with the locale it installed - and with a plain SimpleLocaleContext in place of + // any TimeZoneAwareLocaleContext that was there. + LocaleContextHolder.setLocaleContext(previousLocaleContext); if (logger.isDebugEnabled()) { logger.debug("Cleared Grails thread-bound request context: " + request); } diff --git a/grails-web-mvc/src/test/groovy/org/grails/web/servlet/mvc/GrailsDispatcherServletSpec.groovy b/grails-web-mvc/src/test/groovy/org/grails/web/servlet/mvc/GrailsDispatcherServletSpec.groovy new file mode 100644 index 00000000000..9604d94b14d --- /dev/null +++ b/grails-web-mvc/src/test/groovy/org/grails/web/servlet/mvc/GrailsDispatcherServletSpec.groovy @@ -0,0 +1,136 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.web.servlet.mvc + +import jakarta.servlet.http.HttpServletRequest + +import org.springframework.mock.web.MockHttpServletRequest +import org.springframework.mock.web.MockMultipartFile +import org.springframework.mock.web.MockMultipartHttpServletRequest +import org.springframework.mock.web.MockServletConfig +import org.springframework.mock.web.MockServletContext +import org.springframework.web.context.WebApplicationContext +import org.springframework.web.context.support.StaticWebApplicationContext +import org.springframework.web.multipart.MultipartResolver + +import org.grails.web.util.WebUtils + +import spock.lang.Specification + +class GrailsDispatcherServletSpec extends Specification { + + MockServletContext servletContext = new MockServletContext() + MockMultipartHttpServletRequest resolvedRequest = new MockMultipartHttpServletRequest().tap { + it.addFile(new MockMultipartFile('file', 'test.txt', 'text/plain', 'content'.bytes)) + } + MultipartResolver multipartResolver = Mock(MultipartResolver) + + void 'checkMultipart hands the resolved request to the dispatch and publishes it for application code'() { + given: + def servlet = dispatcherServlet() + def request = multipartRequest() + + when: + def processedRequest = servlet.callCheckMultipart(request) + + then: + 1 * multipartResolver.isMultipart(request) >> true + 1 * multipartResolver.resolveMultipart(request) >> resolvedRequest + + and: 'the dispatch runs against the resolved request, as Spring MVC expects' + processedRequest.is(resolvedRequest) + + and: 'and it is reachable from the request Grails bound before the DispatcherServlet ran' + WebUtils.resolveMultipartRequest(request).is(resolvedRequest) + } + + void 'checkMultipart leaves an ordinary request untouched'() { + given: + def servlet = dispatcherServlet() + def request = new MockHttpServletRequest() + + when: + def processedRequest = servlet.callCheckMultipart(request) + + then: + 1 * multipartResolver.isMultipart(request) >> false + 0 * multipartResolver.resolveMultipart(_) + + and: + processedRequest.is(request) + WebUtils.resolveMultipartRequest(request) == null + } + + void 'checkMultipart does not resolve during an error dispatch'() { + given: + def servlet = dispatcherServlet() + def request = multipartRequest() + request.setAttribute(WebUtils.ERROR_STATUS_CODE_ATTRIBUTE, 500) + + when: + def processedRequest = servlet.callCheckMultipart(request) + + then: 'the parts belong to the original dispatch and must not be resolved a second time' + 0 * multipartResolver.resolveMultipart(_) + processedRequest.is(request) + } + + void 'checkMultipart does not resolve during a forward or include'() { + given: + def servlet = dispatcherServlet() + def request = multipartRequest() + request.setAttribute(attribute, '/target') + + when: + def processedRequest = servlet.callCheckMultipart(request) + + then: + 0 * multipartResolver.resolveMultipart(_) + processedRequest.is(request) + + where: + attribute << [WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE, WebUtils.INCLUDE_REQUEST_URI_ATTRIBUTE] + } + + private MockHttpServletRequest multipartRequest() { + new MockHttpServletRequest(contentType: 'multipart/form-data; boundary=test', method: 'POST') + } + + private TestGrailsDispatcherServlet dispatcherServlet() { + def context = new StaticWebApplicationContext() + context.servletContext = servletContext + context.beanFactory.registerSingleton( + org.springframework.web.servlet.DispatcherServlet.MULTIPART_RESOLVER_BEAN_NAME, multipartResolver) + context.refresh() + def servlet = new TestGrailsDispatcherServlet(context) + servlet.init(new MockServletConfig(servletContext, 'grails')) + servlet + } + + private static class TestGrailsDispatcherServlet extends GrailsDispatcherServlet { + + TestGrailsDispatcherServlet(WebApplicationContext webApplicationContext) { + super(webApplicationContext) + } + + HttpServletRequest callCheckMultipart(HttpServletRequest request) { + checkMultipart(request) + } + } +} diff --git a/grails-web-mvc/src/test/groovy/org/grails/web/servlet/mvc/GrailsWebRequestFilterSpec.groovy b/grails-web-mvc/src/test/groovy/org/grails/web/servlet/mvc/GrailsWebRequestFilterSpec.groovy new file mode 100644 index 00000000000..83065c4299d --- /dev/null +++ b/grails-web-mvc/src/test/groovy/org/grails/web/servlet/mvc/GrailsWebRequestFilterSpec.groovy @@ -0,0 +1,102 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.web.servlet.mvc + +import jakarta.servlet.FilterChain +import jakarta.servlet.http.HttpServletRequest +import jakarta.servlet.http.HttpServletResponse + +import org.springframework.context.i18n.LocaleContextHolder +import org.springframework.context.i18n.SimpleTimeZoneAwareLocaleContext +import org.springframework.mock.web.MockHttpServletRequest +import org.springframework.mock.web.MockHttpServletResponse +import org.springframework.mock.web.MockServletContext +import org.springframework.web.context.request.RequestContextHolder +import org.springframework.web.context.support.StaticWebApplicationContext + +import org.grails.web.util.WebUtils + +import spock.lang.Specification + +class GrailsWebRequestFilterSpec extends Specification { + + MockServletContext servletContext = new MockServletContext() + + def cleanup() { + RequestContextHolder.resetRequestAttributes() + LocaleContextHolder.resetLocaleContext() + } + + void 'the locale context established outside Grails is restored after the request'() { + given: 'a LocaleContext installed by a filter outside Grails, carrying a time zone' + def outerContext = new SimpleTimeZoneAwareLocaleContext(Locale.FRANCE, TimeZone.getTimeZone('Europe/Paris')) + LocaleContextHolder.setLocaleContext(outerContext) + + when: + filter().doFilter(request(Locale.GERMANY), new MockHttpServletResponse(), { req, res -> } as FilterChain) + + then: 'the outer context is put back, time zone and all' + LocaleContextHolder.localeContext.is(outerContext) + LocaleContextHolder.timeZone.ID == 'Europe/Paris' + } + + void 'an include restores the locale context of the enclosing request'() { + given: 'an outer request whose locale context is in place' + def outerContext = new SimpleTimeZoneAwareLocaleContext(Locale.FRANCE, TimeZone.getTimeZone('Europe/Paris')) + LocaleContextHolder.setLocaleContext(outerContext) + + when: 'an include is dispatched through the filter' + def includeRequest = request(Locale.JAPAN) + includeRequest.setAttribute(WebUtils.INCLUDE_REQUEST_URI_ATTRIBUTE, '/some/include') + filter().doFilter(includeRequest, new MockHttpServletResponse(), { req, res -> } as FilterChain) + + then: 'the include does not leave its own locale behind for the rest of the outer request' + LocaleContextHolder.localeContext.is(outerContext) + LocaleContextHolder.timeZone.ID == 'Europe/Paris' + } + + void 'the request locale is in effect while the chain runs'() { + given: + LocaleContextHolder.setLocaleContext(new SimpleTimeZoneAwareLocaleContext(Locale.FRANCE, TimeZone.default)) + Locale seen = null + + when: + filter().doFilter(request(Locale.GERMANY), new MockHttpServletResponse(), + { req, res -> seen = LocaleContextHolder.locale } as FilterChain) + + then: 'the filter installs the locale of the request being handled' + seen == Locale.GERMANY + } + + private MockHttpServletRequest request(Locale locale) { + new MockHttpServletRequest(servletContext).tap { + it.addPreferredLocale(locale) + } + } + + private GrailsWebRequestFilter filter() { + def applicationContext = new StaticWebApplicationContext() + applicationContext.servletContext = servletContext + applicationContext.refresh() + def filter = new GrailsWebRequestFilter() + filter.setApplicationContext(applicationContext) + filter.setServletContext(servletContext) + filter + } +} diff --git a/grails-web-url-mappings/src/main/groovy/grails/web/mapping/ResponseRedirector.groovy b/grails-web-url-mappings/src/main/groovy/grails/web/mapping/ResponseRedirector.groovy index 724ede7cc3d..60f77331e17 100644 --- a/grails-web-url-mappings/src/main/groovy/grails/web/mapping/ResponseRedirector.groovy +++ b/grails-web-url-mappings/src/main/groovy/grails/web/mapping/ResponseRedirector.groovy @@ -63,7 +63,7 @@ class ResponseRedirector { void redirect(Map arguments = Collections.emptyMap()) { def webRequest = GrailsWebRequest.lookup() - HttpServletRequest request = webRequest.currentRequest + HttpServletRequest request = webRequest.request HttpServletResponse response = webRequest.getCurrentResponse() redirect(request, response, arguments) diff --git a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java index e510c1ee66f..d1dea1fa914 100644 --- a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java +++ b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/AbstractUrlMappingInfo.java @@ -142,7 +142,7 @@ protected String evaluateNameForValue(Object value, GrailsWebRequest webRequest) } else if (value instanceof Map) { Map httpMethods = (Map) value; - name = (String) httpMethods.get(webRequest.getCurrentRequest().getMethod()); + name = (String) httpMethods.get(webRequest.getRequest().getMethod()); } else { name = value.toString(); diff --git a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingInfo.java b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingInfo.java index 5b6b24ff432..33dfe7e1b13 100644 --- a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingInfo.java +++ b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingInfo.java @@ -19,22 +19,16 @@ package org.grails.web.mapping; import java.util.Collections; -import java.util.Enumeration; import java.util.Map; import groovy.lang.Closure; -import jakarta.servlet.http.HttpServletRequest; - -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.context.ApplicationContext; import org.springframework.util.Assert; import org.springframework.web.context.request.RequestContextHolder; -import org.springframework.web.multipart.MultipartHttpServletRequest; -import org.springframework.web.multipart.MultipartResolver; -import org.springframework.web.servlet.DispatcherServlet; import grails.core.GrailsApplication; import grails.web.CamelCaseUrlConverter; @@ -53,8 +47,7 @@ */ public class DefaultUrlMappingInfo extends AbstractUrlMappingInfo { - private static final Log LOG = LogFactory.getLog(DefaultUrlMappingInfo.class); - private static final String SETTING_GRAILS_WEB_DISABLE_MULTIPART = "grails.web.disable.multipart"; + private static final Logger LOG = LoggerFactory.getLogger(DefaultUrlMappingInfo.class); private static final String CONTROLLER_PREFIX = "controller:"; private static final String ACTION_PREFIX = "action:"; private static final String PLUGIN_PREFIX = "plugin:"; @@ -220,45 +213,6 @@ public String getId() { return evaluateNameForValue(id); } - private Enumeration tryMultipartParams(HttpServletRequest request, Enumeration originalParams) { - Enumeration paramNames = originalParams; - boolean disabled = isMultipartDisabled(); - if (!disabled) { - MultipartResolver resolver = getMultipartResolver(); - if (resolver != null && resolver.isMultipart(request)) { - MultipartHttpServletRequest resolvedMultipartRequest = getResolvedRequest(request, resolver); - paramNames = resolvedMultipartRequest.getParameterNames(); - } - } - return paramNames; - } - - private MultipartHttpServletRequest getResolvedRequest(HttpServletRequest request, MultipartResolver resolver) { - MultipartHttpServletRequest resolvedMultipartRequest = (MultipartHttpServletRequest) request.getAttribute(MultipartHttpServletRequest.class.getName()); - if (resolvedMultipartRequest == null) { - resolvedMultipartRequest = resolver.resolveMultipart(request); - request.setAttribute(MultipartHttpServletRequest.class.getName(), resolvedMultipartRequest); - } - return resolvedMultipartRequest; - } - - private boolean isMultipartDisabled() { - if (grailsApplication != null) { - return grailsApplication.getConfig().getProperty(SETTING_GRAILS_WEB_DISABLE_MULTIPART, Boolean.class, false); - } - return false; - } - - private MultipartResolver getMultipartResolver() { - if (grailsApplication != null) { - ApplicationContext ctx = grailsApplication.getMainContext(); - if (ctx != null) { - return (MultipartResolver) ctx.getBean(DispatcherServlet.MULTIPART_RESOLVER_BEAN_NAME); - } - } - return null; - } - public String getURI() { return evaluateNameForValue(uri); } diff --git a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingsHolder.java b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingsHolder.java index 6d0ff5f4376..6aa61e019d9 100644 --- a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingsHolder.java +++ b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultUrlMappingsHolder.java @@ -37,8 +37,8 @@ import com.github.benmanes.caffeine.cache.Cache; import com.github.benmanes.caffeine.cache.Caffeine; import com.github.benmanes.caffeine.cache.Weigher; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.core.style.ToStringCreator; import org.springframework.http.HttpMethod; @@ -70,7 +70,7 @@ @SuppressWarnings("rawtypes") public class DefaultUrlMappingsHolder implements UrlMappings { - private static final transient Log LOG = LogFactory.getLog(DefaultUrlMappingsHolder.class); + private static final transient Logger LOG = LoggerFactory.getLogger(DefaultUrlMappingsHolder.class); private static final int DEFAULT_MAX_WEIGHTED_CAPACITY = 1000; public static final UrlMappingInfo[] EMPTY_RESULTS = new UrlMappingInfo[0]; @@ -179,9 +179,7 @@ public void initialize() { UrlMappingsListKey listKey = new UrlMappingsListKey(controllerName, actionName, namespace, pluginName, httpMethod, version); mappingsListLookup.put(listKey, key); - if (LOG.isDebugEnabled()) { - LOG.debug("Reverse mapping: " + key + " -> " + mapping); - } + LOG.debug("Reverse mapping: {} -> {}", key, mapping); Set requiredParamsAndOptionals = new HashSet<>(requiredParams); if (optionalIndex > -1) { for (int j = optionalIndex; j < params.length; j++) { @@ -196,9 +194,7 @@ public void initialize() { listKey = new UrlMappingsListKey(controllerName, actionName, namespace, pluginName, httpMethod, version); mappingsListLookup.put(listKey, key); - if (LOG.isDebugEnabled()) { - LOG.debug("Reverse mapping: " + key + " -> " + mapping); - } + LOG.debug("Reverse mapping: {} -> {}", key, mapping); } } } @@ -512,12 +508,14 @@ public UrlMappingInfo match(String uri) { return info; } + final boolean debugEnabled = LOG.isDebugEnabled(); + final int uriSlashCount = RegexUrlMapping.countSlashes(uri); for (UrlMapping mapping : mappings) { - if (LOG.isDebugEnabled()) { - LOG.debug("Attempting to match URI [" + uri + "] with pattern [" + mapping.getUrlData().getUrlPattern() + "]"); + if (debugEnabled) { + LOG.debug("Attempting to match URI [{}] with pattern [{}]", uri, mapping.getUrlData().getUrlPattern()); } - info = mapping.match(uri); + info = matchMapping(mapping, uri, uriSlashCount); if (info != null) { cachedMatches.put(uri, info); break; @@ -539,15 +537,17 @@ public UrlMappingInfo[] matchAll(String uri, String httpMethod) { List matchingUrls = cachedListMatches.getIfPresent(cacheKey); if (matchingUrls == null) { matchingUrls = new ArrayList<>(); + final boolean debugEnabled = LOG.isDebugEnabled(); + final int uriSlashCount = RegexUrlMapping.countSlashes(uri); for (UrlMapping mapping : mappings) { - if (LOG.isDebugEnabled()) { - LOG.debug("Attempting to match URI [" + uri + "] with pattern [" + mapping.getUrlData().getUrlPattern() + "]"); + if (debugEnabled) { + LOG.debug("Attempting to match URI [{}] with pattern [{}]", uri, mapping.getUrlData().getUrlPattern()); } - UrlMappingInfo current = mapping.match(uri); + UrlMappingInfo current = matchMapping(mapping, uri, uriSlashCount); if (current != null) { - if (LOG.isDebugEnabled()) { - LOG.debug("Matched URI [" + uri + "] with pattern [" + mapping.getUrlData().getUrlPattern() + "], adding to possibilities"); + if (debugEnabled) { + LOG.debug("Matched URI [{}] with pattern [{}], adding to possibilities", uri, mapping.getUrlData().getUrlPattern()); } String mappingHttpMethod = current.getHttpMethod(); @@ -560,6 +560,18 @@ public UrlMappingInfo[] matchAll(String uri, String httpMethod) { return matchingUrls.toArray(new UrlMappingInfo[0]); } + /** + * Matches one mapping, passing the pre-computed URI slash count so a RegexUrlMapping can rule out + * candidate patterns without allocating a Matcher. Any other UrlMapping implementation is matched + * exactly as before. + */ + private static UrlMappingInfo matchMapping(UrlMapping mapping, String uri, int uriSlashCount) { + if (mapping instanceof RegexUrlMapping regexUrlMapping) { + return regexUrlMapping.match(uri, uriSlashCount); + } + return mapping.match(uri); + } + private boolean isExcluded(String uri) { if (excludePatterns != null) { for (Object excludePattern : excludePatterns) { @@ -580,15 +592,17 @@ public UrlMappingInfo[] matchAll(String uri, String httpMethod, String version) matchingUrls = new ArrayList<>(); boolean anyHttpMethod = httpMethod != null && httpMethod.equals(UrlMapping.ANY_HTTP_METHOD); boolean anyVersion = version != null && version.equals(UrlMapping.ANY_VERSION); + final boolean debugEnabled = LOG.isDebugEnabled(); + final int uriSlashCount = RegexUrlMapping.countSlashes(uri); for (UrlMapping mapping : mappings) { - if (LOG.isDebugEnabled()) { - LOG.debug("Attempting to match URI [" + uri + "] with pattern [" + mapping.getUrlData().getUrlPattern() + "]"); + if (debugEnabled) { + LOG.debug("Attempting to match URI [{}] with pattern [{}]", uri, mapping.getUrlData().getUrlPattern()); } - UrlMappingInfo current = mapping.match(uri); + UrlMappingInfo current = matchMapping(mapping, uri, uriSlashCount); if (current != null) { - if (LOG.isDebugEnabled()) { - LOG.debug("Matched URI [" + uri + "] with pattern [" + mapping.getUrlData().getUrlPattern() + "], adding to possibilities"); + if (debugEnabled) { + LOG.debug("Matched URI [{}] with pattern [{}], adding to possibilities", uri, mapping.getUrlData().getUrlPattern()); } String mappingHttpMethod = current.getHttpMethod(); diff --git a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/RegexUrlMapping.java b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/RegexUrlMapping.java index d2283f2204a..5547ed5e344 100644 --- a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/RegexUrlMapping.java +++ b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/RegexUrlMapping.java @@ -21,13 +21,11 @@ import java.io.UnsupportedEncodingException; import java.net.URI; import java.net.URLEncoder; -import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; -import java.util.List; import java.util.Map; import java.util.Set; import java.util.regex.Matcher; @@ -77,7 +75,11 @@ public class RegexUrlMapping extends AbstractUrlMapping { public static final String FORMAT_PARAMETER = "format"; private Pattern[] patterns; - private Map> patternByTokenCount = new HashMap<>(); + /** Marks a pattern containing a ** token, which can match across any number of path segments. */ + private static final int SPANS_SEGMENTS = -1; + + /** Slash count per compiled pattern, parallel to {@link #patterns}, used to skip impossible candidates. */ + private int[] patternSlashCounts; private UrlMappingData urlData; private static final String DEFAULT_ENCODING = "UTF-8"; private static final Logger LOG = LoggerFactory.getLogger(RegexUrlMapping.class); @@ -134,22 +136,17 @@ private void parse(UrlMappingData data, ConstrainedProperty[] constraints) { urlData = data; patterns = new Pattern[urls.length]; + patternSlashCounts = new int[urls.length]; + for (int i = 0; i < urls.length; i++) { String url = urls[i]; - Integer slashCount = org.springframework.util.StringUtils.countOccurrencesOf(url, "/"); - List tokenCountPatterns = patternByTokenCount.get(slashCount); - if (tokenCountPatterns == null) { - tokenCountPatterns = new ArrayList<>(); - patternByTokenCount.put(slashCount, tokenCountPatterns); - } Pattern pattern = convertToRegex(url); if (pattern == null) { throw new IllegalStateException("Cannot use null pattern in regular expression mapping for url [" + data.getUrlPattern() + "]"); } - tokenCountPatterns.add(pattern); this.patterns[i] = pattern; - + this.patternSlashCounts[i] = slashCountFor(url); } if (constraints != null) { @@ -304,8 +301,32 @@ private boolean shouldApplyGreedyForUrl(String url, int greedyTokenIndex) { * @see grails.web.mapping.UrlMappingInfo */ public UrlMappingInfo match(String uri) { - for (Pattern pattern : patterns) { - Matcher m = pattern.matcher(uri); + return match(uri, countSlashes(uri)); + } + + /** + * Matches the given URI, skipping patterns whose segment count rules them out before any regular + * expression work is done. + *

+ * Every construct {@link #convertToRegex(String)} emits is bounded to a single path segment + * ({@code [^/]+?}, {@code ([^/.]+?)} and friends) except {@code .*}, which is produced only by a + * {@code **} token. So for a pattern with no {@code **}, a matching URI has exactly the pattern's + * slash count, or one more because of the optional trailing {@code /??} the pattern ends with. + * Patterns are skipped, never reordered, so the mapping that wins is unchanged. + * + * @param uri The URI to match + * @param uriSlashCount The number of '/' characters in the URI, so callers scanning many mappings + * compute it once rather than once per mapping + * @return A UrlMappingInfo instance or null + */ + public UrlMappingInfo match(String uri, int uriSlashCount) { + for (int i = 0; i < patterns.length; i++) { + int patternSlashCount = patternSlashCounts[i]; + if (patternSlashCount != SPANS_SEGMENTS && + uriSlashCount != patternSlashCount && uriSlashCount != patternSlashCount + 1) { + continue; + } + Matcher m = patterns[i].matcher(uri); if (m.matches()) { UrlMappingInfo urlInfo = createUrlMappingInfo(uri, m); if (urlInfo != null) { @@ -316,6 +337,32 @@ public UrlMappingInfo match(String uri) { return null; } + /** + * @return the number of '/' characters in the logical URL, or {@link #SPANS_SEGMENTS} when it + * contains a {@code **} token and so can match across any number of segments + */ + private static int slashCountFor(String url) { + for (int i = 0; i < url.length() - 1; i++) { + if (url.charAt(i) == '*' && url.charAt(i + 1) == '*') { + return SPANS_SEGMENTS; + } + } + return countSlashes(url); + } + + /** + * @return the number of '/' characters in the given string + */ + public static int countSlashes(String uri) { + int count = 0; + for (int i = 0; i < uri.length(); i++) { + if (uri.charAt(i) == '/') { + count++; + } + } + return count; + } + /** * @see grails.web.mapping.UrlMapping */ diff --git a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/GrailsControllerUrlMappingInfo.groovy b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/GrailsControllerUrlMappingInfo.groovy index d3d9baf2e13..a54aeebb776 100644 --- a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/GrailsControllerUrlMappingInfo.groovy +++ b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/GrailsControllerUrlMappingInfo.groovy @@ -18,6 +18,8 @@ */ package org.grails.web.mapping.mvc +import groovy.transform.CompileStatic + import grails.core.GrailsControllerClass import grails.web.mapping.UrlMappingInfo @@ -26,6 +28,7 @@ import grails.web.mapping.UrlMappingInfo * * @since 3.0 */ +@CompileStatic class GrailsControllerUrlMappingInfo implements UrlMappingInfo { GrailsControllerClass controllerClass diff --git a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/UrlMappingsHandlerMapping.groovy b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/UrlMappingsHandlerMapping.groovy index 7c6a287e9ca..1df6cc3bfc0 100644 --- a/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/UrlMappingsHandlerMapping.groovy +++ b/grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/mvc/UrlMappingsHandlerMapping.groovy @@ -18,7 +18,6 @@ */ package org.grails.web.mapping.mvc -import groovy.transform.CompileDynamic import groovy.transform.CompileStatic import jakarta.servlet.http.HttpServletRequest @@ -59,7 +58,13 @@ class UrlMappingsHandlerMapping extends AbstractHandlerMapping { public static final String MATCHED_REQUEST = 'org.grails.url.match.info' + // Both are stateless, so one shared instance each rather than two allocations per request. + private static final HandlerInterceptor OBSERVATION_ROUTE_HANDLER = new ObservationRouteHandler() + private static final HandlerInterceptor ERROR_HANDLING_HANDLER = new ErrorHandlingHandler() + protected UrlMappingsHolder urlMappingsHolder + // Deliberately not UrlPathHelper.defaultInstance: that instance is read-only, and this field is + // protected, so a subclass configuring it (alwaysUseFullPath and friends) must keep working. protected UrlPathHelper urlHelper = new UrlPathHelper() protected MimeTypeResolver mimeTypeResolver protected HandlerInterceptor[] webRequestHandlerInterceptors @@ -93,36 +98,23 @@ class UrlMappingsHandlerMapping extends AbstractHandlerMapping { @Override protected HandlerExecutionChain getHandlerExecutionChain(Object handler, HttpServletRequest request) { - HandlerExecutionChain chain = (handler instanceof HandlerExecutionChain ? - (HandlerExecutionChain) handler : new HandlerExecutionChain(handler)) + // Let Spring assemble the chain. Re-implementing it here meant Grails-mapped requests silently + // missed whatever AbstractHandlerMapping added later - currently the API version deprecation + // interceptor behind spring.mvc.apiversion.*. + HandlerExecutionChain chain = super.getHandlerExecutionChain(handler, request) - // WebRequestInterceptor need to come first, as these include things like Hibernate OSIV + // WebRequestInterceptors have to come first, as these include things like Hibernate OSIV. if (webRequestHandlerInterceptors) { - chain.addInterceptors(webRequestHandlerInterceptors) - } - - for (HandlerInterceptor interceptor in this.adaptedInterceptors) { - if (interceptor instanceof MappedInterceptor) { - MappedInterceptor mappedInterceptor = mappedInterceptor(interceptor) - if (mappedInterceptor.matches(request)) { - chain.addInterceptor(mappedInterceptor.getInterceptor()) - } - } - else { - chain.addInterceptor(interceptor) + for (int i = 0; i < webRequestHandlerInterceptors.length; i++) { + chain.addInterceptor(i, webRequestHandlerInterceptors[i]) } } - chain.addInterceptor(new ObservationRouteHandler()) - chain.addInterceptor(new ErrorHandlingHandler()) + chain.addInterceptor(OBSERVATION_ROUTE_HANDLER) + chain.addInterceptor(ERROR_HANDLING_HANDLER) return chain } - @CompileDynamic - protected MappedInterceptor mappedInterceptor(HandlerInterceptor interceptor) { - (MappedInterceptor) interceptor - } - @Override protected Object getHandlerInternal(HttpServletRequest request) throws Exception { diff --git a/grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/UrlMappingSegmentFilterSpec.groovy b/grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/UrlMappingSegmentFilterSpec.groovy new file mode 100644 index 00000000000..c77d0ae27e8 --- /dev/null +++ b/grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/UrlMappingSegmentFilterSpec.groovy @@ -0,0 +1,105 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.web.mapping + +import grails.web.mapping.AbstractUrlMappingsSpec + +/** + * Covers the segment-count pre-filter that lets a mapping skip candidate patterns before any regular + * expression work. The filter must only ever remove patterns that provably cannot match, so which + * mapping wins has to be identical with and without it. + */ +class UrlMappingSegmentFilterSpec extends AbstractUrlMappingsSpec { + + void 'a double wildcard still matches across any number of segments'() { + given: 'a mapping whose ** token can span segments, so the segment count cannot rule it out' + def holder = getUrlMappingsHolder { + "/files/$path**"(controller: 'file', action: 'serve') + } + + expect: + holder.match(uri)?.actionName == 'serve' + + where: + uri << ['/files/a', '/files/a/b', '/files/a/b/c/d/e'] + } + + void 'a trailing slash still matches, being one segment longer than the pattern'() { + given: + def holder = getUrlMappingsHolder { + "/book/list"(controller: 'book', action: 'list') + } + + expect: + holder.match('/book/list')?.actionName == 'list' + holder.match('/book/list/')?.actionName == 'list' + } + + void 'a URI with the wrong segment count does not match'() { + given: + def holder = getUrlMappingsHolder { + "/book/list"(controller: 'book', action: 'list') + } + + expect: + holder.match('/book') == null + holder.match('/book/list/extra') == null + } + + void 'the first declared mapping still wins when several could match'() { + given: 'two mappings that both match the same URI, the specific one declared first' + def holder = getUrlMappingsHolder { + "/book/$id"(controller: 'book', action: 'specific') + "/$controller/$action?/$id?"() + } + + expect: 'declaration order decides, exactly as it did before the filter' + holder.match('/book/42').actionName == 'specific' + } + + void 'optional trailing tokens still match at every arity'() { + given: 'one mapping that expands into several logical URLs of different segment counts' + def holder = getUrlMappingsHolder { + "/book/$action?/$id?"(controller: 'book') + } + + expect: 'each arity is still reachable, so no logical URL was filtered out' + holder.match('/book') != null + holder.match('/book/show') != null + holder.match('/book/show/42') != null + + and: 'and one segment too many still does not match' + holder.match('/book/show/42/extra') == null + } + + void 'matchAll returns the same mappings the scan would have produced'() { + given: 'a URI matched by several mappings of differing segment counts' + def holder = getUrlMappingsHolder { + "/api/books/$id"(controller: 'book', action: 'show', method: 'GET') + "/api/books/$id"(controller: 'book', action: 'update', method: 'PUT') + "/api/$other"(controller: 'other', action: 'index') + } + + when: + def all = holder.matchAll('/api/books/42', 'GET') + + then: 'only the GET mapping of the right arity survives, in declaration order' + all*.actionName == ['show'] + } +} diff --git a/grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/mvc/UrlMappingsHandlerMappingSpec.groovy b/grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/mvc/UrlMappingsHandlerMappingSpec.groovy index 39bd16349b9..48241a55877 100644 --- a/grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/mvc/UrlMappingsHandlerMappingSpec.groovy +++ b/grails-web-url-mappings/src/test/groovy/org/grails/web/mapping/mvc/UrlMappingsHandlerMappingSpec.groovy @@ -27,7 +27,13 @@ import grails.web.mapping.AbstractUrlMappingsSpec import org.grails.web.mapping.DefaultUrlMappingData import org.grails.web.mapping.DefaultUrlMappingInfo import org.grails.web.util.WebUtils +import org.springframework.ui.ModelMap import org.springframework.web.context.request.RequestContextHolder +import org.springframework.web.context.request.WebRequest +import org.springframework.web.context.request.WebRequestInterceptor +import org.springframework.web.context.support.StaticWebApplicationContext +import org.springframework.web.servlet.HandlerInterceptor +import org.springframework.web.servlet.handler.WebRequestHandlerInterceptorAdapter import org.springframework.web.servlet.view.InternalResourceView import spock.lang.Issue @@ -229,6 +235,61 @@ class UrlMappingsHandlerMappingSpec extends AbstractUrlMappingsSpec { result.view.getUrl() == "/index.html" } + void "the handler chain keeps WebRequestInterceptors first and appends the Grails interceptors last"() { + given: "a handler mapping with both a WebRequestInterceptor and an ordinary HandlerInterceptor" + def handlerMapping = handlerMapping() + def plainInterceptor = new HandlerInterceptor() {} + def webRequestInterceptor = new WebRequestInterceptor() { + void preHandle(WebRequest request) {} + void postHandle(WebRequest request, ModelMap model) {} + void afterCompletion(WebRequest request, Exception ex) {} + } + handlerMapping.setHandlerInterceptors([plainInterceptor] as HandlerInterceptor[]) + handlerMapping.setWebRequestInterceptors([webRequestInterceptor] as WebRequestInterceptor[]) + handlerMapping.setApplicationContext(new StaticWebApplicationContext()) + + when: + def webRequest = GrailsWebMockUtil.bindMockWebRequest() + webRequest.request.setRequestURI("/foo/bar") + def interceptors = handlerMapping.getHandler(webRequest.request).interceptorList + + then: "the WebRequestInterceptor is adapted and ordered first, ahead of ordinary interceptors" + interceptors[0] instanceof WebRequestHandlerInterceptorAdapter + interceptors.contains(plainInterceptor) + + and: "the Grails interceptors are appended last, in order" + interceptors[-2].class.simpleName == 'ObservationRouteHandler' + interceptors[-1].class.simpleName == 'ErrorHandlingHandler' + } + + void "the Grails interceptors are shared rather than allocated per request"() { + given: + def handlerMapping = handlerMapping() + handlerMapping.setApplicationContext(new StaticWebApplicationContext()) + + when: "two separate requests are handled" + def first = GrailsWebMockUtil.bindMockWebRequest() + first.request.setRequestURI("/foo/bar") + def firstChain = handlerMapping.getHandler(first.request).interceptorList + RequestContextHolder.resetRequestAttributes() + def second = GrailsWebMockUtil.bindMockWebRequest() + second.request.setRequestURI("/foo/bar") + def secondChain = handlerMapping.getHandler(second.request).interceptorList + + then: "both chains end with the very same interceptor instances" + firstChain[-1].is(secondChain[-1]) + firstChain[-2].is(secondChain[-2]) + } + + private UrlMappingsHandlerMapping handlerMapping() { + def grailsApplication = new DefaultGrailsApplication(FooController) + grailsApplication.initialise() + def holder = new GrailsControllerUrlMappings(grailsApplication, getUrlMappingsHolder { + "/foo/bar"(controller: "foo", action: "bar") + }) + new UrlMappingsHandlerMapping(holder) + } + void cleanup() { RequestContextHolder.resetRequestAttributes() }