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 all commits
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
3 changes: 3 additions & 0 deletions ...r/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.appsmith.external.models.ActionDTO;
import com.appsmith.external.models.CreatorContextType;
import com.appsmith.server.acl.AclPermission;
import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.domains.ActionCollection;
import com.appsmith.server.domains.NewPage;
import com.appsmith.server.dtos.ActionCollectionDTO;
Expand Down Expand Up @@ -81,4 +82,6 @@ Flux<ActionCollection> findAllActionCollectionsByContextIdAndContextTypeAndViewM
Mono<Void> bulkValidateAndUpdateActionCollectionInRepository(List<ActionCollection> actionCollectionList);

Mono<Void> saveLastEditInformationInParent(ActionCollectionDTO actionCollectionDTO);

Flux<ActionCollection> findByArtifactIdAndArtifactType(String artifactId, ArtifactType artifactType);
}
10 changes: 7 additions & 3 deletions ...c/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.appsmith.server.acl.AclPermission;
import com.appsmith.server.acl.PolicyGenerator;
import com.appsmith.server.applications.base.ApplicationService;
import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.constants.FieldName;
import com.appsmith.server.domains.ActionCollection;
import com.appsmith.server.domains.NewAction;
Expand Down Expand Up @@ -120,9 +121,7 @@ protected void setGitSyncIdInActionCollection(ActionCollection collection) {

@Override
public Flux<ActionCollection> saveAll(List<ActionCollection> collections) {
collections.forEach(collection -> {
setGitSyncIdInActionCollection(collection);
});
collections.forEach(this::setGitSyncIdInActionCollection);
return repository.saveAll(collections);
}

Expand Down Expand Up @@ -612,4 +611,9 @@ public Mono<Void> saveLastEditInformationInParent(ActionCollectionDTO actionColl
// Do nothing as this is already taken care for JS objects in the context of page
return Mono.empty().then();
}

@Override
public Flux<ActionCollection> findByArtifactIdAndArtifactType(String artifactId, ArtifactType artifactType) {
return repository.findByApplicationId(artifactId);
}
Comment on lines +615 to +618
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 7, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Implementation needs improvement

The current implementation has several issues:

  1. The artifactType parameter is unused
  2. Missing null checks for parameters
  3. No error handling for invalid inputs

Consider this implementation:

 @Override
 public Flux<ActionCollection> findByArtifactIdAndArtifactType(String artifactId, ArtifactType artifactType) {
+    if (artifactId == null) {
+        return Flux.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ARTIFACT_ID));
+    }
+    if (artifactType == null) {
+        return Flux.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ARTIFACT_TYPE));
+    }
+    // Handle different artifact types if needed
+    if (artifactType == ArtifactType.APPLICATION) {
         return repository.findByApplicationId(artifactId);
+    }
+    return Flux.error(new AppsmithException(AppsmithError.UNSUPPORTED_OPERATION));
 }
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Override
public Flux<ActionCollection> findByArtifactIdAndArtifactType(String artifactId, ArtifactType artifactType) {
return repository.findByApplicationId(artifactId);
}
@Override
public Flux<ActionCollection> findByArtifactIdAndArtifactType(String artifactId, ArtifactType artifactType) {
if (artifactId == null) {
return Flux.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ARTIFACT_ID));
}
if (artifactType == null) {
return Flux.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ARTIFACT_TYPE));
}
// Handle different artifact types if needed
if (artifactType == ArtifactType.APPLICATION) {
return repository.findByApplicationId(artifactId);
}
return Flux.error(new AppsmithException(AppsmithError.UNSUPPORTED_OPERATION));
}

}
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
44 changes: 13 additions & 31 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 @@ -20,6 +20,7 @@
import com.appsmith.server.helpers.CollectionUtils;
import com.appsmith.server.helpers.CommonGitFileUtils;
import com.appsmith.server.helpers.GitPrivateRepoHelper;
import com.appsmith.server.helpers.GitUtils;
import com.appsmith.server.migrations.JsonSchemaVersions;
import com.appsmith.server.newactions.base.NewActionService;
import com.appsmith.server.newpages.base.NewPageService;
Expand Down Expand Up @@ -258,41 +259,22 @@ public Mono<Application> disconnectEntitiesOfBaseArtifact(Artifact baseArtifact)
// will be deleted
Flux<NewPage> newPageFlux = Flux.fromIterable(baseApplication.getPages())
.flatMap(page -> newPageService.findById(page.getId(), null))
.map(newPage -> {
newPage.setBaseId(newPage.getId());
newPage.setRefType(null);
newPage.setRefName(null);
return newPage;
})
.map(GitUtils::resetEntityReferences)
.collectList()
.flatMapMany(newPageService::saveAll)
.cache();

