Skip to content

Commit

Permalink
chore: Modified default behaviour of consolidated API for missing url…
Browse files Browse the repository at this point in the history
… params and extra url params (#37274)

## Description
- PR for handling Consolidated API when url params are incomplete or
have extra params.

> [!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/11738054403>
> Commit: f6124cb
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11738054403&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Git`
> Spec:
> <hr>Fri, 08 Nov 2024 08:27:02 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**
- Improved logic for application and page retrieval based on branch
names and application modes.
- Enhanced error handling and logging for better user experience during
page loads.

- **Bug Fixes**
- Resolved issues with application ID fetching when branch names are
present.

- **Tests**
- Added new test cases to cover various scenarios related to page
loading in Git-connected and non-Git applications, ensuring robust
functionality across different contexts.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
sondermanish authored Nov 8, 2024
1 parent da6d497 commit 806c710
Show file tree
Hide file tree
Showing 2 changed files with 657 additions and 42 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.appsmith.server.datasources.base.DatasourceService;
import com.appsmith.server.domains.Application;
import com.appsmith.server.domains.ApplicationMode;
import com.appsmith.server.domains.GitArtifactMetadata;
import com.appsmith.server.domains.NewPage;
import com.appsmith.server.domains.Plugin;
import com.appsmith.server.dtos.ApplicationPagesDTO;
Expand Down Expand Up @@ -42,6 +43,7 @@
import reactor.core.observability.micrometer.Micrometer;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
import reactor.util.function.Tuple2;

import java.util.ArrayList;
import java.util.HashMap;
Expand Down Expand Up @@ -204,7 +206,7 @@ public Mono<ConsolidatedAPIResponseDTO> getConsolidatedInfoForPageLoad(
if (isBlank(basePageId)) {
return Mono.when(fetches).thenReturn(consolidatedAPIResponseDTO);
}
Mono<Application> branchedApplicationMonoCached;

Mono<String> baseApplicationIdMono = Mono.just("");
if (isViewMode) {
// Attempt to retrieve the application ID associated with the given base page ID from the cache.
Expand All @@ -213,40 +215,118 @@ public Mono<ConsolidatedAPIResponseDTO> getConsolidatedInfoForPageLoad(
.switchIfEmpty(Mono.just(""))
.cast(String.class);
}

baseApplicationIdMono = baseApplicationIdMono
.name(getQualifiedSpanName(APPLICATION_ID_FETCH_REDIS_SPAN, mode))
.tap(Micrometer.observation(observationRegistry))
.cache();

Mono<NewPage> branchedPageMonoCached = newPageService
.findByBranchNameAndBasePageIdAndApplicationMode(branchName, basePageId, mode)
.cache();
Mono<Tuple2<Application, NewPage>> applicationAndPageTupleMono = baseApplicationIdMono
.flatMap(cachedBaseApplicationId -> {
Mono<Application> applicationMono;
Mono<NewPage> branchedPageMonoCached;

branchedPageMonoCached = newPageService
.findByBranchNameAndBasePageIdAndApplicationMode(branchName, basePageId, mode)
.cache();

if (StringUtils.hasText(cachedBaseApplicationId)) {
// Handle non-empty baseApplicationId
applicationMono = applicationService.findByBaseIdBranchNameAndApplicationMode(
cachedBaseApplicationId, branchName, mode);
} else {
// Handle empty or null baseApplicationId
applicationMono = branchedPageMonoCached.flatMap(branchedPage ->
// Use the application ID to find the complete application details.
applicationService
.findByBranchedApplicationIdAndApplicationMode(
branchedPage.getApplicationId(), mode)
.flatMap(application -> {
if (isViewMode) {
// Update the cache with the new application’s base ID for future
// queries.
return cacheableRepositoryHelper
.fetchBaseApplicationId(basePageId, application.getBaseId())
.thenReturn(application)
.name(getQualifiedSpanName(
APPLICATION_ID_UPDATE_REDIS_SPAN, mode))
.tap(Micrometer.observation(observationRegistry));
}
return Mono.just(application);
}));
}

branchedApplicationMonoCached = baseApplicationIdMono.flatMap(cachedBaseApplicationId -> {
if (!StringUtils.hasText(cachedBaseApplicationId)) {
// Handle empty or null baseApplicationId
return branchedPageMonoCached.flatMap(branchedPage ->
// Use the application ID to find the complete application details.
applicationService
.findByBranchedApplicationIdAndApplicationMode(branchedPage.getApplicationId(), mode)
.flatMap(application -> {
if (isViewMode) {
// Update the cache with the new application’s base ID for future
// queries.
return cacheableRepositoryHelper
.fetchBaseApplicationId(basePageId, application.getBaseId())
.thenReturn(application)
.name(getQualifiedSpanName(APPLICATION_ID_UPDATE_REDIS_SPAN, mode))
.tap(Micrometer.observation(observationRegistry));
if (StringUtils.hasText(branchName)) {

// If in case the application is a non git connected application and the branch name url param
// is present, then we must default to the app without any branches.
return applicationMono.zipWith(branchedPageMonoCached).onErrorResume(error -> {
// This situation would arise if page or application is not returned.
// here we would land on error instead of empty because both apis which are being
// called errors out on empty returns.

log.info(
"application or page has for base pageId {} and branchName {} has not been found.",
basePageId,
branchName);
if (error instanceof AppsmithException) {
Mono<NewPage> basePageMono =
newPageService.findByBranchNameAndBasePageIdAndApplicationMode(
null, basePageId, mode);

return basePageMono.flatMap(basePage -> {
if (StringUtils.hasText(basePage.getBranchName())) {
// If the branch name is present then the application is git connected
// the error should be thrown.
// TODO: verify if branch name could be residue from old git connection
// Application metadata is absolute check for the same.
return Mono.error(error);
}
return Mono.just(application);
}));
} else {
// Handle non-empty baseApplicationId
return applicationService.findByBaseIdBranchNameAndApplicationMode(
cachedBaseApplicationId, branchName, mode);
}
});

return applicationService
.findByBranchedApplicationIdAndApplicationMode(
basePage.getApplicationId(), mode)
.zipWith(basePageMono)
.map(tuple2 -> {
log.info(
"The branchName url param should not be associated with application {} as this is not a git connected application",
tuple2.getT1().getId());
return tuple2;
});
});
}

return Mono.error(error);
});
}

return applicationMono.flatMap(application -> {
GitArtifactMetadata gitMetadata = application.getGitArtifactMetadata();

if (gitMetadata == null
|| gitMetadata.getDefaultBranchName().equals(gitMetadata.getBranchName())) {
return Mono.just(application).zipWith(branchedPageMonoCached);
}

// 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.

String defaultBranchName = gitMetadata.getDefaultBranchName();

return applicationService
.findByBaseIdBranchNameAndApplicationMode(application.getId(), defaultBranchName, mode)
.zipWith(newPageService.findByBranchNameAndBasePageIdAndApplicationMode(
defaultBranchName, basePageId, mode));
});
})
.cache();

Mono<NewPage> branchedPageMonoCached =
applicationAndPageTupleMono.map(Tuple2::getT2).cache();

Mono<Application> branchedApplicationMonoCached =
applicationAndPageTupleMono.map(Tuple2::getT1).cache();

branchedApplicationMonoCached = branchedApplicationMonoCached
.name(getQualifiedSpanName(APPLICATION_ID_SPAN, mode))
Expand Down Expand Up @@ -307,8 +387,9 @@ public Mono<ConsolidatedAPIResponseDTO> getConsolidatedInfoForPageLoad(

if (!isBlank(basePageId)) {
/* Get current page */
fetches.add(applicationPageService
.getPageAndMigrateDslByBranchAndBasePageId(basePageId, branchName, isViewMode, true)
fetches.add(branchedPageMonoCached
.flatMap(branchedPage -> applicationPageService.getPageAndMigrateDslByBranchedPageId(
branchedPage.getId(), isViewMode, true))
.as(this::toResponseDTO)
.doOnError(e -> log.error("Error fetching current page", e))
.doOnSuccess(consolidatedAPIResponseDTO::setPageWithMigratedDsl)
Expand All @@ -320,11 +401,9 @@ public Mono<ConsolidatedAPIResponseDTO> getConsolidatedInfoForPageLoad(
if (isViewMode) {
/* Get list of all actions of the page in view mode */
if (!isBlank(basePageId)) {
// When branchName is null, we don't need to fetch page from DB to derive pageId
// We can simply reuse the pageId that is passed by client to query actions
Mono<String> branchedPageIdMono = !StringUtils.hasText(branchName)
? Mono.just(basePageId)
: branchedPageMonoCached.map(NewPage::getId);
// For a git connected application the desired branch name may differ from the base if no
// branch name is provided hence, we would still need to check this.
Mono<String> branchedPageIdMono = branchedPageMonoCached.map(NewPage::getId);
fetches.add(branchedPageIdMono
.flatMap(branchedPageId -> newActionService
.getActionsForViewModeByPageId(branchedPageId)
Expand Down
Loading

0 comments on commit 806c710

Please sign in to comment.