This is an automated email from the ASF dual-hosted git repository. jsedding pushed a commit to branch resolver-2.x-backports in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-servlets-resolver.git
commit 3b7e1e07843a043daf04931dc73855eb6e37646e Author: Jörg Hoh <[email protected]> AuthorDate: Tue May 26 14:28:43 2026 +0200 SLING-13133 listChildren must be streaming (#64) SLING-13133 converted merging logic to use a streaming mode, added unit tests (cherry picked from commit 652d7e008c5f791151c30ae250d842dde539d32e) --- .../resource/MergingServletResourceProvider.java | 136 +++++++-- .../MergingServletResourceProviderTest.java | 339 +++++++++++++++++++++ 2 files changed, 452 insertions(+), 23 deletions(-) diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/resource/MergingServletResourceProvider.java b/src/main/java/org/apache/sling/servlets/resolver/internal/resource/MergingServletResourceProvider.java index 61ce1c7..1b30888 100644 --- a/src/main/java/org/apache/sling/servlets/resolver/internal/resource/MergingServletResourceProvider.java +++ b/src/main/java/org/apache/sling/servlets/resolver/internal/resource/MergingServletResourceProvider.java @@ -21,11 +21,12 @@ package org.apache.sling.servlets.resolver.internal.resource; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.Iterator; -import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.NoSuchElementException; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicReference; @@ -180,39 +181,128 @@ public class MergingServletResourceProvider extends ResourceProvider<Object> { @SuppressWarnings("unchecked") public Iterator<Resource> listChildren( @SuppressWarnings("rawtypes") final ResolveContext ctx, final Resource parent) { - Map<String, Resource> result = new LinkedHashMap<>(); - final ResourceProvider<?> parentProvider = ctx.getParentResourceProvider(); - if (parentProvider != null) { - for (Iterator<Resource> iter = parentProvider.listChildren(ctx.getParentResolveContext(), parent); - iter != null && iter.hasNext(); ) { - Resource resource = iter.next(); - result.put(resource.getPath(), resource); + final Iterator<Resource> parentIterator = + parentProvider == null ? null : parentProvider.listChildren(ctx.getParentResolveContext(), parent); + + // Indexed servlet paths under this parent (from tree). Snapshot to array for stable iteration. + final Set<String> paths = tree.get().get(parent.getPath()); + final String[] pathArray = paths == null ? new String[0] : paths.toArray(new String[0]); + // LinkedHashSet preserves order; iterator yields overlay-only paths after parent iteration. + final Set<String> pendingPaths = new LinkedHashSet<>(Arrays.asList(pathArray)); + final Iterator<String> overlayIterator = pendingPaths.iterator(); + + final ConcurrentHashMap<String, Map.Entry<ServletResourceProvider, ServiceReference<?>>> localProviders = + providers.get(); + + return new MergingChildrenIterator(parentIterator, ctx, parent, pendingPaths, overlayIterator, localProviders); + } + + private static final class MergingChildrenIterator implements Iterator<Resource> { + + private final Iterator<Resource> parentIterator; + private final ResolveContext<?> ctx; + private final Resource parent; + private final Set<String> pendingPaths; + private final Set<String> processedPaths = new HashSet<>(); + private final Iterator<String> overlayIterator; + private final ConcurrentHashMap<String, Map.Entry<ServletResourceProvider, ServiceReference<?>>> localProviders; + + private Resource next; + private boolean nextComputed; + + MergingChildrenIterator( + Iterator<Resource> parentIterator, + ResolveContext<?> ctx, + Resource parent, + Set<String> pendingPaths, + Iterator<String> overlayIterator, + ConcurrentHashMap<String, Map.Entry<ServletResourceProvider, ServiceReference<?>>> localProviders) { + this.parentIterator = parentIterator; + this.ctx = ctx; + this.parent = parent; + this.pendingPaths = pendingPaths; + this.overlayIterator = overlayIterator; + this.localProviders = localProviders; + } + + @Override + public boolean hasNext() { + if (!nextComputed) { + next = fetchNext(); + nextComputed = true; } + return next != null; } - Set<String> paths = tree.get().get(parent.getPath()); - if (paths != null) { - for (String path : paths.toArray(new String[0])) { - Map.Entry<ServletResourceProvider, ServiceReference<?>> provider = - providers.get().get(path); + @Override + public Resource next() { + if (!hasNext()) { + throw new NoSuchElementException(); + } + Resource result = next; + next = null; + nextComputed = false; + return result; + } + + /** + * Returns the next merged child, or null when exhausted, either from parent iterator or + * from overlay paths not yet processed. + */ + private Resource fetchNext() { + Resource fromParent = tryNextFromParent(); + if (fromParent != null) { + return fromParent; + } + return tryNextFromOverlay(); + } + + /** advance parent iterator; emit overlay replacement or parent child per path. */ + @SuppressWarnings("unchecked") + private Resource tryNextFromParent() { + if (parentIterator == null || !parentIterator.hasNext()) { + return null; + } + Resource parentChild = parentIterator.next(); + String path = parentChild.getPath(); + if (!pendingPaths.contains(path)) { + return parentChild; + } + processedPaths.add(path); + Map.Entry<ServletResourceProvider, ServiceReference<?>> provider = localProviders.get(path); + if (provider != null) { + Resource resource = provider.getKey().getResource((ResolveContext<Object>) ctx, path, null, parent); + if (resource != null) { + if (resource instanceof ServletResource) { + ((ServletResource) resource).setWrappedResource(parentChild); + } + return resource; + } + } + return parentChild; + } + /** emit overlay-only paths (not already processed) as servlet or synthetic. */ + @SuppressWarnings("unchecked") + private Resource tryNextFromOverlay() { + while (overlayIterator.hasNext()) { + String path = overlayIterator.next(); + if (processedPaths.contains(path)) { + continue; + } + Map.Entry<ServletResourceProvider, ServiceReference<?>> provider = localProviders.get(path); if (provider != null) { - Resource resource = provider.getKey().getResource(ctx, path, null, parent); + Resource resource = provider.getKey().getResource((ResolveContext<Object>) ctx, path, null, parent); if (resource != null) { - Resource wrapped = result.put(path, resource); - if (resource instanceof ServletResource) { - ((ServletResource) resource).setWrappedResource(wrapped); - } + return resource; } } else { - result.computeIfAbsent( - path, - key -> new SyntheticResource( - ctx.getResourceResolver(), key, ResourceProvider.RESOURCE_TYPE_SYNTHETIC)); + return new SyntheticResource( + ctx.getResourceResolver(), path, ResourceProvider.RESOURCE_TYPE_SYNTHETIC); } } + return null; } - return result.values().iterator(); } } diff --git a/src/test/java/org/apache/sling/servlets/resolver/internal/resource/MergingServletResourceProviderTest.java b/src/test/java/org/apache/sling/servlets/resolver/internal/resource/MergingServletResourceProviderTest.java new file mode 100644 index 0000000..2a72586 --- /dev/null +++ b/src/test/java/org/apache/sling/servlets/resolver/internal/resource/MergingServletResourceProviderTest.java @@ -0,0 +1,339 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.servlets.resolver.internal.resource; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.Iterator; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; + +import jakarta.servlet.GenericServlet; +import jakarta.servlet.Servlet; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; +import org.apache.sling.api.resource.Resource; +import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.api.resource.SyntheticResource; +import org.apache.sling.spi.resource.provider.ResolveContext; +import org.apache.sling.spi.resource.provider.ResourceProvider; +import org.junit.Test; +import org.mockito.Mockito; +import org.osgi.framework.ServiceReference; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; + +public class MergingServletResourceProviderTest { + + private static final Servlet TEST_SERVLET = new GenericServlet() { + private static final long serialVersionUID = 1L; + + @Override + public void service(ServletRequest req, ServletResponse res) { + // nothing do do + } + }; + + private interface Marker {} + + /** + * Validates behavior when there is no parent provider and only indexed servlet paths are available, ensuring that + * synthetic intermediate resources are still exposed as children so traversal to deeper servlet paths remains + * possible. + */ + @Test + public void testListChildrenWithNoParentProviderCreatesSyntheticIntermediate() { + final ResourceResolver resolver = Mockito.mock(ResourceResolver.class); + final ResolveContext<Object> ctx = mockContext(resolver, null, null); + final MergingServletResourceProvider mergingProvider = new MergingServletResourceProvider(); + final Resource parent = new SyntheticResource(resolver, "/apps", "type"); + + // Register a deep path so /apps/sling becomes a synthetic child. + addProvider(mergingProvider, "/apps/sling/sample/GET.servlet"); + + final List<Resource> children = toList(mergingProvider.listChildren(ctx, parent)); + + assertEquals(1, children.size()); + assertEquals("/apps/sling", children.get(0).getPath()); + assertTrue(children.get(0) instanceof SyntheticResource); + } + + /** + * Verifies merge semantics when parent children and servlet-backed children overlap, asserting that existing parent + * entries are overridden in place, wrapped-resource adaptation still works, and additional indexed branches are + * represented as synthetic children. + */ + @Test + public void testListChildrenMergesParentAndOverlaysProviderResource() { + final ResourceResolver resolver = Mockito.mock(ResourceResolver.class); + + final Marker marker = new Marker() {}; + final Resource parentA = mockParentChild("/apps/parent/a", marker); + final Resource parentB = mockParentChild("/apps/parent/b", marker); + final Resource parent = new SyntheticResource(resolver, "/apps/parent", "type"); + + @SuppressWarnings("unchecked") + final ResourceProvider<Object> parentProvider = Mockito.mock(ResourceProvider.class); + Mockito.when(parentProvider.listChildren(Mockito.any(), Mockito.eq(parent))) + .thenReturn(List.of(parentA, parentB).iterator()); + + final ResolveContext<Object> parentCtx = Mockito.mock(ResolveContext.class); + final ResolveContext<Object> ctx = mockContext(resolver, parentProvider, parentCtx); + + final MergingServletResourceProvider mergingProvider = new MergingServletResourceProvider(); + addProvider(mergingProvider, "/apps/parent/b", "/apps/parent/c/deep"); + + final List<Resource> children = toList(mergingProvider.listChildren(ctx, parent)); + + assertEquals(3, children.size()); + assertEquals("/apps/parent/a", children.get(0).getPath()); + assertEquals("/apps/parent/b", children.get(1).getPath()); + assertEquals("/apps/parent/c", children.get(2).getPath()); + + assertTrue(children.get(1) instanceof ServletResource); + assertSame(marker, children.get(1).adaptTo(Marker.class)); + assertTrue(children.get(2) instanceof SyntheticResource); + } + + /** + * Covers defensive handling for providers returning a null child iterator, confirming that overlay children are + * still returned and that listChildren continues to produce a valid result without throwing or dropping indexed + * servlet resources. + */ + @Test + public void testListChildrenHandlesNullParentIterator() { + final ResourceResolver resolver = Mockito.mock(ResourceResolver.class); + final Resource parent = new SyntheticResource(resolver, "/apps/parent", "type"); + + @SuppressWarnings("unchecked") + final ResourceProvider<Object> parentProvider = Mockito.mock(ResourceProvider.class); + Mockito.when(parentProvider.listChildren(Mockito.any(), Mockito.eq(parent))) + .thenReturn(null); + + final ResolveContext<Object> parentCtx = Mockito.mock(ResolveContext.class); + final ResolveContext<Object> ctx = mockContext(resolver, parentProvider, parentCtx); + + final MergingServletResourceProvider mergingProvider = new MergingServletResourceProvider(); + addProvider(mergingProvider, "/apps/parent/child"); + + final List<Resource> children = toList(mergingProvider.listChildren(ctx, parent)); + + assertEquals(1, children.size()); + assertEquals("/apps/parent/child", children.get(0).getPath()); + assertTrue(children.get(0) instanceof ServletResource); + } + + /** + * Exercises a large parent-child set to lock in expected merge outcomes under high cardinality, including stable + * presence of overridden entries and appended overlay-only entries, which provides a baseline for later + * performance-focused refactoring. + */ + @Test + public void testListChildrenWithLargeParentChildrenSet() { + final ResourceResolver resolver = Mockito.mock(ResourceResolver.class); + final Resource parent = new SyntheticResource(resolver, "/apps/parent", "type"); + + final int count = 5000; + final List<Resource> parentChildren = new ArrayList<>(count); + for (int i = 0; i < count; i++) { + parentChildren.add(mockParentChild("/apps/parent/child-" + i, null)); + } + + @SuppressWarnings("unchecked") + final ResourceProvider<Object> parentProvider = Mockito.mock(ResourceProvider.class); + Mockito.when(parentProvider.listChildren(Mockito.any(), Mockito.eq(parent))) + .thenReturn(parentChildren.iterator()); + + final ResolveContext<Object> parentCtx = Mockito.mock(ResolveContext.class); + final ResolveContext<Object> ctx = mockContext(resolver, parentProvider, parentCtx); + + final MergingServletResourceProvider mergingProvider = new MergingServletResourceProvider(); + addProvider(mergingProvider, "/apps/parent/child-1200", "/apps/parent/child-2200", "/apps/parent/overlay-only"); + + final List<Resource> children = toList(mergingProvider.listChildren(ctx, parent)); + + assertEquals(count + 1, children.size()); + assertEquals("/apps/parent/child-0", children.get(0).getPath()); + assertEquals( + "/apps/parent/overlay-only", children.get(children.size() - 1).getPath()); + assertTrue(pathExists(children, "/apps/parent/child-1200")); + assertTrue(pathExists(children, "/apps/parent/child-2200")); + assertTrue(pathExists(children, "/apps/parent/overlay-only")); + + final Resource overridden = childByPath(children, "/apps/parent/child-1200"); + assertNotNull(overridden); + assertTrue(overridden instanceof ServletResource); + } + + /** + * Ensures the merged iterator behaves in a streaming fashion by verifying that reading only the first result does + * not force full consumption of the parent iterator, which protects callers that stop early from paying the full + * cost of enumerating all parent children. + */ + @Test + public void testListChildrenStreamsWithoutConsumingAllParentChildrenForFirstResult() { + final ResourceResolver resolver = Mockito.mock(ResourceResolver.class); + final Resource parent = new SyntheticResource(resolver, "/apps/parent", "type"); + + final int count = 5000; + final List<Resource> parentChildren = new ArrayList<>(count); + for (int i = 0; i < count; i++) { + parentChildren.add(mockParentChild("/apps/parent/child-" + i, null)); + } + final AtomicInteger consumed = new AtomicInteger(); + final Iterator<Resource> countingIterator = new Iterator<Resource>() { + private final Iterator<Resource> delegate = parentChildren.iterator(); + + @Override + public boolean hasNext() { + return delegate.hasNext(); + } + + @Override + public Resource next() { + consumed.incrementAndGet(); + return delegate.next(); + } + }; + + @SuppressWarnings("unchecked") + final ResourceProvider<Object> parentProvider = Mockito.mock(ResourceProvider.class); + Mockito.when(parentProvider.listChildren(Mockito.any(), Mockito.eq(parent))) + .thenReturn(countingIterator); + + final ResolveContext<Object> parentCtx = Mockito.mock(ResolveContext.class); + final ResolveContext<Object> ctx = mockContext(resolver, parentProvider, parentCtx); + final MergingServletResourceProvider mergingProvider = new MergingServletResourceProvider(); + addProvider(mergingProvider, "/apps/parent/overlay-only"); + + final Iterator<Resource> children = mergingProvider.listChildren(ctx, parent); + + assertTrue(children.hasNext()); + assertEquals("/apps/parent/child-0", children.next().getPath()); + assertEquals(1, consumed.get()); + assertTrue(consumed.get() < count); + } + + /** + * Validates MergingChildrenIterator semantics from its javadoc: (1) Phase 1 — parent children in order, with + * overlay paths replaced by servlet/synthetic and marked processed, others emitted as-is. (2) Phase 2 — after + * parent exhausted, overlay-only paths emitted. (3) No path emitted twice (processedPaths deduplication). + */ + @Test + public void testMergingChildrenIteratorPhaseOrderAndNoDuplicatePaths() { + final ResourceResolver resolver = Mockito.mock(ResourceResolver.class); + final Resource parent = new SyntheticResource(resolver, "/apps/merge", "type"); + final Resource parentA = mockParentChild("/apps/merge/a", null); + final Resource parentB = mockParentChild("/apps/merge/b", null); + final Resource parentC = mockParentChild("/apps/merge/c", null); + + @SuppressWarnings("unchecked") + final ResourceProvider<Object> parentProvider = Mockito.mock(ResourceProvider.class); + Mockito.when(parentProvider.listChildren(Mockito.any(), Mockito.eq(parent))) + .thenReturn(List.of(parentA, parentB, parentC).iterator()); + + final ResolveContext<Object> parentCtx = Mockito.mock(ResolveContext.class); + final ResolveContext<Object> ctx = mockContext(resolver, parentProvider, parentCtx); + final MergingServletResourceProvider mergingProvider = new MergingServletResourceProvider(); + // Overlay: b (replacement for parent b), d (overlay-only). + addProvider(mergingProvider, "/apps/merge/b", "/apps/merge/d"); + + final List<Resource> children = toList(mergingProvider.listChildren(ctx, parent)); + final List<String> paths = new ArrayList<>(); + for (Resource r : children) { + paths.add(r.getPath()); + } + + // Phase 1 order: parent a (as-is), parent b replaced by servlet, parent c (as-is). Phase 2: overlay-only d. + assertEquals(4, children.size()); + assertEquals("/apps/merge/a", paths.get(0)); + assertEquals("/apps/merge/b", paths.get(1)); + assertEquals("/apps/merge/c", paths.get(2)); + assertEquals("/apps/merge/d", paths.get(3)); + + assertTrue(children.get(1) instanceof ServletResource); + assertTrue(children.get(3) instanceof ServletResource); + + // processedPaths: no path emitted twice. + assertEquals("no duplicate paths", paths.size(), new LinkedHashSet<>(paths).size()); + } + + private static boolean pathExists(List<Resource> resources, String path) { + return childByPath(resources, path) != null; + } + + private static Resource childByPath(List<Resource> resources, String path) { + for (Resource child : resources) { + if (path.equals(child.getPath())) { + return child; + } + } + return null; + } + + private static Resource mockParentChild(String path, Marker marker) { + final Resource resource = Mockito.mock(Resource.class); + Mockito.when(resource.getPath()).thenReturn(path); + Mockito.when(resource.getResourceType()).thenReturn("parent/type"); + if (marker != null) { + Mockito.when(resource.adaptTo(Marker.class)).thenReturn(marker); + } else { + Mockito.when(resource.adaptTo(Marker.class)).thenReturn(null); + } + return resource; + } + + private static ResolveContext<Object> mockContext( + ResourceResolver resolver, ResourceProvider<?> parentProvider, ResolveContext<?> parentCtx) { + @SuppressWarnings("unchecked") + final ResolveContext<Object> ctx = Mockito.mock(ResolveContext.class); + Mockito.when(ctx.getResourceResolver()).thenReturn(resolver); + Mockito.doReturn(parentProvider).when(ctx).getParentResourceProvider(); + Mockito.doReturn(parentCtx).when(ctx).getParentResolveContext(); + return ctx; + } + + @SafeVarargs + private static void addProvider(MergingServletResourceProvider mergingProvider, String... paths) { + final Set<String> servletPaths = new LinkedHashSet<>(); + Collections.addAll(servletPaths, paths); + + final ServletResourceProvider servletProvider = + new ServletResourceProvider(TEST_SERVLET, servletPaths, Collections.emptySet(), null); + @SuppressWarnings("unchecked") + final ServiceReference<Servlet> reference = Mockito.mock(ServiceReference.class); + mergingProvider.add(servletProvider, reference); + } + + private static List<Resource> toList(Iterator<Resource> it) { + if (it == null) { + return Collections.emptyList(); + } + final List<Resource> resources = new ArrayList<>(); + while (it.hasNext()) { + resources.add(it.next()); + } + return resources; + } +}
