Skip to content

Commit

Permalink
chore: git changes for new contract (#39376)
Browse files Browse the repository at this point in the history
## Description
- bug solutions and error handling

## Automation

/ok-to-test tags="@tag.Git"

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/13436821247>
> Commit: 4477846
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13436821247&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Git`
> Spec:
> <hr>Thu, 20 Feb 2025 14:40:06 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [ ] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Artifact type information is now publicly visible in application
responses.
- **Refactor**
- Enhanced repository and file operations with improved path validation,
error handling, and updated dependency integration.
- **Tests**
- Updated test configurations to align with the revised dependency setup
and file handling enhancements.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
sondermanish authored Feb 21, 2025
1 parent 5c92906 commit 98457f9
Show file tree
Hide file tree
Showing 7 changed files with 114 additions and 48 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.appsmith.external.git.FileInterface;
import com.appsmith.external.git.GitExecutor;
import com.appsmith.external.git.constants.GitSpan;
import com.appsmith.external.git.handler.FSGitHandler;
import com.appsmith.external.git.models.GitResourceIdentity;
import com.appsmith.external.git.models.GitResourceMap;
import com.appsmith.external.git.models.GitResourceType;
Expand Down Expand Up @@ -75,6 +76,7 @@
import static com.appsmith.git.constants.GitDirectories.DATASOURCE_DIRECTORY;
import static com.appsmith.git.constants.GitDirectories.JS_LIB_DIRECTORY;
import static com.appsmith.git.constants.GitDirectories.PAGE_DIRECTORY;
import static com.appsmith.git.constants.ce.CommonConstantsCE.DELIMITER_PATH;

@Slf4j
@Getter
Expand All @@ -83,6 +85,7 @@
public class FileUtilsCEImpl implements FileInterface {

private final GitServiceConfig gitServiceConfig;
protected final FSGitHandler fsGitHandler;
private final GitExecutor gitExecutor;
protected final FileOperations fileOperations;
private final ObservationHelper observationHelper;
Expand All @@ -101,11 +104,13 @@ public class FileUtilsCEImpl implements FileInterface {

public FileUtilsCEImpl(
GitServiceConfig gitServiceConfig,
FSGitHandler fsGitHandler,
GitExecutor gitExecutor,
FileOperations fileOperations,
ObservationHelper observationHelper,
ObjectMapper objectMapper) {
this.gitServiceConfig = gitServiceConfig;
this.fsGitHandler = fsGitHandler;
this.gitExecutor = gitExecutor;
this.fileOperations = fileOperations;
this.observationHelper = observationHelper;
Expand Down Expand Up @@ -247,7 +252,7 @@ public Mono<Path> saveArtifactToGitRepo(Path baseRepoSuffix, GitResourceMap gitR
// Repo path will be:
// baseRepo : root/workspaceId/defaultAppId/repoName/{applicationData}
// Checkout to mentioned branch if not already checked-out
return gitExecutor
return fsGitHandler
.resetToLastCommit(baseRepoSuffix, branchName)
.flatMap(isSwitched -> {
Path baseRepo = Paths.get(gitServiceConfig.getGitRootPath()).resolve(baseRepoSuffix);
Expand All @@ -263,12 +268,48 @@ public Mono<Path> saveArtifactToGitRepo(Path baseRepoSuffix, GitResourceMap gitR
.subscribeOn(scheduler);
}

protected Set<String> getWhitelistedPaths() {
String pages = PAGE_DIRECTORY + DELIMITER_PATH;
String datasources = DATASOURCE_DIRECTORY + DELIMITER_PATH;
String themes = CommonConstants.THEME + JSON_EXTENSION;
String application = CommonConstants.APPLICATION + JSON_EXTENSION;
String metadata = CommonConstants.METADATA + JSON_EXTENSION;
String customJsLibs = JS_LIB_DIRECTORY + DELIMITER_PATH;

return new HashSet<>(Set.of(pages, datasources, themes, application, metadata, customJsLibs));
}

protected Boolean isWhiteListedPath(Set<String> whiteListedPaths, String relativePath) {

// Not expecting the relative path to ever be empty.
// .git is internal file this shouldn't be whitelisted
if (!StringUtils.hasText(relativePath) || relativePath.contains(".git/")) {
return Boolean.FALSE;
}

// cases where the path is a direct root config object
if (whiteListedPaths.contains(relativePath)) {
return Boolean.TRUE;
}

String[] tokens = relativePath.strip().split(DELIMITER_PATH);
// it means that path is not a root config object and adheres to the given whitelisted path
if (tokens.length > 1 && whiteListedPaths.contains(tokens[0] + DELIMITER_PATH)) {
return Boolean.TRUE;
}

return Boolean.FALSE;
}

protected Set<String> getExistingFilesInRepo(Path baseRepo) throws IOException {
Set<String> whiteListedPaths = getWhitelistedPaths();
try (Stream<Path> stream = Files.walk(baseRepo).parallel()) {
return stream.filter(path -> {
try {
return !path.toString().contains(".git" + File.separator)
&& (Files.isRegularFile(path) || FileUtils.isEmptyDirectory(path.toFile()));
return (Files.isRegularFile(path) || FileUtils.isEmptyDirectory(path.toFile()))
&& isWhiteListedPath(
whiteListedPaths,
baseRepo.relativize(path).toString());
} catch (IOException e) {
log.error("Unable to find file details. Please check the file at file path: {}", path);
log.error("Assuming that it does not exist for now ...");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.appsmith.external.git.FileInterface;
import com.appsmith.external.git.GitExecutor;
import com.appsmith.external.git.handler.FSGitHandler;
import com.appsmith.external.git.operations.FileOperations;
import com.appsmith.external.helpers.ObservationHelper;
import com.appsmith.git.configurations.GitServiceConfig;
Expand All @@ -21,10 +22,11 @@ public class FileUtilsImpl extends FileUtilsCEImpl implements FileInterface {

public FileUtilsImpl(
GitServiceConfig gitServiceConfig,
FSGitHandler fsGitHandler,
GitExecutor gitExecutor,
FileOperations fileOperations,
ObservationHelper observationHelper,
ObjectMapper objectMapper) {
super(gitServiceConfig, gitExecutor, fileOperations, observationHelper, objectMapper);
super(gitServiceConfig, fsGitHandler, gitExecutor, fileOperations, observationHelper, objectMapper);
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.appsmith.git.helpers;

import com.appsmith.external.git.handler.FSGitHandler;
import com.appsmith.external.git.operations.FileOperations;
import com.appsmith.external.helpers.ObservationHelper;
import com.appsmith.external.models.ApplicationGitReference;
Expand Down Expand Up @@ -30,8 +31,9 @@
import static com.appsmith.git.constants.GitDirectories.PAGE_DIRECTORY;

public class FileUtilsImplTest {
private FileUtilsImpl fileUtils;

private FileUtilsImpl fileUtils;
private FSGitHandler fsGitHandler;
private GitExecutorImpl gitExecutor;

private static final String localTestDirectory = "localTestDirectory";
Expand All @@ -44,7 +46,12 @@ public void setUp() {
gitServiceConfig.setGitRootPath(localTestDirectoryPath.toString());
FileOperations fileOperations = new FileOperationsImpl(null, ObservationHelper.NOOP);
fileUtils = new FileUtilsImpl(
gitServiceConfig, gitExecutor, fileOperations, ObservationHelper.NOOP, new ObjectMapper());
gitServiceConfig,
fsGitHandler,
gitExecutor,
fileOperations,
ObservationHelper.NOOP,
new ObjectMapper());
}

@AfterEach
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ public void setApplicationDetail(ApplicationDetail applicationDetail) {
}

@Override
@JsonView({Views.Internal.class})
@JsonView({Views.Public.class})
public ArtifactType getArtifactType() {
return ArtifactType.APPLICATION;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,15 @@ public Mono<Path> updateImportedRepositoryDetails(
String workspaceId = jsonTransformationDTO.getWorkspaceId();
String placeHolder = jsonTransformationDTO.getBaseArtifactId();
String repoName = jsonTransformationDTO.getRepoName();
Path temporaryStorage = Path.of(workspaceId, placeHolder, repoName);
Path temporaryArtifactPath = Path.of(workspaceId, placeHolder);

ArtifactType artifactType = baseArtifact.getArtifactType();
GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
Path newPath = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifact.getId(), repoName);
Path absoluteArtifactPath = gitArtifactHelper
.getRepoSuffixPath(workspaceId, baseArtifact.getId(), repoName)
.getParent();

return commonGitFileUtils.moveArtifactFromTemporaryToBaseArtifactIdRepository(temporaryStorage, newPath);
return commonGitFileUtils.moveRepositoryFromTemporaryStorage(temporaryArtifactPath, absoluteArtifactPath);
}

@Override
Expand All @@ -237,29 +239,29 @@ public Mono<String> fetchRemoteRepository(
repoSuffix, gitConnectDTO.getRemoteUrl(), gitAuth.getPrivateKey(), gitAuth.getPublicKey())
.onErrorResume(error -> {
log.error("Error while cloning the remote repo, {}", error.getMessage());
return gitAnalyticsUtils
.addAnalyticsForGitOperation(
AnalyticsEvents.GIT_IMPORT,
artifact,
error.getClass().getName(),
error.getMessage(),
false)
.flatMap(user -> commonGitFileUtils
.deleteLocalRepo(repoSuffix)
.then(gitArtifactHelper.deleteArtifact(artifact.getId())))
.flatMap(artifact1 -> {
if (error instanceof TransportException) {
return Mono.error(
new AppsmithException(AppsmithError.INVALID_GIT_SSH_CONFIGURATION));
} else if (error instanceof InvalidRemoteException) {
return Mono.error(
new AppsmithException(AppsmithError.INVALID_PARAMETER, "remote url"));
} else if (error instanceof TimeoutException) {
return Mono.error(new AppsmithException(AppsmithError.GIT_EXECUTION_TIMEOUT));
}
return Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED, "clone", error.getMessage()));
});

Mono<Boolean> deleteLocalRepoMono = commonGitFileUtils.deleteLocalRepo(repoSuffix);
Mono<? extends Artifact> errorAnalyticsMono = gitAnalyticsUtils.addAnalyticsForGitOperation(
AnalyticsEvents.GIT_IMPORT,
artifact,
error.getClass().getName(),
error.getMessage(),
false);

AppsmithException appsmithException;

if (error instanceof TransportException) {
appsmithException = new AppsmithException(AppsmithError.INVALID_GIT_SSH_CONFIGURATION);
} else if (error instanceof InvalidRemoteException) {
appsmithException = new AppsmithException(AppsmithError.INVALID_PARAMETER, "remote url");
} else if (error instanceof TimeoutException) {
appsmithException = new AppsmithException(AppsmithError.GIT_EXECUTION_TIMEOUT);
} else {
appsmithException =
new AppsmithException(AppsmithError.GIT_ACTION_FAILED, "clone", error.getMessage());
}

return deleteLocalRepoMono.zipWith(errorAnalyticsMono).then(Mono.error(appsmithException));
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ protected ArtifactGitFileUtils<?> getArtifactBasedFileHelper(ArtifactType artifa
* This method will save the complete application in the local repo directory.
* Path to repo will be : ./container-volumes/git-repo/workspaceId/defaultApplicationId/repoName/{application_data}
*
* @param baseRepoSuffix path suffix used to create a local repo path
* @param baseRepoSuffix path suffix used to create a local repo path
* @param artifactExchangeJson application reference object from which entire application can be rehydrated
* @param branchName name of the branch for the current application
* @param branchName name of the branch for the current application
* @return repo path where the application is stored
*/
public Mono<Path> saveArtifactToLocalRepo(
Expand Down Expand Up @@ -662,9 +662,9 @@ && hasText(jsonTransformationDTO.getBaseArtifactId())
/**
* Method to reconstruct the application from the local git repo
*
* @param workspaceId To which workspace application needs to be rehydrated
* @param workspaceId To which workspace application needs to be rehydrated
* @param baseArtifactId Root application for the current branched application
* @param branchName for which branch the application needs to rehydrate
* @param branchName for which branch the application needs to rehydrate
* @param artifactType
* @return application reference from which entire application can be rehydrated
*/
Expand Down Expand Up @@ -786,17 +786,31 @@ public Mono<Map<String, Integer>> reconstructMetadataFromRepo(
});
}

public Mono<Path> moveArtifactFromTemporaryToBaseArtifactIdRepository(
Path currentRepositoryPath, Path newRepositoryPath) {
Path currentGitPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(currentRepositoryPath);
Path targetPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(newRepositoryPath);
public Mono<Path> moveRepositoryFromTemporaryStorage(Path temporaryPath, Path absoluteArtifactPath) {

Path currentGitPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(temporaryPath);
Path targetPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(absoluteArtifactPath);

return Mono.fromCallable(() -> {
if (!Files.exists(targetPath)) {
Files.createDirectories(targetPath);
}
try {
if (!Files.exists(targetPath)) {
Files.createDirectories(targetPath);
}

return Files.move(currentGitPath, targetPath, REPLACE_EXISTING);
return Files.move(currentGitPath, targetPath, REPLACE_EXISTING);

} catch (IOException exception) {
log.error("File IO exception while moving repository. {}", exception.getMessage());
throw new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, exception.getMessage());
}
})
.onErrorResume(error -> {
log.error(
"Error while moving repository from temporary storage {} to permanent storage {}",
currentGitPath,
targetPath,
error.getMessage());
return Mono.error(error);
})
.subscribeOn(Schedulers.boundedElastic());
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package com.appsmith.server.git.resourcemap;

import com.appsmith.external.git.GitExecutor;
import com.appsmith.external.git.handler.FSGitHandler;
import com.appsmith.external.git.models.GitResourceIdentity;
import com.appsmith.external.git.models.GitResourceMap;
import com.appsmith.server.constants.ArtifactType;
Expand Down Expand Up @@ -52,7 +52,7 @@ public class ExchangeJsonConversionTests {
CommonGitFileUtils commonGitFileUtils;

@SpyBean
GitExecutor gitExecutor;
FSGitHandler fsGitHandler;

@TestTemplate
public void testConvertArtifactJsonToGitResourceMap_whenArtifactIsFullyPopulated_returnsCorrespondingResourceMap(
Expand Down Expand Up @@ -127,7 +127,7 @@ public void testSerializeArtifactExchangeJson_whenArtifactIsFullyPopulated_retur
ExchangeJsonContext context) throws IOException, GitAPIException {
ArtifactExchangeJson originalArtifactJson = createArtifactJson(context).block();

Mockito.doReturn(Mono.just(true)).when(gitExecutor).resetToLastCommit(Mockito.any(), Mockito.anyString());
Mockito.doReturn(Mono.just(true)).when(fsGitHandler).resetToLastCommit(Mockito.any(), Mockito.anyString());

Files.createDirectories(Path.of("./container-volumes/git-storage/test123"));

Expand Down

0 comments on commit 98457f9

Please sign in to comment.