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

chore: Modified default behaviour of consolidated API for missing url params and extra url params #37274

Merged
merged 2 commits into from
Nov 8, 2024
Merged

chore: Modified default behaviour of consolidated API for missing url params and extra url params #37274

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
2de6e5e
added consolidated api changes
sondermanish Nov 7, 2024
f6124cb
changed method to fetch applicationPage, now using branchedId directly
sondermanish Nov 8, 2024
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
149 changes: 114 additions & 35 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 @@ -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))
Comment on lines +390 to +392
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Both old and new method names are present - needs cleanup

The old method getPageAndMigrateDslByBranchAndBasePageId is still present in:

  • ApplicationPageServiceCE.java interface
  • ApplicationPageServiceCEImpl.java implementation
  • Test files

While the new method getPageAndMigrateDslByBranchedPageId is being used in:

  • PageControllerCE.java
  • ConsolidatedAPIServiceCEImpl.java
  • Test files

Please remove the old method and update all its references to use the new method name.

🔗 Analysis chain

Verify the impact of the method name change.

The method name has changed from getPageAndMigrateDslByBranchAndBasePageId to getPageAndMigrateDslByBranchedPageId. Ensure all callers are updated.

The results show that the old method is still being used in test files and is defined in the CE (Community Edition) interface and implementation. Let's verify the new method to understand the complete picture before making a conclusion.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining usages of the old method name
rg "getPageAndMigrateDslByBranchAndBasePageId"

Length of output: 1461

Script:

#!/bin/bash
# Let's check the interface and implementation of the new method name
ast-grep --pattern 'getPageAndMigrateDslByBranchedPageId'

# Also check for any references to the new method name
rg "getPageAndMigrateDslByBranchedPageId"

Length of output: 4556

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