Skip to content

chore: Add code-split for service layer Git integration for packages #38497

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
subrata71 merged 2 commits into from
Jan 7, 2025
Merged

chore: Add code-split for service layer Git integration for packages #38497

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
426684e
Add code-split for service layer Git integration for packages
subrata71 Jan 6, 2025
f7e7566
Review comments
subrata71 Jan 7, 2025
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
7 changes: 4 additions & 3 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 @@ -209,8 +209,8 @@ public Flux<Application> findByWorkspaceIdAndBaseApplicationsInRecentlyUsedOrder
* - Applications that are not connected to Git.
* - Applications that, when connected, revert with default branch only.
*/
return !GitUtils.isApplicationConnectedToGit(application)
|| GitUtils.isDefaultBranchedApplication(application);
return !GitUtils.isArtifactConnectedToGit(application.getGitArtifactMetadata())
|| GitUtils.isDefaultBranchedArtifact(application.getGitArtifactMetadata());
})));
}

Expand Down Expand Up @@ -900,7 +900,8 @@ public Mono<Boolean> isApplicationConnectedToGit(String applicationId) {
if (!StringUtils.hasLength(applicationId)) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ID));
}
return this.getById(applicationId).map(GitUtils::isApplicationConnectedToGit);
return this.getById(applicationId)
.map(application -> GitUtils.isArtifactConnectedToGit(application.getGitArtifactMetadata()));
}

@Override
Expand Down
31 changes: 14 additions & 17 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package com.appsmith.server.helpers;

import com.appsmith.server.domains.Application;
import com.appsmith.server.domains.GitArtifactMetadata;
import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException;
Expand Down Expand Up @@ -141,28 +140,26 @@ public static String getDefaultBranchName(GitArtifactMetadata gitArtifactMetadat
}

