From 2e71c6a16322705d382d96d1b7638ea8f37ca342 Mon Sep 17 00:00:00 2001 From: Kevan Date: Tue, 13 Oct 2026 11:30:21 +0200 Subject: [PATCH 01/10] fix: collect buildNodeUrl dependencies in a render filter buildNodeUrl now hands each linked path to the Java engine, which keeps it for the resource being rendered. A render filter at priority 17 registers the paths before the cache filters store the fragment. The React renderer and JSScript return to the string contract they had before. - A call that passes its own context now collects, because the collection needs only the render context. - Above 100 paths, the merge groups the paths by site and never builds a regexp above /sites/. A lone path of another site stays exact. - Paths under /modules are dropped, as the core cache filters drop them. --- .../TestAutocollectedCacheDependency.tsx | 41 +++++- .../js/server/CollectedCacheDependencies.java | 134 ++++++++++++++---- .../CollectedCacheDependenciesFilter.java | 63 ++++++++ .../engine/js/server/RenderHelper.java | 14 ++ .../modules/engine/views/JSScript.java | 26 +--- .../CollectedCacheDependenciesTest.java | 120 ++++++++++++++-- .../src/server/init-react.tsx | 16 +-- .../src/hooks/useServerContext.tsx | 7 - .../src/utils/urlBuilder/urlBuilder.ts | 5 +- .../ui/cacheDependencyAutocollectTest.cy.ts | 55 ++++--- 10 files changed, 369 insertions(+), 112 deletions(-) create mode 100644 javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java diff --git a/jahia-test-module/src/react/server/views/testAutocollectedCacheDependency/TestAutocollectedCacheDependency.tsx b/jahia-test-module/src/react/server/views/testAutocollectedCacheDependency/TestAutocollectedCacheDependency.tsx index 634dbf28..c88a73f0 100644 --- a/jahia-test-module/src/react/server/views/testAutocollectedCacheDependency/TestAutocollectedCacheDependency.tsx +++ b/jahia-test-module/src/react/server/views/testAutocollectedCacheDependency/TestAutocollectedCacheDependency.tsx @@ -5,23 +5,23 @@ interface Props { image: JCRNodeWrapper; } -const ImageAndRenderMarker = ({ image, testId }: { image: JCRNodeWrapper; testId: string }) => ( +const ImageAndRenderMarker = ({ src, testId }: { src: string; testId: string }) => (
- + {String(Date.now())}
); /** - * Neither view declares a cache dependency: building the URL is what registers one. The two differ - * only in whether they let the engine collect it. + * No view declares a cache dependency: building the URL is what registers one. The views differ + * only in whether they let the engine collect it, and in how they call `buildNodeUrl`. */ jahiaComponent( { nodeType: "javascriptExample:testAutocollectedCacheDependency", componentType: "view", }, - ({ image }: Props) => , + ({ image }: Props) => , ); jahiaComponent( @@ -31,5 +31,34 @@ jahiaComponent( componentType: "view", properties: { "cache.autocollectDependencies": "false" }, }, - ({ image }: Props) => , + ({ image }: Props) => , +); + +/** The context is passed explicitly, as a helper outside a component does. */ +jahiaComponent( + { + nodeType: "javascriptExample:testAutocollectedCacheDependency", + name: "explicitcontext", + componentType: "view", + }, + ({ image }: Props, { renderContext, currentResource }) => ( + + ), +); + +jahiaComponent( + { + nodeType: "javascriptExample:testAutocollectedCacheDependency", + name: "callopout", + componentType: "view", + }, + ({ image }: Props) => ( + + ), ); diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java index 94bc77a8..7566d767 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java @@ -1,19 +1,32 @@ package org.jahia.modules.javascript.modules.engine.js.server; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.IdentityHashMap; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; import java.util.Set; import java.util.regex.Pattern; +import javax.servlet.ServletRequest; + import org.apache.commons.lang3.StringUtils; import org.jahia.services.render.Resource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** - * Registers the cache dependencies a view autocollected while it - * rendered, so that its fragment is flushed when a node it linked to or - * displayed changes. + * Collects the cache dependencies a view resolves while it renders, and + * registers them on its resource, so that its fragment is flushed when a node + * it linked to or displayed changes. + *

+ * The paths are kept per resource in a request attribute, because a view + * renders other resources inside its own render. The core + * {@code BaseAttributesFilter} keeps the {@code currentResource} request + * attribute on the resource being rendered. */ public final class CollectedCacheDependencies { @@ -25,10 +38,61 @@ public final class CollectedCacheDependencies { */ public static final int DEFAULT_LIMIT = 100; + /** The request attribute that holds the collected paths of each resource. */ + static final String ATTRIBUTE = CollectedCacheDependencies.class.getName(); + + /** The request attribute in which the core render chain keeps the rendered resource. */ + static final String CURRENT_RESOURCE = "currentResource"; + + /** + * The depth a merge never goes above: two segments, such as + * {@code /sites/mysite}. + */ + static final int MIN_MERGE_DEPTH = 2; + private CollectedCacheDependencies() { // static helper } + /** + * Collects a path for the resource being rendered. + * + * @param request the current request + * @param path the canonical path of the node the view depends on + */ + public static void collect(ServletRequest request, String path) { + Object resource = request.getAttribute(CURRENT_RESOURCE); + if (!(resource instanceof Resource) || StringUtils.isEmpty(path)) { + return; + } + + @SuppressWarnings("unchecked") + Map> byResource = (Map>) request.getAttribute(ATTRIBUTE); + if (byResource == null) { + // Resource overrides equals(): two renders of one node are two fragments + byResource = new IdentityHashMap<>(); + request.setAttribute(ATTRIBUTE, byResource); + } + byResource.computeIfAbsent((Resource) resource, r -> new LinkedHashSet<>()).add(path); + } + + /** + * Removes and returns the paths collected for a resource. + * + * @param request the current request + * @param resource the rendered resource + * @return the collected paths, empty when none + */ + public static Set take(ServletRequest request, Resource resource) { + @SuppressWarnings("unchecked") + Map> byResource = (Map>) request.getAttribute(ATTRIBUTE); + if (byResource == null) { + return Set.of(); + } + Set paths = byResource.remove(resource); + return paths == null ? Set.of() : paths; + } + /** * Reduces the collected paths and registers them on the resource, before the * render chain stores its fragment. @@ -69,50 +133,70 @@ private Reduced(Collection paths, Collection regexps) { /** * Reduces a fragment's collected dependencies to at most {@code limit} cache * entries. + *

+ * The paths under {@code /modules} are dropped, because the core cache + * filters store no dependency there. Above the limit, the paths are grouped + * by their first {@value #MIN_MERGE_DEPTH} segments, and each group with more + * than one path becomes one regexp on its deepest common folder. A merge + * therefore never covers more than one site. * * @param dependencies the paths the fragment resolved * @param limit the number of entries a fragment may write * @return the paths and regexps to register */ static Reduced reduce(Collection dependencies, int limit) { - if (dependencies.size() <= limit) { - return new Reduced(dependencies, Set.of()); + Set kept = new LinkedHashSet<>(); + for (String path : dependencies) { + if (!isUnderModules(path)) { + kept.add(path); + } + } + + if (kept.size() <= limit) { + return new Reduced(kept, Set.of()); + } + + Map> groups = new LinkedHashMap<>(); + for (String path : kept) { + String[] segments = StringUtils.split(path, '/'); + String scope = String.join("/", Arrays.copyOf(segments, Math.min(segments.length, MIN_MERGE_DEPTH))); + groups.computeIfAbsent(scope, s -> new ArrayList<>()).add(segments); } - return new Reduced(Set.of(), Set.of(commonRootRegex(dependencies))); + Set paths = new LinkedHashSet<>(); + Set regexps = new LinkedHashSet<>(); + for (List group : groups.values()) { + if (group.size() == 1) { + paths.add("/" + String.join("/", group.get(0))); + } else { + regexps.add(commonRootRegex(group)); + } + } + return new Reduced(paths, regexps); + } + + private static boolean isUnderModules(String path) { + return "/modules".equals(path) || path.startsWith("/modules/"); } /** - * A regexp matching the deepest path every dependency lives under and - * everything - * below it, or everything under the repository root when they share no first - * segment. + * A regexp matching the deepest folder every path of the group lives under, + * and everything below it. The paths of a group share their first + * {@value #MIN_MERGE_DEPTH} segments, or are that folder itself. */ - private static String commonRootRegex(Collection paths) { - String[] common = null; - for (String path : paths) { - String[] segments = StringUtils.split(path, '/'); - if (common == null) { - common = segments; - continue; - } + private static String commonRootRegex(List group) { + String[] common = group.get(0); + for (String[] segments : group) { int shared = 0; while (shared < common.length && shared < segments.length && common[shared].equals(segments[shared])) { shared++; } - if (shared == 0) { - return "/.*"; - } if (shared < common.length) { common = Arrays.copyOf(common, shared); } } - if (common == null || common.length == 0) { - return "/.*"; - } - return "/" + Pattern.quote(String.join("/", common)) + "(/.*)?"; } } diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java new file mode 100644 index 00000000..077a333b --- /dev/null +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java @@ -0,0 +1,63 @@ +package org.jahia.modules.javascript.modules.engine.js.server; + +import java.util.Properties; +import java.util.Set; + +import javax.jcr.RepositoryException; + +import org.jahia.services.render.RenderContext; +import org.jahia.services.render.Resource; +import org.jahia.services.render.View; +import org.jahia.services.render.filter.AbstractFilter; +import org.jahia.services.render.filter.RenderChain; +import org.jahia.services.render.filter.RenderFilter; +import org.jahia.services.render.scripting.Script; +import org.osgi.service.component.annotations.Component; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Registers on each rendered resource the cache dependencies its view + * autocollected, unless the view sets {@value #DISABLE_PROPERTY} to + * {@code false}. + *

+ * The priority puts {@link #execute} after the script and the wrappers, and + * before the cache filters (16 and 16.5) store the dependencies of the fragment. + */ +@Component(service = RenderFilter.class, immediate = true) +public class CollectedCacheDependenciesFilter extends AbstractFilter { + + private static final Logger logger = LoggerFactory.getLogger(CollectedCacheDependenciesFilter.class); + + /** The view property that turns autocollection off for a view. */ + static final String DISABLE_PROPERTY = "cache.autocollectDependencies"; + + static final float PRIORITY = 17f; + + public CollectedCacheDependenciesFilter() { + setPriority(PRIORITY); + setDescription("Registers the cache dependencies a JavaScript view autocollected while it rendered."); + } + + @Override + public String execute(String previousOut, RenderContext renderContext, Resource resource, RenderChain chain) + throws Exception { + Set paths = CollectedCacheDependencies.take(renderContext.getRequest(), resource); + if (!paths.isEmpty() && isEnabled(renderContext, resource)) { + CollectedCacheDependencies.register(resource, paths); + } + return previousOut; + } + + private static boolean isEnabled(RenderContext renderContext, Resource resource) { + try { + Script script = resource.getScript(renderContext); + View view = script == null ? null : script.getView(); + Properties properties = view == null ? null : view.getProperties(); + return properties == null || !"false".equals(properties.getProperty(DISABLE_PROPERTY)); + } catch (RepositoryException e) { + logger.debug("Cannot resolve the view of {}, its autocollected dependencies are registered", resource, e); + return true; + } + } +} diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java index db0b42be..83e035d8 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java @@ -343,6 +343,20 @@ public void addCacheDependency(Map attr, RenderContext renderCon renderTag(new AddCacheDependencyTag(), attr, renderContext); } + /** + * Collect a cache dependency for the resource being rendered. Unlike + * {@link #addCacheDependency}, the dependency is registered once the view has + * rendered, unless the view sets {@code cache.autocollectDependencies} to + * {@code false}, and many collected paths are merged into a few regular + * expressions. + * + * @param path the canonical path of the node to add as a dependency + * @param renderContext the current rendering context + */ + public void autocollectCacheDependency(String path, RenderContext renderContext) { + CollectedCacheDependencies.collect(renderContext.getRequest(), path); + } + /** * Calculate the relative path from one node to another node. * diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/views/JSScript.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/views/JSScript.java index ef1f7566..d0306735 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/views/JSScript.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/views/JSScript.java @@ -18,10 +18,6 @@ import org.graalvm.polyglot.Value; import org.graalvm.polyglot.proxy.ProxyObject; import org.jahia.modules.javascript.modules.engine.jsengine.GraalVMEngine; -import java.util.ArrayList; -import java.util.List; - -import org.jahia.modules.javascript.modules.engine.js.server.CollectedCacheDependencies; import org.jahia.services.render.RenderContext; import org.jahia.services.render.RenderException; import org.jahia.services.render.Resource; @@ -65,7 +61,7 @@ public String execute(Resource resource, RenderContext renderContext) throws Ren viewValues.put("bundle", Value.asValue(jsView.getModule().getBundle())); Object executionResult = Value.asValue(viewRenderer.get("render")).execute(resource, renderContext, ProxyObject.fromMap(viewValues)); - return readRenderResult(Value.asValue(executionResult), resource); + return Value.asValue(executionResult).asString(); })); if (jsView.isTemplate()) { @@ -81,26 +77,6 @@ public String execute(Resource resource, RenderContext renderContext) throws Ren return output; } - /** - * Reads what a view renderer returned: the markup, and the cache dependencies - * the view autocollected while it rendered. - * - * @param result what the renderer returned - * @param resource the resource that was rendered - * @return the markup - */ - private String readRenderResult(Value result, Resource resource) { - Value dependencies = result.getMember("autocollectedDependencies"); - int length = (int) dependencies.getArraySize(); - List collected = new ArrayList<>(length); - for (int i = 0; i < length; i++) { - collected.add(dependencies.getArrayElement(i).asString()); - } - CollectedCacheDependencies.register(resource, collected); - - return result.getMember("html").asString(); - } - @Override public View getView() { return jsView; diff --git a/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesTest.java b/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesTest.java index b0e11cfd..27ccacd0 100644 --- a/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesTest.java +++ b/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesTest.java @@ -4,9 +4,15 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import java.lang.reflect.Proxy; +import java.util.HashMap; import java.util.HashSet; +import java.util.Map; import java.util.Set; +import javax.servlet.http.HttpServletRequest; + +import org.jahia.services.render.Resource; import org.junit.Test; public class CollectedCacheDependenciesTest { @@ -27,6 +33,27 @@ private static String mergedInto(Set dependencies) { return reduced.regexps.iterator().next(); } + private static Resource resource(String path) { + return new Resource(path, path, null, "html", "default", "module"); + } + + /** A request that only keeps its attributes. */ + private static HttpServletRequest request() { + Map attributes = new HashMap<>(); + return (HttpServletRequest) Proxy.newProxyInstance(HttpServletRequest.class.getClassLoader(), + new Class[] {HttpServletRequest.class}, (proxy, method, args) -> { + switch (method.getName()) { + case "getAttribute": + return attributes.get(args[0]); + case "setAttribute": + attributes.put(args[0], args[1]); + return null; + default: + throw new UnsupportedOperationException(method.getName()); + } + }); + } + @Test public void shouldKeepEveryDependencyExactWithinBudget() { Set dependencies = children("/sites/mysite/contents/posts", 100); @@ -66,41 +93,64 @@ public void shouldMergeToTheDeepestSharedFolder() { } @Test - public void shouldCompareWholeSegmentsRatherThanCharacters() { + public void shouldNeverMergeAboveTheSite() { + Set dependencies = children("/sites/mysite/home", 80); + dependencies.addAll(children("/sites/mysite/files", 80)); + + String regexp = mergedInto(dependencies); + + assertTrue("/sites/mysite/home/item-1".matches(regexp)); + assertTrue("/sites/mysite/files/item-1".matches(regexp)); + assertFalse("another site is not covered", "/sites/other/home/item-1".matches(regexp)); + } + + @Test + public void shouldMergeEachSiteSeparately() { Set dependencies = children("/sites/mysite/contents", 80); dependencies.addAll(children("/sites/mysite2/contents", 80)); - String regexp = mergedInto(dependencies); + CollectedCacheDependencies.Reduced reduced = CollectedCacheDependencies.reduce(dependencies, 100); // /sites/mysite is a character prefix of /sites/mysite2, but not an ancestor of // it - assertTrue("/sites/mysite2/contents/item-1".matches(regexp)); - assertTrue("/sites/mysite/contents/item-1".matches(regexp)); + assertTrue(reduced.paths.isEmpty()); + assertEquals(Set.of("/\\Qsites/mysite/contents\\E(/.*)?", "/\\Qsites/mysite2/contents\\E(/.*)?"), + new HashSet<>(reduced.regexps)); } @Test - public void shouldMergeToTheRepositoryRootWhenNothingIsShared() { + public void shouldNeverMergeToTheRepositoryRoot() { Set dependencies = children("/sites/mysite/contents", 80); dependencies.addAll(children("/users/root/files", 80)); - String regexp = mergedInto(dependencies); + CollectedCacheDependencies.Reduced reduced = CollectedCacheDependencies.reduce(dependencies, 100); + + assertEquals(2, reduced.regexps.size()); + for (String regexp : reduced.regexps) { + assertFalse(regexp + " covers another site", "/sites/other/contents/whatever".matches(regexp)); + } + } - assertTrue("/sites/mysite/contents/item-1".matches(regexp)); - assertTrue("/users/root/files/item-1".matches(regexp)); - assertTrue("a fragment merged to the root is flushed by anything", - "/sites/other/contents/whatever".matches(regexp)); + @Test + public void shouldKeepALoneDependencyOfAnotherSiteExact() { + Set dependencies = children("/sites/mysite/contents/posts", 150); + dependencies.add("/sites/shared/files/logo.png"); + + CollectedCacheDependencies.Reduced reduced = CollectedCacheDependencies.reduce(dependencies, 100); + + assertEquals(Set.of("/sites/shared/files/logo.png"), new HashSet<>(reduced.paths)); + assertEquals(Set.of("/\\Qsites/mysite/contents/posts\\E(/.*)?"), new HashSet<>(reduced.regexps)); } @Test - public void shouldLetOneStrayDependencyWidenTheMerge() { + public void shouldDropModuleDependenciesAsTheCoreCacheDoes() { Set dependencies = children("/sites/mysite/contents/posts", 150); dependencies.add("/modules/mymodule/templates/one"); String regexp = mergedInto(dependencies); - // The stray path shares no first segment with the listing, so the merge reaches - // the root - assertTrue("/sites/other/contents/whatever".matches(regexp)); + assertFalse("/sites/other/contents/whatever".matches(regexp)); + assertTrue(CollectedCacheDependencies.reduce(Set.of("/modules/mymodule/templates/one"), 100).paths.isEmpty()); } @Test @@ -110,4 +160,46 @@ public void shouldQuoteThePathSoADotIsNotAWildcard() { assertTrue("/sites/mysite/files/my.docs/item-1".matches(regexp)); assertFalse("/sites/mysite/files/myXdocs/item-1".matches(regexp)); } + + @Test + public void shouldCollectForTheResourceBeingRendered() { + HttpServletRequest request = request(); + Resource parent = resource("/sites/mysite/home"); + Resource child = resource("/sites/mysite/home/area/child"); + + request.setAttribute(CollectedCacheDependencies.CURRENT_RESOURCE, parent); + CollectedCacheDependencies.collect(request, "/sites/mysite/home/about"); + // a nested render: the core render chain sets the child, then restores the parent + request.setAttribute(CollectedCacheDependencies.CURRENT_RESOURCE, child); + CollectedCacheDependencies.collect(request, "/sites/mysite/files/logo.png"); + request.setAttribute(CollectedCacheDependencies.CURRENT_RESOURCE, parent); + CollectedCacheDependencies.collect(request, "/sites/mysite/home/contact"); + + assertEquals(Set.of("/sites/mysite/files/logo.png"), CollectedCacheDependencies.take(request, child)); + assertEquals(Set.of("/sites/mysite/home/about", "/sites/mysite/home/contact"), + CollectedCacheDependencies.take(request, parent)); + assertTrue("take removes what it returns", CollectedCacheDependencies.take(request, parent).isEmpty()); + } + + @Test + public void shouldTellTwoRendersOfOneNodeApart() { + HttpServletRequest request = request(); + Resource first = resource("/sites/mysite/home/teaser"); + Resource second = resource("/sites/mysite/home/teaser"); + + request.setAttribute(CollectedCacheDependencies.CURRENT_RESOURCE, first); + CollectedCacheDependencies.collect(request, "/sites/mysite/home/about"); + + assertTrue(CollectedCacheDependencies.take(request, second).isEmpty()); + assertEquals(Set.of("/sites/mysite/home/about"), CollectedCacheDependencies.take(request, first)); + } + + @Test + public void shouldCollectNothingOutsideARender() { + HttpServletRequest request = request(); + + CollectedCacheDependencies.collect(request, "/sites/mysite/home/about"); + + assertTrue(CollectedCacheDependencies.take(request, resource("/sites/mysite/home")).isEmpty()); + } } diff --git a/javascript-modules-engine/src/server/init-react.tsx b/javascript-modules-engine/src/server/init-react.tsx index d3143805..4ec98129 100644 --- a/javascript-modules-engine/src/server/init-react.tsx +++ b/javascript-modules-engine/src/server/init-react.tsx @@ -28,11 +28,6 @@ server.registry.add("viewRenderer", "react", { // But assumes a single language per page, so i18n.changeLanguage(lang) is safe in this context. i18n.changeLanguage(language); - const autocollectedDependencies = - view.properties?.["cache.autocollectDependencies"] === "false" - ? undefined - : new Set(); - // SSR const currentNode = currentResource.getNode(); const mainNode = renderContext.getMainResource().getNode(); @@ -45,7 +40,6 @@ server.registry.add("viewRenderer", "react", { mainNode={mainNode} jcrSession={currentNode.getSession()} bundleKey={bundleKey} - autocollectedDependencies={autocollectedDependencies} > @@ -53,7 +47,7 @@ server.registry.add("viewRenderer", "react", { ); - const html = + return ( // In page mode, prepend the rendered HTML with the HTML5 doctype (currentResource.getContextConfiguration() === "page" ? "" : "") + // We use a `` element to wrap raw HTML output because React does not allow @@ -61,11 +55,7 @@ server.registry.add("viewRenderer", "react", { // having them in the final output. // `` SHOULD NOT be used in userland code, it is an internal implementation // detail. - ReactDOMServer.renderToString(element).replaceAll(/<\/?jsm-raw-html>/g, ""); - - return { - html, - autocollectedDependencies: [...(autocollectedDependencies ?? [])], - }; + ReactDOMServer.renderToString(element).replaceAll(/<\/?jsm-raw-html>/g, "") + ); }, }); diff --git a/javascript-modules-library/src/hooks/useServerContext.tsx b/javascript-modules-library/src/hooks/useServerContext.tsx index 0a43b3ea..bb60086a 100644 --- a/javascript-modules-library/src/hooks/useServerContext.tsx +++ b/javascript-modules-library/src/hooks/useServerContext.tsx @@ -25,13 +25,6 @@ export interface ServerContext { jcrSession: JCRSessionWrapper; /** The OSGi bundle key of the current module being rendered */ bundleKey: string; - - /** - * Paths of all dependencies collected during render, unless autocollection is disabled. - * - * @internal Can change at any time without notice. - */ - autocollectedDependencies?: Set; } const ServerContext = createContext({} as never); diff --git a/javascript-modules-library/src/utils/urlBuilder/urlBuilder.ts b/javascript-modules-library/src/utils/urlBuilder/urlBuilder.ts index 611ae199..1ae9c760 100644 --- a/javascript-modules-library/src/utils/urlBuilder/urlBuilder.ts +++ b/javascript-modules-library/src/utils/urlBuilder/urlBuilder.ts @@ -83,14 +83,13 @@ export function buildNodeUrl( context: { renderContext?: RenderContext; currentResource?: Resource; - autocollectedDependencies?: Set; } = useServerContext(), ): string { if (!node) throw new Error("Expected a node in buildNodeUrl, received undefined"); - if (config.autocollectDependency !== false && context.autocollectedDependencies) { + if (config.autocollectDependency !== false && context.renderContext) { // getCanonicalPath accounts for mounted and versioned nodes - context.autocollectedDependencies.add(node.getCanonicalPath()); + server.render.autocollectCacheDependency(node.getCanonicalPath(), context.renderContext); } // URL building is an old thing in Jahia, with a lot of branches and special cases: diff --git a/tests/cypress/e2e/ui/cacheDependencyAutocollectTest.cy.ts b/tests/cypress/e2e/ui/cacheDependencyAutocollectTest.cy.ts index e7a93531..4b5a0f66 100644 --- a/tests/cypress/e2e/ui/cacheDependencyAutocollectTest.cy.ts +++ b/tests/cypress/e2e/ui/cacheDependencyAutocollectTest.cy.ts @@ -8,6 +8,10 @@ describe("Cache dependencies collected by the URL builder", () => { const IMAGE = "autocollected.jpg"; const RENAMED = "autocollected-renamed.jpg"; const LIVE_PAGE = `/cms/render/live/en/sites/${GENERIC_SITE_KEY}/home/${PAGE}.html`; + /** The test ids of the views whose dependency is collected */ + const COLLECTED = ["autocollected", "explicitcontext"]; + /** The test ids of the views that opted out, for the whole view or for one call */ + const OPTED_OUT = ["optedout", "callopout"]; /** Renders LIVE_PAGE as an anonymous visitor */ const renderLive = () => @@ -36,7 +40,7 @@ describe("Cache dependencies collected by the URL builder", () => { ], }); - before("Create the page, the image and the two contents referencing it", () => { + before("Create the page, the image and the contents referencing it", () => { cy.fixture("testData/image.jpg", "binary").then((content) => { const blob = Cypress.Blob.binaryStringToBlob(content, "image/jpeg"); cy.apollo({ @@ -59,6 +63,16 @@ describe("Cache dependencies collected by the URL builder", () => { ["jmix:renderable"], [{ name: "j:view", value: "noautocollect", type: "STRING" }], ); + addImageReference( + "explicitContext", + ["jmix:renderable"], + [{ name: "j:view", value: "explicitcontext", type: "STRING" }], + ); + addImageReference( + "callOptOut", + ["jmix:renderable"], + [{ name: "j:view", value: "callopout", type: "STRING" }], + ); }); publishAndWaitJobEnding(`/sites/${GENERIC_SITE_KEY}`); @@ -70,20 +84,20 @@ describe("Cache dependencies collected by the URL builder", () => { publishAndWaitJobEnding(`/sites/${GENERIC_SITE_KEY}`); }); - it("flushes the view that built the URL, and leaves the one that opted out", () => { + it("flushes the views that built the URL, and leaves the ones that opted out", () => { renderLive().then((first) => { - expect(imageSrc(first, "autocollected")).to.include(`/files/live${FILES}/${IMAGE}`); - expect(imageSrc(first, "optedout")).to.include(`/files/live${FILES}/${IMAGE}`); + for (const view of [...COLLECTED, ...OPTED_OUT]) { + expect(imageSrc(first, view), view).to.include(`/files/live${FILES}/${IMAGE}`); + } // Liveness: without cached fragments there is nothing for the rename to flush, and every // assertion below would pass on a page that is simply rendered afresh every time. renderLive().then((second) => { - expect(renderedAt(second, "autocollected"), "the fragment must be cached").to.eq( - renderedAt(first, "autocollected"), - ); - expect(renderedAt(second, "optedout"), "the fragment must be cached").to.eq( - renderedAt(first, "optedout"), - ); + for (const view of [...COLLECTED, ...OPTED_OUT]) { + expect(renderedAt(second, view), `the ${view} fragment must be cached`).to.eq( + renderedAt(first, view), + ); + } cy.apollo({ mutationFile: "graphql/jcrRenameNode.graphql", @@ -102,16 +116,19 @@ describe("Cache dependencies collected by the URL builder", () => { cy.request(`/files/live${FILES}/${RENAMED}`).its("status").should("eq", 200); renderLive().then((third) => { - expect( - renderedAt(third, "autocollected"), - "the collected view must be flushed", - ).to.not.eq(renderedAt(first, "autocollected")); - expect(imageSrc(third, "autocollected")).to.include(`/files/live${FILES}/${RENAMED}`); + for (const view of COLLECTED) { + expect(renderedAt(third, view), `the ${view} view must be flushed`).to.not.eq( + renderedAt(first, view), + ); + expect(imageSrc(third, view), view).to.include(`/files/live${FILES}/${RENAMED}`); + } - expect(renderedAt(third, "optedout"), "the opted-out view must be untouched").to.eq( - renderedAt(first, "optedout"), - ); - expect(imageSrc(third, "optedout")).to.include(`/files/live${FILES}/${IMAGE}`); + for (const view of OPTED_OUT) { + expect(renderedAt(third, view), `the ${view} view must be untouched`).to.eq( + renderedAt(first, view), + ); + expect(imageSrc(third, view), view).to.include(`/files/live${FILES}/${IMAGE}`); + } }); }); }); From 43342842dc4bddecf39e10f12c08267114b535dc Mon Sep 17 00:00:00 2001 From: Kevan Date: Tue, 13 Oct 2026 12:15:24 +0200 Subject: [PATCH 02/10] docs(changelog): state how a view with more than 100 links refreshes --- .chachalog/Kp7mQx2R.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.chachalog/Kp7mQx2R.md b/.chachalog/Kp7mQx2R.md index fcf32e38..67b367cc 100644 --- a/.chachalog/Kp7mQx2R.md +++ b/.chachalog/Kp7mQx2R.md @@ -3,6 +3,8 @@ javascript-modules: minor --- -`buildNodeUrl` now autocollects cache dependencies for the content it links to, preventing 404 errors when the linked content is renamed or moved. (#783) +`buildNodeUrl` now autocollects cache dependencies for the content it links to, preventing 404 errors when the linked content is renamed or moved. (#783, #810) This mechanism is enabled by default but can be disabled on a per-view basis by setting `"cache.autocollectDependencies": "false"` in the view's properties, or on a per-call basis with the new `autocollectDependency: false` option in `buildNodeUrl`. + +When a view links to more than 100 contents, a change anywhere in the folder that holds them refreshes the view, and a change in another site never does. From 1c55ec38feaac443eaf756668e008673adb5f213 Mon Sep 17 00:00:00 2001 From: Kevan Date: Tue, 13 Oct 2026 15:07:32 +0200 Subject: [PATCH 03/10] refactor: scope the collected dependencies to the render chain The filter now pushes a collector for each resource in prepare, with RenderChain.pushAttribute. A nested render pushes its own collector, and the render chain restores the collector of the parent when that render ends. This replaces the request-wide map of the resources. The view property is read once, at the first collected path, because the script of the resource is then resolved. A view that opted out stores no path. --- .../js/server/CollectedCacheDependencies.java | 95 +++++++++++++------ .../CollectedCacheDependenciesFilter.java | 46 +++------ .../engine/js/server/RenderHelper.java | 2 +- .../CollectedCacheDependenciesTest.java | 59 ++++++++---- 4 files changed, 125 insertions(+), 77 deletions(-) diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java index 7566d767..eadfe62a 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java @@ -3,18 +3,23 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import java.util.IdentityHashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Properties; import java.util.Set; +import java.util.function.Predicate; import java.util.regex.Pattern; +import javax.jcr.RepositoryException; import javax.servlet.ServletRequest; import org.apache.commons.lang3.StringUtils; +import org.jahia.services.render.RenderContext; import org.jahia.services.render.Resource; +import org.jahia.services.render.View; +import org.jahia.services.render.scripting.Script; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -23,10 +28,10 @@ * registers them on its resource, so that its fragment is flushed when a node * it linked to or displayed changes. *

- * The paths are kept per resource in a request attribute, because a view - * renders other resources inside its own render. The core - * {@code BaseAttributesFilter} keeps the {@code currentResource} request - * attribute on the resource being rendered. + * {@link CollectedCacheDependenciesFilter} puts a {@link Collector} for each + * rendered resource in a request attribute, with + * {@code RenderChain.pushAttribute}. A nested render puts its own, and the + * render chain restores the one of the parent when the nested render ends. */ public final class CollectedCacheDependencies { @@ -38,11 +43,11 @@ public final class CollectedCacheDependencies { */ public static final int DEFAULT_LIMIT = 100; - /** The request attribute that holds the collected paths of each resource. */ + /** The request attribute that holds the {@link Collector} of the rendered resource. */ static final String ATTRIBUTE = CollectedCacheDependencies.class.getName(); - /** The request attribute in which the core render chain keeps the rendered resource. */ - static final String CURRENT_RESOURCE = "currentResource"; + /** The view property that turns autocollection off for a view. */ + static final String DISABLE_PROPERTY = "cache.autocollectDependencies"; /** * The depth a merge never goes above: two segments, such as @@ -54,43 +59,75 @@ private CollectedCacheDependencies() { // static helper } + /** The paths collected for one rendered resource. */ + static final class Collector { + final Resource resource; + /** Whether the view of the resource collects, read at the first collected path. */ + Boolean enabled; + final Set paths = new LinkedHashSet<>(); + + Collector(Resource resource) { + this.resource = resource; + } + } + /** - * Collects a path for the resource being rendered. + * Collects a path for the resource being rendered, unless its view sets + * {@value #DISABLE_PROPERTY} to {@code false}. * - * @param request the current request - * @param path the canonical path of the node the view depends on + * @param renderContext the current rendering context + * @param path the canonical path of the node the view depends on */ - public static void collect(ServletRequest request, String path) { - Object resource = request.getAttribute(CURRENT_RESOURCE); - if (!(resource instanceof Resource) || StringUtils.isEmpty(path)) { + public static void collect(RenderContext renderContext, String path) { + collect(renderContext.getRequest(), path, resource -> isEnabled(renderContext, resource)); + } + + static void collect(ServletRequest request, String path, Predicate isEnabled) { + Object collector = request.getAttribute(ATTRIBUTE); + if (!(collector instanceof Collector) || StringUtils.isEmpty(path)) { return; } - @SuppressWarnings("unchecked") - Map> byResource = (Map>) request.getAttribute(ATTRIBUTE); - if (byResource == null) { - // Resource overrides equals(): two renders of one node are two fragments - byResource = new IdentityHashMap<>(); - request.setAttribute(ATTRIBUTE, byResource); + Collector current = (Collector) collector; + if (current.enabled == null) { + current.enabled = isEnabled.test(current.resource); + } + if (current.enabled) { + current.paths.add(path); } - byResource.computeIfAbsent((Resource) resource, r -> new LinkedHashSet<>()).add(path); } /** - * Removes and returns the paths collected for a resource. + * Returns the paths collected for a resource. * * @param request the current request * @param resource the rendered resource * @return the collected paths, empty when none */ - public static Set take(ServletRequest request, Resource resource) { - @SuppressWarnings("unchecked") - Map> byResource = (Map>) request.getAttribute(ATTRIBUTE); - if (byResource == null) { - return Set.of(); + static Set collected(ServletRequest request, Resource resource) { + Object collector = request.getAttribute(ATTRIBUTE); + // Resource overrides equals(): two renders of one node are two fragments + if (collector instanceof Collector && ((Collector) collector).resource == resource) { + return ((Collector) collector).paths; + } + return Set.of(); + } + + /** + * Reads the view property at collection time: the script then runs, so the + * resource has already resolved it. An earlier call would fix the script + * before the filters that change the template run. + */ + private static boolean isEnabled(RenderContext renderContext, Resource resource) { + try { + Script script = resource.getScript(renderContext); + View view = script == null ? null : script.getView(); + Properties properties = view == null ? null : view.getProperties(); + return properties == null || !"false".equals(properties.getProperty(DISABLE_PROPERTY)); + } catch (RepositoryException e) { + logger.debug("Cannot resolve the view of {}, its dependencies are collected", resource, e); + return true; } - Set paths = byResource.remove(resource); - return paths == null ? Set.of() : paths; } /** diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java index 077a333b..cbd8e3ad 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java @@ -1,37 +1,28 @@ package org.jahia.modules.javascript.modules.engine.js.server; -import java.util.Properties; import java.util.Set; -import javax.jcr.RepositoryException; - import org.jahia.services.render.RenderContext; import org.jahia.services.render.Resource; -import org.jahia.services.render.View; import org.jahia.services.render.filter.AbstractFilter; import org.jahia.services.render.filter.RenderChain; import org.jahia.services.render.filter.RenderFilter; -import org.jahia.services.render.scripting.Script; import org.osgi.service.component.annotations.Component; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; /** - * Registers on each rendered resource the cache dependencies its view - * autocollected, unless the view sets {@value #DISABLE_PROPERTY} to - * {@code false}. + * Gives each rendered resource a collector for the cache dependencies its view + * resolves, and registers what it collected. *

* The priority puts {@link #execute} after the script and the wrappers, and * before the cache filters (16 and 16.5) store the dependencies of the fragment. + * A fragment served from the cache stops the chain before this filter, so it + * collects nothing. A render filter whose priority is below this one runs its + * own {@code execute} after this filter, so a path it collects there is not + * registered. */ @Component(service = RenderFilter.class, immediate = true) public class CollectedCacheDependenciesFilter extends AbstractFilter { - private static final Logger logger = LoggerFactory.getLogger(CollectedCacheDependenciesFilter.class); - - /** The view property that turns autocollection off for a view. */ - static final String DISABLE_PROPERTY = "cache.autocollectDependencies"; - static final float PRIORITY = 17f; public CollectedCacheDependenciesFilter() { @@ -39,25 +30,18 @@ public CollectedCacheDependenciesFilter() { setDescription("Registers the cache dependencies a JavaScript view autocollected while it rendered."); } + @Override + public String prepare(RenderContext renderContext, Resource resource, RenderChain chain) throws Exception { + chain.pushAttribute(renderContext.getRequest(), CollectedCacheDependencies.ATTRIBUTE, + new CollectedCacheDependencies.Collector(resource)); + return null; + } + @Override public String execute(String previousOut, RenderContext renderContext, Resource resource, RenderChain chain) throws Exception { - Set paths = CollectedCacheDependencies.take(renderContext.getRequest(), resource); - if (!paths.isEmpty() && isEnabled(renderContext, resource)) { - CollectedCacheDependencies.register(resource, paths); - } + Set paths = CollectedCacheDependencies.collected(renderContext.getRequest(), resource); + CollectedCacheDependencies.register(resource, paths); return previousOut; } - - private static boolean isEnabled(RenderContext renderContext, Resource resource) { - try { - Script script = resource.getScript(renderContext); - View view = script == null ? null : script.getView(); - Properties properties = view == null ? null : view.getProperties(); - return properties == null || !"false".equals(properties.getProperty(DISABLE_PROPERTY)); - } catch (RepositoryException e) { - logger.debug("Cannot resolve the view of {}, its autocollected dependencies are registered", resource, e); - return true; - } - } } diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java index 83e035d8..d2e0ecfc 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java @@ -354,7 +354,7 @@ public void addCacheDependency(Map attr, RenderContext renderCon * @param renderContext the current rendering context */ public void autocollectCacheDependency(String path, RenderContext renderContext) { - CollectedCacheDependencies.collect(renderContext.getRequest(), path); + CollectedCacheDependencies.collect(renderContext, path); } /** diff --git a/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesTest.java b/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesTest.java index 27ccacd0..0021fe33 100644 --- a/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesTest.java +++ b/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesTest.java @@ -9,6 +9,8 @@ import java.util.HashSet; import java.util.Map; import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Predicate; import javax.servlet.http.HttpServletRequest; @@ -161,24 +163,31 @@ public void shouldQuoteThePathSoADotIsNotAWildcard() { assertFalse("/sites/mysite/files/myXdocs/item-1".matches(regexp)); } + private static final Predicate ENABLED = resource -> true; + + /** Starts the render of a resource, as the filter does in prepare. */ + private static void startRender(HttpServletRequest request, Resource resource) { + request.setAttribute(CollectedCacheDependencies.ATTRIBUTE, new CollectedCacheDependencies.Collector(resource)); + } + @Test public void shouldCollectForTheResourceBeingRendered() { HttpServletRequest request = request(); Resource parent = resource("/sites/mysite/home"); Resource child = resource("/sites/mysite/home/area/child"); - request.setAttribute(CollectedCacheDependencies.CURRENT_RESOURCE, parent); - CollectedCacheDependencies.collect(request, "/sites/mysite/home/about"); - // a nested render: the core render chain sets the child, then restores the parent - request.setAttribute(CollectedCacheDependencies.CURRENT_RESOURCE, child); - CollectedCacheDependencies.collect(request, "/sites/mysite/files/logo.png"); - request.setAttribute(CollectedCacheDependencies.CURRENT_RESOURCE, parent); - CollectedCacheDependencies.collect(request, "/sites/mysite/home/contact"); + startRender(request, parent); + CollectedCacheDependencies.collect(request, "/sites/mysite/home/about", ENABLED); + // a nested render: the render chain pushes the child's collector, then restores the parent's + Object parentCollector = request.getAttribute(CollectedCacheDependencies.ATTRIBUTE); + startRender(request, child); + CollectedCacheDependencies.collect(request, "/sites/mysite/files/logo.png", ENABLED); + assertEquals(Set.of("/sites/mysite/files/logo.png"), CollectedCacheDependencies.collected(request, child)); + request.setAttribute(CollectedCacheDependencies.ATTRIBUTE, parentCollector); + CollectedCacheDependencies.collect(request, "/sites/mysite/home/contact", ENABLED); - assertEquals(Set.of("/sites/mysite/files/logo.png"), CollectedCacheDependencies.take(request, child)); assertEquals(Set.of("/sites/mysite/home/about", "/sites/mysite/home/contact"), - CollectedCacheDependencies.take(request, parent)); - assertTrue("take removes what it returns", CollectedCacheDependencies.take(request, parent).isEmpty()); + CollectedCacheDependencies.collected(request, parent)); } @Test @@ -187,19 +196,37 @@ public void shouldTellTwoRendersOfOneNodeApart() { Resource first = resource("/sites/mysite/home/teaser"); Resource second = resource("/sites/mysite/home/teaser"); - request.setAttribute(CollectedCacheDependencies.CURRENT_RESOURCE, first); - CollectedCacheDependencies.collect(request, "/sites/mysite/home/about"); + startRender(request, first); + CollectedCacheDependencies.collect(request, "/sites/mysite/home/about", ENABLED); - assertTrue(CollectedCacheDependencies.take(request, second).isEmpty()); - assertEquals(Set.of("/sites/mysite/home/about"), CollectedCacheDependencies.take(request, first)); + assertTrue(CollectedCacheDependencies.collected(request, second).isEmpty()); + assertEquals(Set.of("/sites/mysite/home/about"), CollectedCacheDependencies.collected(request, first)); } @Test public void shouldCollectNothingOutsideARender() { HttpServletRequest request = request(); - CollectedCacheDependencies.collect(request, "/sites/mysite/home/about"); + CollectedCacheDependencies.collect(request, "/sites/mysite/home/about", ENABLED); + + assertTrue(CollectedCacheDependencies.collected(request, resource("/sites/mysite/home")).isEmpty()); + } + + @Test + public void shouldCollectNothingForAViewThatOptedOutAndReadItsViewOnce() { + HttpServletRequest request = request(); + Resource resource = resource("/sites/mysite/home"); + AtomicInteger reads = new AtomicInteger(); + + startRender(request, resource); + for (int i = 0; i < 3; i++) { + CollectedCacheDependencies.collect(request, "/sites/mysite/home/item-" + i, r -> { + reads.incrementAndGet(); + return false; + }); + } - assertTrue(CollectedCacheDependencies.take(request, resource("/sites/mysite/home")).isEmpty()); + assertTrue(CollectedCacheDependencies.collected(request, resource).isEmpty()); + assertEquals(1, reads.get()); } } From f686ec509306f0aecc6650d0fcaf87bbbb7f3846 Mon Sep 17 00:00:00 2001 From: Kevan Date: Tue, 13 Oct 2026 15:48:43 +0200 Subject: [PATCH 04/10] fix: collect into the fragment the cache stores, and merge only what the limit needs The filter now has the conditions of AggregateCacheFilter: live mode, no json, no include, wrapper or option, no fragment cached with its parent, and no render under skip aggregation. A resource the cache does not store gets no collector, so its view collects into the fragment that holds its markup. The filter reads the opt-out of the view in prepare, because NodeAttributesFilter has already resolved the script at priority 16.8. Above 100 paths, the merge takes the deepest folder that covers two entries or more, and repeats until the entries fit. A lone link outside a listing stays exact. No merge goes above /sites/, and a warning reports a result that stays above the limit. --- .chachalog/Kp7mQx2R.md | 2 +- .../TestAutocollectedCacheDependency.tsx | 24 ++- .../js/server/CollectedCacheDependencies.java | 187 ++++++++--------- .../CollectedCacheDependenciesFilter.java | 57 +++++- .../engine/js/server/RenderHelper.java | 8 +- .../CollectedCacheDependenciesTest.java | 188 ++++++++++-------- .../ui/cacheDependencyAutocollectTest.cy.ts | 7 +- 7 files changed, 283 insertions(+), 190 deletions(-) diff --git a/.chachalog/Kp7mQx2R.md b/.chachalog/Kp7mQx2R.md index 67b367cc..198f3ac8 100644 --- a/.chachalog/Kp7mQx2R.md +++ b/.chachalog/Kp7mQx2R.md @@ -7,4 +7,4 @@ javascript-modules: minor This mechanism is enabled by default but can be disabled on a per-view basis by setting `"cache.autocollectDependencies": "false"` in the view's properties, or on a per-call basis with the new `autocollectDependency: false` option in `buildNodeUrl`. -When a view links to more than 100 contents, a change anywhere in the folder that holds them refreshes the view, and a change in another site never does. +When a view links to more than 100 contents, a change in a folder that holds many of them also refreshes the view. diff --git a/jahia-test-module/src/react/server/views/testAutocollectedCacheDependency/TestAutocollectedCacheDependency.tsx b/jahia-test-module/src/react/server/views/testAutocollectedCacheDependency/TestAutocollectedCacheDependency.tsx index c88a73f0..a10da60a 100644 --- a/jahia-test-module/src/react/server/views/testAutocollectedCacheDependency/TestAutocollectedCacheDependency.tsx +++ b/jahia-test-module/src/react/server/views/testAutocollectedCacheDependency/TestAutocollectedCacheDependency.tsx @@ -1,4 +1,4 @@ -import { buildNodeUrl, jahiaComponent } from "@jahia/javascript-modules-library"; +import { buildNodeUrl, jahiaComponent, Render } from "@jahia/javascript-modules-library"; import type { JCRNodeWrapper } from "org.jahia.services.content"; interface Props { @@ -62,3 +62,25 @@ jahiaComponent( /> ), ); + +/** + * The cache stores an include inside the fragment of its parent. The included view builds the URL, + * so the parent fragment must depend on the image. + */ +jahiaComponent( + { + nodeType: "javascriptExample:testAutocollectedCacheDependency", + name: "includeparent", + componentType: "view", + }, + () => , +); + +jahiaComponent( + { + nodeType: "javascriptExample:testAutocollectedCacheDependency", + name: "includedimage", + componentType: "view", + }, + ({ image }: Props) => , +); diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java index eadfe62a..212c744e 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java @@ -1,25 +1,21 @@ package org.jahia.modules.javascript.modules.engine.js.server; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.HashMap; +import java.util.Iterator; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; -import java.util.Properties; import java.util.Set; -import java.util.function.Predicate; import java.util.regex.Pattern; -import javax.jcr.RepositoryException; import javax.servlet.ServletRequest; import org.apache.commons.lang3.StringUtils; import org.jahia.services.render.RenderContext; import org.jahia.services.render.Resource; -import org.jahia.services.render.View; -import org.jahia.services.render.scripting.Script; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -29,8 +25,8 @@ * it linked to or displayed changes. *

* {@link CollectedCacheDependenciesFilter} puts a {@link Collector} for each - * rendered resource in a request attribute, with - * {@code RenderChain.pushAttribute}. A nested render puts its own, and the + * cached fragment in a request attribute, with + * {@code RenderChain.pushAttribute}. A nested fragment puts its own, and the * render chain restores the one of the parent when the nested render ends. */ public final class CollectedCacheDependencies { @@ -43,12 +39,9 @@ public final class CollectedCacheDependencies { */ public static final int DEFAULT_LIMIT = 100; - /** The request attribute that holds the {@link Collector} of the rendered resource. */ + /** The request attribute that holds the {@link Collector} of the rendered fragment. */ static final String ATTRIBUTE = CollectedCacheDependencies.class.getName(); - /** The view property that turns autocollection off for a view. */ - static final String DISABLE_PROPERTY = "cache.autocollectDependencies"; - /** * The depth a merge never goes above: two segments, such as * {@code /sites/mysite}. @@ -59,41 +52,33 @@ private CollectedCacheDependencies() { // static helper } - /** The paths collected for one rendered resource. */ + /** The paths collected for one cached fragment. */ static final class Collector { final Resource resource; - /** Whether the view of the resource collects, read at the first collected path. */ - Boolean enabled; + /** False when the view sets the opt-out property. */ + final boolean enabled; final Set paths = new LinkedHashSet<>(); - Collector(Resource resource) { + Collector(Resource resource, boolean enabled) { this.resource = resource; + this.enabled = enabled; } } /** - * Collects a path for the resource being rendered, unless its view sets - * {@value #DISABLE_PROPERTY} to {@code false}. + * Collects a path for the fragment being rendered. * * @param renderContext the current rendering context * @param path the canonical path of the node the view depends on */ public static void collect(RenderContext renderContext, String path) { - collect(renderContext.getRequest(), path, resource -> isEnabled(renderContext, resource)); + collect(renderContext.getRequest(), path); } - static void collect(ServletRequest request, String path, Predicate isEnabled) { + static void collect(ServletRequest request, String path) { Object collector = request.getAttribute(ATTRIBUTE); - if (!(collector instanceof Collector) || StringUtils.isEmpty(path)) { - return; - } - - Collector current = (Collector) collector; - if (current.enabled == null) { - current.enabled = isEnabled.test(current.resource); - } - if (current.enabled) { - current.paths.add(path); + if (collector instanceof Collector && ((Collector) collector).enabled && StringUtils.isNotEmpty(path)) { + ((Collector) collector).paths.add(path); } } @@ -113,23 +98,6 @@ static Set collected(ServletRequest request, Resource resource) { return Set.of(); } - /** - * Reads the view property at collection time: the script then runs, so the - * resource has already resolved it. An earlier call would fix the script - * before the filters that change the template run. - */ - private static boolean isEnabled(RenderContext renderContext, Resource resource) { - try { - Script script = resource.getScript(renderContext); - View view = script == null ? null : script.getView(); - Properties properties = view == null ? null : view.getProperties(); - return properties == null || !"false".equals(properties.getProperty(DISABLE_PROPERTY)); - } catch (RepositoryException e) { - logger.debug("Cannot resolve the view of {}, its dependencies are collected", resource, e); - return true; - } - } - /** * Reduces the collected paths and registers them on the resource, before the * render chain stores its fragment. @@ -143,11 +111,17 @@ public static void register(Resource resource, Collection paths, int lim resource.getDependencies().addAll(reduced.paths); resource.getRegexpDependencies().addAll(reduced.regexps); - if (!reduced.regexps.isEmpty()) { - logger.info( - "{} autocollected {} node dependencies, more than the {} limit, so they were merged into {}. " - + "If incorrect, narrow what the view lists, or set cache.autocollectDependencies=false on it and declare the dependency it needs.", - resource, paths.size(), limit, reduced.regexps); + int entries = reduced.paths.size() + reduced.regexps.size(); + if (entries > limit) { + logger.warn("{} autocollected {} node dependencies, and {} remain after the merge, more than the {} limit. " + + "No merge goes above /sites/. Narrow what the view links to, or set " + + "cache.autocollectDependencies=false on it and declare the dependency it needs.", + resource, reduced.collected, entries, limit); + } else if (!reduced.regexps.isEmpty()) { + logger.info("{} autocollected {} node dependencies, more than the {} limit, so some were merged into {}. " + + "If incorrect, narrow what the view links to, or set " + + "cache.autocollectDependencies=false on it and declare the dependency it needs.", + resource, reduced.collected, limit, reduced.regexps); } } @@ -158,10 +132,13 @@ public static void register(Resource resource, Collection paths) { /** The dependencies to register, after reduction. */ static final class Reduced { + /** The number of collected paths the reduction kept. */ + final int collected; final Collection paths; final Collection regexps; - private Reduced(Collection paths, Collection regexps) { + private Reduced(int collected, Collection paths, Collection regexps) { + this.collected = collected; this.paths = paths; this.regexps = regexps; } @@ -169,47 +146,48 @@ private Reduced(Collection paths, Collection regexps) { /** * Reduces a fragment's collected dependencies to at most {@code limit} cache - * entries. + * entries, when a merge can reach that number. *

* The paths under {@code /modules} are dropped, because the core cache - * filters store no dependency there. Above the limit, the paths are grouped - * by their first {@value #MIN_MERGE_DEPTH} segments, and each group with more - * than one path becomes one regexp on its deepest common folder. A merge - * therefore never covers more than one site. + * filters store no dependency there. Above the limit, the deepest folder that + * covers two entries or more becomes one regexp. The merge repeats until the + * entries fit. A merge never covers a folder above {@value #MIN_MERGE_DEPTH} + * segments, such as {@code /sites}, so the result can stay above the limit. * * @param dependencies the paths the fragment resolved * @param limit the number of entries a fragment may write * @return the paths and regexps to register */ static Reduced reduce(Collection dependencies, int limit) { - Set kept = new LinkedHashSet<>(); + // the exact paths, each with its segments + Map exact = new LinkedHashMap<>(); for (String path : dependencies) { if (!isUnderModules(path)) { - kept.add(path); + exact.put(path, StringUtils.split(path, '/')); } } - - if (kept.size() <= limit) { - return new Reduced(kept, Set.of()); + int collected = exact.size(); + if (collected <= limit) { + return new Reduced(collected, exact.keySet(), Set.of()); } - Map> groups = new LinkedHashMap<>(); - for (String path : kept) { - String[] segments = StringUtils.split(path, '/'); - String scope = String.join("/", Arrays.copyOf(segments, Math.min(segments.length, MIN_MERGE_DEPTH))); - groups.computeIfAbsent(scope, s -> new ArrayList<>()).add(segments); + // the merged folders, each with its segments + Map folders = new LinkedHashMap<>(); + while (exact.size() + folders.size() > limit) { + String[] folder = deepestSharedFolder(exact.values(), folders.values()); + if (folder == null) { + break; + } + removeUnder(exact, folder); + removeUnder(folders, folder); + folders.put(String.join("/", folder), folder); } - Set paths = new LinkedHashSet<>(); Set regexps = new LinkedHashSet<>(); - for (List group : groups.values()) { - if (group.size() == 1) { - paths.add("/" + String.join("/", group.get(0))); - } else { - regexps.add(commonRootRegex(group)); - } + for (String folder : folders.keySet()) { + regexps.add("/" + Pattern.quote(folder) + "(/.*)?"); } - return new Reduced(paths, regexps); + return new Reduced(collected, new LinkedHashSet<>(exact.keySet()), regexps); } private static boolean isUnderModules(String path) { @@ -217,23 +195,54 @@ private static boolean isUnderModules(String path) { } /** - * A regexp matching the deepest folder every path of the group lives under, - * and everything below it. The paths of a group share their first - * {@value #MIN_MERGE_DEPTH} segments, or are that folder itself. + * The deepest folder of at least {@value #MIN_MERGE_DEPTH} segments that + * covers two entries or more, or null when no folder covers two. At one depth, + * the folder that covers the most entries wins. An entry covers its own path, + * because a regexp on a folder matches the folder too. */ - private static String commonRootRegex(List group) { - String[] common = group.get(0); - for (String[] segments : group) { - int shared = 0; - while (shared < common.length && shared < segments.length - && common[shared].equals(segments[shared])) { - shared++; + private static String[] deepestSharedFolder(Collection exact, Collection folders) { + Map covered = new HashMap<>(); + Map segmentsOf = new HashMap<>(); + for (Collection entries : List.of(exact, folders)) { + for (String[] segments : entries) { + for (int depth = MIN_MERGE_DEPTH; depth <= segments.length; depth++) { + String[] folder = Arrays.copyOf(segments, depth); + String key = String.join("/", folder); + covered.merge(key, 1, Integer::sum); + segmentsOf.putIfAbsent(key, folder); + } } - if (shared < common.length) { - common = Arrays.copyOf(common, shared); + } + + String best = null; + for (Map.Entry entry : covered.entrySet()) { + if (entry.getValue() >= 2 && (best == null + || isBetter(segmentsOf.get(entry.getKey()), entry.getValue(), segmentsOf.get(best), covered.get(best)))) { + best = entry.getKey(); } } + return best == null ? null : segmentsOf.get(best); + } - return "/" + Pattern.quote(String.join("/", common)) + "(/.*)?"; + /** Deeper first, then the folder that covers more entries, then the first in path order. */ + private static boolean isBetter(String[] candidate, int count, String[] best, int bestCount) { + if (candidate.length != best.length) { + return candidate.length > best.length; + } + if (count != bestCount) { + return count > bestCount; + } + return String.join("/", candidate).compareTo(String.join("/", best)) < 0; + } + + /** Removes the entries at or below a folder, comparing whole segments. */ + private static void removeUnder(Map entries, String[] folder) { + for (Iterator it = entries.values().iterator(); it.hasNext();) { + String[] segments = it.next(); + if (segments.length >= folder.length + && Arrays.equals(segments, 0, folder.length, folder, 0, folder.length)) { + it.remove(); + } + } } } diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java index cbd8e3ad..f57af11e 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java @@ -1,39 +1,68 @@ package org.jahia.modules.javascript.modules.engine.js.server; +import java.util.Properties; import java.util.Set; +import javax.jcr.RepositoryException; + +import org.jahia.api.Constants; import org.jahia.services.render.RenderContext; import org.jahia.services.render.Resource; +import org.jahia.services.render.View; import org.jahia.services.render.filter.AbstractFilter; import org.jahia.services.render.filter.RenderChain; import org.jahia.services.render.filter.RenderFilter; +import org.jahia.services.render.scripting.Script; import org.osgi.service.component.annotations.Component; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** - * Gives each rendered resource a collector for the cache dependencies its view - * resolves, and registers what it collected. + * Gives each resource that the cache stores as a fragment a collector for the + * cache dependencies its view resolves, and registers what it collected. + *

+ * The conditions are those of {@code AggregateCacheFilter}. A resource that + * filter skips, such as an include, a wrapper, an option or a fragment cached + * with its parent, gets no collector of its own. Its view then collects into the + * collector of the enclosing fragment, which holds its markup. *

* The priority puts {@link #execute} after the script and the wrappers, and - * before the cache filters (16 and 16.5) store the dependencies of the fragment. - * A fragment served from the cache stops the chain before this filter, so it - * collects nothing. A render filter whose priority is below this one runs its - * own {@code execute} after this filter, so a path it collects there is not - * registered. + * before the cache filter (16) stores the dependencies of the fragment. A render + * filter whose priority is below this one runs its own {@code execute} after + * this filter, so a path it collects there is not registered. */ @Component(service = RenderFilter.class, immediate = true) public class CollectedCacheDependenciesFilter extends AbstractFilter { + private static final Logger logger = LoggerFactory.getLogger(CollectedCacheDependenciesFilter.class); + + /** The view property that turns autocollection off for a view. */ + static final String DISABLE_PROPERTY = "cache.autocollectDependencies"; + static final float PRIORITY = 17f; + /** + * The request attribute of {@code AggregateCacheFilter.SKIP_AGGREGATION}. The + * core this module compiles against does not declare it, and sets no such + * attribute. + */ + static final String SKIP_AGGREGATION = "aggregateFilter.skip"; + public CollectedCacheDependenciesFilter() { setPriority(PRIORITY); setDescription("Registers the cache dependencies a JavaScript view autocollected while it rendered."); + setApplyOnModes("live"); + setSkipOnTemplateTypes("json"); + setSkipOnConfigurations("include,wrapper,option"); + addCondition((renderContext, resource) -> + !Boolean.TRUE.equals(resource.getModuleParams().get(Constants.TO_CACHE_WITH_PARENT_FRAGMENT)) + && !Boolean.TRUE.equals(renderContext.getRequest().getAttribute(SKIP_AGGREGATION))); } @Override public String prepare(RenderContext renderContext, Resource resource, RenderChain chain) throws Exception { chain.pushAttribute(renderContext.getRequest(), CollectedCacheDependencies.ATTRIBUTE, - new CollectedCacheDependencies.Collector(resource)); + new CollectedCacheDependencies.Collector(resource, isEnabled(renderContext, resource))); return null; } @@ -44,4 +73,16 @@ public String execute(String previousOut, RenderContext renderContext, Resource CollectedCacheDependencies.register(resource, paths); return previousOut; } + + private static boolean isEnabled(RenderContext renderContext, Resource resource) { + try { + Script script = resource.getScript(renderContext); + View view = script == null ? null : script.getView(); + Properties properties = view == null ? null : view.getProperties(); + return properties == null || !"false".equals(properties.getProperty(DISABLE_PROPERTY)); + } catch (RepositoryException e) { + logger.debug("Cannot resolve the view of {}, its dependencies are collected", resource, e); + return true; + } + } } diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java index d2e0ecfc..0fdbb9bc 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java @@ -344,11 +344,11 @@ public void addCacheDependency(Map attr, RenderContext renderCon } /** - * Collect a cache dependency for the resource being rendered. Unlike + * Collect a cache dependency for the fragment being rendered. Unlike * {@link #addCacheDependency}, the dependency is registered once the view has - * rendered, unless the view sets {@code cache.autocollectDependencies} to - * {@code false}, and many collected paths are merged into a few regular - * expressions. + * rendered. A view that sets {@code cache.autocollectDependencies} to + * {@code false} collects nothing. Above 100 paths, the paths are merged into + * regular expressions. * * @param path the canonical path of the node to add as a dependency * @param renderContext the current rendering context diff --git a/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesTest.java b/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesTest.java index 0021fe33..d8dd4101 100644 --- a/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesTest.java +++ b/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesTest.java @@ -9,8 +9,6 @@ import java.util.HashSet; import java.util.Map; import java.util.Set; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Predicate; import javax.servlet.http.HttpServletRequest; @@ -27,12 +25,12 @@ private static Set children(String parent, int count) { return paths; } - private static String mergedInto(Set dependencies) { - CollectedCacheDependencies.Reduced reduced = CollectedCacheDependencies.reduce(dependencies, 100); - assertFalse("the dependencies were expected to be merged", reduced.regexps.isEmpty()); - assertTrue("a merged fragment keeps no exact dependency", reduced.paths.isEmpty()); - assertEquals(1, reduced.regexps.size()); - return reduced.regexps.iterator().next(); + private static String regexp(String folder) { + return "/\\Q" + folder.substring(1) + "\\E(/.*)?"; + } + + private static CollectedCacheDependencies.Reduced reduce(Set dependencies) { + return CollectedCacheDependencies.reduce(dependencies, 100); } private static Resource resource(String path) { @@ -56,11 +54,17 @@ private static HttpServletRequest request() { }); } + /** Starts the render of a cached fragment, as the filter does in prepare. */ + private static void startRender(HttpServletRequest request, Resource resource, boolean enabled) { + request.setAttribute(CollectedCacheDependencies.ATTRIBUTE, + new CollectedCacheDependencies.Collector(resource, enabled)); + } + @Test public void shouldKeepEveryDependencyExactWithinBudget() { Set dependencies = children("/sites/mysite/contents/posts", 100); - CollectedCacheDependencies.Reduced reduced = CollectedCacheDependencies.reduce(dependencies, 100); + CollectedCacheDependencies.Reduced reduced = reduce(dependencies); assertEquals(dependencies, reduced.paths); assertTrue(reduced.regexps.isEmpty()); @@ -68,80 +72,104 @@ public void shouldKeepEveryDependencyExactWithinBudget() { @Test public void shouldMergeAListingIntoTheFolderItLivesIn() { - String regexp = mergedInto(children("/sites/mysite/contents/posts", 150)); + CollectedCacheDependencies.Reduced reduced = reduce(children("/sites/mysite/contents/posts", 150)); - assertTrue("/sites/mysite/contents/posts/item-1".matches(regexp)); + assertTrue(reduced.paths.isEmpty()); + assertEquals(Set.of(regexp("/sites/mysite/contents/posts")), reduced.regexps); + String regexp = reduced.regexps.iterator().next(); // an edited title lands on a subnode of the item assertTrue("/sites/mysite/contents/posts/item-1/j:translation_en".matches(regexp)); - // the folder may itself be one of the linked nodes: renaming it must flush the - // fragment - assertTrue("the merged folder itself is covered", - "/sites/mysite/contents/posts".matches(regexp)); - assertFalse("a sibling folder is not covered", - "/sites/mysite/contents/events/item-1".matches(regexp)); + // the folder may itself be one of the linked nodes: renaming it must flush the fragment + assertTrue("/sites/mysite/contents/posts".matches(regexp)); + assertFalse("/sites/mysite/contents/events/item-1".matches(regexp)); } @Test - public void shouldMergeToTheDeepestSharedFolder() { - Set dependencies = children("/sites/mysite/contents/posts", 80); - dependencies.addAll(children("/sites/mysite/contents/events", 80)); + public void shouldKeepALoneLinkOutsideTheListingExact() { + Set dependencies = children("/sites/mysite/home", 120); + dependencies.add("/sites/mysite/files/logo.png"); - String regexp = mergedInto(dependencies); + CollectedCacheDependencies.Reduced reduced = reduce(dependencies); - assertTrue("/sites/mysite/contents/posts/item-1".matches(regexp)); - assertTrue("/sites/mysite/contents/events/item-1".matches(regexp)); - assertFalse("nothing above the shared folder is covered", - "/sites/mysite/files/logo.png".matches(regexp)); + assertEquals(Set.of("/sites/mysite/files/logo.png"), new HashSet<>(reduced.paths)); + assertEquals(Set.of(regexp("/sites/mysite/home")), reduced.regexps); } @Test - public void shouldNeverMergeAboveTheSite() { - Set dependencies = children("/sites/mysite/home", 80); - dependencies.addAll(children("/sites/mysite/files", 80)); + public void shouldMergeOnlyWhatTheLimitNeeds() { + Set dependencies = children("/sites/mysite/contents/events", 80); + dependencies.addAll(children("/sites/mysite/contents/posts", 80)); - String regexp = mergedInto(dependencies); + CollectedCacheDependencies.Reduced reduced = reduce(dependencies); - assertTrue("/sites/mysite/home/item-1".matches(regexp)); - assertTrue("/sites/mysite/files/item-1".matches(regexp)); - assertFalse("another site is not covered", "/sites/other/home/item-1".matches(regexp)); + // merging one folder brings 160 entries down to 81, so the other one stays exact + assertEquals(Set.of(regexp("/sites/mysite/contents/events")), reduced.regexps); + assertEquals(children("/sites/mysite/contents/posts", 80), new HashSet<>(reduced.paths)); } @Test - public void shouldMergeEachSiteSeparately() { - Set dependencies = children("/sites/mysite/contents", 80); - dependencies.addAll(children("/sites/mysite2/contents", 80)); + public void shouldMergeTheBiggestFolderFirstAtOneDepth() { + Set dependencies = children("/sites/mysite/contents/events", 30); + dependencies.addAll(children("/sites/mysite/contents/posts", 90)); - CollectedCacheDependencies.Reduced reduced = CollectedCacheDependencies.reduce(dependencies, 100); + CollectedCacheDependencies.Reduced reduced = reduce(dependencies); - // /sites/mysite is a character prefix of /sites/mysite2, but not an ancestor of - // it + assertEquals(Set.of(regexp("/sites/mysite/contents/posts")), reduced.regexps); + assertEquals(30, reduced.paths.size()); + } + + @Test + public void shouldMergeUpwardsUntilTheEntriesFit() { + Set dependencies = new HashSet<>(); + for (int i = 0; i < 150; i++) { + dependencies.add("/sites/mysite/home/section-" + i + "/page"); + } + + CollectedCacheDependencies.Reduced reduced = reduce(dependencies); + + // no section holds two pages, so the merge goes up to the folder that holds the sections assertTrue(reduced.paths.isEmpty()); - assertEquals(Set.of("/\\Qsites/mysite/contents\\E(/.*)?", "/\\Qsites/mysite2/contents\\E(/.*)?"), - new HashSet<>(reduced.regexps)); + assertEquals(Set.of(regexp("/sites/mysite/home")), reduced.regexps); } @Test - public void shouldNeverMergeToTheRepositoryRoot() { - Set dependencies = children("/sites/mysite/contents", 80); - dependencies.addAll(children("/users/root/files", 80)); + public void shouldCompareWholeSegmentsInsideOneSite() { + Set dependencies = children("/sites/mysite/contents", 60); + dependencies.addAll(children("/sites/mysite/contents2", 60)); - CollectedCacheDependencies.Reduced reduced = CollectedCacheDependencies.reduce(dependencies, 100); + CollectedCacheDependencies.Reduced reduced = reduce(dependencies); - assertEquals(2, reduced.regexps.size()); - for (String regexp : reduced.regexps) { - assertFalse(regexp + " covers another site", "/sites/other/contents/whatever".matches(regexp)); + assertEquals(Set.of(regexp("/sites/mysite/contents")), reduced.regexps); + // contents is a character prefix of contents2, but not an ancestor of it + assertFalse("/sites/mysite/contents2/item-1".matches(reduced.regexps.iterator().next())); + assertEquals(children("/sites/mysite/contents2", 60), new HashSet<>(reduced.paths)); + } + + @Test + public void shouldNeverMergeAboveTheSite() { + Set dependencies = new HashSet<>(); + for (int i = 0; i < 150; i++) { + dependencies.add("/sites/site-" + i + "/home"); } + + CollectedCacheDependencies.Reduced reduced = reduce(dependencies); + + // no folder below /sites holds two links: the paths stay exact, above the limit + assertEquals(dependencies, new HashSet<>(reduced.paths)); + assertTrue(reduced.regexps.isEmpty()); } @Test - public void shouldKeepALoneDependencyOfAnotherSiteExact() { - Set dependencies = children("/sites/mysite/contents/posts", 150); - dependencies.add("/sites/shared/files/logo.png"); + public void shouldNeverMergeToTheRepositoryRoot() { + Set dependencies = children("/sites/mysite/contents", 80); + dependencies.addAll(children("/users/root/files", 80)); - CollectedCacheDependencies.Reduced reduced = CollectedCacheDependencies.reduce(dependencies, 100); + CollectedCacheDependencies.Reduced reduced = reduce(dependencies); - assertEquals(Set.of("/sites/shared/files/logo.png"), new HashSet<>(reduced.paths)); - assertEquals(Set.of("/\\Qsites/mysite/contents/posts\\E(/.*)?"), new HashSet<>(reduced.regexps)); + assertEquals(81, reduced.paths.size() + reduced.regexps.size()); + for (String regexp : reduced.regexps) { + assertFalse(regexp + " covers another site", "/sites/other/contents/whatever".matches(regexp)); + } } @Test @@ -149,42 +177,37 @@ public void shouldDropModuleDependenciesAsTheCoreCacheDoes() { Set dependencies = children("/sites/mysite/contents/posts", 150); dependencies.add("/modules/mymodule/templates/one"); - String regexp = mergedInto(dependencies); + CollectedCacheDependencies.Reduced reduced = reduce(dependencies); - assertFalse("/sites/other/contents/whatever".matches(regexp)); - assertTrue(CollectedCacheDependencies.reduce(Set.of("/modules/mymodule/templates/one"), 100).paths.isEmpty()); + assertEquals(150, reduced.collected); + assertEquals(Set.of(regexp("/sites/mysite/contents/posts")), reduced.regexps); + assertTrue(reduce(Set.of("/modules/mymodule/templates/one")).paths.isEmpty()); } @Test public void shouldQuoteThePathSoADotIsNotAWildcard() { - String regexp = mergedInto(children("/sites/mysite/files/my.docs", 150)); + CollectedCacheDependencies.Reduced reduced = reduce(children("/sites/mysite/files/my.docs", 150)); + String regexp = reduced.regexps.iterator().next(); assertTrue("/sites/mysite/files/my.docs/item-1".matches(regexp)); assertFalse("/sites/mysite/files/myXdocs/item-1".matches(regexp)); } - private static final Predicate ENABLED = resource -> true; - - /** Starts the render of a resource, as the filter does in prepare. */ - private static void startRender(HttpServletRequest request, Resource resource) { - request.setAttribute(CollectedCacheDependencies.ATTRIBUTE, new CollectedCacheDependencies.Collector(resource)); - } - @Test - public void shouldCollectForTheResourceBeingRendered() { + public void shouldCollectForTheFragmentBeingRendered() { HttpServletRequest request = request(); Resource parent = resource("/sites/mysite/home"); Resource child = resource("/sites/mysite/home/area/child"); - startRender(request, parent); - CollectedCacheDependencies.collect(request, "/sites/mysite/home/about", ENABLED); - // a nested render: the render chain pushes the child's collector, then restores the parent's + startRender(request, parent, true); + CollectedCacheDependencies.collect(request, "/sites/mysite/home/about"); + // a nested fragment: the render chain pushes the child's collector, then restores the parent's Object parentCollector = request.getAttribute(CollectedCacheDependencies.ATTRIBUTE); - startRender(request, child); - CollectedCacheDependencies.collect(request, "/sites/mysite/files/logo.png", ENABLED); + startRender(request, child, true); + CollectedCacheDependencies.collect(request, "/sites/mysite/files/logo.png"); assertEquals(Set.of("/sites/mysite/files/logo.png"), CollectedCacheDependencies.collected(request, child)); request.setAttribute(CollectedCacheDependencies.ATTRIBUTE, parentCollector); - CollectedCacheDependencies.collect(request, "/sites/mysite/home/contact", ENABLED); + CollectedCacheDependencies.collect(request, "/sites/mysite/home/contact"); assertEquals(Set.of("/sites/mysite/home/about", "/sites/mysite/home/contact"), CollectedCacheDependencies.collected(request, parent)); @@ -196,37 +219,30 @@ public void shouldTellTwoRendersOfOneNodeApart() { Resource first = resource("/sites/mysite/home/teaser"); Resource second = resource("/sites/mysite/home/teaser"); - startRender(request, first); - CollectedCacheDependencies.collect(request, "/sites/mysite/home/about", ENABLED); + startRender(request, first, true); + CollectedCacheDependencies.collect(request, "/sites/mysite/home/about"); assertTrue(CollectedCacheDependencies.collected(request, second).isEmpty()); assertEquals(Set.of("/sites/mysite/home/about"), CollectedCacheDependencies.collected(request, first)); } @Test - public void shouldCollectNothingOutsideARender() { + public void shouldCollectNothingOutsideAFragment() { HttpServletRequest request = request(); - CollectedCacheDependencies.collect(request, "/sites/mysite/home/about", ENABLED); + CollectedCacheDependencies.collect(request, "/sites/mysite/home/about"); assertTrue(CollectedCacheDependencies.collected(request, resource("/sites/mysite/home")).isEmpty()); } @Test - public void shouldCollectNothingForAViewThatOptedOutAndReadItsViewOnce() { + public void shouldCollectNothingForAViewThatOptedOut() { HttpServletRequest request = request(); Resource resource = resource("/sites/mysite/home"); - AtomicInteger reads = new AtomicInteger(); - - startRender(request, resource); - for (int i = 0; i < 3; i++) { - CollectedCacheDependencies.collect(request, "/sites/mysite/home/item-" + i, r -> { - reads.incrementAndGet(); - return false; - }); - } + + startRender(request, resource, false); + CollectedCacheDependencies.collect(request, "/sites/mysite/home/about"); assertTrue(CollectedCacheDependencies.collected(request, resource).isEmpty()); - assertEquals(1, reads.get()); } } diff --git a/tests/cypress/e2e/ui/cacheDependencyAutocollectTest.cy.ts b/tests/cypress/e2e/ui/cacheDependencyAutocollectTest.cy.ts index 4b5a0f66..9d8fe729 100644 --- a/tests/cypress/e2e/ui/cacheDependencyAutocollectTest.cy.ts +++ b/tests/cypress/e2e/ui/cacheDependencyAutocollectTest.cy.ts @@ -9,7 +9,7 @@ describe("Cache dependencies collected by the URL builder", () => { const RENAMED = "autocollected-renamed.jpg"; const LIVE_PAGE = `/cms/render/live/en/sites/${GENERIC_SITE_KEY}/home/${PAGE}.html`; /** The test ids of the views whose dependency is collected */ - const COLLECTED = ["autocollected", "explicitcontext"]; + const COLLECTED = ["autocollected", "explicitcontext", "included"]; /** The test ids of the views that opted out, for the whole view or for one call */ const OPTED_OUT = ["optedout", "callopout"]; @@ -73,6 +73,11 @@ describe("Cache dependencies collected by the URL builder", () => { ["jmix:renderable"], [{ name: "j:view", value: "callopout", type: "STRING" }], ); + addImageReference( + "includeParent", + ["jmix:renderable"], + [{ name: "j:view", value: "includeparent", type: "STRING" }], + ); }); publishAndWaitJobEnding(`/sites/${GENERIC_SITE_KEY}`); From f1aadf31a8e54044f9dd2addbc9e58628ad12446 Mon Sep 17 00:00:00 2001 From: Kevan Date: Tue, 13 Oct 2026 16:37:25 +0200 Subject: [PATCH 05/10] test: cover a virtual node that renders inside the fragment of its parent A view renders a virtual node with Render content, which the render helper marks to be cached with its parent fragment. The view of the virtual node builds the image URL, and the rename of the image must flush the parent. The Javadoc of the filter states that the opt-out of the enclosing view decides for a view that renders inside its fragment. --- .../TestAutocollectedCacheDependency.tsx | 31 +++++++++++++++++++ .../CollectedCacheDependenciesFilter.java | 4 ++- .../ui/cacheDependencyAutocollectTest.cy.ts | 7 ++++- 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/jahia-test-module/src/react/server/views/testAutocollectedCacheDependency/TestAutocollectedCacheDependency.tsx b/jahia-test-module/src/react/server/views/testAutocollectedCacheDependency/TestAutocollectedCacheDependency.tsx index a10da60a..825bc342 100644 --- a/jahia-test-module/src/react/server/views/testAutocollectedCacheDependency/TestAutocollectedCacheDependency.tsx +++ b/jahia-test-module/src/react/server/views/testAutocollectedCacheDependency/TestAutocollectedCacheDependency.tsx @@ -84,3 +84,34 @@ jahiaComponent( }, ({ image }: Props) => , ); + +/** + * A virtual node renders inside the fragment of its parent, which the cache stores with it. The + * view of the virtual node builds the URL, so the parent fragment must depend on the image. + */ +jahiaComponent( + { + nodeType: "javascriptExample:testAutocollectedCacheDependency", + name: "virtualparent", + componentType: "view", + }, + ({ image }: Props) => ( + + ), +); + +jahiaComponent( + { + nodeType: "javascriptExample:testAutocollectedCacheDependency", + name: "virtualimage", + componentType: "view", + }, + ({ image }: Props) => , +); diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java index f57af11e..6468226a 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java @@ -24,7 +24,9 @@ * The conditions are those of {@code AggregateCacheFilter}. A resource that * filter skips, such as an include, a wrapper, an option or a fragment cached * with its parent, gets no collector of its own. Its view then collects into the - * collector of the enclosing fragment, which holds its markup. + * collector of the enclosing fragment, which holds its markup. The opt-out of + * the enclosing view then decides for both views, and the opt-out of the inner + * view has no effect. *

* The priority puts {@link #execute} after the script and the wrappers, and * before the cache filter (16) stores the dependencies of the fragment. A render diff --git a/tests/cypress/e2e/ui/cacheDependencyAutocollectTest.cy.ts b/tests/cypress/e2e/ui/cacheDependencyAutocollectTest.cy.ts index 9d8fe729..14b98e9a 100644 --- a/tests/cypress/e2e/ui/cacheDependencyAutocollectTest.cy.ts +++ b/tests/cypress/e2e/ui/cacheDependencyAutocollectTest.cy.ts @@ -9,7 +9,7 @@ describe("Cache dependencies collected by the URL builder", () => { const RENAMED = "autocollected-renamed.jpg"; const LIVE_PAGE = `/cms/render/live/en/sites/${GENERIC_SITE_KEY}/home/${PAGE}.html`; /** The test ids of the views whose dependency is collected */ - const COLLECTED = ["autocollected", "explicitcontext", "included"]; + const COLLECTED = ["autocollected", "explicitcontext", "included", "virtual"]; /** The test ids of the views that opted out, for the whole view or for one call */ const OPTED_OUT = ["optedout", "callopout"]; @@ -78,6 +78,11 @@ describe("Cache dependencies collected by the URL builder", () => { ["jmix:renderable"], [{ name: "j:view", value: "includeparent", type: "STRING" }], ); + addImageReference( + "virtualParent", + ["jmix:renderable"], + [{ name: "j:view", value: "virtualparent", type: "STRING" }], + ); }); publishAndWaitJobEnding(`/sites/${GENERIC_SITE_KEY}`); From 28f7616882d5fef5edb10655efec07a260d8a769 Mon Sep 17 00:00:00 2001 From: Kevan Date: Tue, 13 Oct 2026 22:51:45 +0200 Subject: [PATCH 06/10] test: hold the invariants of the merge on generated input A test generates 500 sets of paths over sites, user folders and modules, with a fixed seed. On each set it checks that no path is lost, that nothing merges within the limit, that no merge goes above /sites/, that each merged folder holds two paths or more, that no entry lies inside a merged folder, and that a result above the limit has no folder left to merge. The merge now groups the entries once per depth, from the deepest. Two folders at one depth never overlap, so the result is the same, and 5000 paths in 2500 folders reduce in a few milliseconds. Other tests cover the boundary at 100 paths, a deeper merge absorbed by a higher one, an exact path equal to the merged folder, the paths above the floor, and what register() puts on the resource. --- .../js/server/CollectedCacheDependencies.java | 115 ++++---- .../CollectedCacheDependenciesReduceTest.java | 267 ++++++++++++++++++ 2 files changed, 322 insertions(+), 60 deletions(-) create mode 100644 javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesReduceTest.java diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java index 212c744e..cd02df98 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java @@ -1,9 +1,10 @@ package org.jahia.modules.javascript.modules.engine.js.server; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Comparator; import java.util.HashMap; -import java.util.Iterator; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; @@ -150,37 +151,52 @@ private Reduced(int collected, Collection paths, Collection rege *

* The paths under {@code /modules} are dropped, because the core cache * filters store no dependency there. Above the limit, the deepest folder that - * covers two entries or more becomes one regexp. The merge repeats until the - * entries fit. A merge never covers a folder above {@value #MIN_MERGE_DEPTH} - * segments, such as {@code /sites}, so the result can stay above the limit. + * covers two entries or more becomes one regexp, the biggest first at one + * depth. The merge goes up one depth at a time until the entries fit. A merge + * never covers a folder above {@value #MIN_MERGE_DEPTH} segments, such as + * {@code /sites}, so the result can stay above the limit. * * @param dependencies the paths the fragment resolved * @param limit the number of entries a fragment may write * @return the paths and regexps to register */ static Reduced reduce(Collection dependencies, int limit) { - // the exact paths, each with its segments + // the exact paths and the merged folders, each with its segments Map exact = new LinkedHashMap<>(); + Map folders = new LinkedHashMap<>(); + int deepest = 0; for (String path : dependencies) { if (!isUnderModules(path)) { - exact.put(path, StringUtils.split(path, '/')); + String[] segments = StringUtils.split(path, '/'); + exact.put(path, segments); + deepest = Math.max(deepest, segments.length); } } int collected = exact.size(); - if (collected <= limit) { - return new Reduced(collected, exact.keySet(), Set.of()); - } - // the merged folders, each with its segments - Map folders = new LinkedHashMap<>(); - while (exact.size() + folders.size() > limit) { - String[] folder = deepestSharedFolder(exact.values(), folders.values()); - if (folder == null) { - break; + // Two folders at one depth never overlap, so a merge at one depth leaves the + // other folders of that depth as they were + for (int depth = deepest; depth >= MIN_MERGE_DEPTH && exact.size() + folders.size() > limit; depth--) { + Map atDepth = new HashMap<>(); + group(exact, depth, atDepth, true); + group(folders, depth, atDepth, false); + + List candidates = new ArrayList<>(); + for (Folder folder : atDepth.values()) { + if (folder.size() >= 2) { + candidates.add(folder); + } + } + candidates.sort(Comparator.comparingInt(Folder::size).reversed().thenComparing(folder -> folder.key)); + + for (Folder folder : candidates) { + if (exact.size() + folders.size() <= limit) { + break; + } + folder.exact.forEach(exact::remove); + folder.folders.forEach(folders::remove); + folders.put(folder.key, folder.segments); } - removeUnder(exact, folder); - removeUnder(folders, folder); - folders.put(String.join("/", folder), folder); } Set regexps = new LinkedHashSet<>(); @@ -194,55 +210,34 @@ private static boolean isUnderModules(String path) { return "/modules".equals(path) || path.startsWith("/modules/"); } - /** - * The deepest folder of at least {@value #MIN_MERGE_DEPTH} segments that - * covers two entries or more, or null when no folder covers two. At one depth, - * the folder that covers the most entries wins. An entry covers its own path, - * because a regexp on a folder matches the folder too. - */ - private static String[] deepestSharedFolder(Collection exact, Collection folders) { - Map covered = new HashMap<>(); - Map segmentsOf = new HashMap<>(); - for (Collection entries : List.of(exact, folders)) { - for (String[] segments : entries) { - for (int depth = MIN_MERGE_DEPTH; depth <= segments.length; depth++) { - String[] folder = Arrays.copyOf(segments, depth); - String key = String.join("/", folder); - covered.merge(key, 1, Integer::sum); - segmentsOf.putIfAbsent(key, folder); - } - } - } + /** The entries at or below one folder. An entry covers its own path, because a regexp on a folder matches it. */ + private static final class Folder { + final String key; + final String[] segments; + final List exact = new ArrayList<>(); + final List folders = new ArrayList<>(); - String best = null; - for (Map.Entry entry : covered.entrySet()) { - if (entry.getValue() >= 2 && (best == null - || isBetter(segmentsOf.get(entry.getKey()), entry.getValue(), segmentsOf.get(best), covered.get(best)))) { - best = entry.getKey(); - } + Folder(String key, String[] segments) { + this.key = key; + this.segments = segments; } - return best == null ? null : segmentsOf.get(best); - } - /** Deeper first, then the folder that covers more entries, then the first in path order. */ - private static boolean isBetter(String[] candidate, int count, String[] best, int bestCount) { - if (candidate.length != best.length) { - return candidate.length > best.length; - } - if (count != bestCount) { - return count > bestCount; + int size() { + return exact.size() + folders.size(); } - return String.join("/", candidate).compareTo(String.join("/", best)) < 0; } - /** Removes the entries at or below a folder, comparing whole segments. */ - private static void removeUnder(Map entries, String[] folder) { - for (Iterator it = entries.values().iterator(); it.hasNext();) { - String[] segments = it.next(); - if (segments.length >= folder.length - && Arrays.equals(segments, 0, folder.length, folder, 0, folder.length)) { - it.remove(); + /** Groups the entries deep enough by their folder at one depth. */ + private static void group(Map entries, int depth, Map atDepth, boolean exact) { + for (Map.Entry entry : entries.entrySet()) { + String[] segments = entry.getValue(); + if (segments.length < depth) { + continue; } + String[] folderSegments = Arrays.copyOf(segments, depth); + Folder folder = atDepth.computeIfAbsent(String.join("/", folderSegments), + key -> new Folder(key, folderSegments)); + (exact ? folder.exact : folder.folders).add(entry.getKey()); } } } diff --git a/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesReduceTest.java b/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesReduceTest.java new file mode 100644 index 00000000..3f7f84ab --- /dev/null +++ b/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesReduceTest.java @@ -0,0 +1,267 @@ +package org.jahia.modules.javascript.modules.engine.js.server; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Random; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import org.jahia.services.render.Resource; +import org.junit.Test; + +/** + * The invariants of {@link CollectedCacheDependencies#reduce} on generated + * input, and its edge cases. + */ +public class CollectedCacheDependenciesReduceTest { + + private static final Pattern FOLDER_OF_REGEXP = Pattern.compile("/\\\\Q(.*)\\\\E\\(/\\.\\*\\)\\?"); + + private static final long SEED = 20261013L; + private static final int CASES = 500; + private static final int[] LIMITS = {1, 2, 5, 20, 100}; + + private static String regexp(String folder) { + return "/\\Q" + folder.substring(1) + "\\E(/.*)?"; + } + + /** The folder a regexp of the reduction covers, without its leading slash. */ + private static String folderOf(String regexp) { + Matcher matcher = FOLDER_OF_REGEXP.matcher(regexp); + assertTrue("not a regexp of the reduction: " + regexp, matcher.matches()); + return matcher.group(1); + } + + private static boolean isUnderModules(String path) { + return "/modules".equals(path) || path.startsWith("/modules/"); + } + + private static boolean isAtOrBelow(String path, String folder) { + String relative = path.startsWith("/") ? path.substring(1) : path; + return relative.equals(folder) || relative.startsWith(folder + "/"); + } + + /** Paths over a few sites, user folders and modules, where siblings share names and character prefixes. */ + private static List generate(Random random) { + String[] roots = {"/sites/s0", "/sites/s1", "/sites/s10", "/users/ab/cd", "/modules/m"}; + String[] names = {"n0", "n1", "n10", "a.b", "aXb"}; + List paths = new ArrayList<>(); + int count = 1 + random.nextInt(400); + for (int i = 0; i < count; i++) { + StringBuilder path = new StringBuilder(roots[random.nextInt(roots.length)]); + int depth = random.nextInt(5); + for (int d = 0; d < depth; d++) { + path.append('/').append(names[random.nextInt(names.length)]).append(random.nextInt(4)); + } + paths.add(path.toString()); + } + if (random.nextInt(10) == 0) { + paths.add("/sites"); + } + if (random.nextInt(10) == 0) { + paths.add("/"); + } + return paths; + } + + @Test + public void shouldHoldItsInvariantsOnGeneratedInput() { + Random random = new Random(SEED); + for (int c = 0; c < CASES; c++) { + List input = generate(random); + int limit = LIMITS[random.nextInt(LIMITS.length)]; + String context = "case " + c + " (seed " + SEED + "), limit " + limit + ", input " + input; + + CollectedCacheDependencies.Reduced reduced = CollectedCacheDependencies.reduce(input, limit); + Set kept = new LinkedHashSet<>(); + for (String path : input) { + if (!isUnderModules(path)) { + kept.add(path); + } + } + List folders = new ArrayList<>(); + for (String regexp : reduced.regexps) { + folders.add(folderOf(regexp)); + } + + assertEquals(context, kept.size(), reduced.collected); + assertTrue("an exact path that was not collected: " + context, kept.containsAll(reduced.paths)); + + // nothing is lost: each kept path is exact or covered by a regexp + for (String path : kept) { + boolean covered = reduced.paths.contains(path) + || reduced.regexps.stream().anyMatch(path::matches); + assertTrue("lost " + path + ": " + context, covered); + } + + // nothing is merged within the limit + if (kept.size() <= limit) { + assertEquals(context, kept, new HashSet<>(reduced.paths)); + assertTrue(context, reduced.regexps.isEmpty()); + continue; + } + + for (String folder : folders) { + // the floor: never above /sites/ + assertTrue("merged above the floor: " + folder + ": " + context, + folder.split("/").length >= CollectedCacheDependencies.MIN_MERGE_DEPTH); + assertFalse("merged a module folder: " + context, folder.startsWith("modules")); + // precision: a merged folder holds two collected paths or more + long holds = kept.stream().filter(path -> isAtOrBelow(path, folder)).count(); + assertTrue("merged " + folder + " for " + holds + " path: " + context, holds >= 2); + // no redundancy: no entry lies inside a merged folder + for (String path : reduced.paths) { + assertFalse(path + " lies in " + folder + ": " + context, isAtOrBelow(path, folder)); + } + for (String other : folders) { + assertTrue(other + " lies in " + folder + ": " + context, + other.equals(folder) || !isAtOrBelow(other, folder)); + } + } + + // the bound: above the limit, no folder of two segments or more holds two entries + int entries = reduced.paths.size() + reduced.regexps.size(); + if (entries > limit) { + Map covered = new HashMap<>(); + Collection all = new ArrayList<>(reduced.paths); + folders.forEach(folder -> all.add("/" + folder)); + for (String entry : all) { + String[] segments = entry.substring(1).split("/"); + for (int depth = CollectedCacheDependencies.MIN_MERGE_DEPTH; depth <= segments.length; depth++) { + covered.merge(String.join("/", Arrays.copyOf(segments, depth)), 1, Integer::sum); + } + } + covered.forEach((folder, count) -> { + if (count >= 2) { + fail(folder + " could still merge " + count + " entries: " + context); + } + }); + } + + // the result does not depend on the order of the input + List shuffled = new ArrayList<>(input); + java.util.Collections.shuffle(shuffled, new Random(c)); + CollectedCacheDependencies.Reduced again = CollectedCacheDependencies.reduce(shuffled, limit); + assertEquals(context, new HashSet<>(reduced.paths), new HashSet<>(again.paths)); + assertEquals(context, new HashSet<>(reduced.regexps), new HashSet<>(again.regexps)); + } + } + + @Test + public void shouldNotMergeAtTheLimit() { + Set paths = new HashSet<>(); + for (int i = 0; i < 100; i++) { + paths.add("/sites/mysite/home/item-" + i); + } + + assertTrue(CollectedCacheDependencies.reduce(paths, 100).regexps.isEmpty()); + + paths.add("/sites/mysite/home/item-100"); + assertEquals(Set.of(regexp("/sites/mysite/home")), CollectedCacheDependencies.reduce(paths, 100).regexps); + } + + @Test + public void shouldAbsorbADeeperMergeIntoAHigherOne() { + List paths = List.of("/sites/a/x/p1", "/sites/a/x/p2", "/sites/a/x/p3", "/sites/a/y/q1", + "/sites/a/z/r1"); + + CollectedCacheDependencies.Reduced toThree = CollectedCacheDependencies.reduce(paths, 3); + assertEquals(Set.of(regexp("/sites/a/x")), toThree.regexps); + assertEquals(Set.of("/sites/a/y/q1", "/sites/a/z/r1"), new HashSet<>(toThree.paths)); + + // x merges first, then the site folder takes x, y and z + CollectedCacheDependencies.Reduced toOne = CollectedCacheDependencies.reduce(paths, 1); + assertEquals(Set.of(regexp("/sites/a")), toOne.regexps); + assertTrue(toOne.paths.isEmpty()); + } + + @Test + public void shouldDropTheExactFolderItMerges() { + Set paths = new HashSet<>(); + paths.add("/sites/mysite/contents/posts"); + for (int i = 0; i < 150; i++) { + paths.add("/sites/mysite/contents/posts/item-" + i); + } + + CollectedCacheDependencies.Reduced reduced = CollectedCacheDependencies.reduce(paths, 100); + + assertTrue(reduced.paths.isEmpty()); + assertEquals(Set.of(regexp("/sites/mysite/contents/posts")), reduced.regexps); + } + + @Test + public void shouldKeepPathsAboveTheFloorExact() { + List paths = List.of("/", "/sites", "/sites/a", "/sites/b"); + + CollectedCacheDependencies.Reduced reduced = CollectedCacheDependencies.reduce(paths, 1); + + assertEquals(new HashSet<>(paths), new HashSet<>(reduced.paths)); + assertTrue(reduced.regexps.isEmpty()); + } + + @Test + public void shouldRegisterExactPathsWithinTheLimit() { + Resource resource = new Resource("/sites/mysite/home", "/sites/mysite/home", null, "html", "default", "module"); + + CollectedCacheDependencies.register(resource, List.of("/sites/mysite/files/logo.png", "/modules/m/x")); + + assertEquals(Set.of("/sites/mysite/home", "/sites/mysite/files/logo.png"), resource.getDependencies()); + assertTrue(resource.getRegexpDependencies().isEmpty()); + } + + @Test + public void shouldRegisterTheMergedFolders() { + Resource resource = new Resource("/sites/mysite/home", "/sites/mysite/home", null, "html", "default", "module"); + List paths = new ArrayList<>(); + for (int i = 0; i < 150; i++) { + paths.add("/sites/mysite/contents/posts/item-" + i); + } + paths.add("/sites/mysite/files/logo.png"); + + CollectedCacheDependencies.register(resource, paths); + + assertEquals(Set.of("/sites/mysite/home", "/sites/mysite/files/logo.png"), resource.getDependencies()); + assertEquals(Set.of(regexp("/sites/mysite/contents/posts")), resource.getRegexpDependencies()); + } + + @Test + public void shouldRegisterEveryPathWhenTheLimitCannotBeReached() { + Resource resource = new Resource("/sites/portal/home", "/sites/portal/home", null, "html", "default", "module"); + List paths = new ArrayList<>(); + for (int i = 0; i < 150; i++) { + paths.add("/sites/site-" + i + "/home"); + } + + CollectedCacheDependencies.register(resource, paths); + + assertEquals(151, resource.getDependencies().size()); + assertTrue(resource.getRegexpDependencies().isEmpty()); + } + + /** The worst case for a merge: many folders of two paths, so each merge removes one entry. */ + @Test(timeout = 5000) + public void shouldReduceManySmallFoldersQuickly() { + List paths = new ArrayList<>(); + for (int i = 0; i < 2500; i++) { + paths.add("/sites/mysite/folder-" + i + "/a"); + paths.add("/sites/mysite/folder-" + i + "/b"); + } + + CollectedCacheDependencies.Reduced reduced = CollectedCacheDependencies.reduce(paths, 100); + + assertEquals(Set.of(regexp("/sites/mysite")), reduced.regexps); + assertTrue(reduced.paths.isEmpty()); + } +} From 7c3538582671cac5b65fea5cf62fbb25120c0260 Mon Sep 17 00:00:00 2001 From: Kevan Date: Wed, 14 Oct 2026 00:08:50 +0200 Subject: [PATCH 07/10] fix: pass the request to autocollectCacheDependency buildNodeUrl reads its context through JavaScript calls only, so a view may pass an object that stands in for the render context. The test view of #811 does this to imitate a link to another site. autocollectCacheDependency took a RenderContext, and GraalVM cannot convert such an object to that Java type, so the render failed. The method now takes the request, which buildNodeUrl reads with getRequest(). --- .../engine/js/server/CollectedCacheDependencies.java | 9 ++------- .../modules/engine/js/server/RenderHelper.java | 9 +++++---- .../src/utils/urlBuilder/urlBuilder.ts | 5 ++++- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java index cd02df98..5014db40 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java @@ -15,7 +15,6 @@ import javax.servlet.ServletRequest; import org.apache.commons.lang3.StringUtils; -import org.jahia.services.render.RenderContext; import org.jahia.services.render.Resource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -69,13 +68,9 @@ static final class Collector { /** * Collects a path for the fragment being rendered. * - * @param renderContext the current rendering context - * @param path the canonical path of the node the view depends on + * @param request the current request + * @param path the canonical path of the node the view depends on */ - public static void collect(RenderContext renderContext, String path) { - collect(renderContext.getRequest(), path); - } - static void collect(ServletRequest request, String path) { Object collector = request.getAttribute(ATTRIBUTE); if (collector instanceof Collector && ((Collector) collector).enabled && StringUtils.isNotEmpty(path)) { diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java index 0fdbb9bc..89de4ac9 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/RenderHelper.java @@ -44,6 +44,7 @@ import javax.annotation.Nullable; import javax.inject.Inject; import javax.jcr.RepositoryException; +import javax.servlet.http.HttpServletRequest; import javax.servlet.jsp.JspException; import javax.servlet.jsp.tagext.BodyTagSupport; import javax.servlet.jsp.tagext.TagSupport; @@ -350,11 +351,11 @@ public void addCacheDependency(Map attr, RenderContext renderCon * {@code false} collects nothing. Above 100 paths, the paths are merged into * regular expressions. * - * @param path the canonical path of the node to add as a dependency - * @param renderContext the current rendering context + * @param path the canonical path of the node to add as a dependency + * @param request the current request, which holds the collector of the fragment */ - public void autocollectCacheDependency(String path, RenderContext renderContext) { - CollectedCacheDependencies.collect(renderContext, path); + public void autocollectCacheDependency(String path, HttpServletRequest request) { + CollectedCacheDependencies.collect(request, path); } /** diff --git a/javascript-modules-library/src/utils/urlBuilder/urlBuilder.ts b/javascript-modules-library/src/utils/urlBuilder/urlBuilder.ts index 1ae9c760..491fa23c 100644 --- a/javascript-modules-library/src/utils/urlBuilder/urlBuilder.ts +++ b/javascript-modules-library/src/utils/urlBuilder/urlBuilder.ts @@ -89,7 +89,10 @@ export function buildNodeUrl( if (config.autocollectDependency !== false && context.renderContext) { // getCanonicalPath accounts for mounted and versioned nodes - server.render.autocollectCacheDependency(node.getCanonicalPath(), context.renderContext); + server.render.autocollectCacheDependency( + node.getCanonicalPath(), + context.renderContext.getRequest(), + ); } // URL building is an old thing in Jahia, with a lot of branches and special cases: From 02c42ffec86683f9328f49bdaa0e5038a9e51995 Mon Sep 17 00:00:00 2001 From: Kevan Date: Wed, 14 Oct 2026 10:42:01 +0200 Subject: [PATCH 08/10] fix: collect only for a resource the cache filter stores The filter copied the conditions of AggregateCacheFilter, and those differ between core versions. Before 8.2.3.0, AggregateCacheFilter stores a resource rendered under aggregateFilter.skip as its own fragment. The filter skipped that resource, so the fragment kept no dependency on the nodes it linked to. AggregateCacheFilter keeps the key of the fragment it stores in the module param cacheKey, from its prepare to its execute. The filter now runs only on a resource that holds that param, so it follows the cache on every version. --- .../CollectedCacheDependenciesFilter.java | 54 ++++++++++--------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java index 6468226a..a78d9d0f 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilter.java @@ -5,7 +5,6 @@ import javax.jcr.RepositoryException; -import org.jahia.api.Constants; import org.jahia.services.render.RenderContext; import org.jahia.services.render.Resource; import org.jahia.services.render.View; @@ -21,15 +20,24 @@ * Gives each resource that the cache stores as a fragment a collector for the * cache dependencies its view resolves, and registers what it collected. *

- * The conditions are those of {@code AggregateCacheFilter}. A resource that - * filter skips, such as an include, a wrapper, an option or a fragment cached - * with its parent, gets no collector of its own. Its view then collects into the - * collector of the enclosing fragment, which holds its markup. The opt-out of - * the enclosing view then decides for both views, and the opt-out of the inner - * view has no effect. + * {@code AggregateCacheFilter} (16) keeps the key of the fragment it stores in + * the module param {@code cacheKey}, from its {@code prepare} to its + * {@code execute}. This filter runs only on a resource that holds that param, + * so it follows the decision of the cache on every core version. A resource the + * cache does not store, such as an include, a wrapper, an option or a fragment + * cached with its parent, gets no collector of its own. Its view then collects + * into the collector of the enclosing fragment, which holds its markup. The + * opt-out of the enclosing view then decides for both views, and the opt-out of + * the inner view has no effect. *

- * The priority puts {@link #execute} after the script and the wrappers, and - * before the cache filter (16) stores the dependencies of the fragment. A render + * The deprecated cache implementation, {@code AggregateFilter} with + * {@code CacheFilter}, sets no such param, so this filter collects nothing + * with it. + *

+ * The priority puts {@link #prepare} after the cache filter (16) sets the + * param. It puts {@link #execute} after the script and the wrappers, and before + * the cache filter removes the param and stores the dependencies of the + * fragment. A render * filter whose priority is below this one runs its own {@code execute} after * this filter, so a path it collects there is not registered. */ @@ -43,22 +51,13 @@ public class CollectedCacheDependenciesFilter extends AbstractFilter { static final float PRIORITY = 17f; - /** - * The request attribute of {@code AggregateCacheFilter.SKIP_AGGREGATION}. The - * core this module compiles against does not declare it, and sets no such - * attribute. - */ - static final String SKIP_AGGREGATION = "aggregateFilter.skip"; + /** The module param of the fragment key that {@code AggregateCacheFilter} stores. */ + static final String CACHE_KEY = "cacheKey"; public CollectedCacheDependenciesFilter() { setPriority(PRIORITY); setDescription("Registers the cache dependencies a JavaScript view autocollected while it rendered."); - setApplyOnModes("live"); - setSkipOnTemplateTypes("json"); - setSkipOnConfigurations("include,wrapper,option"); - addCondition((renderContext, resource) -> - !Boolean.TRUE.equals(resource.getModuleParams().get(Constants.TO_CACHE_WITH_PARENT_FRAGMENT)) - && !Boolean.TRUE.equals(renderContext.getRequest().getAttribute(SKIP_AGGREGATION))); + addCondition((renderContext, resource) -> resource.getModuleParams().get(CACHE_KEY) != null); } @Override @@ -76,15 +75,20 @@ public String execute(String previousOut, RenderContext renderContext, Resource return previousOut; } - private static boolean isEnabled(RenderContext renderContext, Resource resource) { + private boolean isEnabled(RenderContext renderContext, Resource resource) { try { - Script script = resource.getScript(renderContext); - View view = script == null ? null : script.getView(); - Properties properties = view == null ? null : view.getProperties(); + Properties properties = viewProperties(renderContext, resource); return properties == null || !"false".equals(properties.getProperty(DISABLE_PROPERTY)); } catch (RepositoryException e) { logger.debug("Cannot resolve the view of {}, its dependencies are collected", resource, e); return true; } } + + /** The properties of the view that renders the resource, or null when it has none. */ + Properties viewProperties(RenderContext renderContext, Resource resource) throws RepositoryException { + Script script = resource.getScript(renderContext); + View view = script == null ? null : script.getView(); + return view == null ? null : view.getProperties(); + } } From 13cb308d62aca566c33496c99825d45294050999 Mon Sep 17 00:00:00 2001 From: Kevan Date: Wed, 14 Oct 2026 10:42:02 +0200 Subject: [PATCH 09/10] perf: skip the merge of the collected paths within the limit Most fragments collect a few paths. reduce now drops the paths under /modules and returns the rest when they fit, before it splits each path into segments. --- .../js/server/CollectedCacheDependencies.java | 22 +++++++++++++------ .../CollectedCacheDependenciesReduceTest.java | 12 ++++++++++ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java index 5014db40..53ea90a2 100644 --- a/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java +++ b/javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependencies.java @@ -156,18 +156,26 @@ private Reduced(int collected, Collection paths, Collection rege * @return the paths and regexps to register */ static Reduced reduce(Collection dependencies, int limit) { + Set kept = new LinkedHashSet<>(); + for (String path : dependencies) { + if (!isUnderModules(path)) { + kept.add(path); + } + } + int collected = kept.size(); + if (collected <= limit) { + return new Reduced(collected, kept, Set.of()); + } + // the exact paths and the merged folders, each with its segments Map exact = new LinkedHashMap<>(); Map folders = new LinkedHashMap<>(); int deepest = 0; - for (String path : dependencies) { - if (!isUnderModules(path)) { - String[] segments = StringUtils.split(path, '/'); - exact.put(path, segments); - deepest = Math.max(deepest, segments.length); - } + for (String path : kept) { + String[] segments = StringUtils.split(path, '/'); + exact.put(path, segments); + deepest = Math.max(deepest, segments.length); } - int collected = exact.size(); // Two folders at one depth never overlap, so a merge at one depth leaves the // other folders of that depth as they were diff --git a/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesReduceTest.java b/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesReduceTest.java index 3f7f84ab..ecd63faa 100644 --- a/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesReduceTest.java +++ b/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesReduceTest.java @@ -172,6 +172,18 @@ public void shouldNotMergeAtTheLimit() { assertEquals(Set.of(regexp("/sites/mysite/home")), CollectedCacheDependencies.reduce(paths, 100).regexps); } + @Test + public void shouldCountTheLimitAfterDroppingModulePaths() { + List paths = List.of("/sites/a/x/p1", "/modules", "/modules/m/views/one", "/sites/a/x/p2"); + + // four paths, but two once /modules is dropped, so a limit of two merges nothing + CollectedCacheDependencies.Reduced reduced = CollectedCacheDependencies.reduce(paths, 2); + + assertEquals(2, reduced.collected); + assertEquals(List.of("/sites/a/x/p1", "/sites/a/x/p2"), new ArrayList<>(reduced.paths)); + assertTrue(reduced.regexps.isEmpty()); + } + @Test public void shouldAbsorbADeeperMergeIntoAHigherOne() { List paths = List.of("/sites/a/x/p1", "/sites/a/x/p2", "/sites/a/x/p3", "/sites/a/y/q1", From 4c808fd1716d4bde6ef2c46753caf061e1e360f5 Mon Sep 17 00:00:00 2001 From: Kevan Date: Wed, 14 Oct 2026 10:42:02 +0200 Subject: [PATCH 10/10] test: run the collection filter in a render chain The test runs the filter in a real RenderChain, between a cache filter and a view that stand in for those of Jahia. It covers a nested fragment, a resource the cache does not store, the opt-out, and a render outside any fragment. --- .../CollectedCacheDependenciesFilterTest.java | 207 ++++++++++++++++++ 1 file changed, 207 insertions(+) create mode 100644 javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilterTest.java diff --git a/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilterTest.java b/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilterTest.java new file mode 100644 index 00000000..e78669a1 --- /dev/null +++ b/javascript-modules-engine-java/src/test/java/org/jahia/modules/javascript/modules/engine/js/server/CollectedCacheDependenciesFilterTest.java @@ -0,0 +1,207 @@ +package org.jahia.modules.javascript.modules.engine.js.server; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import java.lang.reflect.Proxy; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.IdentityHashMap; +import java.util.List; +import java.util.Map; +import java.util.Properties; +import java.util.Set; +import java.util.stream.Collectors; + +import javax.servlet.http.HttpServletRequest; + +import org.jahia.services.render.RenderContext; +import org.jahia.services.render.Resource; +import org.jahia.services.render.filter.AbstractFilter; +import org.jahia.services.render.filter.RenderChain; +import org.junit.Before; +import org.junit.Test; + +/** + * Runs {@link CollectedCacheDependenciesFilter} in a real {@link RenderChain}, + * between a cache filter and a view that stand in for those of Jahia. + */ +public class CollectedCacheDependenciesFilterTest { + + private RenderContext renderContext; + // keyed by identity: Resource overrides equals, and two renders of one node are two fragments + /** The resources the cache stores, and the dependencies each one had when the cache stored it. */ + private final Set cached = Collections.newSetFromMap(new IdentityHashMap<>()); + private final Map> stored = new IdentityHashMap<>(); + /** What each view does while it renders. */ + private final Map views = new IdentityHashMap<>(); + private final Set optedOut = Collections.newSetFromMap(new IdentityHashMap<>()); + + /** Stands in for AggregateCacheFilter: it sets the cache key of a fragment it stores, and removes it in execute. */ + private final AbstractFilter cache = new AbstractFilter() { + { + setPriority(16); + } + + @Override + public String prepare(RenderContext renderContext, Resource resource, RenderChain chain) { + if (cached.contains(resource)) { + resource.getModuleParams().put(CollectedCacheDependenciesFilter.CACHE_KEY, "key"); + } + return null; + } + + @Override + public String execute(String previousOut, RenderContext renderContext, Resource resource, RenderChain chain) { + if (resource.getModuleParams().remove(CollectedCacheDependenciesFilter.CACHE_KEY) != null) { + Set dependencies = new HashSet<>(resource.getDependencies()); + dependencies.addAll(resource.getRegexpDependencies()); + dependencies.remove(resource.getNodePath()); + stored.put(resource, dependencies); + } + return previousOut; + } + }; + + private final CollectedCacheDependenciesFilter filter = new CollectedCacheDependenciesFilter() { + @Override + Properties viewProperties(RenderContext renderContext, Resource resource) { + Properties properties = new Properties(); + if (optedOut.contains(resource)) { + properties.setProperty(DISABLE_PROPERTY, "false"); + } + return properties; + } + }; + + /** Stands in for the script: it runs the view of the resource. */ + private final AbstractFilter script = new AbstractFilter() { + { + setPriority(100); + } + + @Override + public String prepare(RenderContext renderContext, Resource resource, RenderChain chain) { + views.getOrDefault(resource, () -> { }).run(); + return "

"; + } + }; + + /** A request that only keeps its attributes. */ + private static HttpServletRequest request() { + Map attributes = new HashMap<>(); + return (HttpServletRequest) Proxy.newProxyInstance(HttpServletRequest.class.getClassLoader(), + new Class[] {HttpServletRequest.class}, (proxy, method, args) -> { + switch (method.getName()) { + case "getAttribute": + return attributes.get(args[0]); + case "setAttribute": + attributes.put(args[0], args[1]); + return null; + case "removeAttribute": + attributes.remove(args[0]); + return null; + default: + throw new UnsupportedOperationException(method.getName()); + } + }); + } + + private static Resource resource(String path) { + return new Resource(path, path, null, "html", "default", "module"); + } + + @Before + public void setUp() { + renderContext = new RenderContext(request(), null, null); + } + + private void render(Resource resource) throws Exception { + new RenderChain(cache, filter, script).doFilter(renderContext, resource); + } + + private void collect(String path) { + CollectedCacheDependencies.collect(renderContext.getRequest(), path); + } + + @Test + public void shouldRegisterWhatTheViewCollectedBeforeTheCacheStoresIt() throws Exception { + Resource page = resource("/sites/mysite/home"); + cached.add(page); + views.put(page, () -> collect("/sites/mysite/home/about")); + + render(page); + + assertEquals(Set.of("/sites/mysite/home/about"), stored.get(page)); + assertNull(renderContext.getRequest().getAttribute(CollectedCacheDependencies.ATTRIBUTE)); + } + + @Test + public void shouldKeepTheCollectorOfANestedFragmentApart() throws Exception { + Resource page = resource("/sites/mysite/home"); + Resource teaser = resource("/sites/mysite/home/area/teaser"); + cached.add(page); + cached.add(teaser); + views.put(teaser, () -> collect("/sites/mysite/files/logo.png")); + views.put(page, () -> { + collect("/sites/mysite/home/about"); + try { + render(teaser); + } catch (Exception e) { + throw new IllegalStateException(e); + } + collect("/sites/mysite/home/contact"); + }); + + render(page); + + assertEquals(Set.of("/sites/mysite/files/logo.png"), stored.get(teaser)); + assertEquals(Set.of("/sites/mysite/home/about", "/sites/mysite/home/contact"), stored.get(page)); + } + + @Test + public void shouldCollectIntoTheEnclosingFragmentWhatTheCacheDoesNotStore() throws Exception { + Resource page = resource("/sites/mysite/home"); + Resource include = resource("/sites/mysite/home/area/include"); + cached.add(page); + views.put(include, () -> collect("/sites/mysite/files/logo.png")); + views.put(page, () -> { + try { + render(include); + } catch (Exception e) { + throw new IllegalStateException(e); + } + }); + + render(page); + + assertEquals(Set.of("/sites/mysite/files/logo.png"), stored.get(page)); + assertEquals(List.of(page), new ArrayList<>(stored.keySet())); + } + + @Test + public void shouldCollectNothingForAViewThatOptedOut() throws Exception { + Resource page = resource("/sites/mysite/home"); + cached.add(page); + optedOut.add(page); + views.put(page, () -> collect("/sites/mysite/home/about")); + + render(page); + + assertEquals(Set.of(), stored.get(page)); + } + + @Test + public void shouldCollectNothingOutsideACachedFragment() throws Exception { + Resource page = resource("/sites/mysite/home"); + views.put(page, () -> collect("/sites/mysite/home/about")); + + render(page); + + assertEquals(Set.of(), page.getDependencies().stream() + .filter(path -> !path.equals(page.getNodePath())).collect(Collectors.toSet())); + assertNull(renderContext.getRequest().getAttribute(CollectedCacheDependencies.ATTRIBUTE)); + } +}