This is an automated email from the ASF dual-hosted git repository. joerghoh pushed a commit to branch SLING-13133 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-servlets-resolver.git
commit 20b2f95886662d2dfac01fe532f8e121f4a9edb5 Author: Joerg Hoh <[email protected]> AuthorDate: Fri Mar 6 16:55:44 2026 +0100 SLING-13133 converted merging logic into streaming, added unit tests --- .../resource/MergingServletResourceProvider.java | 133 +++++++++++++++++---- .../MergingServletResourceProviderTest.java | 94 +++++++++++++++ 2 files changed, 204 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..6ad2dbd 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,125 @@ 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. Two phases: + * (1) Advance the parent iterator; foreach parent child path that is also in the overlay set + * (pendingPaths), try to replace it with the servlet or synthetic resource and mark that path as + * processed; otherwise emit the parent child as-is. + * (2) After parent is exhausted, iterate overlay paths and emit any that were not already processed + * (overlay-only children, as synthetic or from provider). processedPaths ensures we do not emit the + * same path twice. + */ + @SuppressWarnings("unchecked") + private Resource fetchNext() { + if (parentIterator != null) { + while (parentIterator.hasNext()) { + Resource parentChild = parentIterator.next(); + String path = parentChild.getPath(); + // If this path is in the overlay set, try to replace with servlet/synthetic (original used a map + // and overwrote by path); otherwise emit parent child as-is. + if (pendingPaths.contains(path)) { + 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; + } + } + // No overlay replacement (provider null or getResource null): keep parent child. + } + return parentChild; + } + } + while (overlayIterator.hasNext()) { + String path = overlayIterator.next(); + // avoid duplicates + 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 index d117208..504d513 100644 --- 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 @@ -24,6 +24,7 @@ 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; @@ -183,6 +184,99 @@ public class MergingServletResourceProviderTest { 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; }
