jamesfredley commented on code in PR #16499:
URL: https://github.com/apache/grails-core/pull/16499#discussion_r4177989158
##########
grails-forge/grails-forge-core/src/main/resources/gsp/index.gsp:
##########
@@ -577,25 +577,57 @@
<g:def type="List" var="mimeTypeProviders"
value="${applicationContext.getBeansOfType(grails.web.mime.MimeTypeProvider)
.entrySet().toList().sort {
it.key.toLowerCase() }}"/>
- <%-- The filters still on the call stack ARE this request's
pipeline, in
- execution order: walk the reversed stack, keep Filter
classes, collapse
- the extra frames a filter contributes through its
abstract bases, and
- number what remains. No registry can report this actual
order. --%>
- <g:def type="List" var="requestFilters"
-
value="${Thread.currentThread().stackTrace.toList().reverse()
- .findResults { ste ->
- def cls = null
- try { cls = Class.forName(ste.className,
false, Thread.currentThread().contextClassLoader) } catch (Throwable ignored) {
}
- (cls != null &&
jakarta.servlet.Filter.isAssignableFrom(cls)) ? cls : null
- }
- .inject([]) { acc, cls ->
- Class prev = acc ? (Class) acc[-1] : null
- if (prev == cls) { return acc }
- if (prev != null &&
prev.isAssignableFrom(cls)) { acc[-1] = cls; return acc }
- if (prev != null &&
cls.isAssignableFrom(prev)) { return acc }
- acc << cls
- }
- .unique()}"/>
+ <%-- Every filter running in the container, in chain order.
Tomcat's filter
+ maps are the exact chain: FilterRegistrationBeans, plain
Filter beans Boot
+ adapted and container-added filters like WsFilter alike.
Elsewhere, replay
+ the order Boot registers its filters in
(ServletContextInitializerBeans,
+ disabled ones skipped, matchAfter ones last), then append
what else the
+ Servlet API reports, unnumbered because no portable API
exposes its
+ position. Either way the spec chains URL-pattern matches
before
+ servlet-name matches, hence the stable sorts. --%>
+ <g:set var="tomcatContext"
+ value="${ { ->
+ try {
+
applicationContext.webServer.tomcat.host.findChildren().find { it.path ==
request.contextPath }
+ } catch (Throwable ignored) {
+ null
+ }
+ }() }"/>
+ <g:def type="List" var="servletFilters"
+ value="${tomcatContext
+ ? tomcatContext.findFilterMaps().toList()
+ .inject([:]) { Map acc, fm ->
Review Comment:
Grouping by filter name before separating URL-pattern mappings from
servlet-name mappings drops chain order. Tomcat builds the chain as URL-pattern
matches first, then servlet-name matches. With maps ordered `A ->
dispatcherServlet`, `B -> /*`, `A -> /*`, this page lists A then B, but Tomcat
runs B then A. A filter with both kinds of mapping can also sit in different
positions for different URLs, so one unconditional ordinal is not a universal
chain.
Preserve mapping-level order, apply the URL-then-servlet-name split before
dedupe, and add that mixed-mapping case. The same block in
`grails-profiles/web/skeleton/grails-app/views/index.gsp` has the same bug.
##########
grails-forge/grails-forge-core/src/main/resources/gsp/index.gsp:
##########
@@ -577,25 +577,57 @@
<g:def type="List" var="mimeTypeProviders"
value="${applicationContext.getBeansOfType(grails.web.mime.MimeTypeProvider)
.entrySet().toList().sort {
it.key.toLowerCase() }}"/>
- <%-- The filters still on the call stack ARE this request's
pipeline, in
- execution order: walk the reversed stack, keep Filter
classes, collapse
- the extra frames a filter contributes through its
abstract bases, and
- number what remains. No registry can report this actual
order. --%>
- <g:def type="List" var="requestFilters"
-
value="${Thread.currentThread().stackTrace.toList().reverse()
- .findResults { ste ->
- def cls = null
- try { cls = Class.forName(ste.className,
false, Thread.currentThread().contextClassLoader) } catch (Throwable ignored) {
}
- (cls != null &&
jakarta.servlet.Filter.isAssignableFrom(cls)) ? cls : null
- }
- .inject([]) { acc, cls ->
- Class prev = acc ? (Class) acc[-1] : null
- if (prev == cls) { return acc }
- if (prev != null &&
prev.isAssignableFrom(cls)) { acc[-1] = cls; return acc }
- if (prev != null &&
cls.isAssignableFrom(prev)) { return acc }
- acc << cls
- }
- .unique()}"/>
+ <%-- Every filter running in the container, in chain order.
Tomcat's filter
+ maps are the exact chain: FilterRegistrationBeans, plain
Filter beans Boot
+ adapted and container-added filters like WsFilter alike.
Elsewhere, replay
+ the order Boot registers its filters in
(ServletContextInitializerBeans,
+ disabled ones skipped, matchAfter ones last), then append
what else the
+ Servlet API reports, unnumbered because no portable API
exposes its
+ position. Either way the spec chains URL-pattern matches
before
+ servlet-name matches, hence the stable sorts. --%>
+ <g:set var="tomcatContext"
+ value="${ { ->
+ try {
+
applicationContext.webServer.tomcat.host.findChildren().find { it.path ==
request.contextPath }
+ } catch (Throwable ignored) {
+ null
+ }
+ }() }"/>
+ <g:def type="List" var="servletFilters"
+ value="${tomcatContext
+ ? tomcatContext.findFilterMaps().toList()
+ .inject([:]) { Map acc, fm ->
+ Map row =
acc.computeIfAbsent(fm.filterName) { n ->
+ [name: n, className:
tomcatContext.findFilterDef(n)?.filterClass ?: '', urlPatterns: [], mappings:
[], ordered: true]
+ }
+ row.urlPatterns.addAll(fm.URLPatterns)
+ row.mappings.addAll(fm.URLPatterns)
+ row.mappings.addAll(fm.servletNames)
+ acc
+ }
+ .values().toList()
+ .sort { it.urlPatterns ? 0 : 1 }
+ : { ->
+ List springFilters = new
org.springframework.boot.web.servlet.ServletContextInitializerBeans(
+
(org.springframework.beans.factory.ListableBeanFactory)
applicationContext).toList()
+ .findAll { it instanceof
org.springframework.boot.web.servlet.AbstractFilterRegistrationBean &&
it.enabled }
+ .collect { initializer ->
+ def rb =
(org.springframework.boot.web.servlet.AbstractFilterRegistrationBean)
initializer
+ List servletNames =
(rb.servletNames as List) + rb.servletRegistrationBeans*.servletName
+ List urlPatterns =
(rb.urlPatterns || servletNames) ? rb.urlPatterns as List : ['/*']
+ [name: rb.filterName,
className: rb.filter?.getClass()?.name ?: '', urlPatterns: urlPatterns,
+ mappings: urlPatterns +
servletNames, matchAfter: rb.matchAfter, ordered: true]
+ }
+ .sort { (it.urlPatterns ? 0 : 2) +
(it.matchAfter ? 1 : 0) }
Review Comment:
This fallback numbers reconstructed Spring registrations and then appends
every other filter. That invents positions the Servlet API does not expose. A
container mapping order of `external, springA, springB` displays as `springA,
springB, external` with ordinals `1, 2, --`. This path also runs when the
embedded-Tomcat lookup fails, including an external container.
Do not assign global execution ordinals here. If Spring registration order
is kept, label it as registration order, not the container chain. Same code in
the profile skeleton.
##########
grails-profiles/web/skeleton/grails-app/views/index.gsp:
##########
@@ -577,25 +577,57 @@
<g:def type="List" var="mimeTypeProviders"
value="${applicationContext.getBeansOfType(grails.web.mime.MimeTypeProvider)
.entrySet().toList().sort {
it.key.toLowerCase() }}"/>
- <%-- The filters still on the call stack ARE this request's
pipeline, in
- execution order: walk the reversed stack, keep Filter
classes, collapse
- the extra frames a filter contributes through its
abstract bases, and
- number what remains. No registry can report this actual
order. --%>
- <g:def type="List" var="requestFilters"
-
value="${Thread.currentThread().stackTrace.toList().reverse()
- .findResults { ste ->
- def cls = null
- try { cls = Class.forName(ste.className,
false, Thread.currentThread().contextClassLoader) } catch (Throwable ignored) {
}
- (cls != null &&
jakarta.servlet.Filter.isAssignableFrom(cls)) ? cls : null
- }
- .inject([]) { acc, cls ->
- Class prev = acc ? (Class) acc[-1] : null
- if (prev == cls) { return acc }
- if (prev != null &&
prev.isAssignableFrom(cls)) { acc[-1] = cls; return acc }
- if (prev != null &&
cls.isAssignableFrom(prev)) { return acc }
- acc << cls
- }
- .unique()}"/>
+ <%-- Every filter running in the container, in chain order.
Tomcat's filter
+ maps are the exact chain: FilterRegistrationBeans, plain
Filter beans Boot
+ adapted and container-added filters like WsFilter alike.
Elsewhere, replay
+ the order Boot registers its filters in
(ServletContextInitializerBeans,
+ disabled ones skipped, matchAfter ones last), then append
what else the
+ Servlet API reports, unnumbered because no portable API
exposes its
+ position. Either way the spec chains URL-pattern matches
before
+ servlet-name matches, hence the stable sorts. --%>
+ <g:set var="tomcatContext"
+ value="${ { ->
+ try {
+
applicationContext.webServer.tomcat.host.findChildren().find { it.path ==
request.contextPath }
+ } catch (Throwable ignored) {
+ null
+ }
+ }() }"/>
+ <g:def type="List" var="servletFilters"
+ value="${tomcatContext
+ ? tomcatContext.findFilterMaps().toList()
+ .inject([:]) { Map acc, fm ->
Review Comment:
Same defect as the Forge `index.gsp`. Grouping by filter name before the
URL-pattern vs servlet-name split lists `A, B` for maps ordered `A ->
dispatcherServlet`, `B -> /*`, `A -> /*`, while Tomcat's URL-first chain runs
`B, A`. Keep the two templates in sync when this is fixed.
##########
grails-forge/grails-forge-core/src/main/resources/gsp/index.gsp:
##########
@@ -577,25 +577,57 @@
<g:def type="List" var="mimeTypeProviders"
value="${applicationContext.getBeansOfType(grails.web.mime.MimeTypeProvider)
.entrySet().toList().sort {
it.key.toLowerCase() }}"/>
- <%-- The filters still on the call stack ARE this request's
pipeline, in
- execution order: walk the reversed stack, keep Filter
classes, collapse
- the extra frames a filter contributes through its
abstract bases, and
- number what remains. No registry can report this actual
order. --%>
- <g:def type="List" var="requestFilters"
-
value="${Thread.currentThread().stackTrace.toList().reverse()
- .findResults { ste ->
- def cls = null
- try { cls = Class.forName(ste.className,
false, Thread.currentThread().contextClassLoader) } catch (Throwable ignored) {
}
- (cls != null &&
jakarta.servlet.Filter.isAssignableFrom(cls)) ? cls : null
- }
- .inject([]) { acc, cls ->
- Class prev = acc ? (Class) acc[-1] : null
- if (prev == cls) { return acc }
- if (prev != null &&
prev.isAssignableFrom(cls)) { acc[-1] = cls; return acc }
- if (prev != null &&
cls.isAssignableFrom(prev)) { return acc }
- acc << cls
- }
- .unique()}"/>
+ <%-- Every filter running in the container, in chain order.
Tomcat's filter
+ maps are the exact chain: FilterRegistrationBeans, plain
Filter beans Boot
+ adapted and container-added filters like WsFilter alike.
Elsewhere, replay
+ the order Boot registers its filters in
(ServletContextInitializerBeans,
+ disabled ones skipped, matchAfter ones last), then append
what else the
+ Servlet API reports, unnumbered because no portable API
exposes its
+ position. Either way the spec chains URL-pattern matches
before
+ servlet-name matches, hence the stable sorts. --%>
+ <g:set var="tomcatContext"
+ value="${ { ->
+ try {
+
applicationContext.webServer.tomcat.host.findChildren().find { it.path ==
request.contextPath }
+ } catch (Throwable ignored) {
+ null
+ }
+ }() }"/>
+ <g:def type="List" var="servletFilters"
+ value="${tomcatContext
+ ? tomcatContext.findFilterMaps().toList()
+ .inject([:]) { Map acc, fm ->
+ Map row =
acc.computeIfAbsent(fm.filterName) { n ->
+ [name: n, className:
tomcatContext.findFilterDef(n)?.filterClass ?: '', urlPatterns: [], mappings:
[], ordered: true]
+ }
+ row.urlPatterns.addAll(fm.URLPatterns)
+ row.mappings.addAll(fm.URLPatterns)
+ row.mappings.addAll(fm.servletNames)
Review Comment:
Non-blocking, but related. Tomcat stores a wildcard `*` mapping in
`matchAllUrlPatterns` / `matchAllServletNames` and leaves the arrays empty
(confirmed against Tomcat 11). Reading only `URLPatterns` and `servletNames`
drops those mappings, and a match-all URL mapping is then sorted as if it had
no URL mapping. Read the match-all flags in both templates.
##########
grails-forge/grails-forge-core/src/test/groovy/org/grails/forge/feature/view/GrailsGspSpec.groovy:
##########
@@ -250,11 +250,13 @@ class GrailsGspSpec extends ApplicationContextSpec
implements CommandOutputFixtu
index.contains('mappingContext.eventListeners')
index.contains('<g:message code="welcome.datastores.listeners"/>')
- and: "the request's effective filter pipeline is derived from the
rendering call stack"
+ and: "servlet filters list every filter in the container in chain
order, with a portable fallback"
index.contains('data-switch-type="filters"')
- index.contains('Thread.currentThread().stackTrace')
- index.contains('jakarta.servlet.Filter.isAssignableFrom')
- index.contains('<g:message code="welcome.filters.request"/>')
+ index.contains('tomcatContext.findFilterMaps()')
Review Comment:
Non-blocking on its own, but these assertions only check that source strings
are present. Dropping the disabled-registration filter, changing the sort, or
omitting mapping collection would still pass. A render-level test for mixed
URL/servlet-name mappings, a disabled registration, and a non-Spring filter
would have caught the order bugs above.
##########
grails-profiles/web/skeleton/grails-app/views/index.gsp:
##########
@@ -577,25 +577,57 @@
<g:def type="List" var="mimeTypeProviders"
value="${applicationContext.getBeansOfType(grails.web.mime.MimeTypeProvider)
.entrySet().toList().sort {
it.key.toLowerCase() }}"/>
- <%-- The filters still on the call stack ARE this request's
pipeline, in
- execution order: walk the reversed stack, keep Filter
classes, collapse
- the extra frames a filter contributes through its
abstract bases, and
- number what remains. No registry can report this actual
order. --%>
- <g:def type="List" var="requestFilters"
-
value="${Thread.currentThread().stackTrace.toList().reverse()
- .findResults { ste ->
- def cls = null
- try { cls = Class.forName(ste.className,
false, Thread.currentThread().contextClassLoader) } catch (Throwable ignored) {
}
- (cls != null &&
jakarta.servlet.Filter.isAssignableFrom(cls)) ? cls : null
- }
- .inject([]) { acc, cls ->
- Class prev = acc ? (Class) acc[-1] : null
- if (prev == cls) { return acc }
- if (prev != null &&
prev.isAssignableFrom(cls)) { acc[-1] = cls; return acc }
- if (prev != null &&
cls.isAssignableFrom(prev)) { return acc }
- acc << cls
- }
- .unique()}"/>
+ <%-- Every filter running in the container, in chain order.
Tomcat's filter
+ maps are the exact chain: FilterRegistrationBeans, plain
Filter beans Boot
+ adapted and container-added filters like WsFilter alike.
Elsewhere, replay
+ the order Boot registers its filters in
(ServletContextInitializerBeans,
+ disabled ones skipped, matchAfter ones last), then append
what else the
+ Servlet API reports, unnumbered because no portable API
exposes its
+ position. Either way the spec chains URL-pattern matches
before
+ servlet-name matches, hence the stable sorts. --%>
+ <g:set var="tomcatContext"
+ value="${ { ->
+ try {
+
applicationContext.webServer.tomcat.host.findChildren().find { it.path ==
request.contextPath }
+ } catch (Throwable ignored) {
+ null
+ }
+ }() }"/>
+ <g:def type="List" var="servletFilters"
+ value="${tomcatContext
+ ? tomcatContext.findFilterMaps().toList()
+ .inject([:]) { Map acc, fm ->
+ Map row =
acc.computeIfAbsent(fm.filterName) { n ->
+ [name: n, className:
tomcatContext.findFilterDef(n)?.filterClass ?: '', urlPatterns: [], mappings:
[], ordered: true]
+ }
+ row.urlPatterns.addAll(fm.URLPatterns)
+ row.mappings.addAll(fm.URLPatterns)
+ row.mappings.addAll(fm.servletNames)
+ acc
+ }
+ .values().toList()
+ .sort { it.urlPatterns ? 0 : 1 }
+ : { ->
+ List springFilters = new
org.springframework.boot.web.servlet.ServletContextInitializerBeans(
+
(org.springframework.beans.factory.ListableBeanFactory)
applicationContext).toList()
+ .findAll { it instanceof
org.springframework.boot.web.servlet.AbstractFilterRegistrationBean &&
it.enabled }
+ .collect { initializer ->
+ def rb =
(org.springframework.boot.web.servlet.AbstractFilterRegistrationBean)
initializer
+ List servletNames =
(rb.servletNames as List) + rb.servletRegistrationBeans*.servletName
+ List urlPatterns =
(rb.urlPatterns || servletNames) ? rb.urlPatterns as List : ['/*']
+ [name: rb.filterName,
className: rb.filter?.getClass()?.name ?: '', urlPatterns: urlPatterns,
+ mappings: urlPatterns +
servletNames, matchAfter: rb.matchAfter, ordered: true]
+ }
+ .sort { (it.urlPatterns ? 0 : 2) +
(it.matchAfter ? 1 : 0) }
Review Comment:
Same fallback as the Forge template. Numbering Spring registrations and
appending other filters afterward is not the container chain. A real order of
`external, springA, springB` shows up as `springA, springB, external`. Do not
present those ordinals as execution order.
--
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]