/**
* This method checks if the application is connected to git and is the default branched.
* This method checks if the artifact is connected to git and is the default branched.
*
* @param application application to be checked
* @return true if the application is default branched, false otherwise
* @param gitArtifactMetadata gitArtifactMetadata to be checked
* @return true if the artifact is default branched, false otherwise
*/
public static boolean isDefaultBranchedApplication(Application application) {
GitArtifactMetadata metadata = application.getGitApplicationMetadata();
return isApplicationConnectedToGit(application)
&& !StringUtils.isEmptyOrNull(metadata.getRefName())
&& metadata.getRefName().equals(metadata.getDefaultBranchName());
public static boolean isDefaultBranchedArtifact(GitArtifactMetadata gitArtifactMetadata) {
return isArtifactConnectedToGit(gitArtifactMetadata)
&& !StringUtils.isEmptyOrNull(gitArtifactMetadata.getRefName())
&& gitArtifactMetadata.getRefName().equals(gitArtifactMetadata.getDefaultBranchName());
}

/**
* This method checks if the application is connected to Git or not.
* @param application application to be checked
* @return true if the application is connected to Git, false otherwise
* This method checks if the artifact is connected to Git or not.
* @param gitArtifactMetadata gitArtifactMetadata to be checked
* @return true if the artifact is connected to Git, false otherwise
*/
public static boolean isApplicationConnectedToGit(Application application) {
GitArtifactMetadata metadata = application.getGitApplicationMetadata();
return metadata != null
&& !StringUtils.isEmptyOrNull(metadata.getDefaultArtifactId())
&& !StringUtils.isEmptyOrNull(metadata.getRemoteUrl());
public static boolean isArtifactConnectedToGit(GitArtifactMetadata gitArtifactMetadata) {
return gitArtifactMetadata != null
&& !StringUtils.isEmptyOrNull(gitArtifactMetadata.getDefaultArtifactId())
&& !StringUtils.isEmptyOrNull(gitArtifactMetadata.getRemoteUrl());
}

public static boolean isMigrationRequired(JSONObject layoutDsl, Integer latestDslVersion) {
Expand Down
4 changes: 2 additions & 2 deletions ...h-server/src/main/java/com/appsmith/server/searchentities/SearchEntitySolutionCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ public Mono<SearchEntityDTO> searchEntity(
* OR
* - Applications that, when connected, revert with default branch only.
*/
return !GitUtils.isApplicationConnectedToGit(application)
|| GitUtils.isDefaultBranchedApplication(application);
return !GitUtils.isArtifactConnectedToGit(application.getGitArtifactMetadata())
|| GitUtils.isDefaultBranchedArtifact(application.getGitArtifactMetadata());
})
.collectList();
}
Expand Down
2 changes: 1 addition & 1 deletion ...th-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,7 @@ public Mono<Application> deleteApplication(String id) {
return applicationMono
.flatMapMany(application -> {
GitArtifactMetadata gitData = application.getGitApplicationMetadata();
if (GitUtils.isApplicationConnectedToGit(application)) {
if (GitUtils.isArtifactConnectedToGit(application.getGitArtifactMetadata())) {
return applicationService.findAllApplicationsByBaseApplicationId(
gitData.getDefaultArtifactId(), applicationPermission.getDeletePermission());
}
Comment on lines +523 to 526

@coderabbitai coderabbitai Bot Jan 6, 2025

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.

💡 Codebase verification

Migration from isApplicationConnectedToGit to isArtifactConnectedToGit appears incomplete

The old method is still present in the codebase and being actively used. In ApplicationImportServiceCEImpl.java, the new isArtifactConnectedToGit method is actually calling the old isApplicationConnectedToGit method, suggesting an incomplete migration.

  • app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java: Review if this implementation should be updated to use the new Git utility method directly
  • app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCE.java: Consider if the old method should be deprecated
🔗 Analysis chain

Confirm all references to the old method are removed.

This line correctly adopts the artifact-level check instead of the legacy application-level Git check. Ensure that any remaining usage of GitUtils.isApplicationConnectedToGit has been replaced with isArtifactConnectedToGit throughout the codebase.

Run the following script to scan for any references to the now retired isApplicationConnectedToGit:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Searching for references to isApplicationConnectedToGit across the repository
ast-grep --pattern $'isApplicationConnectedToGit($_)'

Length of output: 66109

Script:

#!/bin/bash
# Search for any remaining references to isApplicationConnectedToGit in Java files
rg "isApplicationConnectedToGit" -t java -C 2

Length of output: 2401

Expand Down
26 changes: 13 additions & 13 deletions app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@

import java.util.UUID;

import static com.appsmith.server.helpers.GitUtils.isApplicationConnectedToGit;
import static com.appsmith.server.helpers.GitUtils.isDefaultBranchedApplication;
import static com.appsmith.server.helpers.GitUtils.isArtifactConnectedToGit;
import static com.appsmith.server.helpers.GitUtils.isDefaultBranchedArtifact;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
Expand Down Expand Up @@ -216,15 +216,15 @@ public void testIsApplicationConnectedToGit_Connected() {
gitMetadata.setDefaultApplicationId(UUID.randomUUID().toString());
connectedApplication.setGitApplicationMetadata(gitMetadata);

assertTrue(isApplicationConnectedToGit(connectedApplication));
assertTrue(isArtifactConnectedToGit(connectedApplication.getGitArtifactMetadata()));
}

@Test
public void testIsApplicationConnectedToGit_NotConnected_NullMetadata() {
// Create a mock Application with null Git metadata
Application notConnectedApplication = new Application();

assertFalse(isApplicationConnectedToGit(notConnectedApplication));
assertFalse(isArtifactConnectedToGit(notConnectedApplication.getGitArtifactMetadata()));
}

@Test
Expand All @@ -235,7 +235,7 @@ public void testIsApplicationConnectedToGit_NotConnected_NullDefaultApplicationI
gitMetadata.setRemoteUrl("https://git.example.com/repo.git");
notConnectedApplication.setGitApplicationMetadata(gitMetadata);

assertFalse(isApplicationConnectedToGit(notConnectedApplication));
assertFalse(isArtifactConnectedToGit(notConnectedApplication.getGitArtifactMetadata()));
}

@Test
Expand All @@ -246,7 +246,7 @@ public void testIsApplicationConnectedToGit_NotConnected_NullRemoteUrl() {
gitMetadata.setDefaultApplicationId(UUID.randomUUID().toString());
notConnectedApplication.setGitApplicationMetadata(gitMetadata);

assertFalse(isApplicationConnectedToGit(notConnectedApplication));
assertFalse(isArtifactConnectedToGit(notConnectedApplication.getGitArtifactMetadata()));
}

@Test
Expand All @@ -258,7 +258,7 @@ public void testIsApplicationConnectedToGit_NotConnected_EmptyMetadata() {
gitMetadata.setRemoteUrl("");
notConnectedApplication.setGitApplicationMetadata(gitMetadata);

assertFalse(isApplicationConnectedToGit(notConnectedApplication));
assertFalse(isArtifactConnectedToGit(notConnectedApplication.getGitArtifactMetadata()));
}

@Test
Expand All @@ -272,7 +272,7 @@ public void testIsDefaultBranchedApplication_DefaultBranch() {
metadata.setDefaultBranchName("main");
defaultBranchApplication.setGitApplicationMetadata(metadata);

assertTrue(isDefaultBranchedApplication(defaultBranchApplication));
assertTrue(isDefaultBranchedArtifact(defaultBranchApplication.getGitArtifactMetadata()));
}

@Test
Expand All @@ -286,15 +286,15 @@ public void testIsDefaultBranchedApplication_NotDefaultBranch() {
metadata.setDefaultBranchName("main");
nonDefaultBranchApplication.setGitApplicationMetadata(metadata);

assertFalse(isDefaultBranchedApplication(nonDefaultBranchApplication));
assertFalse(isDefaultBranchedArtifact(nonDefaultBranchApplication.getGitArtifactMetadata()));
}

@Test
public void testIsDefaultBranchedApplication_NotConnected() {
// Create a mock Application without connected Git metadata
Application notConnectedApplication = new Application();

assertFalse(isDefaultBranchedApplication(notConnectedApplication));
assertFalse(isDefaultBranchedArtifact(notConnectedApplication.getGitArtifactMetadata()));
}

@Test
Expand All @@ -303,7 +303,7 @@ public void testIsDefaultBranchedApplication_NullMetadata() {
Application nullMetadataApplication = new Application();
nullMetadataApplication.setGitApplicationMetadata(null);

assertFalse(isDefaultBranchedApplication(nullMetadataApplication));
assertFalse(isDefaultBranchedArtifact(nullMetadataApplication.getGitArtifactMetadata()));
}

@Test
Expand All @@ -317,7 +317,7 @@ public void testIsDefaultBranchedApplication_NullBranchName() {
metadata.setDefaultBranchName("main");
nullBranchNameApplication.setGitApplicationMetadata(metadata);

assertFalse(isDefaultBranchedApplication(nullBranchNameApplication));
assertFalse(isDefaultBranchedArtifact(nullBranchNameApplication.getGitArtifactMetadata()));
}

@Test
Expand All @@ -331,7 +331,7 @@ public void testIsDefaultBranchedApplication_NullDefaultBranchName() {
metadata.setDefaultBranchName(null);
nullDefaultBranchNameApplication.setGitApplicationMetadata(metadata);

assertFalse(isDefaultBranchedApplication(nullDefaultBranchNameApplication));
assertFalse(isDefaultBranchedArtifact(nullDefaultBranchNameApplication.getGitArtifactMetadata()));
}

@Test
Expand Down