chore: Modified default behaviour of consolidated API for missing url params and extra url params #37274
Conversation
WalkthroughThe changes in this pull request primarily enhance the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
sondermanish
added
Git Product
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (1)
281-283: Address the TODO regarding branch name residuePlease resolve the TODO comment about verifying if the branch name could be residue from an old Git connection.
Would you like assistance in addressing this TODO item?
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java (2)
883-1069: Consider refactoring duplicated setup codeThe test method
testPageLoadResponseForViewMode_whenBranchNameIsPresentInNonGitAppcontains setup code that is similar to other test methods in this class. Extracting common code into helper methods or using@BeforeEachcould improve maintainability and reduce code duplication.
1072-1419: Refactor to eliminate repetitive code in testThe test method
testPageLoadResponseForEditModeWhenDefaultBranchIsDifferentFromDefaultduplicates setup logic found in other tests. Consider refactoring shared code into common setup methods to enhance readability and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java(6 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java(4 hunks)
🧰 Additional context used
📓 Learnings (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java (2)
Learnt from: sumitsum
PR: appsmithorg/appsmith#29875
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0
Timestamp: 2023-12-27T21:27:19.887Z
Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
Learnt from: sumitsum
PR: appsmithorg/appsmith#29875
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0
Timestamp: 2024-10-08T15:32:34.114Z
Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (5)
11-11: Importing GitArtifactMetadata
The addition of GitArtifactMetadata import is appropriate for handling Git-related functionality.
46-46: Importing Tuple2
Including Tuple2 is necessary for encapsulating Application and NewPage objects.
224-257: Refactored application and page retrieval logic
The use of Tuple2<Application, NewPage> improves code clarity and streamlines data handling.
390-392: Optimized page retrieval with DSL migration
Fetching and migrating the DSL in getPageAndMigrateDslByBranchAndBasePageId enhances efficiency.
404-406: Verify action fetching with correct branch handling
Ensure that actions are fetched correctly when the branch name may differ from the base in Git-connected applications.
Run the following script to confirm actions are associated with the correct branch:
✅ Verification successful
Let me gather more information about the branch handling logic.
Branch handling for action fetching is correctly implemented
The code correctly handles branch-specific action fetching through multiple safeguards:
- Validates branch name presence and Git connection status
- Falls back to default branch when needed via
defaultBranchName - Uses cached branched page ID to fetch actions for the correct branch context
- Handles error cases for non-Git applications attempting to use branches
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that actions are fetched for the correct branch in view mode.
# Test: Search for actions linked to the specified branch. Expect: Actions should belong to the correct branch.
rg -A 5 $'getActionsForViewModeByPageId'
Length of output: 4946
Script:
#!/bin/bash
# Search for branch-related logic in action fetching
rg -A 10 "branchedPageId|defaultBranchName|branchName" app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
# Check implementation of action service for branch handling
ast-grep --pattern 'class NewActionServiceCEImpl {
$$$
getActionsForViewModeByPageId($$$) {
$$$
}
$$$
}'
Length of output: 5980
sondermanish
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (3)
224-324: Consider breaking down the complex application and page retrieval logic into smaller methods.The application and page retrieval logic is quite complex and handles multiple scenarios. Consider extracting the logic into smaller, more focused methods to improve readability and maintainability.
+ private Mono<Tuple2<Application, NewPage>> fetchApplicationAndPage( + String cachedBaseApplicationId, + String branchName, + String basePageId, + ApplicationMode mode + ) { + Mono<Application> applicationMono = fetchApplication(cachedBaseApplicationId, branchName, mode); + Mono<NewPage> branchedPageMono = fetchBranchedPage(branchName, basePageId, mode); + return applicationMono.zipWith(branchedPageMono); + }
281-282: Address the TODO comment regarding branch name verification.The TODO comment suggests a potential issue with residual branch names from old Git connections. This should be addressed to prevent potential bugs.
Would you like me to help create a GitHub issue to track this technical debt?
404-406: Consider adding debug logging for branch name resolution.For Git-connected applications where the desired branch name differs from the base, adding debug logging would help in troubleshooting issues.
+ log.debug( + "Resolving branch name for Git-connected application. Desired: {}, Base: {}", + branchName, + application.getGitArtifactMetadata().getDefaultBranchName() + );app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java (1)
1071-1415: LGTM! Thorough test for edit mode with different default branch.The test comprehensively covers the scenario where the default branch differs from the expected default, including Git metadata handling.
Consider extracting common setup code (user, profile, feature flags, etc.) into a shared setup method to reduce duplication across test methods.
+private void setupCommonMocks() { + 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)); + + // ... other common setup code +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java(6 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java(7 hunks)
🧰 Additional context used
📓 Learnings (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (2)
Learnt from: sumitsum
PR: appsmithorg/appsmith#29875
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0
Timestamp: 2023-12-27T21:27:19.887Z
Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
Learnt from: sumitsum
PR: appsmithorg/appsmith#29875
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0
Timestamp: 2024-10-08T15:32:34.114Z
Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java (2)
Learnt from: sumitsum
PR: appsmithorg/appsmith#29875
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0
Timestamp: 2023-12-27T21:27:19.887Z
Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
Learnt from: sumitsum
PR: appsmithorg/appsmith#29875
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0
Timestamp: 2024-10-08T15:32:34.114Z
Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (1)
259-301: Add unit tests for non-Git connected application scenarios.
The logic for handling branch names in non-Git connected applications is complex and requires thorough testing.
#!/bin/bash
# Check if there are existing tests for non-Git connected applications
rg -A 5 "testPageLoadResponseForViewMode_whenBranchNameIsPresentInNonGitApp" app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java (2)
11-13: LGTM!
The new imports are required for the Git-related test cases.
882-1069: LGTM! Comprehensive test coverage for view mode.
The test thoroughly validates the consolidated API response when a branch name is provided in a non-Git application context.
| fetches.add(branchedPageMonoCached | ||
| .flatMap(branchedPage -> applicationPageService.getPageAndMigrateDslByBranchedPageId( | ||
| branchedPage.getId(), isViewMode, true)) |
There was a problem hiding this comment.
💡 Codebase verification
Both old and new method names are present - needs cleanup
The old method getPageAndMigrateDslByBranchAndBasePageId is still present in:
ApplicationPageServiceCE.javainterfaceApplicationPageServiceCEImpl.javaimplementation- Test files
While the new method getPageAndMigrateDslByBranchedPageId is being used in:
PageControllerCE.javaConsolidatedAPIServiceCEImpl.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
| @@ -262,7 +263,7 @@ public void testPageLoadResponseForViewMode() { | |||
| samplePageDTO.setName("samplePageDTO"); | |||
| doReturn(Mono.just(samplePageDTO)) | |||
| .when(spyApplicationPageService) | |||
| .getPageAndMigrateDslByBranchAndBasePageId(anyString(), anyString(), anyBoolean(), anyBoolean()); | |||
| .getPageAndMigrateDslByBranchedPageId(anyString(), anyBoolean(), anyBoolean()); | |||
There was a problem hiding this comment.
💡 Codebase verification
Method name inconsistency found - needs update across codebase
The old method name getPageAndMigrateDslByBranchAndBasePageId is still in use in several files while the new name getPageAndMigrateDslByBranchedPageId is also present, indicating an incomplete refactoring:
ApplicationPageServiceCE.java: Both old and new method signatures existApplicationPageServiceCEImpl.java: Both implementations presentNewPageServiceTest.java: Still using old method nameApplicationPageServiceTest.java: Multiple occurrences of old method name
🔗 Analysis chain
Verify method name consistency across codebase.
The method name has been updated from getPageAndMigrateDslByBranchAndBasePageId to getPageAndMigrateDslByBranchedPageId.
Also applies to: 436-436, 458-458
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining occurrences of the old method name
# Test: Search for old method name. Expect: No occurrences.
rg "getPageAndMigrateDslByBranchAndBasePageId"
# Test: Verify all occurrences use the new method name
rg "getPageAndMigrateDslByBranchedPageId"
Length of output: 3615
github-actions
bot
added
App Navigation
github-actions
bot
added
Bug
… params and extra url params (appsmithorg#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 -->
Description
Fixes #36537, #37316
Automation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11738054403
Commit: f6124cb
Cypress dashboard.
Tags:
@tag.GitSpec:
Fri, 08 Nov 2024 08:27:02 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests