#172 Fixed issue with sorting roles

Author: rjrudin

src/main/java/com/marklogic/appdeployer/command/AbstractCommand.java

@@ -142,7 +142,9 @@ protected void storeTokenForResourceId(SaveReceipt receipt, CommandContext conte
 
     protected File[] listFilesInDirectory(File dir) {
         File[] files = dir.listFiles(resourceFilenameFilter);
-        Arrays.sort(files);
+        if (files != null && files.length > 1) {
+	        Arrays.sort(files);
+        }
         return files;
     }
 

src/main/java/com/marklogic/appdeployer/command/security/DeployRolesCommand.java

@@ -31,7 +31,7 @@ public DeployRolesCommand() {
 	protected File[] listFilesInDirectory(File dir, CommandContext context) {
 		File[] files = super.listFilesInDirectory(dir);
 
-		if (context.getAppConfig().isSortRolesByDependencies()) {
+		if (context.getAppConfig().isSortRolesByDependencies() && files != null && files.length > 0) {
 			if (logger.isInfoEnabled()) {
 				logger.info("Sorting role files by role dependencies");
 			}
@@ -47,6 +47,9 @@ protected File[] listFilesInDirectory(File dir, CommandContext context) {
 
 	protected List<RoleFile> sortFilesBasedOnRoleDependencies(File[] files, CommandContext context) {
 		List<RoleFile> roleFiles = new ArrayList<>();
+		if (files == null || files.length < 1) {
+			return roleFiles;
+		}
 		PayloadParser parser = new PayloadParser();
 		for (File f : files) {
 			RoleFile rf = new RoleFile(f);
@@ -69,7 +72,12 @@ protected List<RoleFile> sortFilesBasedOnRoleDependencies(File[] files, CommandC
 			roleFiles.add(rf);
 		}
 
-		Collections.sort(roleFiles);
+		return sortRoleFiles(roleFiles);
+	}
+
+	protected List<RoleFile> sortRoleFiles(List<RoleFile> roleFiles) {
+		RoleFileComparator comparator = new RoleFileComparator(roleFiles);
+		Collections.sort(roleFiles, comparator);
 		return roleFiles;
 	}
 
@@ -84,7 +92,7 @@ protected ResourceManager getResourceManager(CommandContext context) {
 
 }
 
-class RoleFile implements Comparable<RoleFile> {
+class RoleFile {
 
 	File file;
 	Role role;
@@ -95,8 +103,77 @@ public RoleFile(File file) {
 		this.role.setRole(new ArrayList<String>());
 	}
 
+}
+
+/**
+ * This comparator is designed to handle a scenario where two roles are next to each other, but they don't have any
+ * dependencies in common, nor does one depend on the other. In this scenario, we need to know which role has a
+ * dependency on a role furthest to the end of the list of roles. In order to know that, we first build up a data
+ * structure that tracks the highest position of a dependency in the list of roles for each role.
+ */
+class RoleFileComparator implements Comparator<RoleFile> {
+
+	private Map<String, Integer> highestDependencyPositionMap;
+
+	public RoleFileComparator(List<RoleFile> roleFiles) {
+		Map<String, Integer> rolePositions = new HashMap<>();
+		for (int i = 0; i < roleFiles.size(); i++) {
+			rolePositions.put(roleFiles.get(i).role.getRoleName(), i);
+		}
+
+		highestDependencyPositionMap = new HashMap<>();
+		for (RoleFile rf : roleFiles) {
+			String roleName = rf.role.getRoleName();
+			int highest = -1;
+			for (String role : rf.role.getRole()) {
+				if (rolePositions.containsKey(role)) {
+					int pos = rolePositions.get(role);
+					if (pos > highest) {
+						highest = pos;
+					}
+				}
+			}
+			highestDependencyPositionMap.put(roleName, highest);
+		}
+	}
+
 	@Override
-	public int compareTo(RoleFile o) {
-		return this.role.compareTo(o.role);
+	public int compare(RoleFile o1, RoleFile o2) {
+		if (o1 == null && o2 != null) {
+			return 1;
+		}
+		if (o2 == null) {
+			return -1;
+		}
+		if (o1.role.getRole() == null || o1.role.getRole().isEmpty()) {
+			return -1;
+		}
+		if (o2.role.getRole() == null || o2.role.getRole().isEmpty()) {
+			return 1;
+		}
+		if (o2.role.getRole().contains(o1.role.getRoleName())) {
+			return -1;
+		}
+		if (o1.role.getRole().contains(o2.role.getRoleName())) {
+			return 1;
+		}
+
+		/**
+		 * If the roles aren't dependent on each other, then we want to base this on which role has a dependency further
+		 * to the right.
+		 */
+		int o1Pos = highestDependencyPositionMap.get(o1.role.getRoleName());
+		int o2Pos = highestDependencyPositionMap.get(o2.role.getRoleName());
+		if (o1Pos > o2Pos) {
+			return 1;
+		}
+		if (o2Pos > o1Pos) {
+			return -1;
+		}
+
+		/**
+		 * This would be for two roles that depend on the same other role.
+		 */
+		return 0;
 	}
 }

src/main/java/com/marklogic/appdeployer/export/AbstractNamedResourceExporter.java

@@ -32,14 +32,16 @@ protected AbstractNamedResourceExporter(ManageClient manageClient, String... res
 
 	@Override
 	public ExportedResources exportResources(File baseDir) {
-		ResourceManager mgr = newResourceManager(getManageClient());
-		File resourceDir = getResourceDirectory(baseDir);
-		resourceDir.mkdirs();
 		List<File> files = new ArrayList<>();
-		for (String resourceName : resourceNames) {
-			File f = exportToFile(mgr, resourceName, resourceDir);
-			if (f != null) {
-				files.add(f);
+		if (resourceNames != null && resourceNames.length > 0) {
+			ResourceManager mgr = newResourceManager(getManageClient());
+			File resourceDir = getResourceDirectory(baseDir);
+			resourceDir.mkdirs();
+			for (String resourceName : resourceNames) {
+				File f = exportToFile(mgr, resourceName, resourceDir);
+				if (f != null) {
+					files.add(f);
+				}
 			}
 		}
 		return new ExportedResources(files, getExportMessages());

src/main/java/com/marklogic/appdeployer/export/security/RoleExporter.java

@@ -26,6 +26,6 @@ protected File getResourceDirectory(File baseDir) {
 
 	@Override
 	protected String[] getExportMessages() {
-		return new String[] {"The exported role files may need to be renamed to guarantee they are deployed in an order that respects the dependencies between them."};
+		return new String[] {"The exported role files may have circular dependencies between them that must be resolved before they can be deployed."};
 	}
 }

src/main/java/com/marklogic/mgmt/api/security/Role.java

@@ -1,14 +1,14 @@
 package com.marklogic.mgmt.api.security;
 
-import java.util.ArrayList;
-import java.util.List;
-
 import com.marklogic.mgmt.ResourceManager;
 import com.marklogic.mgmt.api.API;
 import com.marklogic.mgmt.api.Resource;
 import com.marklogic.mgmt.security.RoleManager;
 
-public class Role extends Resource implements Comparable<Role> {
+import java.util.ArrayList;
+import java.util.List;
+
+public class Role extends Resource {
 
     private String roleName;
     private String description;
@@ -27,26 +27,6 @@ public Role(API api, String roleName) {
         this.roleName = roleName;
     }
 
-	@Override
-	public int compareTo(Role other) {
-    	if (other == null) {
-    		return 0;
-	    }
-    	if (this.role == null || this.role.isEmpty()) {
-    		return -1;
-	    }
-	    if (other.role == null || other.role.isEmpty()) {
-    		return 1;
-	    }
-		if (other.role.contains(this.roleName)) {
-    		return -1;
-		}
-		if (this.role.contains(other.roleName)) {
-    		return 1;
-		}
-		return -1;
-	}
-
 	public void addExternalName(String name) {
         if (externalName == null) {
             externalName = new ArrayList<>();

src/test/java/com/marklogic/appdeployer/command/security/DeployRolesWithDependenciesTest.java

@@ -10,13 +10,13 @@
 
 public class DeployRolesWithDependenciesTest extends AbstractAppDeployerTest {
 
-	@Before
-	public void setup() {
-		appConfig.getConfigDir().setBaseDir(new File("src/test/resources/sample-app/roles-with-dependencies"));
-	}
-
+	/**
+	 * This scenario has a mix of XML/JSON files (to verify that we parse each correctly) and a big jumble of roles.
+	 */
 	@Test
 	public void testSorting() {
+		appConfig.getConfigDir().setBaseDir(new File("src/test/resources/sample-app/roles-with-dependencies"));
+
 		DeployRolesCommand command = new DeployRolesCommand();
 		File[] files = command.listFilesInDirectory(appConfig.getConfigDir().getRolesDir(),
 			new CommandContext(appConfig, manageClient, null));
@@ -29,8 +29,27 @@ public void testSorting() {
 		assertEquals("role0.json", files[5].getName());
 	}
 
+	/**
+	 * This scenario tests that when two roles are next to each other with different dependencies - role1 and role2 -
+	 * that we figure out that role1 has a dependency on a role closest to the end of the current list of files.
+	 */
+	@Test
+	public void anotherSortingTest() {
+		appConfig.getConfigDir().setBaseDir(new File("src/test/resources/sample-app/more-roles-with-dependencies"));
+
+		DeployRolesCommand command = new DeployRolesCommand();
+		File[] files = command.listFilesInDirectory(appConfig.getConfigDir().getRolesDir(),
+			new CommandContext(appConfig, manageClient, null));
+
+		assertEquals("role0.json", files[0].getName());
+		assertEquals("role2.json", files[1].getName());
+		assertEquals("role3.json", files[2].getName());
+		assertEquals("role1.json", files[3].getName());
+	}
+
 	@Test
 	public void test() {
+		appConfig.getConfigDir().setBaseDir(new File("src/test/resources/sample-app/roles-with-dependencies"));
 		initializeAppDeployer(new DeployRolesCommand());
 
 		try {

src/test/resources/sample-app/more-roles-with-dependencies/security/roles/role0.json

@@ -0,0 +1,4 @@
+{
+	"role-name" : "more-roles-role0",
+	"role" : [ "app-user" ]
+}

src/test/resources/sample-app/more-roles-with-dependencies/security/roles/role1.json

@@ -0,0 +1,4 @@
+{
+  "role-name" : "more-roles-role1",
+  "role" : [ "more-roles-role3" ]
+}

src/test/resources/sample-app/more-roles-with-dependencies/security/roles/role2.json

@@ -0,0 +1,4 @@
+{
+  "role-name" : "more-roles-role2",
+  "role" : [ "more-roles-role0"]
+}

src/test/resources/sample-app/more-roles-with-dependencies/security/roles/role3.json

@@ -0,0 +1,4 @@
+{
+  "role-name" : "more-roles-role3",
+  "role" : [ "more-roles-role0" ]
+}