Skip to content

Commit

Permalink
fix: added edge case fixes for consolidated api and publishing (#37399)
Browse files Browse the repository at this point in the history
## Description

Fixes #`Issue Number`  
_or_  
Fixes `Issue URL`
> [!WARNING]  
> _If no issue exists, please create an issue first, and check with the
maintainers if the issue is valid._

## Automation

/ok-to-test tags="@tag.Git"

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/11851164290>
> Commit: 3a76919
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11851164290&attempt=2"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Git`
> Spec:
> <hr>Fri, 15 Nov 2024 07:01:40 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [ ] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
- Enhanced handling of Git operations, including improved branch
management and artifact importation.
- Expanded analytics integration for tracking changes in branch
protection settings.

- **Bug Fixes**
- Improved error handling for various Git operations and
application/page retrieval scenarios.

- **Tests**
- Added new test cases for Git-related functionalities and improved
error handling in tests.
- Enhanced test coverage for the `getConsolidatedInfoForPageLoad`
method, particularly for feature branches.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
sondermanish authored Nov 15, 2024
1 parent 77bae7c commit 6883e49
Show file tree
Hide file tree
Showing 4 changed files with 245 additions and 19 deletions.
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())))
.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));
});
});
}
Expand Down
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
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
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();
}
}

0 comments on commit 6883e49

Please sign in to comment.