Flux<NewAction> newActionFlux = newPageFlux.flatMap(newPage -> {
return newActionService
.findByPageId(newPage.getId(), Optional.empty())
.map(newAction -> {
newAction.setBaseId(newAction.getId());
newAction.setRefType(null);
newAction.setRefName(null);
return newAction;
})
.collectList()
.flatMapMany(newActionService::saveAll);
});

Flux<ActionCollection> actionCollectionFlux = newPageFlux.flatMap(newPage -> {
return actionCollectionService
.findByPageId(newPage.getId())
.map(actionCollection -> {
actionCollection.setBaseId(actionCollection.getId());
actionCollection.setRefType(null);
actionCollection.setRefName(null);
return actionCollection;
})
.collectList()
.flatMapMany(actionCollectionService::saveAll);
});
Flux<NewAction> newActionFlux = newPageFlux.flatMap(newPage -> newActionService
.findByPageId(newPage.getId(), Optional.empty())
.map(GitUtils::resetEntityReferences)
.collectList()
.flatMapMany(newActionService::saveAll));

Flux<ActionCollection> actionCollectionFlux = newPageFlux.flatMap(newPage -> actionCollectionService
.findByPageId(newPage.getId())
.map(GitUtils::resetEntityReferences)
.collectList()
.flatMapMany(actionCollectionService::saveAll));

return Flux.merge(actionCollectionFlux, newActionFlux).then(Mono.just(baseApplication));
}
Expand Down
40 changes: 23 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,6 @@
package com.appsmith.server.helpers;

import com.appsmith.server.domains.Application;
import com.appsmith.external.models.RefAwareDomain;
import com.appsmith.server.domains.GitArtifactMetadata;
import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException;
Expand Down Expand Up @@ -141,28 +141,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 Expand Up @@ -193,4 +191,12 @@ public static boolean isAutoCommitEnabled(GitArtifactMetadata gitArtifactMetadat
return gitArtifactMetadata.getAutoCommitConfig() == null
|| gitArtifactMetadata.getAutoCommitConfig().getEnabled();
}

// Helper method to reset baseId, refType, and refName for entities
public static <T extends RefAwareDomain> T resetEntityReferences(T entity) {
entity.setBaseId(entity.getId());
entity.setRefType(null);
entity.setRefName(null);
return entity;
}
}
11 changes: 4 additions & 7 deletions ...mith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1226,23 +1226,20 @@ private Mono<DatasourceContext> getRemoteDatasourceContext(Plugin plugin, Dataso
public Mono<NewAction> save(NewAction action) {
// gitSyncId will be used to sync resource across instances
if (action.getGitSyncId() == null) {
action.setGitSyncId(action.getApplicationId() + "_" + Instant.now().toString());
setGitSyncIdInNewAction(action);
}

return sanitizeAction(action).flatMap(sanitizedAction -> repository.save(sanitizedAction));
return sanitizeAction(action).flatMap(repository::save);
}

@Override
public Flux<NewAction> saveAll(List<NewAction> actions) {
actions.stream()
.filter(action -> action.getGitSyncId() == null)
.forEach(action -> action.setGitSyncId(
action.getApplicationId() + "_" + Instant.now().toString()));
actions.stream().filter(action -> action.getGitSyncId() == null).forEach(this::setGitSyncIdInNewAction);

return Flux.fromIterable(actions)
.flatMap(this::sanitizeAction)
.collectList()
.flatMapMany(actionList -> repository.saveAll(actionList));
.flatMapMany(repository::saveAll);
}

@Override
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
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 6, 2025

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