From 3a76919cab6a000ae1d2dcac76cbaf6afee33aa7 Mon Sep 17 00:00:00 2001 From: sondermanish Date: Fri, 15 Nov 2024 11:30:13 +0530 Subject: [PATCH] added edge case fixes for consolidated api and publishing --- .../git/common/CommonGitServiceCEImpl.java | 27 ++- .../ce/ConsolidatedAPIServiceCEImpl.java | 29 ++- .../server/git/CommonGitServiceCETest.java | 4 +- .../ce/ConsolidatedAPIServiceImplTest.java | 204 ++++++++++++++++++ 4 files changed, 245 insertions(+), 19 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java index 098f8c6667a..93c08cdf248 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java @@ -1487,18 +1487,15 @@ private Mono 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()))) .tag(GitConstants.GitMetricConstants.CHECKOUT_REMOTE, TRUE.toString()) .name(GitSpan.OPS_CHECKOUT_BRANCH) .tap(Micrometer.observation(observationRegistry)); @@ -1870,9 +1867,11 @@ private Mono 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)); }); }); } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java index ec39de3168a..71c417afaf1 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java @@ -300,14 +300,35 @@ public Mono 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. diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java index 31d88eaa56b..307014c1735 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java @@ -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(); } diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java index 288cef9864c..ccc70eb9315 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java @@ -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; @@ -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; @@ -131,6 +135,9 @@ public class ConsolidatedAPIServiceImplTest { @SpyBean NewPageRepository mockNewPageRepository; + @Autowired + CacheableRepositoryHelper cacheableRepositoryHelper; + @Test public void testErrorWhenModeIsNullAndPageIdAvailable() { Mono consolidatedInfoForPageLoad = @@ -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 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> 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 consolidatedInfoForPageLoad = + consolidatedAPIService.getConsolidatedInfoForPageLoad( + FEATURE_PAGE_ID, null, null, ApplicationMode.PUBLISHED); + StepVerifier.create(consolidatedInfoForPageLoad) + .assertNext(consolidatedAPIResponseDTO -> { + assertNotNull(consolidatedAPIResponseDTO); + + ResponseDTO pages = consolidatedAPIResponseDTO.getPages(); + ResponseDTO 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(); + } }