Skip to content

chore: Converging git refactor changes #33971

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nidhi-nair merged 11 commits into from
Jun 4, 2024
Merged

chore: Converging git refactor changes #33971

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b8d4ca3
resolved stash
sondermanish May 28, 2024
8e0668b
Moved over to refactored service
nidhi-nair May 29, 2024
aeeb005
added implementation for empty context check
sondermanish May 30, 2024
b91b380
fixes and merge w release
nidhi-nair Jun 4, 2024
8afe8ee
test fixes
nidhi-nair Jun 4, 2024
ff57d32
Merge branch 'release' of github.com:appsmithorg/appsmith-ee into cho…
nidhi-nair Jun 4, 2024
2f5a12c
Reverted unnecessary changes
nidhi-nair Jun 4, 2024
7dcb38f
Reverted unnecessary changes
nidhi-nair Jun 4, 2024
4e0a2d9
Reverted unnecessary changes
nidhi-nair Jun 4, 2024
ca6d066
Reverted unnecessary changes
nidhi-nair Jun 4, 2024
4bb1a86
revert run file
nidhi-nair Jun 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/server/.run/ServerApplication.run.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,4 @@
<option name="Make" enabled="true" />
</method>
</configuration>
</component>
</component>
Comment thread
nidhi-nair marked this conversation as resolved.
Outdated
18 changes: 9 additions & 9 deletions ...-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,10 +301,10 @@ public Mono<Application> update(String id, Application application) {
GitArtifactMetadata gitData = application.getGitApplicationMetadata();
if (gitData != null
&& !StringUtils.isEmpty(gitData.getBranchName())
&& !StringUtils.isEmpty(gitData.getDefaultApplicationId())) {
&& !StringUtils.isEmpty(gitData.getDefaultArtifactId())) {
applicationIdMono = this.findByBranchNameAndDefaultApplicationId(
gitData.getBranchName(),
gitData.getDefaultApplicationId(),
gitData.getDefaultArtifactId(),
applicationPermission.getEditPermission())
.map(Application::getId);
} else {
Expand Down Expand Up @@ -690,8 +690,8 @@ public Mono<GitAuth> createOrUpdateSshKeyPair(String applicationId, String keyTy
// Check if the current application is the root application

if (gitData != null
&& !StringUtils.isEmpty(gitData.getDefaultApplicationId())
&& applicationId.equals(gitData.getDefaultApplicationId())) {
&& !StringUtils.isEmpty(gitData.getDefaultArtifactId())
&& applicationId.equals(gitData.getDefaultArtifactId())) {
// This is the root application with update SSH key request
gitAuth.setRegeneratedKey(true);
gitData.setGitAuth(gitAuth);
Expand All @@ -707,15 +707,15 @@ public Mono<GitAuth> createOrUpdateSshKeyPair(String applicationId, String keyTy
// Children application with update SSH key request for root application
// Fetch root application and then make updates. We are storing the git metadata only in root
// application
if (StringUtils.isEmpty(gitData.getDefaultApplicationId())) {
if (StringUtils.isEmpty(gitData.getDefaultArtifactId())) {
throw new AppsmithException(
AppsmithError.INVALID_GIT_CONFIGURATION,
"Unable to find root application, please connect your application to remote repo to resolve this issue.");
}
gitAuth.setRegeneratedKey(true);

return repository
.findById(gitData.getDefaultApplicationId(), applicationPermission.getEditPermission())
.findById(gitData.getDefaultArtifactId(), applicationPermission.getEditPermission())
.flatMap(defaultApplication -> {
GitArtifactMetadata gitArtifactMetadata =
defaultApplication.getGitApplicationMetadata();
Expand Down Expand Up @@ -770,7 +770,7 @@ public Mono<GitAuthDTO> getSshKey(String applicationId) {
"Can't find valid SSH key. Please configure the application with git"));
}
// Check if the application is root application
if (applicationId.equals(gitData.getDefaultApplicationId())) {
if (applicationId.equals(gitData.getDefaultArtifactId())) {
gitData.getGitAuth().setDocUrl(Assets.GIT_DEPLOY_KEY_DOC_URL);
GitAuthDTO gitAuthDTO = new GitAuthDTO();
gitAuthDTO.setPublicKey(gitData.getGitAuth().getPublicKey());
Expand All @@ -779,14 +779,14 @@ public Mono<GitAuthDTO> getSshKey(String applicationId) {
gitAuthDTO.setGitSupportedSSHKeyType(gitDeployKeyDTOList);
return Mono.just(gitAuthDTO);
}
if (gitData.getDefaultApplicationId() == null) {
if (gitData.getDefaultArtifactId() == null) {
throw new AppsmithException(
AppsmithError.INVALID_GIT_CONFIGURATION,
"Can't find root application. Please configure the application with git");
}

return repository
.findById(gitData.getDefaultApplicationId(), applicationPermission.getEditPermission())
.findById(gitData.getDefaultArtifactId(), applicationPermission.getEditPermission())
.map(rootApplication -> {
GitAuthDTO gitAuthDTO = new GitAuthDTO();
GitAuth gitAuth = rootApplication
Expand Down
29 changes: 25 additions & 4 deletions ...server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
import com.appsmith.server.domains.GitArtifactMetadata;
import com.appsmith.server.domains.NewAction;
import com.appsmith.server.domains.NewPage;
import com.appsmith.server.dtos.ApplicationJson;
import com.appsmith.server.dtos.ArtifactExchangeJson;
import com.appsmith.server.dtos.GitAuthDTO;
import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException;
Expand Down Expand Up @@ -47,7 +49,7 @@
@RequiredArgsConstructor
public class GitApplicationHelperCEImpl implements GitArtifactHelperCE<Application> {

private final CommonGitFileUtils gitFileUtils;
private final CommonGitFileUtils commonGitFileUtils;
private final GitPrivateRepoHelper gitPrivateRepoHelper;

private final ApplicationService applicationService;
Expand Down Expand Up @@ -178,9 +180,9 @@ public Mono<Application> isPrivateRepoLimitReached(Artifact artifact, boolean is
}

@Override
public Mono<Application> publishArtifact(Artifact artifact) {
public Mono<Application> publishArtifact(Artifact artifact, Boolean isPublishedManually) {
Application application = (Application) artifact;
return applicationPageService.publish(application.getId(), true).then(Mono.just(application));
return applicationPageService.publish(application.getId(), isPublishedManually);
}

// TODO: scope for improvement
Expand All @@ -204,7 +206,7 @@ public Mono<Path> intialiseReadMe(Artifact artifact, Path readMePath, String ori
String editModeUrl = Paths.get(viewModeUrl, "edit").toString();
// Initialize the repo with readme file

return gitFileUtils
return commonGitFileUtils
.initializeReadme(readMePath, originHeader + viewModeUrl, originHeader + editModeUrl)
.onErrorMap(throwable -> {
log.error("Error while initialising git repo, {0}", throwable);
Expand Down Expand Up @@ -305,4 +307,23 @@ public Mono<Application> disconnectEntitiesOfDefaultArtifact(Artifact defaultArt
public Application updateArtifactWithDefaultReponseUtils(Artifact artifact) {
return responseUtils.updateApplicationWithDefaultResources((Application) artifact);
}

@Override
public Mono<Application> createArtifactForImport(String workspaceId, String repoName) {
Application newApplication = new Application();
newApplication.setName(repoName);
newApplication.setWorkspaceId(workspaceId);
newApplication.setGitApplicationMetadata(new GitArtifactMetadata());
return applicationPageService.createOrUpdateSuffixedApplication(newApplication, newApplication.getName(), 0);
}
Comment on lines +312 to +318

@coderabbitai coderabbitai Bot Jun 4, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding error handling for the createArtifactForImport method.

+        return applicationPageService.createOrUpdateSuffixedApplication(newApplication, newApplication.getName(), 0)
+            .onErrorResume(e -> {
+                log.error("Failed to create artifact for import: {}", e.getMessage());
+                return Mono.error(new AppsmithException(AppsmithError.GENERIC_BAD_REQUEST, e.getMessage()));
+            });

Adding error handling will improve robustness by providing feedback when the creation process fails.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
public Mono<Application> createArtifactForImport(String workspaceId, String repoName) {
Application newApplication = new Application();
newApplication.setName(repoName);
newApplication.setWorkspaceId(workspaceId);
newApplication.setGitApplicationMetadata(new GitArtifactMetadata());
return applicationPageService.createOrUpdateSuffixedApplication(newApplication, newApplication.getName(), 0);
}
public Mono<Application> createArtifactForImport(String workspaceId, String repoName) {
Application newApplication = new Application();
newApplication.setName(repoName);
newApplication.setWorkspaceId(workspaceId);
newApplication.setGitApplicationMetadata(new GitArtifactMetadata());
return applicationPageService.createOrUpdateSuffixedApplication(newApplication, newApplication.getName(), 0)
.onErrorResume(e -> {
log.error("Failed to create artifact for import: {}", e.getMessage());
return Mono.error(new AppsmithException(AppsmithError.GENERIC_BAD_REQUEST, e.getMessage()));
});
}


@Override
public Mono<Application> deleteArtifact(String artifactId) {
return applicationPageService.deleteApplication(artifactId);
}

@Override
public Boolean isContextInArtifactEmpty(ArtifactExchangeJson artifactExchangeJson) {
return CollectionUtils.isNullOrEmpty(((ApplicationJson) artifactExchangeJson).getPageList());
}
}
2 changes: 1 addition & 1 deletion ...rc/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ public Mono<Application> updateAndSaveArtifactInContext(
Mono<Application> parentApplicationMono;
if (application.getGitApplicationMetadata() != null) {
parentApplicationMono = applicationService.findById(
application.getGitApplicationMetadata().getDefaultApplicationId());
application.getGitApplicationMetadata().getDefaultArtifactId());

@coderabbitai coderabbitai Bot Jun 4, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a more specific exception type for better error handling.

- return Mono.error(new AppsmithException(
-         AppsmithError.ACL_NO_RESOURCE_FOUND,
-         FieldName.APPLICATION,
-         importingMetaDTO.getArtifactId()));
+ return Mono.error(new NotFoundException(
+         "Application not found with ID: " + importingMetaDTO.getArtifactId()));

This change would make the error more specific and informative, improving the maintainability and debuggability of the code.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
application.getGitApplicationMetadata().getDefaultArtifactId());
application.getGitApplicationMetadata().getDefaultArtifactId()));
return Mono.error(new NotFoundException(
"Application not found with ID: " + importingMetaDTO.getArtifactId()));

} else {
parentApplicationMono = Mono.just(application);
}
Expand Down
4 changes: 2 additions & 2 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/GitController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import com.appsmith.server.constants.Url;
import com.appsmith.server.controllers.ce.GitControllerCE;
import com.appsmith.server.services.GitService;
import com.appsmith.server.git.common.CommonGitService;
import lombok.extern.slf4j.Slf4j;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;
Expand All @@ -12,7 +12,7 @@
@RequestMapping(Url.GIT_URL)
public class GitController extends GitControllerCE {

public GitController(GitService service) {
public GitController(CommonGitService service) {
super(service);
}
}
Loading