Skip to content
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

Jump to bottom

fix: added edge case fixes for consolidated api and publishing #37399

Merged
merged 1 commit into from
Nov 15, 2024
Merged

fix: added edge case fixes for consolidated api and publishing #37399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 13 additions & 14 deletions .../appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1487,18 +1487,15 @@ private Mono<? extends Artifact> checkoutRemoteBranch(Artifact baseArtifact, Str
// Get the latest artifact mono with all the changes
ArtifactExchangeJson artifactExchangeJson = tuple.getT1();
Artifact artifact = tuple.getT2();
return importService
.importArtifactInWorkspaceFromGit(
artifact.getWorkspaceId(),
artifact.getId(),
artifactExchangeJson,
finalRemoteBranchName)
.flatMap(artifact1 -> addAnalyticsForGitOperation(
AnalyticsEvents.GIT_CHECKOUT_REMOTE_BRANCH,
artifact1,
Boolean.TRUE.equals(
artifact1.getGitArtifactMetadata().getIsRepoPrivate())));
return importService.importArtifactInWorkspaceFromGit(
artifact.getWorkspaceId(), artifact.getId(), artifactExchangeJson, finalRemoteBranchName);
})
.flatMap(importedArtifact -> gitArtifactHelper.publishArtifact(importedArtifact, false))
.flatMap(publishedArtifact -> addAnalyticsForGitOperation(
AnalyticsEvents.GIT_CHECKOUT_REMOTE_BRANCH,
publishedArtifact,
Boolean.TRUE.equals(
publishedArtifact.getGitArtifactMetadata().getIsRepoPrivate())))
Comment on lines +1493 to +1498
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure error handling in reactive chain

Consider adding error handling to manage exceptions from publishArtifact and addAnalyticsForGitOperation to prevent potential unhandled errors.

.tag(GitConstants.GitMetricConstants.CHECKOUT_REMOTE, TRUE.toString())
.name(GitSpan.OPS_CHECKOUT_BRANCH)
.tap(Micrometer.observation(observationRegistry));
Expand Down Expand Up @@ -1870,9 +1867,11 @@ private Mono<GitPullDTO> pullAndRehydrateArtifact(Artifact baseArtifact, Artifac
gitPullDTO.setMergeStatus(status);
gitPullDTO.setArtifact(importedBranchedArtifact);

return this.commitArtifact(
commitDTO, baseArtifact, importedBranchedArtifact, false, false)
.thenReturn(gitPullDTO);
return gitArtifactHelper
.publishArtifact(importedBranchedArtifact, false)
.then(commitArtifact(
commitDTO, baseArtifact, importedBranchedArtifact, false, false)
.thenReturn(gitPullDTO));
Comment on lines +1870 to +1874
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle exceptions in reactive calls

Ensure that any errors from publishArtifact and commitArtifact are properly handled and propagated to avoid silent failures.

});
});
}
Expand Down
29 changes: 25 additions & 4 deletions ...th-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,14 +300,35 @@ public Mono<ConsolidatedAPIResponseDTO> getConsolidatedInfoForPageLoad(
});
}

