From f1d5c1331a024fd3c0b9ba3385c1126d071fe67c Mon Sep 17 00:00:00 2001 From: Patrique Legault Date: Thu, 23 Nov 2023 22:59:39 -0500 Subject: [PATCH 1/3] Adding fix for SLING-8974 --- .../servlets/post/impl/SlingPostServlet.java | 7 +- .../post/impl/operations/DeleteOperation.java | 5 ++ .../impl/operations/DeleteOperationTest.java | 69 +++++++++++++++++++ 3 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java diff --git a/src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java b/src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java index cc6a5a4..df3b1a6 100644 --- a/src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java +++ b/src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java @@ -248,8 +248,13 @@ protected void doPost(final SlingHttpServletRequest request, try { operation.run(request, htmlResponse, processors); } catch (ResourceNotFoundException rnfe) { - htmlResponse.setStatus(HttpServletResponse.SC_NOT_FOUND, + if (operation instanceof DeleteOperation) { + htmlResponse.setStatus(HttpServletResponse.SC_BAD_REQUEST, rnfe.getMessage()); + } else { + htmlResponse.setStatus(HttpServletResponse.SC_NOT_FOUND, + rnfe.getMessage()); + } } catch (final PreconditionViolatedPersistenceException e) { logPersistenceException(request, operation, e); if (backwardsCompatibleStatuscode) { diff --git a/src/main/java/org/apache/sling/servlets/post/impl/operations/DeleteOperation.java b/src/main/java/org/apache/sling/servlets/post/impl/operations/DeleteOperation.java index a21c7d6..3d28976 100644 --- a/src/main/java/org/apache/sling/servlets/post/impl/operations/DeleteOperation.java +++ b/src/main/java/org/apache/sling/servlets/post/impl/operations/DeleteOperation.java @@ -23,8 +23,10 @@ import org.apache.sling.api.SlingHttpServletRequest; import org.apache.sling.api.request.RequestPathInfo; +import org.apache.sling.api.resource.NonExistingResource; import org.apache.sling.api.resource.PersistenceException; import org.apache.sling.api.resource.Resource; +import org.apache.sling.api.resource.ResourceNotFoundException; import org.apache.sling.servlets.post.Modification; import org.apache.sling.servlets.post.PostResponse; import org.apache.sling.servlets.post.SlingPostConstants; @@ -69,6 +71,9 @@ protected void doRun(final SlingHttpServletRequest request, final Iterator res = getApplyToResources(request); if (res == null) { final Resource resource = request.getResource(); + if (resource instanceof NonExistingResource) { + throw new ResourceNotFoundException(String.format("Cannot delete NonExistingResource %s", resource.getPath())); + } deleteResource(resource, changes, versioningConfiguration, deleteChunks); } else { diff --git a/src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java b/src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java new file mode 100644 index 0000000..beee622 --- /dev/null +++ b/src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java @@ -0,0 +1,69 @@ +/* + * 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.post.impl.operations; + +import static org.junit.Assert.assertEquals; + +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.List; + +import org.apache.sling.api.SlingHttpServletRequest; +import org.apache.sling.api.request.RequestPathInfo; +import org.apache.sling.api.request.RequestProgressTracker; +import org.apache.sling.api.resource.NonExistingResource; +import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.servlets.post.PostResponse; +import org.junit.Assert; +import org.junit.Test; +import org.mockito.Mockito; + +public class DeleteOperationTest { + + @Test + public void testDeletingNonExistingResource() throws Exception { + + DeleteOperation deleteOperation = new DeleteOperation(); + + SlingHttpServletRequest request = Mockito.mock(SlingHttpServletRequest.class); + ResourceResolver resourceResolver = Mockito.mock(ResourceResolver.class); + RequestPathInfo requestPathInfo = Mockito.mock(RequestPathInfo.class); + RequestProgressTracker requestProgressTracker = Mockito.mock(RequestProgressTracker.class); + NonExistingResource resource = Mockito.mock(NonExistingResource.class, Mockito.RETURNS_DEEP_STUBS); + Mockito.when(request.getResourceResolver()).thenReturn(resourceResolver); + + Mockito.when(request.getParameter(":operation")).thenReturn("delete"); + Mockito.when(request.getRequestProgressTracker()).thenReturn(requestProgressTracker); + Mockito.when(request.getResource()).thenReturn(resource); + Mockito.when(resource.getPath()).thenReturn("/content/pat/123"); + Mockito.when(request.getRequestPathInfo()).thenReturn(requestPathInfo); + Mockito.when(requestPathInfo.getSuffix()).thenReturn(null); + Mockito.doNothing().when(requestProgressTracker).log(Mockito.anyString(), Mockito.any()); + + Method doRun = deleteOperation.getClass().getDeclaredMethod("doRun", SlingHttpServletRequest.class, PostResponse.class, List.class); + doRun.setAccessible(true); + + try { + doRun.invoke(deleteOperation, request, null, null); + } catch (Exception e) { + assertEquals("org.apache.sling.api.resource.ResourceNotFoundException", e.getCause().getClass().getName()); + } + } +} From eb4a82877f6bd6e4b57a05d9cd102d2d80964c5d Mon Sep 17 00:00:00 2001 From: Patrique Legault Date: Tue, 28 Nov 2023 20:21:37 -0500 Subject: [PATCH 2/3] Scanning :applyTo for non existing resources --- .../post/impl/operations/DeleteOperation.java | 17 ++++--- .../impl/operations/DeleteOperationTest.java | 45 +++++++++++++++++-- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/apache/sling/servlets/post/impl/operations/DeleteOperation.java b/src/main/java/org/apache/sling/servlets/post/impl/operations/DeleteOperation.java index 3d28976..b63d835 100644 --- a/src/main/java/org/apache/sling/servlets/post/impl/operations/DeleteOperation.java +++ b/src/main/java/org/apache/sling/servlets/post/impl/operations/DeleteOperation.java @@ -18,6 +18,8 @@ import java.util.Iterator; import java.util.List; +import java.util.Optional; +import java.util.stream.StreamSupport; import javax.servlet.http.HttpServletResponse; @@ -77,12 +79,17 @@ protected void doRun(final SlingHttpServletRequest request, deleteResource(resource, changes, versioningConfiguration, deleteChunks); } else { - while (res.hasNext()) { - final Resource resource = res.next(); - deleteResource(resource, changes, versioningConfiguration, - deleteChunks); + Iterable iteratorRes = () -> res; + Optional existing = StreamSupport.stream(iteratorRes.spliterator(), true).filter(r -> r instanceof NonExistingResource).findAny(); + if (existing.isEmpty()) { + while (res.hasNext()) { + final Resource resource = res.next(); + deleteResource(resource, changes, versioningConfiguration, + deleteChunks); + } + } else { + throw new ResourceNotFoundException(String.format("Cannot delete NonExistingResource %s", existing.get().getPath())); } - } } diff --git a/src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java b/src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java index beee622..821a594 100644 --- a/src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java +++ b/src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java @@ -23,21 +23,22 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +import java.util.Arrays; import java.util.List; import org.apache.sling.api.SlingHttpServletRequest; import org.apache.sling.api.request.RequestPathInfo; import org.apache.sling.api.request.RequestProgressTracker; import org.apache.sling.api.resource.NonExistingResource; +import org.apache.sling.api.resource.Resource; import org.apache.sling.api.resource.ResourceResolver; import org.apache.sling.servlets.post.PostResponse; -import org.junit.Assert; import org.junit.Test; import org.mockito.Mockito; public class DeleteOperationTest { - - @Test + + @Test public void testDeletingNonExistingResource() throws Exception { DeleteOperation deleteOperation = new DeleteOperation(); @@ -57,6 +58,44 @@ public void testDeletingNonExistingResource() throws Exception { Mockito.when(requestPathInfo.getSuffix()).thenReturn(null); Mockito.doNothing().when(requestProgressTracker).log(Mockito.anyString(), Mockito.any()); + Method doRun = deleteOperation.getClass().getDeclaredMethod("doRun", SlingHttpServletRequest.class, + PostResponse.class, List.class); + doRun.setAccessible(true); + + try { + doRun.invoke(deleteOperation, request, null, null); + } catch (Exception e) { + assertEquals("org.apache.sling.api.resource.ResourceNotFoundException", e.getCause().getClass().getName()); + } + } + + @Test + public void testDeletingNonExistingResourcewithApplyTo() throws Exception { + + DeleteOperation deleteOperation = new DeleteOperation(); + + SlingHttpServletRequest request = Mockito.mock(SlingHttpServletRequest.class); + ResourceResolver resourceResolver = Mockito.mock(ResourceResolver.class); + RequestPathInfo requestPathInfo = Mockito.mock(RequestPathInfo.class); + RequestProgressTracker requestProgressTracker = Mockito.mock(RequestProgressTracker.class); + Resource resource = Mockito.mock(Resource.class, Mockito.RETURNS_DEEP_STUBS); + NonExistingResource nonExistingResource = Mockito.mock(NonExistingResource.class, Mockito.RETURNS_DEEP_STUBS); + NonExistingResource nonExistingResource_1 = Mockito.mock(NonExistingResource.class, Mockito.RETURNS_DEEP_STUBS); + NonExistingResource nonExistingResource_2 = Mockito.mock(NonExistingResource.class, Mockito.RETURNS_DEEP_STUBS); + List nonExistinResources = Arrays.asList(nonExistingResource_1, nonExistingResource_2); + + Mockito.when(resourceResolver.getResource(resource, "/content/pat/123")).thenReturn(nonExistingResource); + Mockito.when(nonExistingResource.listChildren()).thenReturn(nonExistinResources.iterator()); + Mockito.when(request.getResourceResolver()).thenReturn(resourceResolver); + Mockito.when(request.getParameter(":operation")).thenReturn("delete"); + Mockito.when(request.getParameterValues(":applyTo")).thenReturn(new String[]{"/content/pat/123/*", "/content/serge/123/456", "/content/justin/123/456"}); + Mockito.when(request.getRequestProgressTracker()).thenReturn(requestProgressTracker); + Mockito.when(request.getResource()).thenReturn(resource); + Mockito.when(resource.getPath()).thenReturn("/content/pat/123"); + Mockito.when(request.getRequestPathInfo()).thenReturn(requestPathInfo); + Mockito.when(requestPathInfo.getSuffix()).thenReturn(null); + Mockito.doNothing().when(requestProgressTracker).log(Mockito.anyString(), Mockito.any()); + Method doRun = deleteOperation.getClass().getDeclaredMethod("doRun", SlingHttpServletRequest.class, PostResponse.class, List.class); doRun.setAccessible(true); From 3e371eed21199f599a45426227d8088416aa2259 Mon Sep 17 00:00:00 2001 From: Eric Norman Date: Thu, 30 Nov 2023 11:19:51 -0800 Subject: [PATCH 3/3] SLING-8974 fix delete resources supplied by applyTo param Also provide new tests to improve code coverage --- .../post/impl/operations/DeleteOperation.java | 21 ++--- .../impl/operations/DeleteOperationTest.java | 86 ++++++++++++++++++- 2 files changed, 89 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/apache/sling/servlets/post/impl/operations/DeleteOperation.java b/src/main/java/org/apache/sling/servlets/post/impl/operations/DeleteOperation.java index b63d835..081fe70 100644 --- a/src/main/java/org/apache/sling/servlets/post/impl/operations/DeleteOperation.java +++ b/src/main/java/org/apache/sling/servlets/post/impl/operations/DeleteOperation.java @@ -18,8 +18,6 @@ import java.util.Iterator; import java.util.List; -import java.util.Optional; -import java.util.stream.StreamSupport; import javax.servlet.http.HttpServletResponse; @@ -70,26 +68,19 @@ protected void doRun(final SlingHttpServletRequest request, final VersioningConfiguration versioningConfiguration = getVersioningConfiguration(request); final boolean deleteChunks = isDeleteChunkRequest(request); - final Iterator res = getApplyToResources(request); + Iterator res = getApplyToResources(request); if (res == null) { + // no :applyTo params, so just iterate over the current resource instead final Resource resource = request.getResource(); + res = List.of(resource).iterator(); + } + while (res.hasNext()) { + final Resource resource = res.next(); if (resource instanceof NonExistingResource) { throw new ResourceNotFoundException(String.format("Cannot delete NonExistingResource %s", resource.getPath())); } deleteResource(resource, changes, versioningConfiguration, deleteChunks); - } else { - Iterable iteratorRes = () -> res; - Optional existing = StreamSupport.stream(iteratorRes.spliterator(), true).filter(r -> r instanceof NonExistingResource).findAny(); - if (existing.isEmpty()) { - while (res.hasNext()) { - final Resource resource = res.next(); - deleteResource(resource, changes, versioningConfiguration, - deleteChunks); - } - } else { - throw new ResourceNotFoundException(String.format("Cannot delete NonExistingResource %s", existing.get().getPath())); - } } } diff --git a/src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java b/src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java index 821a594..7734319 100644 --- a/src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java +++ b/src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java @@ -21,8 +21,8 @@ import static org.junit.Assert.assertEquals; -import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -32,6 +32,8 @@ import org.apache.sling.api.resource.NonExistingResource; import org.apache.sling.api.resource.Resource; import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.servlets.post.Modification; +import org.apache.sling.servlets.post.ModificationType; import org.apache.sling.servlets.post.PostResponse; import org.junit.Test; import org.mockito.Mockito; @@ -81,7 +83,9 @@ public void testDeletingNonExistingResourcewithApplyTo() throws Exception { Resource resource = Mockito.mock(Resource.class, Mockito.RETURNS_DEEP_STUBS); NonExistingResource nonExistingResource = Mockito.mock(NonExistingResource.class, Mockito.RETURNS_DEEP_STUBS); NonExistingResource nonExistingResource_1 = Mockito.mock(NonExistingResource.class, Mockito.RETURNS_DEEP_STUBS); + Mockito.when(nonExistingResource_1.getPath()).thenReturn("/content/pat/123/existing1"); NonExistingResource nonExistingResource_2 = Mockito.mock(NonExistingResource.class, Mockito.RETURNS_DEEP_STUBS); + Mockito.when(nonExistingResource_2.getPath()).thenReturn("/content/pat/123/existing2"); List nonExistinResources = Arrays.asList(nonExistingResource_1, nonExistingResource_2); Mockito.when(resourceResolver.getResource(resource, "/content/pat/123")).thenReturn(nonExistingResource); @@ -98,11 +102,87 @@ public void testDeletingNonExistingResourcewithApplyTo() throws Exception { Method doRun = deleteOperation.getClass().getDeclaredMethod("doRun", SlingHttpServletRequest.class, PostResponse.class, List.class); doRun.setAccessible(true); - + try { doRun.invoke(deleteOperation, request, null, null); } catch (Exception e) { assertEquals("org.apache.sling.api.resource.ResourceNotFoundException", e.getCause().getClass().getName()); - } + } + } + + @Test + public void testDeletingExistingResource() throws Exception { + + DeleteOperation deleteOperation = new DeleteOperation(); + + SlingHttpServletRequest request = Mockito.mock(SlingHttpServletRequest.class); + ResourceResolver resourceResolver = Mockito.mock(ResourceResolver.class); + RequestPathInfo requestPathInfo = Mockito.mock(RequestPathInfo.class); + RequestProgressTracker requestProgressTracker = Mockito.mock(RequestProgressTracker.class); + Resource resource = Mockito.mock(Resource.class, Mockito.RETURNS_DEEP_STUBS); + Mockito.when(request.getResourceResolver()).thenReturn(resourceResolver); + + Mockito.when(request.getParameter(":operation")).thenReturn("delete"); + Mockito.when(request.getRequestProgressTracker()).thenReturn(requestProgressTracker); + Mockito.when(request.getResource()).thenReturn(resource); + Mockito.when(resource.getPath()).thenReturn("/content/pat/123"); + Mockito.when(request.getRequestPathInfo()).thenReturn(requestPathInfo); + Mockito.when(requestPathInfo.getSuffix()).thenReturn(null); + Mockito.doNothing().when(requestProgressTracker).log(Mockito.anyString(), Mockito.any()); + + Method doRun = deleteOperation.getClass().getDeclaredMethod("doRun", SlingHttpServletRequest.class, + PostResponse.class, List.class); + doRun.setAccessible(true); + + List changes = new ArrayList<>(); + doRun.invoke(deleteOperation, request, null, changes); + assertEquals(1, changes.size()); + Modification modification = changes.get(0); + assertEquals(ModificationType.DELETE, modification.getType()); + assertEquals("/content/pat/123", modification.getSource()); + } + + @Test + public void testDeletingExistingResourcewithApplyTo() throws Exception { + + DeleteOperation deleteOperation = new DeleteOperation(); + + SlingHttpServletRequest request = Mockito.mock(SlingHttpServletRequest.class); + ResourceResolver resourceResolver = Mockito.mock(ResourceResolver.class); + RequestPathInfo requestPathInfo = Mockito.mock(RequestPathInfo.class); + RequestProgressTracker requestProgressTracker = Mockito.mock(RequestProgressTracker.class); + Resource resource = Mockito.mock(Resource.class, Mockito.RETURNS_DEEP_STUBS); + Resource existingResource = Mockito.mock(Resource.class, Mockito.RETURNS_DEEP_STUBS); + Resource existingResource_1 = Mockito.mock(Resource.class, Mockito.RETURNS_DEEP_STUBS); + Mockito.when(existingResource_1.getPath()).thenReturn("/content/pat/123/existing1"); + Resource existingResource_2 = Mockito.mock(Resource.class, Mockito.RETURNS_DEEP_STUBS); + Mockito.when(existingResource_2.getPath()).thenReturn("/content/pat/123/existing2"); + List existingResources = Arrays.asList(existingResource_1, existingResource_2); + + Mockito.when(resourceResolver.getResource(resource, "/content/pat/123")).thenReturn(existingResource); + Mockito.when(existingResource.listChildren()).thenReturn(existingResources.iterator()); + Mockito.when(request.getResourceResolver()).thenReturn(resourceResolver); + Mockito.when(request.getParameter(":operation")).thenReturn("delete"); + Mockito.when(request.getParameterValues(":applyTo")).thenReturn(new String[]{"/content/pat/123/*", "/content/serge/123/456", "/content/justin/123/456"}); + Mockito.when(request.getRequestProgressTracker()).thenReturn(requestProgressTracker); + Mockito.when(request.getResource()).thenReturn(resource); + Mockito.when(resource.getPath()).thenReturn("/content/pat/123"); + Mockito.when(request.getRequestPathInfo()).thenReturn(requestPathInfo); + Mockito.when(requestPathInfo.getSuffix()).thenReturn(null); + Mockito.doNothing().when(requestProgressTracker).log(Mockito.anyString(), Mockito.any()); + + Method doRun = deleteOperation.getClass().getDeclaredMethod("doRun", SlingHttpServletRequest.class, PostResponse.class, List.class); + doRun.setAccessible(true); + + List changes = new ArrayList<>(); + doRun.invoke(deleteOperation, request, null, changes); + assertEquals(2, changes.size()); + Modification modification1 = changes.get(0); + assertEquals(ModificationType.DELETE, modification1.getType()); + assertEquals("/content/pat/123/existing1", modification1.getSource()); + Modification modification2 = changes.get(1); + assertEquals(ModificationType.DELETE, modification2.getType()); + assertEquals("/content/pat/123/existing2", modification2.getSource()); } + }