From b0411cb4bb517c98a2ea3cd8521e70828c05b4d7 Mon Sep 17 00:00:00 2001 From: Jonathan Gamba Date: Wed, 13 Dec 2023 14:50:24 -0600 Subject: [PATCH] fix(CLI): Update path handling in CLI command (#26999) * #26985 Update path handling in CLI command The code has been updated to more robustly handle file paths in CLI commands. It now ensures that paths are within a workspace or throws an error if not, while also checking if files exist and can be read. This update improves error handling and makes the command usage clearer for the user. * #26985 Refactor workspace and path validation in FilesPush. * #26985 Adding more integration tests to cover the changes. --- .../command/files/AbstractFilesCommand.java | 36 ------ .../dotcms/cli/command/files/FilesPush.java | 103 +++++++++++++----- .../java/com/dotcms/cli/common/PushMixin.java | 2 +- .../cli/command/files/FilesPushCommandIT.java | 70 ++++++++++-- 4 files changed, 136 insertions(+), 75 deletions(-) diff --git a/tools/dotcms-cli/cli/src/main/java/com/dotcms/cli/command/files/AbstractFilesCommand.java b/tools/dotcms-cli/cli/src/main/java/com/dotcms/cli/command/files/AbstractFilesCommand.java index 3d960aed7061..b203ee94a673 100644 --- a/tools/dotcms-cli/cli/src/main/java/com/dotcms/cli/command/files/AbstractFilesCommand.java +++ b/tools/dotcms-cli/cli/src/main/java/com/dotcms/cli/command/files/AbstractFilesCommand.java @@ -4,10 +4,6 @@ import com.dotcms.cli.common.HelpOptionMixin; import com.dotcms.cli.common.OutputOptionMixin; import com.dotcms.common.WorkspaceManager; -import com.dotcms.model.config.Workspace; -import java.io.File; -import java.io.IOException; -import java.nio.file.Path; import java.util.HashSet; import java.util.Set; import javax.inject.Inject; @@ -52,36 +48,4 @@ protected Set parsePatternOption(String patterns) { return patternsSet; } - /** - * Returns the directory where workspace files are stored. If the directory does not exist, - * it will be created. - * - * @param workspacePath the file object representing a directory within the workspace - * @return the workspace files directory - * @throws IOException if an I/O error occurs while creating the directory - */ - protected File getOrCreateWorkspaceFilesDirectory(final Path workspacePath) throws IOException { - final Workspace workspace = workspaceManager.getOrCreate(workspacePath); - return workspace.files().toFile(); - } - - /** - * - * @param path represents a directory within the workspace - * @return the workspace files directory - * @throws IllegalArgumentException if a valid workspace is not found from the provided path - */ - protected File getWorkspaceDirectory(final Path path) { - - final var workspace = workspaceManager.findWorkspace(path); - - if (workspace.isPresent()) { - return workspace.get().root().toFile(); - } - - throw new IllegalArgumentException( - String.format("No valid workspace found at path: [%s]", path.toAbsolutePath())); - } - - } diff --git a/tools/dotcms-cli/cli/src/main/java/com/dotcms/cli/command/files/FilesPush.java b/tools/dotcms-cli/cli/src/main/java/com/dotcms/cli/command/files/FilesPush.java index 46c9394e17de..a3efed19fc8e 100644 --- a/tools/dotcms-cli/cli/src/main/java/com/dotcms/cli/command/files/FilesPush.java +++ b/tools/dotcms-cli/cli/src/main/java/com/dotcms/cli/command/files/FilesPush.java @@ -6,23 +6,28 @@ import com.dotcms.api.client.files.PushService; import com.dotcms.api.client.files.traversal.PushTraverseParams; -import com.dotcms.cli.command.PushContext; import com.dotcms.api.client.files.traversal.TraverseResult; import com.dotcms.api.traversal.TreeNode; import com.dotcms.api.traversal.TreeNodePushInfo; import com.dotcms.cli.command.DotCommand; import com.dotcms.cli.command.DotPush; +import com.dotcms.cli.command.PushContext; import com.dotcms.cli.common.ConsoleLoadingAnimation; import com.dotcms.cli.common.OutputOptionMixin; import com.dotcms.cli.common.PushMixin; import com.dotcms.common.AssetsUtils; import com.dotcms.common.LocalPathStructure; +import com.dotcms.model.config.Workspace; +import java.io.File; +import java.io.IOException; +import java.nio.file.Path; import java.util.List; import java.util.Optional; import java.util.concurrent.Callable; import java.util.concurrent.CompletableFuture; import javax.enterprise.context.control.ActivateRequestContext; import javax.inject.Inject; +import org.apache.commons.lang3.tuple.Pair; import picocli.CommandLine; @ActivateRequestContext @@ -58,7 +63,6 @@ public class FilesPush extends AbstractFilesCommand implements Callable @Override public Integer call() throws Exception { - // When calling from the global push we should avoid the validation of the unmatched // arguments as we may send arguments meant for other push subcommands if (!pushMixin.noValidateUnmatchedArguments) { @@ -66,18 +70,19 @@ public Integer call() throws Exception { output.throwIfUnmatchedArguments(spec.commandLine()); } - // Getting the workspace - var workspace = getWorkspaceDirectory(pushMixin.path()); + // Validating and resolving the workspace and path + var resolvedWorkspaceAndPath = resolveWorkspaceAndPath(); + File finalInputFile = resolvedWorkspaceAndPath.getRight(); CompletableFuture> folderTraversalFuture = CompletableFuture.supplyAsync( () -> - // Service to handle the traversal of the folder - pushService.traverseLocalFolders(output, workspace, - pushMixin.path().toFile(), - filesPushMixin.removeAssets, filesPushMixin.removeFolders, - false, true) - ); + // Service to handle the traversal of the folder + pushService.traverseLocalFolders( + output, resolvedWorkspaceAndPath.getLeft().root().toFile(), + finalInputFile, filesPushMixin.removeAssets, + filesPushMixin.removeFolders, false, true) + ); // ConsoleLoadingAnimation instance to handle the waiting "animation" ConsoleLoadingAnimation consoleLoadingAnimation = new ConsoleLoadingAnimation( @@ -137,16 +142,19 @@ public Integer call() throws Exception { // Pushing the tree if (!pushMixin.dryRun) { - pushService.processTreeNodes(output, treeNodePushInfo, - PushTraverseParams.builder() - .workspacePath(workspace.getAbsolutePath()) - .rootNode(treeNode) - .localPaths(localPaths) - .failFast(pushMixin.failFast) - .maxRetryAttempts(pushMixin.retryAttempts) - .pushContext(pushContext) - .build() - ); + pushService.processTreeNodes(output, treeNodePushInfo, + PushTraverseParams.builder() + .workspacePath( + resolvedWorkspaceAndPath.getLeft().root().toFile() + .getAbsolutePath() + ) + .rootNode(treeNode) + .localPaths(localPaths) + .failFast(pushMixin.failFast) + .maxRetryAttempts(pushMixin.retryAttempts) + .pushContext(pushContext) + .build() + ); } } else { @@ -167,6 +175,43 @@ public Integer call() throws Exception { return CommandLine.ExitCode.OK; } + /** + * Resolves the workspace and path for the current operation. + * + * @return A Pair object containing the Workspace and File objects representing the resolved + * workspace and path, respectively. + * @throws IOException If there is an error accessing the path or if no valid workspace is + * found. + */ + private Pair resolveWorkspaceAndPath() throws IOException { + + // Make sure the path is within a workspace + final Optional workspace = workspaceManager.findWorkspace( + this.getPushMixin().path() + ); + if (workspace.isEmpty()) { + throw new IllegalArgumentException( + String.format("No valid workspace found at path: [%s]", + this.getPushMixin().path())); + } + + File inputFile = this.getPushMixin().path().toFile(); + if (!inputFile.isAbsolute()) { + // If the path is not absolute, we assume it is relative to the files folder + inputFile = Path.of( + workspace.get().files().toString(), inputFile.getPath() + ).toFile(); + } + if (!inputFile.exists() || !inputFile.canRead()) { + throw new IOException(String.format( + "Unable to access the path [%s] check that it does exist and that you have " + + "read permissions on it.", inputFile) + ); + } + + return Pair.of(workspace.get(), inputFile); + } + private void header(int count, LocalPathStructure localPaths, StringBuilder outputBuilder) { outputBuilder.append(count == 0 ? "\r\n" : "\n\n"). @@ -202,11 +247,11 @@ private void changesSummary(TreeNodePushInfo pushInfo, StringBuilder outputBuild "- @|bold,%s [%s]|@ Folders to push " + "- @|bold,%s [%s]|@ Folders to delete\n\n", pushInfo.assetsToPushCount(), - COLOR_MODIFIED,pushInfo.assetsNewCount(), - COLOR_DELETED,pushInfo.assetsModifiedCount(), - COLOR_NEW,pushInfo.assetsToDeleteCount(), - COLOR_NEW,pushInfo.foldersToPushCount(), - COLOR_DELETED,pushInfo.foldersToDeleteCount()) + COLOR_MODIFIED, pushInfo.assetsNewCount(), + COLOR_DELETED, pushInfo.assetsModifiedCount(), + COLOR_NEW, pushInfo.assetsToDeleteCount(), + COLOR_NEW, pushInfo.foldersToPushCount(), + COLOR_DELETED, pushInfo.foldersToDeleteCount()) ); } else { outputBuilder.append(String.format(" Push Data: " + @@ -214,10 +259,10 @@ private void changesSummary(TreeNodePushInfo pushInfo, StringBuilder outputBuild "- @|bold,%s [%s]|@ Assets to delete " + "- @|bold,%s [%s]|@ Folders to push " + "- @|bold,%s [%s]|@ Folders to delete\n\n", - COLOR_NEW,pushInfo.assetsToPushCount(), - COLOR_DELETED,pushInfo.assetsToDeleteCount(), - COLOR_NEW,pushInfo.foldersToPushCount(), - COLOR_DELETED,pushInfo.foldersToDeleteCount())); + COLOR_NEW, pushInfo.assetsToPushCount(), + COLOR_DELETED, pushInfo.assetsToDeleteCount(), + COLOR_NEW, pushInfo.foldersToPushCount(), + COLOR_DELETED, pushInfo.foldersToDeleteCount())); } } diff --git a/tools/dotcms-cli/cli/src/main/java/com/dotcms/cli/common/PushMixin.java b/tools/dotcms-cli/cli/src/main/java/com/dotcms/cli/common/PushMixin.java index 4703b6f90089..310aa185269f 100644 --- a/tools/dotcms-cli/cli/src/main/java/com/dotcms/cli/common/PushMixin.java +++ b/tools/dotcms-cli/cli/src/main/java/com/dotcms/cli/common/PushMixin.java @@ -49,7 +49,7 @@ public class PushMixin { */ public Path path() { if (null == path) { - return Path.of(""); + return Path.of("").toAbsolutePath(); } return path.toPath(); } diff --git a/tools/dotcms-cli/cli/src/test/java/com/dotcms/cli/command/files/FilesPushCommandIT.java b/tools/dotcms-cli/cli/src/test/java/com/dotcms/cli/command/files/FilesPushCommandIT.java index 4ce737e925a7..427a9e59a729 100644 --- a/tools/dotcms-cli/cli/src/test/java/com/dotcms/cli/command/files/FilesPushCommandIT.java +++ b/tools/dotcms-cli/cli/src/test/java/com/dotcms/cli/command/files/FilesPushCommandIT.java @@ -1,5 +1,8 @@ package com.dotcms.cli.command.files; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; + import com.dotcms.DotCMSITProfile; import com.dotcms.api.AuthenticationContext; import com.dotcms.api.client.files.PushService; @@ -7,12 +10,6 @@ import com.dotcms.common.WorkspaceManager; import io.quarkus.test.junit.QuarkusTest; import io.quarkus.test.junit.TestProfile; -import org.junit.jupiter.api.*; -import org.mockito.InjectMocks; -import org.mockito.Mockito; -import picocli.CommandLine; - -import javax.inject.Inject; import java.io.IOException; import java.io.PrintWriter; import java.io.StringWriter; @@ -23,9 +20,13 @@ import java.nio.file.attribute.BasicFileAttributes; import java.util.Collections; import java.util.UUID; - -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyBoolean; +import javax.inject.Inject; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.InjectMocks; +import org.mockito.Mockito; +import picocli.CommandLine; @QuarkusTest @TestProfile(DotCMSITProfile.class) @@ -78,6 +79,57 @@ void testPushNoWorkspace() throws IOException { } } + /** + * This method tests the behavior of pushing files with no specified path and a valid + * workspace. + * + * @throws IOException If an I/O error occurs. + */ + @Test + void testPushNoPath() throws IOException { + + // Create a workspace in the current directory + workspaceManager.getOrCreate(); + + final CommandLine commandLine = createCommand(); + + final StringWriter writer = new StringWriter(); + try (PrintWriter out = new PrintWriter(writer)) { + commandLine.setOut(out); + final int status = commandLine.execute( + FilesCommand.NAME, FilesPush.NAME, "--dry-run" + ); + Assertions.assertEquals(CommandLine.ExitCode.OK, status); + } + } + + /** + * This method tests the behavior of pushing files with no specified path and an invalid + * workspace. + * + * @throws IOException If an I/O error occurs. + */ + @Test + void testPushNoPathInvalidWorkspace() throws IOException { + + // Cleaning up old traces of a possible workspace created in the current directory + // by other tests + var workspace = workspaceManager.findWorkspace(); + if (workspace.isPresent()) { + workspaceManager.destroy(workspace.get()); + } + + final CommandLine commandLine = createCommand(); + final StringWriter writer = new StringWriter(); + try (PrintWriter out = new PrintWriter(writer)) { + commandLine.setOut(out); + final int status = commandLine.execute( + FilesCommand.NAME, FilesPush.NAME + ); + Assertions.assertEquals(CommandLine.ExitCode.USAGE, status); + } + } + @Test void testPush() throws IOException {