Commit 5e6f2670 authored by Nicola Fulvio Calabria's avatar Nicola Fulvio Calabria
Browse files

Bugfixes + unit tests for MoveService

parent 9fe11b4b

src/main/java/it/inaf/oats/vospace/MoveService.java

+6 −11
Original line number Diff line number Diff line
@@ -59,7 +59,7 @@ public class MoveService { 
        

        // Check if destination is subpath of source

        // Linux-like: "cannot move to a subdirectory of itself" 

        if(destinationPath.startsWith(sourcePath)) {

        if(destinationPath.startsWith(sourcePath+"/")) {

            throw new IllegalArgumentException("Cannot move node to a subdirectory of its own path");

        }
@@ -85,8 +85,8 @@ public class MoveService { 
                ShortNodeDescriptor snd = destShortNodeDescriptor.get();

                

                if(snd.isBusy()) throw new NodeBusyException(destinationPath);

                // TODO: can split permission denied condition further.

                if(!snd.isWritable()) throw new PermissionDeniedException(destinationPath);

                if(snd.isPermissionDenied()) throw new PermissionDeniedException(destinationPath);                                                

                if(!snd.isWritable()) throw new InternalFaultException("Destination is not writable: "+ destinationPath);

                if(!snd.isContainer()) throw new InternalFaultException("Existing destination is not a container: " + destinationPath);

                

                destinationNodeLtreePath = snd.getDestinationNodeLtreePath();
@@ -119,11 +119,6 @@ public class MoveService { 
        }

    }    

    

    private boolean checkNodeExistence(String path) {

        Optional<Long> optNodeId = nodeDao.getNodeId(path);

        return optNodeId.isPresent();

    }

    

    /*

    // Returns node id of created destination

    private Long createDestination(String path, User user) {

src/main/java/it/inaf/oats/vospace/persistence/NodeDAO.java

+34 −14
Original line number Diff line number Diff line
@@ -5,7 +5,6 @@ 
 */

package it.inaf.oats.vospace.persistence;



import it.inaf.ia2.aa.data.User;

import it.inaf.oats.vospace.DeleteNodeController;

import it.inaf.oats.vospace.datamodel.NodeProperties;

import it.inaf.oats.vospace.datamodel.NodeUtils;
@@ -279,14 +278,13 @@ public class NodeDAO { 
                throw new InternalFaultException("More than 1 node at path: " + nodeVosPath);

        }

    }*/



    public Optional<ShortNodeDescriptor> getShortNodeDescriptor(String nodeVosPath,

            String userId, List<String> userGroups) {

                        

        String sql = "SELECT path,\n"

                + "NOT (n.async_trans OR n.sticky OR loc.location_type = 'async' \n"

                + "OR ( (SELECT COUNT(*) FROM (SELECT UNNEST(?) INTERSECT SELECT UNNEST(n.group_write)) AS allowed_groups ) = 0 AND\n"

                + "n.creator_id <> ?)) AS is_writable,\n"

                + "NOT (n.async_trans OR n.sticky OR loc.location_type = 'async') AS is_writable,\n"

                + "((SELECT COUNT(*) FROM (SELECT UNNEST(?) INTERSECT SELECT UNNEST(n.group_write)) AS allowed_groups ) = 0 AND\n"

                + "n.creator_id <> ?) AS is_permission_denied,\n"

                + "n.type = 'container' AS is_container,\n"

                + "n.busy_state\n"

                + "FROM node n \n"
@@ -296,7 +294,15 @@ public class NodeDAO { 


        Optional<ShortNodeDescriptor> sndOpt = jdbcTemplate.query(conn -> {

            PreparedStatement ps = conn.prepareStatement(sql);

            ps.setArray(1, ps.getConnection().createArrayOf("varchar", userGroups.toArray(String[]::new)));

            

            String[] groups;

            if(userGroups == null){

                groups = new String[0];                                

            } else {

                groups = userGroups.toArray(String[]::new);

            }

            ps.setArray(1, ps.getConnection().createArrayOf("varchar", groups));

            

            ps.setString(2, userId);

            ps.setString(3, nodeVosPath);

            return ps;
@@ -309,8 +315,9 @@ public class NodeDAO { 
            Boolean isContainer = rs.getBoolean(("is_container"));

            Boolean isWritable = rs.getBoolean("is_writable");

            Boolean isBusy = rs.getBoolean("busy_state");

            Boolean isPermissionDenied = rs.getBoolean("is_permission_denied");



            ShortNodeDescriptor result = new ShortNodeDescriptor(nodePath, isContainer, isWritable, isBusy);

            ShortNodeDescriptor result = new ShortNodeDescriptor(nodePath, isContainer, isWritable, isBusy, isPermissionDenied);



            return Optional.of(result);

        });
@@ -396,7 +403,6 @@ public class NodeDAO { 
            return ps;

        });

    }*/



    public void renameNode(Long nodeId, String name) {

        String sql = "UPDATE node SET name = ?\n"

                + "WHERE path ~ ('*.' || ?)::lquery";
@@ -450,7 +456,15 @@ public class NodeDAO { 


        return jdbcTemplate.query(sql, ps -> {

            ps.setLong(1, parentNodeId);

            ps.setArray(2, ps.getConnection().createArrayOf("varchar", userGroups.toArray(String[]::new)));

            

            String[] groups;

            if(userGroups == null){

                groups = new String[0];                                

            } else {

                groups = userGroups.toArray(String[]::new);

            }            

            ps.setArray(2, ps.getConnection().createArrayOf("varchar", groups));

            

            ps.setString(3, userId);

        }, row -> {

            if (!row.next()) {
@@ -716,12 +730,14 @@ public class NodeDAO { 
        private final boolean container;

        private final boolean writable;

        private final boolean busy;

        private final boolean permissionDenied;



        public ShortNodeDescriptor(String nodeLtreePath, boolean container, boolean writable, boolean busy) {

        public ShortNodeDescriptor(String nodeLtreePath, boolean container, boolean writable, boolean busy, boolean permissionDenied) {

            this.nodeLtreePath = nodeLtreePath;

            this.container = container;

            this.writable = writable;

            this.busy = busy;

            this.permissionDenied = permissionDenied;

        }



        public String getDestinationNodeLtreePath() {
@@ -740,6 +756,10 @@ public class NodeDAO { 
            return busy;

        }

        

        public boolean isPermissionDenied() {

            return permissionDenied;

        }



    }



    private class NodePaths {

src/test/java/it/inaf/oats/vospace/MoveServiceTest.java

+56 −43
Original line number Diff line number Diff line
@@ -7,13 +7,12 @@ package it.inaf.oats.vospace; 


import it.inaf.ia2.aa.data.User;

import it.inaf.oats.vospace.datamodel.NodeProperties;

import it.inaf.oats.vospace.exception.DuplicateNodeException;

import it.inaf.oats.vospace.exception.InternalFaultException;

import it.inaf.oats.vospace.exception.NodeBusyException;

import it.inaf.oats.vospace.exception.NodeNotFoundException;

import it.inaf.oats.vospace.exception.PermissionDeniedException;

import it.inaf.oats.vospace.persistence.DataSourceConfigSingleton;

import it.inaf.oats.vospace.persistence.NodeDAO;

import java.util.List;

import java.util.Optional;

import javax.servlet.http.HttpServletRequest;

import net.ivoa.xml.vospace.v2.ContainerNode;
@@ -93,6 +92,19 @@ public class MoveServiceTest { 
    

    @Test

    @Order(2)

    public void testMoveToSubnodeOfItself() {

        User user = mock(User.class);        

        when(user.getName()).thenReturn("user3");

        when(servletRequest.getUserPrincipal()).thenReturn(user);

        

        assertThrows(IllegalArgumentException.class, () -> {

            moveService.processMoveJob(getTransfer("/test3/m1", "/test3/m1/m2"));

        }

        );

    } 

            

    @Test

    @Order(3)

    public void testNonExistingSourceNode() {        

        assertThrows(NodeNotFoundException.class, () -> {

            moveService.processMoveJob(getTransfer("/pippo", "/test2"));
@@ -101,17 +113,21 @@ public class MoveServiceTest { 
    }

  

    @Test

    @Order(3)

    public void testMoveDeniedOnAsync() {        

        assertThrows(InternalFaultException.class, () -> {

            moveService.processMoveJob(getTransfer("/test1", "/test4"));

    @Order(4)

    public void testMoveDeniedOnBusySource() {

        User user = mock(User.class);        

        when(user.getName()).thenReturn("user3");

        when(servletRequest.getUserPrincipal()).thenReturn(user);

        

        assertThrows(NodeBusyException.class, () -> {

            moveService.processMoveJob(getTransfer("/test3/mbusy", "/test3/m1"));

        }

        );        

    }

    

    @Test

    @Order(4)

    public void testPermissionDenied() {

    @Order(5)

    public void testPermissionDeniedOnSource() {

        User user = mock(User.class);        

        when(user.getName()).thenReturn("user1");

        when(servletRequest.getUserPrincipal()).thenReturn(user);
@@ -124,46 +140,42 @@ public class MoveServiceTest { 
    

    @Test

    @Order(5)

    public void testDestinationNodeAlreadyExisting() {

    public void testDontMoveIfStickySource() {

        User user = mock(User.class);        

        when(user.getName()).thenReturn("user3");

        when(servletRequest.getUserPrincipal()).thenReturn(user);

                

        assertThrows(DuplicateNodeException.class, () -> {

            moveService.processMoveJob(getTransfer("/test3/m1", "/test4"));

        assertThrows(PermissionDeniedException.class, () -> {

            moveService.processMoveJob(getTransfer("/test3/mstick", "/test4"));

        }

        );

    }    

    

    @Test

    @Order(6)

    public void testBusyNodeInSourceBranch() {

    public void testPermissionDeniedOnExistingDestination() {

        User user = mock(User.class);        

        when(user.getName()).thenReturn("user3");

        when(user.getName()).thenReturn("user1");        

        when(user.getGroups()).thenReturn(List.of("group1"));

        when(servletRequest.getUserPrincipal()).thenReturn(user);

        

        //nodeDao.setBranchBusy(nodeDao.getNodeId("/test3/m1/m2").orElseThrow(), true);

        

        assertThrows(NodeBusyException.class, () -> {

            moveService.processMoveJob(getTransfer("/test3/m1", "/test4"));

        assertThrows(PermissionDeniedException.class, () -> {

            moveService.processMoveJob(getTransfer("/test3/group1", "/test3/m1/m2"));

        }

        );       

                

        //nodeDao.setBranchBusy(nodeDao.getNodeId("/test3/m1/m2").orElseThrow(), false);        

    }    

    

    @Test

    @Order(7)

    public void testNoMoveOnSticky() {

    public void testDestinationExistsAndIsBusy() {

        User user = mock(User.class);        

        when(user.getName()).thenReturn("user3");

        when(servletRequest.getUserPrincipal()).thenReturn(user);        

        

        assertThrows(NodeBusyException.class, () -> {

            moveService.processMoveJob(getTransfer("/test3/mstick", "/test4"));

            moveService.processMoveJob(getTransfer("/test3/m1", "/test3/mbusy"));

        }

        );        

        

    }      

     

    @Test
@@ -195,7 +207,7 @@ public class MoveServiceTest { 
    

    @Test

    @Order(9)

    public void testMoveToExistingParent(){

    public void testMoveToExistingDestination(){

        User user = mock(User.class);        

        when(user.getName()).thenReturn("user3");

        when(servletRequest.getUserPrincipal()).thenReturn(user);
@@ -209,11 +221,11 @@ public class MoveServiceTest { 
        Optional<Long> destParentId = nodeDao.getNodeId("/test4");

        assertTrue(destParentId.isPresent());

        

        Optional<Long> destId = nodeDao.getNodeId("/test4/dest1");

        assertTrue(destId.isEmpty());

        Optional<Long> destId = nodeDao.getNodeId("/test4");

        assertTrue(destId.isPresent());

        

        // move

        moveService.processMoveJob(getTransfer("/test3/m1", "/test4/dest1"));

        moveService.processMoveJob(getTransfer("/test3/m1", "/test4"));



        // source has been moved

        Optional<Long> oldSourceId = nodeDao.getNodeId("/test3/m1");
@@ -221,16 +233,17 @@ public class MoveServiceTest { 
        Optional<Long> oldChildId = nodeDao.getNodeId("/test3/m1/m2");

        assertTrue(oldChildId.isEmpty());

        

        Optional<Long> newSourceId = nodeDao.getNodeId("/test4/dest1");

        Optional<Long> newSourceId = nodeDao.getNodeId("/test4/m1");

        assertTrue(newSourceId.isPresent());

        assertEquals(sourceId.get(), newSourceId.get());

        

        Optional<Long> newChildId = nodeDao.getNodeId("/test4/dest1/m2");

        Optional<Long> newChildId = nodeDao.getNodeId("/test4/m1/m2");

        assertTrue(newChildId.isPresent());

        assertEquals(childId.get(), newChildId.get());

        

    }

    

    /*

    @Test

    @Order(10)

    public void testMoveToUnexistingParent() {
@@ -266,7 +279,7 @@ public class MoveServiceTest { 
        assertTrue(newChildId.isPresent());

        assertEquals(childId.get(), newChildId.get());        

        

    }     

    }    */ 



    private Transfer getTransfer(String vosTarget, String vosDestination) {

        Transfer transfer = new Transfer();

src/test/java/it/inaf/oats/vospace/persistence/NodeDAOTest.java

+8 −4
Original line number Diff line number Diff line
@@ -5,7 +5,6 @@ 
 */

package it.inaf.oats.vospace.persistence;



import it.inaf.ia2.aa.data.User;

import it.inaf.oats.vospace.datamodel.NodeProperties;

import it.inaf.oats.vospace.exception.InternalFaultException;

import it.inaf.oats.vospace.persistence.NodeDAO.ShortNodeDescriptor;
@@ -23,7 +22,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; 
import net.ivoa.xml.vospace.v2.Node;

import net.ivoa.xml.vospace.v2.Property;

import static org.junit.jupiter.api.Assertions.assertFalse;

import static org.junit.jupiter.api.Assertions.assertThrows;

import static org.junit.jupiter.api.Assertions.assertTrue;

import org.junit.jupiter.api.BeforeEach;

import org.junit.jupiter.api.Test;
@@ -175,6 +173,7 @@ public class NodeDAOTest { 
        assertTrue(snd1Opt.isPresent());

        ShortNodeDescriptor snd1 = snd1Opt.get();

        assertTrue(snd1.isContainer());

        assertFalse(snd1.isPermissionDenied());

        assertFalse(snd1.isWritable());

        assertFalse(snd1.isBusy());
@@ -185,6 +184,7 @@ public class NodeDAOTest { 
        assertTrue(snd1Opt.isPresent());

        snd1 = snd1Opt.get();        

        assertTrue(snd1.isWritable());

        assertFalse(snd1.isPermissionDenied());

        assertTrue(snd1.isBusy());

        

        snd1Opt =
@@ -207,25 +207,29 @@ public class NodeDAOTest { 
        snd1 = snd1Opt.get();        

        assertFalse(snd1.isContainer());

        assertFalse(snd1.isWritable());

        assertFalse(snd1.isPermissionDenied());

        assertFalse(snd1.isBusy());

        

        snd1Opt = 

                dao.getShortNodeDescriptor("/test3/group1", "user1", userGroups);

        assertTrue(snd1Opt.isPresent());

        snd1 = snd1Opt.get();                

        assertFalse(snd1.isWritable());

        assertTrue(snd1.isWritable());

        assertTrue(snd1.isPermissionDenied());

        

        snd1Opt = 

                dao.getShortNodeDescriptor("/test3/group1", "user3", List.of("group99"));

        assertTrue(snd1Opt.isPresent());

        snd1 = snd1Opt.get();                

        assertTrue(snd1.isWritable());

        assertFalse(snd1.isPermissionDenied());

        

        snd1Opt = 

                dao.getShortNodeDescriptor("/test3/group1", "user1", List.of("group1", "group2"));

        assertTrue(snd1Opt.isPresent());

        snd1 = snd1Opt.get();                

        assertTrue(snd1.isWritable()); 

        assertFalse(snd1.isPermissionDenied());

    }

    

    @Test