return applicationMono.flatMap(application -> {
return applicationMono.zipWith(branchedPageMonoCached).flatMap(tuple2 -> {
Application application = tuple2.getT1();
NewPage branchedPage = tuple2.getT2();

GitArtifactMetadata gitMetadata = application.getGitArtifactMetadata();

if (gitMetadata == null
|| gitMetadata.getDefaultBranchName().equals(gitMetadata.getBranchName())) {
return Mono.just(application).zipWith(branchedPageMonoCached);
boolean isNotAGitApp = gitMetadata == null;
boolean isDefaultBranchNameAbsent =
isNotAGitApp || !StringUtils.hasText(gitMetadata.getDefaultBranchName());
boolean isBranchDefault = !isDefaultBranchNameAbsent
&& gitMetadata.getDefaultBranchName().equals(gitMetadata.getBranchName());

// This last check is specially for view mode, when a queried page which is not present
// in default branch, and cacheable repository refers to the base application
// from given page id. then the branched page may not belong to the base application
// hence a validation is required.
// This condition is always true for a non git app
boolean isPageFromSameApplication = application.getId().equals(branchedPage.getApplicationId());

if ((isNotAGitApp || isDefaultBranchNameAbsent || isBranchDefault)
&& (!isViewMode || isPageFromSameApplication)) {
return applicationMono.zipWith(branchedPageMonoCached);
}

log.info(
"ConsolidatedApi for page id {}, and application id {} has been queried without a branch url param",
branchedPage.getId(),
application.getId());

// The git connected application has not been queried with branch param,
// and the base branch is not same as the default branch.
// we need to find return the default branch from here.
Expand Down
4 changes: 3 additions & 1 deletion app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2025,8 +2025,10 @@ public void checkoutRemoteBranch_CustomThemeSetToDefaultAppAndRemoteBranch_AppAn
// names should be same as the name from the application JSON
assertThat(editModeThemeInBranchedApp.getDisplayName())
.isEqualTo(editModeCustomTheme.getDisplayName());
// While checking out from remote, it's also published.
// When published, the resources are copied from unpublished field to published filed.
assertThat(viewModeThemeInBranchedApp.getDisplayName())
.isEqualTo(viewModeCustomTheme.getDisplayName());
.isEqualTo(editModeCustomTheme.getDisplayName());
})
.verifyComplete();
}
Expand Down
204 changes: 204 additions & 0 deletions ...-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,16 @@
import com.appsmith.server.dtos.PageDTO;
import com.appsmith.server.dtos.PageNameIdDTO;
import com.appsmith.server.dtos.ProductAlertResponseDTO;
import com.appsmith.server.dtos.ResponseDTO;
import com.appsmith.server.dtos.UserProfileDTO;
import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException;
import com.appsmith.server.jslibs.base.CustomJSLibService;
import com.appsmith.server.newactions.base.NewActionService;
import com.appsmith.server.newpages.base.NewPageService;
import com.appsmith.server.plugins.base.PluginService;
import com.appsmith.server.repositories.ApplicationRepository;
import com.appsmith.server.repositories.CacheableRepositoryHelper;
import com.appsmith.server.repositories.NewPageRepository;
import com.appsmith.server.services.ApplicationPageService;
import com.appsmith.server.services.ConsolidatedAPIService;
Expand Down Expand Up @@ -65,6 +68,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
Expand Down Expand Up @@ -131,6 +135,9 @@ public class ConsolidatedAPIServiceImplTest {
@SpyBean
NewPageRepository mockNewPageRepository;

@Autowired
CacheableRepositoryHelper cacheableRepositoryHelper;

@Test
public void testErrorWhenModeIsNullAndPageIdAvailable() {
Mono<ConsolidatedAPIResponseDTO> consolidatedInfoForPageLoad =
Expand Down Expand Up @@ -1413,4 +1420,201 @@ public void testPageLoadResponseForEditModeWhenDefaultBranchIsDifferentFromDefau
})
.verifyComplete();
}

@Test
public void testPageLoadWhenPageFromFeatureBranchAndCacheableRepositoryReturnsBaseApplicationId() {
User sampleUser = new User();
when(mockSessionUserService.getCurrentUser()).thenReturn(Mono.just(sampleUser));

UserProfileDTO sampleUserProfileDTO = new UserProfileDTO();
sampleUserProfileDTO.setName("sampleUserProfileDTO");
when(mockUserService.buildUserProfileDTO(any())).thenReturn(Mono.just(sampleUserProfileDTO));

Map<String, Boolean> sampleFeatureFlagMap = new HashMap<>();
sampleFeatureFlagMap.put("sampleFeatureFlag", true);
when(mockUserDataService.getFeatureFlagsForCurrentUser()).thenReturn(Mono.just(sampleFeatureFlagMap));

when(mockUserDataService.updateLastUsedResourceAndWorkspaceList(any(), any(), any()))
.thenReturn(Mono.just(new UserData()));

Tenant sampleTenant = new Tenant();
sampleTenant.setDisplayName("sampleTenant");
when(mockTenantService.getTenantConfiguration()).thenReturn(Mono.just(sampleTenant));

ProductAlertResponseDTO sampleProductAlertResponseDTO = new ProductAlertResponseDTO();
sampleProductAlertResponseDTO.setTitle("sampleProductAlert");
when(mockProductAlertService.getSingleApplicableMessage())
.thenReturn(Mono.just(List.of(sampleProductAlertResponseDTO)));

final String WORKSPACE_ID = "sampleWorkspaceId";
final String DEFAULT_APPLICATION_ID = "defaultApplicationId";
final String BASE_APPLICATION_ID = "baseApplicationId";
final String FEATURE_APPLICATION_ID = "featureApplicationId";
final String DEFAULT_BRANCH = "defaultBranch";
final String BASE_BRANCH = "baseBranch";
final String FEATURE_PAGE_ID = "featurePageId";

ApplicationPagesDTO sampleApplicationPagesDTO = new ApplicationPagesDTO();
sampleApplicationPagesDTO.setWorkspaceId(WORKSPACE_ID);

// base metadata
GitArtifactMetadata baseMetadata = new GitArtifactMetadata();
baseMetadata.setBranchName(BASE_BRANCH);
baseMetadata.setDefaultBranchName(DEFAULT_BRANCH);
baseMetadata.setDefaultApplicationId(DEFAULT_APPLICATION_ID);

Application baseBranchApplication = new Application();
baseBranchApplication.setGitArtifactMetadata(baseMetadata);
baseBranchApplication.setWorkspaceId(WORKSPACE_ID);
baseBranchApplication.setId(BASE_APPLICATION_ID);

// caching the base application id for the test case.
cacheableRepositoryHelper
.fetchBaseApplicationId(FEATURE_PAGE_ID, BASE_APPLICATION_ID)
.block();

doReturn(Mono.just(baseBranchApplication))
.when(spyApplicationService)
.findByBaseIdBranchNameAndApplicationMode(eq(BASE_APPLICATION_ID), eq(null), any());

ApplicationPage defaultApplicationPage = new ApplicationPage();
defaultApplicationPage.setIsDefault(true);
defaultApplicationPage.setId("defaultPageId");

// default metadata
GitArtifactMetadata defaultMetadata = new GitArtifactMetadata();
defaultMetadata.setBranchName(DEFAULT_BRANCH);
defaultMetadata.setDefaultBranchName(DEFAULT_BRANCH);
defaultMetadata.setDefaultApplicationId(DEFAULT_APPLICATION_ID);

Application defaultBranchApplication = new Application();
defaultBranchApplication.setGitArtifactMetadata(defaultMetadata);
defaultBranchApplication.setWorkspaceId(WORKSPACE_ID);
defaultBranchApplication.setId(DEFAULT_APPLICATION_ID);
defaultBranchApplication.setPages(List.of(defaultApplicationPage));

doReturn(Mono.just(defaultBranchApplication))
.when(spyApplicationService)
.findByBaseIdBranchNameAndApplicationMode(eq(BASE_APPLICATION_ID), eq(DEFAULT_BRANCH), any());

NewPage featureBranchPage = new NewPage();
featureBranchPage.setId(FEATURE_PAGE_ID);
featureBranchPage.setApplicationId(FEATURE_APPLICATION_ID);
featureBranchPage.setUnpublishedPage(new PageDTO());

doReturn(Mono.just(featureBranchPage))
.when(spyNewPageService)
.findByBranchNameAndBasePageIdAndApplicationMode(eq(null), eq(FEATURE_PAGE_ID), any());

doReturn(Mono.just(new PageDTO()))
.when(spyApplicationPageService)
.getPageAndMigrateDslByBranchedPageId(anyString(), anyBoolean(), anyBoolean());

Theme sampleTheme = new Theme();
sampleTheme.setName("sampleTheme");
doReturn(Mono.just(sampleTheme)).when(spyThemeService).getApplicationTheme(anyString(), any());
doReturn(Flux.just(sampleTheme)).when(spyThemeService).getApplicationThemes(anyString());

CustomJSLib sampleCustomJSLib = new CustomJSLib();
sampleCustomJSLib.setName("sampleJSLib");
doReturn(Mono.just(List.of(sampleCustomJSLib)))
.when(spyCustomJSLibService)
.getAllJSLibsInContext(anyString(), any(), anyBoolean());

PageDTO samplePageDTO = new PageDTO();
samplePageDTO.setName("samplePageDTO");
doReturn(Mono.just(samplePageDTO))
.doReturn(Mono.just(samplePageDTO))
.when(spyApplicationPageService)
.getPageAndMigrateDslByBranchedPageId(anyString(), anyBoolean(), anyBoolean());

doReturn(Mono.just(samplePageDTO))
.doReturn(Mono.just(samplePageDTO))
.when(spyApplicationPageService)
.getPageDTOAfterMigratingDSL(any(), anyBoolean(), anyBoolean());

doReturn(Mono.just(samplePageDTO))
.doReturn(Mono.just(samplePageDTO))
.when(spyApplicationPageService)
.getPageDTOAfterMigratingDSL(any(), anyBoolean(), anyBoolean());

ActionDTO sampleActionDTO = new ActionDTO();
sampleActionDTO.setName("sampleActionDTO");
sampleActionDTO.setUpdatedAt(Instant.now());
doReturn(Flux.just(sampleActionDTO)).when(spyNewActionService).getUnpublishedActions(any(), anyBoolean());

ActionCollectionDTO sampleActionCollectionDTO = new ActionCollectionDTO();
sampleActionCollectionDTO.setName("sampleActionCollectionDTO");
doReturn(Flux.just(sampleActionCollectionDTO))
.when(spyActionCollectionService)
.getPopulatedActionCollectionsByViewMode(any(), anyBoolean());

PageNameIdDTO samplePageNameIdDTO = new PageNameIdDTO();
samplePageNameIdDTO.setName("samplePageNameIdDTO");
sampleApplicationPagesDTO.setPages(List.of(samplePageNameIdDTO));

Plugin samplePlugin = new Plugin();
samplePlugin.setName("samplePlugin");
samplePlugin.setId("samplePluginId");
samplePlugin.setPackageName("sample-plugin");
Plugin sampleRestApiPlugin = new Plugin();
sampleRestApiPlugin.setName("sampleRestApiPlugin");
sampleRestApiPlugin.setId("sampleRestApiPluginId");
sampleRestApiPlugin.setPackageName(REST_API_PLUGIN);
Plugin sampleGraphqlPlugin = new Plugin();
sampleGraphqlPlugin.setName("sampleGraphqlPlugin");
sampleGraphqlPlugin.setId("sampleGraphqlPluginId");
sampleGraphqlPlugin.setPackageName(GRAPHQL_PLUGIN);
Plugin sampleAiPlugin = new Plugin();
sampleAiPlugin.setName("sampleAiPlugin");
sampleAiPlugin.setId("sampleAiPluginId");
sampleAiPlugin.setPackageName(APPSMITH_AI_PLUGIN);
when(mockPluginService.getInWorkspace(anyString()))
.thenReturn(Flux.just(samplePlugin, sampleRestApiPlugin, sampleGraphqlPlugin, sampleAiPlugin));

Datasource sampleDatasource = new Datasource();
sampleDatasource.setName("sampleDatasource");
sampleDatasource.setPluginId("samplePluginId");
when(mockDatasourceService.getAllWithStorages(any())).thenReturn(Flux.just(sampleDatasource));

Map<String, Map<?, ?>> sampleFormConfig = new HashMap<>();
sampleFormConfig.put("key", Map.of());
when(mockPluginService.getFormConfig(anyString())).thenReturn(Mono.just(sampleFormConfig));

MockDataSet sampleMockDataSet = new MockDataSet();
sampleMockDataSet.setName("sampleMockDataSet");
MockDataDTO sampleMockDataDTO = new MockDataDTO();
sampleMockDataDTO.setMockdbs(List.of(sampleMockDataSet));
when(mockMockDataService.getMockDataSet()).thenReturn(Mono.just(sampleMockDataDTO));

Mono<ConsolidatedAPIResponseDTO> consolidatedInfoForPageLoad =
consolidatedAPIService.getConsolidatedInfoForPageLoad(
FEATURE_PAGE_ID, null, null, ApplicationMode.PUBLISHED);
StepVerifier.create(consolidatedInfoForPageLoad)
.assertNext(consolidatedAPIResponseDTO -> {
assertNotNull(consolidatedAPIResponseDTO);

ResponseDTO<ApplicationPagesDTO> pages = consolidatedAPIResponseDTO.getPages();
ResponseDTO<PageDTO> pageWithMigratedDsl = consolidatedAPIResponseDTO.getPageWithMigratedDsl();

assertNull(pages.getData());
assertNotNull(pages.getResponseMeta());
assertNotNull(pages.getResponseMeta().getError());

assertThat(pages.getResponseMeta().getError().getCode())
.isEqualTo(AppsmithError.NO_RESOURCE_FOUND.getAppErrorCode());
assertThat(pages.getResponseMeta().getError().getMessage())
.isEqualTo("Unable to find page featurePageId, defaultBranch");

assertNull(pageWithMigratedDsl.getData());
assertNotNull(pageWithMigratedDsl.getResponseMeta());
assertNotNull(pageWithMigratedDsl.getResponseMeta().getError());

assertThat(pageWithMigratedDsl.getResponseMeta().getError().getCode())
.isEqualTo(AppsmithError.NO_RESOURCE_FOUND.getAppErrorCode());
assertThat(pageWithMigratedDsl.getResponseMeta().getError().getMessage())
.isEqualTo("Unable to find page featurePageId, defaultBranch");
})
.verifyComplete();
}
}
Loading