codeconsole commented on code in PR #16760:
URL: https://github.com/apache/grails-core/pull/16760#discussion_r4390165732
##########
grails-web-common/src/main/groovy/org/grails/web/config/http/GrailsFilters.java:
##########
@@ -28,7 +28,18 @@ public enum GrailsFilters {
FIRST,
ASSET_PIPELINE_FILTER,
+
+ /**
+ * The order of the character encoding filter Grails registered before 9.0.
+ *
+ * @deprecated since 9.0, for removal. Grails no longer registers a
character encoding filter; Spring Boot
+ * registers one at {@code Ordered.HIGHEST_PRECEDENCE}, ahead of every
other filter. The filters after it
+ * take their orders from their position in this enum, so when it is
removed, give them their current orders
+ * explicitly.
+ */
+ @Deprecated(since = "9.0", forRemoval = true)
CHARACTER_ENCODING_FILTER,
Review Comment:
Done in e131c085ec: `CHARACTER_ENCODING_FILTER(Ordered.HIGHEST_PRECEDENCE)`,
still deprecated for removal. The Javadoc now says it is the order of the
encoding filter that runs, Spring Boot's, and that
`CHARACTER_ENCODING_FILTER.getOrder() + 1` runs right after it. The note about
the later filters taking their orders from `ordinal()` stays.
With #16768, both branches now declare the constant the same way, so at the
merge-up `GrailsFilters` only differs in the Javadoc and the deprecation. The
change is visible to applications coming from 8.0, where the constant was -180,
so §20 and the `grails-9-upgrade` skill say so, and the skill has a checklist
entry for it. `GrailsFiltersSpec`, new in `grails-web-common`, checks that the
constant is `HIGHEST_PRECEDENCE` and ahead of every other Grails filter, and
pins the orders of the others: -200, -190, -170, -160, -150 and -110.
##########
grails-test-examples/app1/src/integration-test/groovy/functionaltests/encoding/CharacterEncodingSpec.groovy:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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 functionaltests.encoding
+
+import java.net.http.HttpRequest
+
+import spock.lang.Issue
+import spock.lang.Specification
+import spock.lang.Tag
+import spock.lang.Unroll
+
+import grails.testing.mixin.integration.Integration
+import org.apache.grails.testing.http.client.HttpClientSupport
+
+/**
+ * A filter registered at {@code Ordered.HIGHEST_PRECEDENCE + 1} reads a
request parameter before Grails sees the
+ * request. Once a filter reads a parameter, the container has decoded the
body, so the encoding filter has to run
+ * before it. The other cases check which forms the forced request encoding
decodes as UTF-8 when they declare
+ * another charset.
+ */
+@Integration
+@Tag('http-client')
+@Issue('https://github.com/apache/grails-core/issues/15921')
+class CharacterEncodingSpec extends Specification implements HttpClientSupport
{
Review Comment:
Added in bce01442ea, with the files from #16768: `EncodingRecordingFilter`,
its `FilterRegistrationBean` at `HIGHEST_PRECEDENCE` on `/encoding/*` in
`resources.groovy`, the controller's `encodingAtHighestPrecedence`, and the
case "runs a filter registered at HIGHEST_PRECEDENCE through a
FilterRegistrationBean ahead of the encoding filter". With the registration
moved to `HIGHEST_PRECEDENCE + 1`, the filter finds UTF-8 and the case fails.
`resources.groovy`, `EncodingController.groovy`,
`EncodingRecordingFilter.groovy` and `ParameterReadingFilter.groovy` are now
byte-identical on the two branches. `CharacterEncodingSpec` differs only by the
9.0 cases for the forced request encoding.
##########
grails-test-examples/app1/src/integration-test/groovy/functionaltests/encoding/CharacterEncodingSpec.groovy:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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 functionaltests.encoding
+
+import java.net.http.HttpRequest
+
+import spock.lang.Issue
+import spock.lang.Specification
+import spock.lang.Tag
+import spock.lang.Unroll
+
+import grails.testing.mixin.integration.Integration
+import org.apache.grails.testing.http.client.HttpClientSupport
+
+/**
+ * A filter registered at {@code Ordered.HIGHEST_PRECEDENCE + 1} reads a
request parameter before Grails sees the
+ * request. Once a filter reads a parameter, the container has decoded the
body, so the encoding filter has to run
+ * before it. The other cases check which forms the forced request encoding
decodes as UTF-8 when they declare
+ * another charset.
+ */
+@Integration
+@Tag('http-client')
+@Issue('https://github.com/apache/grails-core/issues/15921')
+class CharacterEncodingSpec extends Specification implements HttpClientSupport
{
+
+ void 'decodes a form post as UTF-8 when a filter ahead of Grails reads its
parameters'() {
+ when: 'a form is posted without a charset, through a filter that reads
one of its parameters'
+ def response = httpPost('/encoding/echo',
'text=Caf%C3%A9+%E2%80%93+%F0%9F%8D%82', 'application/x-www-form-urlencoded')
Review Comment:
Done in abde6e69d7, for the first case and for the new
`FilterRegistrationBean` case, which posts the same kind of form. The second
case stays on `httpPost`, as you say. #16768 got the same change (1fe1d29864),
so the spec reads the same on both branches.
##########
grails-test-examples/app1/src/integration-test/groovy/functionaltests/encoding/CharacterEncodingSpec.groovy:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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 functionaltests.encoding
+
+import java.net.http.HttpRequest
+
+import spock.lang.Issue
+import spock.lang.Specification
+import spock.lang.Tag
+import spock.lang.Unroll
+
+import grails.testing.mixin.integration.Integration
+import org.apache.grails.testing.http.client.HttpClientSupport
+
+/**
+ * A filter registered at {@code Ordered.HIGHEST_PRECEDENCE + 1} reads a
request parameter before Grails sees the
+ * request. Once a filter reads a parameter, the container has decoded the
body, so the encoding filter has to run
+ * before it. The other cases check which forms the forced request encoding
decodes as UTF-8 when they declare
+ * another charset.
+ */
+@Integration
+@Tag('http-client')
+@Issue('https://github.com/apache/grails-core/issues/15921')
+class CharacterEncodingSpec extends Specification implements HttpClientSupport
{
+
+ void 'decodes a form post as UTF-8 when a filter ahead of Grails reads its
parameters'() {
+ when: 'a form is posted without a charset, through a filter that reads
one of its parameters'
+ def response = httpPost('/encoding/echo',
'text=Caf%C3%A9+%E2%80%93+%F0%9F%8D%82', 'application/x-www-form-urlencoded')
+
+ then: 'the filter and the controller both see the text as it was typed'
+ response.assertJsonContains(200, [text: 'Café – 🍂', readByFilter:
'Café – 🍂'])
+ }
+
+ void 'decodes a form post as UTF-8 when it declares another charset'() {
+ when: 'a form encoded as UTF-8 is posted with ISO-8859-1 declared'
+ def response = httpPost('/encoding/echo', 'text=Caf%C3%A9',
'application/x-www-form-urlencoded; charset=ISO-8859-1')
+
+ then: 'the forced request encoding wins over the declared charset'
+ response.assertJsonContains(200, [text: 'Café', readByFilter: 'Café'])
+ }
+
+ @Unroll
Review Comment:
Removed in abde6e69d7.
##########
grails-test-examples/external-configuration/src/integration-test/groovy/test/app/LegacyEncodingConfigSpec.groovy:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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 test.app
+
+import grails.testing.mixin.integration.Integration
+import org.springframework.test.annotation.DirtiesContext
+import org.springframework.web.filter.CharacterEncodingFilter
+import spock.lang.Issue
+import spock.lang.Specification
+import spock.util.environment.RestoreSystemProperties
+
+/**
+ * {@code grails.config.locations} files are loaded after Spring Boot's
environment post-processors run, and the
+ * deprecated {@code grails.filter.*} encoding properties in them still
configure the encoding filter.
+ */
+@Integration
+@RestoreSystemProperties
Review Comment:
Done in abde6e69d7: `@TestPropertySource(properties =
'grails.config.locations=classpath:legacyEncodingConfig.yml')` replaces
`@RestoreSystemProperties`, `@DirtiesContext` and `setupSpec()`. I ran it both
ways too: it passes, and without the annotation it fails with UTF-8 and a
forced request. The module's 50 integration tests pass.
##########
grails-doc/src/en/guide/plugins/hookingIntoRuntimeConfiguration.adoc:
##########
@@ -384,11 +384,11 @@ However, if you want to control the order of filter
registrations you will need
myFilter(FilterRegistrationBean) {
filter = bean(MyFilter)
urlPatterns = ['/*']
- order = Ordered.HIGHEST_PRECEDENCE
+ order = Ordered.HIGHEST_PRECEDENCE + 1
}
----
-NOTE: Grails' internal registered filters (`GrailsWebRequestFilter`,
`HiddenHttpMethodFilter` etc.) are defined by incrementing `HIGHEST_PRECEDENCE`
by 10 thus allowing several filters to be inserted before or between Grails'
filters.
+NOTE: Spring Boot registers the character encoding filter at
`Ordered.HIGHEST_PRECEDENCE`, ahead of every other filter. Once any filter
reads a request parameter, the servlet container has decoded the request body,
so a filter that reads parameters needs a later order than the encoding filter,
or form input arrives decoded as ISO-8859-1. A filter registered at exactly
`Ordered.HIGHEST_PRECEDENCE` through a `FilterRegistrationBean` runs before it.
Grails' own filters (`HiddenHttpMethodFilter`, `GrailsWebRequestFilter` etc.)
have orders from -200 to -110, ten apart, ahead of the Spring Security filter
chain at -100, which leaves room to insert filters before or between them.
Review Comment:
Taken as you wrote it, in 97fabd3dc7. #16768 has the same sentence in the
same note, so it got the same fix (bb4dcd95c2).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
