jdaugherty commented on code in PR #15585: URL: https://github.com/apache/grails-core/pull/15585#discussion_r3130667170
########## grails-gsp/grails-sitemesh3/src/main/java/org/grails/plugins/sitemesh3/Sitemesh3CapturedPage.java: ########## @@ -0,0 +1,359 @@ +/* + * 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.sitemesh3; + +import java.io.IOException; +import java.io.Writer; +import java.nio.CharBuffer; +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.Map; + +import org.sitemesh.content.Content; +import org.sitemesh.content.ContentChunk; +import org.sitemesh.content.ContentProperty; +import org.sitemesh.content.memory.InMemoryContent; +import org.sitemesh.tagprocessor.CharSequenceBuffer; + +import org.grails.buffer.StreamCharBuffer; + +/** + * A SiteMesh 3 {@link Content} implementation that is populated by the GSP + * capture taglib at render time. Because the capture taglib runs during GSP + * execution, there is no need for SiteMesh to parse the response body; the + * data is already chunked up. + * + * <p>Backed by an {@link InMemoryContent} so that SiteMesh content properties + * can be traversed in the usual way (e.g. {@code head}, {@code body}, {@code + * title}, {@code page.<name>}, {@code meta.<name>}).</p> + */ +public class Sitemesh3CapturedPage implements Content { + + public static final String REQUEST_ATTRIBUTE = Sitemesh3CapturedPage.class.getName(); + + private final InMemoryContent delegate = new InMemoryContent(); + + private StreamCharBuffer headBuffer; + private StreamCharBuffer bodyBuffer; + private StreamCharBuffer titleBuffer; + private StreamCharBuffer pageBuffer; + private CharSequence renderedContent; + + private final Map<String, StreamCharBuffer> contentBuffers = new LinkedHashMap<>(); + private final Map<String, String> pageProperties = new HashMap<>(); + + private boolean used; + private boolean titleCaptured; + // Volatile because a captured page can be passed to an async dispatch + // thread (Grails 7 supports @Async controller returns and + // Callable-returning actions). Without volatile, the JMM gives no + // happens-before guarantee on the flag across threads, and two threads + // could race to materialize the property tree. + private volatile boolean propertiesMaterialized; + + public void setHeadBuffer(StreamCharBuffer buffer) { + this.headBuffer = buffer; + markUsed(); + } + + public void setBodyBuffer(StreamCharBuffer buffer) { + this.bodyBuffer = buffer; + markUsed(); + } + + public void setTitleBuffer(StreamCharBuffer buffer) { + this.titleBuffer = buffer; + } + + public void setPageBuffer(StreamCharBuffer buffer) { + this.pageBuffer = buffer; + } + + // Attaches fully-rendered content (e.g. a layout's output after + // inline-expanded taglibs have run) as the page's data, bypassing the + // HTML parse step that would otherwise build the data from captured + // buffers. Held as a CharSequence so callers can pass a CharBuffer + // straight through without allocating an intermediate String — the + // RawDataChunk writes via Writer.write(char[], int, int) when possible. + public void setRenderedContent(CharSequence content) { + this.renderedContent = content; + markUsed(); + } + + public StreamCharBuffer getHeadBuffer() { + return headBuffer; + } + + public StreamCharBuffer getBodyBuffer() { + return bodyBuffer; + } + + public StreamCharBuffer getTitleBuffer() { + return titleBuffer; + } + + public StreamCharBuffer getPageBuffer() { + return pageBuffer; + } + + public void addContentBuffer(String tag, StreamCharBuffer buffer) { + contentBuffers.put(tag, buffer); + markUsed(); + } + + public void addProperty(String name, String value) { + if (name == null || value == null) { + return; + } + pageProperties.put(name, value); + markUsed(); + } + + public boolean isUsed() { + return used; + } + + public void markUsed() { + this.used = true; + } + + public boolean isTitleCaptured() { + return titleCaptured; + } + + public void setTitleCaptured(boolean titleCaptured) { + this.titleCaptured = titleCaptured; + } + + /** + * Writes the full original page (unmerged) to the given appendable. + * Used when decoration is skipped and the caller needs to fall back to + * the raw response. + */ + public void writeOriginal(Appendable out) throws IOException { + if (pageBuffer != null) { + pageBuffer.writeTo(appendableToWriter(out)); + } + } + + @Override + public ContentChunk getData() { + materializeProperties(); + if (renderedContent != null) { + return new RawDataChunk(renderedContent, this); + } + return delegate.getData(); + } + + @Override + public ContentProperty getExtractedProperties() { + materializeProperties(); + return delegate.getExtractedProperties(); + } + + @Override + public CharSequenceBuffer createDataOnlyBuffer() { + return delegate.createDataOnlyBuffer(); + } + + private void materializeProperties() { + if (propertiesMaterialized) { + return; + } + propertiesMaterialized = true; Review Comment: The `propertiesMaterialized` flag protects against *redundant* materialization but not *concurrent* materialization: two async-dispatch threads can both see `false`, both set it to `true`, and both walk the delegate's property tree, which is not synchronized. In the worst case the `InMemoryContent` delegate ends up with partially-initialized children. Consider wrapping the body in `synchronized(this)` with a double-checked pattern, or pre-materializing once on write rather than on read. ########## grails-gsp/grails-sitemesh3/src/main/java/org/grails/plugins/sitemesh3/Sitemesh3LayoutFinder.java: ########## @@ -0,0 +1,255 @@ +/* + * 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.sitemesh3; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +import groovy.lang.GroovyObject; + +import jakarta.servlet.http.HttpServletRequest; + +import org.sitemesh.DecoratorSelector; +import org.sitemesh.SiteMeshContext; +import org.sitemesh.content.Content; +import org.sitemesh.content.ContentProperty; +import org.sitemesh.webapp.WebAppContext; + +import grails.util.Environment; +import grails.util.GrailsClassUtils; +import grails.util.GrailsNameUtils; +import grails.util.GrailsStringUtils; +import org.grails.core.artefact.ControllerArtefactHandler; +import org.grails.gsp.io.GroovyPageScriptSource; +import org.grails.io.support.GrailsResourceUtils; +import org.grails.web.gsp.io.GrailsConventionGroovyPageLocator; +import org.grails.web.servlet.mvc.GrailsWebRequest; +import org.grails.web.util.GrailsApplicationAttributes; +import org.grails.web.util.WebUtils; + +/** + * {@link DecoratorSelector} that resolves layout paths using Grails + * conventions. The resolution order is: + * + * <ol> + * <li>Request attribute {@link WebUtils#LAYOUT_ATTRIBUTE}</li> + * <li>Content {@code meta.layout} property</li> + * <li>Controller's {@code static layout = 'x'} property</li> + * <li>{@code /layouts/<controllerName>/<actionUri>.gsp}</li> + * <li>{@code /layouts/<controllerName>.gsp}</li> + * <li>Configured default (e.g. {@code grails.sitemesh.default.layout})</li> + * </ol> + * + * <p>Results are cached by (controllerName, actionUri) outside of the + * DEVELOPMENT environment.</p> + */ +public class Sitemesh3LayoutFinder implements DecoratorSelector<SiteMeshContext> { + + private static final String LAYOUTS_PATH = "/layouts"; + private static final long LAYOUT_CACHE_EXPIRATION_MILLIS = Long.getLong("grails.gsp.reload.interval", 5000); Review Comment: Two concerns here: 1. Reading via `Long.getLong("grails.gsp.reload.interval", ...)` reaches into JVM system properties, which is unusual for Grails — the rest of the plugin reads from `grailsApplication.config`. This key won't show up when a user greps application.yml/config.groovy for `grails.gsp`. 2. The name `grails.gsp.reload.interval` suggests *GSP* reload timing, not *layout-cache* expiration. Using the same property for two unrelated concepts will be confusing if someone tunes one expecting the other. Consider a dedicated key like `grails.sitemesh.layout.cache.interval`, read through `grailsApplication.config`, with the value passed in via a setter (same pattern as `gspReloadEnabled` / `defaultDecoratorName`). ########## grails-gsp/grails-sitemesh3/src/main/groovy/org/grails/plugins/sitemesh3/Sitemesh3LayoutTagLib.groovy: ########## @@ -0,0 +1,208 @@ +/* + * 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.sitemesh3 + +import groovy.transform.CompileStatic + +import jakarta.servlet.http.HttpServletRequest + +import grails.artefact.TagLibrary +import grails.gsp.TagLib +import org.grails.buffer.FastStringWriter +import org.grails.buffer.GrailsPrintWriter +import org.grails.buffer.StreamCharBuffer +import org.grails.encoder.CodecLookup +import org.grails.encoder.Encoder +import org.grails.gsp.compiler.GrailsLayoutPreprocessor + +/** + * SiteMesh 3 counterpart of {@code GrailsLayoutTagLib}: populates a + * {@link Sitemesh3CapturedPage} at GSP render time so that SiteMesh 3 can + * decorate without parsing the HTML. + * + * <p>Registered under the {@code grailsLayout} namespace because the GSP + * compile-time preprocessor ({@link GrailsLayoutPreprocessor}) rewrites + * {@code <head>}, {@code <body>}, etc. to {@code <grailsLayout:capture*>} + * tags.</p> + */ +@CompileStatic +@TagLib +class Sitemesh3LayoutTagLib implements TagLibrary { + + static String namespace = 'grailsLayout' + + CodecLookup codecLookup + + def captureTagContent(GrailsPrintWriter writer, String tagname, Map attrs, Object body, boolean noEndTagForEmpty = false) { Review Comment: Let's add documentation to these new tags? ########## grails-gsp/grails-sitemesh3/src/main/groovy/org/grails/plugins/sitemesh3/Sitemesh3GrailsPlugin.groovy: ########## @@ -48,55 +55,73 @@ class Sitemesh3GrailsPlugin extends Plugin { def loadBefore = ['groovyPages'] def providedArtefacts = [ - RenderSitemeshTagLib, + RenderSitemeshTagLib, + Sitemesh3LayoutTagLib, ] static PropertySource getDefaultPropertySource(ConfigurableEnvironment configurableEnvironment, String defaultLayout) { - Map props = [ - 'grails.gsp.view.layoutViewResolver': 'false', 'sitemesh.decorator.metaTag': 'layout', 'sitemesh.decorator.attribute': WebUtils.LAYOUT_ATTRIBUTE, 'sitemesh.decorator.prefix': '/layouts/', - 'sitemesh.filter.order': GrailsFilters.SITEMESH_FILTER.order, - 'sitemesh.decorator.tagRuleBundles': ['org.sitemesh.content.tagrules.html.Sm2TagRuleBundle'] ] if (defaultLayout) { props['sitemesh.decorator.default'] = defaultLayout } - // if property already exists, don't override props.clone().each { if (configurableEnvironment.getProperty(it.key)) { props.remove(it.key) } } - return new MapPropertySource('defaultSitemesh3Properties', props) + new MapPropertySource('defaultSitemesh3Properties', props) } Closure doWithSpring() { { -> ConfigurableEnvironment configurableEnvironment = grailsApplication.mainContext.environment as ConfigurableEnvironment def propertySources = configurableEnvironment.getPropertySources() - // https://grails.apache.org/docs/latest/guide/single.html#layouts - // Default view should be application, but it is inefficient to add a rule for a page that may not exist. String defaultLayout = grailsApplication.getConfig().getProperty('grails.sitemesh.default.layout') propertySources.addFirst(getDefaultPropertySource(configurableEnvironment, defaultLayout)) - propertySources.addFirst(new MapPropertySource('requiredSitemesh3Properties', [ - (GroovyPageParser.CONFIG_PROPERTY_GSP_GRAILS_LAYOUT_PREPROCESS): 'false' - ])) (grailsApplication as DefaultGrailsApplication).config = new PropertySourcesConfig(propertySources) - grailsLayoutHandlerMapping(GrailsLayoutHandlerMapping) + Config config = grailsApplication.getConfig() + boolean developmentMode = Metadata.getCurrent().isDevelopmentEnvironmentAvailable() + Environment env = Environment.current + boolean enableReload = env.isReloadEnabled() || + config.getProperty('grails.gsp.enable.reload', Boolean, false) || + (developmentMode && env == Environment.DEVELOPMENT) + String resolvedDefaultLayout = config.getProperty('grails.sitemesh.default.layout') ?: + config.getProperty('sitemesh.decorator.default') Review Comment: Nit: `defaultLayout` was already resolved on line 83 from the same config key. The fallback to `sitemesh.decorator.default` here is redundant because line 84 just added a property source that writes `sitemesh.decorator.default` = `defaultLayout` whenever `defaultLayout` is truthy. You can collapse this to `String resolvedDefaultLayout = defaultLayout`. ########## grails-gsp/grails-sitemesh3/src/main/groovy/org/grails/plugins/web/taglib/RenderSitemeshTagLib.groovy: ########## @@ -16,78 +16,105 @@ * specific language governing permissions and limitations * under the License. */ - package org.grails.plugins.web.taglib import java.nio.CharBuffer +import org.sitemesh.DecoratorSelector +import org.sitemesh.SiteMeshContext import org.sitemesh.content.Content +import org.sitemesh.content.ContentProcessor import org.sitemesh.content.ContentProperty -import org.sitemesh.webapp.SiteMeshFilter import org.sitemesh.webapp.WebAppContext import org.sitemesh.webapp.contentfilter.ResponseMetaData import org.springframework.beans.factory.annotation.Autowired import org.springframework.beans.factory.annotation.Qualifier -import org.springframework.boot.web.servlet.FilterRegistrationBean +import org.springframework.context.annotation.Lazy +import org.springframework.web.servlet.ViewResolver import grails.artefact.TagLibrary import grails.gsp.TagLib +import org.grails.plugins.sitemesh3.GrailsSiteMeshViewContext import org.grails.web.util.WebUtils /** - * The tags in this library are rendered by sitemesh itself instead of the grails tags so they should always be written - * as a 'sitemesh' namespace. + * Tags rendered by SiteMesh itself (not the Grails taglib pipeline) when a + * layout GSP is being processed. Kept in the {@code sitemesh} namespace. */ @TagLib class RenderSitemeshTagLib implements TagLibrary { - SiteMeshFilter siteMeshFilter - @Autowired - RenderSitemeshTagLib(@Qualifier('sitemesh') FilterRegistrationBean sitemesh) { - this.siteMeshFilter = (SiteMeshFilter) sitemesh.getFilter() - } + ContentProcessor contentProcessor + @Autowired + DecoratorSelector<SiteMeshContext> decoratorSelector + + // Break the circular dependency + // RenderSitemeshTagLib -> ViewResolver -> groovyPagesTemplateEngine -> + // gspTagLibraryLookup -> RenderSitemeshTagLib by deferring resolution. + // @Qualifier is required because the context has several ViewResolver + // beans (mvcViewResolver, beanNameViewResolver, groovyMarkupViewResolver, + // jspViewResolver, gspViewResolver) and autowiring by type is ambiguous. + @Autowired + @Lazy + @Qualifier('jspViewResolver') + ViewResolver viewResolver + + // Dispatches via GrailsSiteMeshViewContext so the layout is rendered + // through Spring's View API rather than RequestDispatcher.forward(). + // Using the default WebAppContext here would re-enter the servlet + // pipeline on every <g:applyLayout> call, and nesting (applyLayout + // inside applyLayout inside a Sitemesh3LayoutView render) would tear + // down the outer request scope before the outer render finished — + // causing "request is not active anymore" errors. Closure applyLayout = { Map attrs, body -> String savedAttribute = request.getAttribute(WebUtils.LAYOUT_ATTRIBUTE) - WebAppContext context = new WebAppContext('text/html', request, response, - servletContext, siteMeshFilter.contentProcessor, new ResponseMetaData(), false) - Content content = siteMeshFilter.contentProcessor.build(CharBuffer.wrap(body()), context) - if (attrs.name) { - request.setAttribute(WebUtils.LAYOUT_ATTRIBUTE, attrs.name) - } - String[] decoratorPaths = siteMeshFilter.decoratorSelector.selectDecoratorPaths(content, context) - for (String decoratorPath : decoratorPaths) { - content = context.decorate(decoratorPath, content) + GrailsSiteMeshViewContext context = new GrailsSiteMeshViewContext( + 'text/html', request, response, servletContext, + contentProcessor, new ResponseMetaData(), false, + viewResolver, request.getLocale()) + try { + Content content = contentProcessor.build(CharBuffer.wrap(body()), context) + if (attrs.name) { + request.setAttribute(WebUtils.LAYOUT_ATTRIBUTE, attrs.name) + } + String[] decoratorPaths = decoratorSelector.selectDecoratorPaths(content, context) + for (String decoratorPath : decoratorPaths) { + Content next = context.decorate(decoratorPath, content) + if (next == null) { + break + } + content = next + } + if (content != null) { + content.getData().writeValueTo(out) + } + } finally { + if (savedAttribute != null) { + request.setAttribute(WebUtils.LAYOUT_ATTRIBUTE, savedAttribute) + } else { + request.removeAttribute(WebUtils.LAYOUT_ATTRIBUTE) + } } - content.getData().writeValueTo(out) - request.setAttribute(WebUtils.LAYOUT_ATTRIBUTE, savedAttribute) } private ContentProperty getContentProperty(String name) { if (!name) { return null } - Content content = request.getAttribute(WebAppContext.CONTENT_KEY) + Content content = (Content) request.getAttribute(WebAppContext.CONTENT_KEY) + if (content == null) { + return null + } ContentProperty currentProperty = content.getExtractedProperties() for (String childPropertyName : name.split('\\.')) { currentProperty = currentProperty.getChild(childPropertyName) } currentProperty } - /** - * Used to retrieve a property of the decorated page.<br/> - * - * <g:pageProperty default="defaultValue" name="body.onload" /><br/> - * - * @emptyTag - * - * @attr REQUIRED name the property name - * @attr default the default value to use if the property is null - * @attr writeEntireProperty if true, writes the property in the form 'foo = "bar"', otherwise renders 'bar' - */ Closure pageProperty = { attrs -> Review Comment: Why remove the documentation? ########## grails-gsp/grails-sitemesh3/src/main/java/org/grails/plugins/sitemesh3/Sitemesh3CapturedPage.java: ########## @@ -0,0 +1,359 @@ +/* + * 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.sitemesh3; + +import java.io.IOException; +import java.io.Writer; +import java.nio.CharBuffer; +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.Map; + +import org.sitemesh.content.Content; +import org.sitemesh.content.ContentChunk; +import org.sitemesh.content.ContentProperty; +import org.sitemesh.content.memory.InMemoryContent; +import org.sitemesh.tagprocessor.CharSequenceBuffer; + +import org.grails.buffer.StreamCharBuffer; + +/** + * A SiteMesh 3 {@link Content} implementation that is populated by the GSP + * capture taglib at render time. Because the capture taglib runs during GSP + * execution, there is no need for SiteMesh to parse the response body; the + * data is already chunked up. + * + * <p>Backed by an {@link InMemoryContent} so that SiteMesh content properties + * can be traversed in the usual way (e.g. {@code head}, {@code body}, {@code + * title}, {@code page.<name>}, {@code meta.<name>}).</p> + */ +public class Sitemesh3CapturedPage implements Content { + + public static final String REQUEST_ATTRIBUTE = Sitemesh3CapturedPage.class.getName(); + + private final InMemoryContent delegate = new InMemoryContent(); + + private StreamCharBuffer headBuffer; + private StreamCharBuffer bodyBuffer; + private StreamCharBuffer titleBuffer; + private StreamCharBuffer pageBuffer; + private CharSequence renderedContent; + + private final Map<String, StreamCharBuffer> contentBuffers = new LinkedHashMap<>(); + private final Map<String, String> pageProperties = new HashMap<>(); + + private boolean used; Review Comment: `used` (and `titleCaptured` below) should also be `volatile` for the same reason `propertiesMaterialized` is, per the comment on line 63. `isUsed()` is read by `CaptureAwareContentProcessor.build(...)` to pick the short-circuit vs. fallback path — and in the async-dispatch scenario called out in the javadoc, the capture taglibs may run on one thread and `CaptureAwareContentProcessor` may run on another. A plain boolean read there has no happens-before with the write in `markUsed()`, so the short-circuit could be missed (or worse, taken with a partially-written state in the rest of the object). ########## grails-gsp/grails-sitemesh3/src/main/groovy/org/grails/plugins/sitemesh3/Sitemesh3GrailsPlugin.groovy: ########## @@ -48,55 +55,73 @@ class Sitemesh3GrailsPlugin extends Plugin { def loadBefore = ['groovyPages'] def providedArtefacts = [ - RenderSitemeshTagLib, + RenderSitemeshTagLib, + Sitemesh3LayoutTagLib, ] static PropertySource getDefaultPropertySource(ConfigurableEnvironment configurableEnvironment, String defaultLayout) { - Map props = [ - 'grails.gsp.view.layoutViewResolver': 'false', 'sitemesh.decorator.metaTag': 'layout', 'sitemesh.decorator.attribute': WebUtils.LAYOUT_ATTRIBUTE, 'sitemesh.decorator.prefix': '/layouts/', - 'sitemesh.filter.order': GrailsFilters.SITEMESH_FILTER.order, - 'sitemesh.decorator.tagRuleBundles': ['org.sitemesh.content.tagrules.html.Sm2TagRuleBundle'] ] if (defaultLayout) { props['sitemesh.decorator.default'] = defaultLayout } - // if property already exists, don't override props.clone().each { if (configurableEnvironment.getProperty(it.key)) { props.remove(it.key) } } - return new MapPropertySource('defaultSitemesh3Properties', props) + new MapPropertySource('defaultSitemesh3Properties', props) } Closure doWithSpring() { { -> ConfigurableEnvironment configurableEnvironment = grailsApplication.mainContext.environment as ConfigurableEnvironment def propertySources = configurableEnvironment.getPropertySources() - // https://grails.apache.org/docs/latest/guide/single.html#layouts - // Default view should be application, but it is inefficient to add a rule for a page that may not exist. String defaultLayout = grailsApplication.getConfig().getProperty('grails.sitemesh.default.layout') propertySources.addFirst(getDefaultPropertySource(configurableEnvironment, defaultLayout)) - propertySources.addFirst(new MapPropertySource('requiredSitemesh3Properties', [ - (GroovyPageParser.CONFIG_PROPERTY_GSP_GRAILS_LAYOUT_PREPROCESS): 'false' - ])) (grailsApplication as DefaultGrailsApplication).config = new PropertySourcesConfig(propertySources) - grailsLayoutHandlerMapping(GrailsLayoutHandlerMapping) + Config config = grailsApplication.getConfig() + boolean developmentMode = Metadata.getCurrent().isDevelopmentEnvironmentAvailable() + Environment env = Environment.current + boolean enableReload = env.isReloadEnabled() || + config.getProperty('grails.gsp.enable.reload', Boolean, false) || + (developmentMode && env == Environment.DEVELOPMENT) + String resolvedDefaultLayout = config.getProperty('grails.sitemesh.default.layout') ?: + config.getProperty('sitemesh.decorator.default') + + // Bean names match the @ConditionalOnMissingBean(name = "contentProcessor"/"decoratorSelector") + // guards on upstream's SiteMeshViewResolverAutoConfiguration, so + // our implementations replace upstream's defaults. + contentProcessor(CaptureAwareContentProcessor) + + decoratorSelector(Sitemesh3LayoutFinder, ref('groovyPageLocator')) { + gspReloadEnabled = enableReload + defaultDecoratorName = resolvedDefaultLayout ?: null + } + + // Replace the filter registration from + // org.sitemesh.autoconfigure.SiteMeshAutoConfiguration with a no-op + // filter bean under the same name. SiteMeshAutoConfiguration is + // @ConditionalOnMissingBean(name = "sitemesh") so registering this + // bean disables the upstream filter-based integration entirely. + // Decoration is done by the Spring MVC view resolver chain. + sitemesh(FilterRegistrationBean) { bean -> Review Comment: The `NoopSitemeshFilter` is never actually invoked (the registration has `enabled = false`), so the whole no-op filter class feels like scaffolding required only because the upstream bean type is `FilterRegistrationBean`. This works, but a one-line comment on `NoopSitemeshFilter` pointing to this explanation would help the next person touching it understand why an apparently-dead class exists. ########## grails-gsp/grails-sitemesh3/src/main/groovy/org/grails/plugins/web/taglib/RenderSitemeshTagLib.groovy: ########## @@ -148,40 +163,44 @@ class RenderSitemeshTagLib implements TagLibrary { } } + // layoutTitle/layoutHead/layoutBody inline-expand at tag-render time. + // This avoids emitting <sitemesh:write> placeholders that would otherwise + // require a second HTML parse of the layout output to expand. The + // property is pulled directly from the Content being merged (set on the + // request under WebAppContext.CONTENT_KEY by WebAppContext.decorate). Closure layoutTitle = { attrs -> - out << """<sitemesh:write property="title">${attrs.default ?: ''}</sitemesh:write>""".toString() + ContentProperty titleProp = getContentProperty('title') + String defaultValue = attrs.default?.toString() ?: '' + if (titleProp?.hasValue()) { + titleProp.writeValueTo(out) + } else if (defaultValue) { + out << defaultValue + } } Closure layoutHead = { attrs, body -> - StringBuilder tag = new StringBuilder('<sitemesh:write property="head"') - String bodyContent = body() - if (bodyContent) { - tag.append('>') - tag.append(bodyContent) - tag.append('</sitemesh:write>') - } else { - tag.append('/>') + ContentProperty headProp = getContentProperty('head') + if (headProp?.hasValue()) { + headProp.writeValueTo(out) + } else if (body) { + out << body() } - out << tag.toString() } Closure layoutBody = { attrs, body -> - StringBuilder tag = new StringBuilder('<sitemesh:write property="body"') - String bodyContent = body() - if (bodyContent) { - tag.append('>') - tag.append(bodyContent) - tag.append('</sitemesh:write>') - } else { - tag.append('/>') + ContentProperty bodyProp = getContentProperty('body') + if (bodyProp?.hasValue()) { + bodyProp.writeValueTo(out) + } else if (body) { + out << body() } - out << tag.toString() } Closure content = { attrs, body -> - StringBuilder tag = new StringBuilder("""<content tag="${attrs.tag}">""") - tag.append(body()) - tag.append('</content>') - out << tag.toString() + out << '<content tag="' + out << attrs.tag Review Comment: `attrs.tag` is written directly into an HTML attribute value without encoding. If a tag name ever contains `"`, `<`, or `&` the produced markup is malformed. `Sitemesh3LayoutTagLib.captureTagContent` already does the right thing via `codecLookup.lookupEncoder('HTML')` — worth mirroring here for consistency. Likely harmless in practice (tag names are usually static strings) but cheap to fix. ########## grails-gsp/grails-sitemesh3/src/main/groovy/org/grails/plugins/web/taglib/RenderSitemeshTagLib.groovy: ########## @@ -109,18 +136,6 @@ class RenderSitemeshTagLib implements TagLibrary { } } - /** - * Invokes the body of this tag if the page property exists:<br/> - * - * <g:ifPageProperty name="meta.index">body to invoke</g:ifPageProperty><br/> - * - * or it equals a certain value:<br/> - * - * <g:ifPageProperty name="meta.index" equals="blah">body to invoke</g:ifPageProperty> - * - * @attr name REQUIRED the property name - * @attr equals optional value to test against - */ Review Comment: Why remove the documentation? ########## grails-gsp/grails-sitemesh3/src/main/java/org/grails/plugins/sitemesh3/Sitemesh3LayoutFinder.java: ########## @@ -0,0 +1,255 @@ +/* + * 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.sitemesh3; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +import groovy.lang.GroovyObject; + +import jakarta.servlet.http.HttpServletRequest; + +import org.sitemesh.DecoratorSelector; +import org.sitemesh.SiteMeshContext; +import org.sitemesh.content.Content; +import org.sitemesh.content.ContentProperty; +import org.sitemesh.webapp.WebAppContext; + +import grails.util.Environment; +import grails.util.GrailsClassUtils; +import grails.util.GrailsNameUtils; +import grails.util.GrailsStringUtils; +import org.grails.core.artefact.ControllerArtefactHandler; +import org.grails.gsp.io.GroovyPageScriptSource; +import org.grails.io.support.GrailsResourceUtils; +import org.grails.web.gsp.io.GrailsConventionGroovyPageLocator; +import org.grails.web.servlet.mvc.GrailsWebRequest; +import org.grails.web.util.GrailsApplicationAttributes; +import org.grails.web.util.WebUtils; + +/** + * {@link DecoratorSelector} that resolves layout paths using Grails + * conventions. The resolution order is: + * + * <ol> + * <li>Request attribute {@link WebUtils#LAYOUT_ATTRIBUTE}</li> + * <li>Content {@code meta.layout} property</li> + * <li>Controller's {@code static layout = 'x'} property</li> + * <li>{@code /layouts/<controllerName>/<actionUri>.gsp}</li> + * <li>{@code /layouts/<controllerName>.gsp}</li> + * <li>Configured default (e.g. {@code grails.sitemesh.default.layout})</li> + * </ol> + * + * <p>Results are cached by (controllerName, actionUri) outside of the + * DEVELOPMENT environment.</p> + */ +public class Sitemesh3LayoutFinder implements DecoratorSelector<SiteMeshContext> { + + private static final String LAYOUTS_PATH = "/layouts"; + private static final long LAYOUT_CACHE_EXPIRATION_MILLIS = Long.getLong("grails.gsp.reload.interval", 5000); + + private final GrailsConventionGroovyPageLocator groovyPageLocator; + + private String defaultDecoratorName; + private boolean gspReloadEnabled; + private boolean cacheEnabled = (Environment.getCurrent() != Environment.DEVELOPMENT); + + private final Map<String, LayoutCacheValue> namedDecoratorCache = new ConcurrentHashMap<>(); + private final Map<LayoutCacheKey, LayoutCacheValue> layoutDecoratorCache = new ConcurrentHashMap<>(); Review Comment: Both caches are unbounded. In practice they're bounded by the number of distinct (controllerName, actionUri) pairs, which is fine for typical apps — but worth adding a note in the javadoc, or a bounded cache (e.g. an LRU via `LinkedHashMap` + `removeEldestEntry`) to be safe for apps with many wildcard-matched actions. Non-blocking, but worth thinking about. -- 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]
