Adding fix for SLING-8974 #26

pat-lego · 2023-11-24T04:00:45Z

Added unit test to validate NonExistingResource and Post Servlet Returns a 400

rombert · 2023-11-27T10:32:56Z

@kwin @stefanseifert - any thoughts on this? Look OK to me, but I don't know this area in depth.

pat-lego · 2023-11-27T14:27:07Z

Please note that this PR returns a 400 in the case of a NonExistingResource as agreed upon here: https://issues.apache.org/jira/browse/SLING-8974?focusedCommentId=17764586&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17764586

joerghoh

LGTM, although that improvement in the test would be nice.

joerghoh · 2023-11-28T16:29:13Z

src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java

+
+public class DeleteOperationTest {
+
+     @Test


instead of doing the exception handling in this method in the try-catch block you can mark this test to expect to fail with that exception.

Suggested change

@Test

@Test(expected=ResourceNotFoundException.class)

joerghoh · 2023-11-28T16:29:23Z

src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java

+        Method doRun = deleteOperation.getClass().getDeclaredMethod("doRun", SlingHttpServletRequest.class, PostResponse.class, List.class);
+        doRun.setAccessible(true);
+
+        try {


enapps-enorman

Please see my review comments related to handling for delete of non-existing resources specified via ":applyTo" request parameters.

Also, there still seems to be some backward incompatibly with the previous behavior. Perhaps that can be sufficiently dealt with via new documentation?

enapps-enorman · 2023-11-28T22:23:59Z

src/main/java/org/apache/sling/servlets/post/impl/operations/DeleteOperation.java

@@ -69,6 +71,9 @@ protected void doRun(final SlingHttpServletRequest request,
        final Iterator<Resource> res = getApplyToResources(request);
        if (res == null) {
            final Resource resource = request.getResource();
+            if (resource instanceof NonExistingResource) {


I think this could still use some clarification about what should happen in the alternate code path where the resources to delete are provided by one or more ":applyTo" request parameters. If I am not mistaken, this looks like you are only checking if the resource exists in the simple case where the resource to delete is itself and no ":applyTo" arguments are provided?

enapps-enorman · 2023-11-28T22:25:25Z

src/test/java/org/apache/sling/servlets/post/impl/operations/DeleteOperationTest.java

+public class DeleteOperationTest {
+
+     @Test
+    public void testDeletingNonExistingResource() throws Exception {


Would also be good to have a test for the scenario where the (non-existing) resource(s) to delete are provided via ":applyTo" request parameters.

enapps-enorman

@pat-lego Thanks for the changes to address my concerns. It looks better to me. I see that the sonarcloud analysis has detected a new (Replace this lambda with method reference) "Code Smell" in the code that was just changed that looks like a trivial fix that could be cleaned up?

Also, it seems like the only way to solve the sonarcloud code coverage quality gate check would be to add some additional tests that cover the delete operation when the resource does exist so it touches those other code paths that were also modified. Is that something you could do?

Perhaps utilizing the sling-mock stuff for the tests could make them easier and more succinct to write with less Mockito code?

https://sling.apache.org/documentation/development/sling-mock.html

pat-lego · 2023-11-30T02:23:40Z

@enapps-enorman I suggest that we log a new SLING ticket for that. This would require some additional work that is outside the scope of this ticket. I am in agreement that this should be done but it should be done in an appropriately described ticket.

Also, we should limit the code changes to reflect the scope of the work so that we do not include file changes that do not reflect the work being done.

I can log the appropriate ticket and when time permits start working on it.

cc: @joerghoh

enapps-enorman · 2023-11-30T04:25:35Z

I suggest that we log a new SLING ticket for that. This would require some additional work that is outside the scope of this ticket.

@pat-lego I'm not opposed to further refactoring in another ticket. I was just hoping to not have merge while there are known failures in the quality gate rules. If we can get the quality gate checking to be green before merging it would be better. That would mean getting the code coverage on the changed lines from 53.3% to greater than 80%. The current report shows that the testing does not exercise 3 lines on new code and 4 conditions on new code.

Also provide new tests to improve code coverage

enapps-enorman · 2023-11-30T19:23:10Z

@pat-lego After a closer look at this, it seems that any existing resources specified via ":applyTo" parameters would not get deleted after these changes since the applyTo resources iterator is consumed during the check to see if all are all existing. The second iteration would always be empty so no deleteResource calls would be performed.

I discovered this while experimenting with new tests to improve the code coverage. I have fixed the problem and refactored a bit to remove duplication. I have committed the changes to this PR at 3e371ee for the others to review.