chore: Migration for missing datasource configuration on default rest datasources for git connected app. #36203
Conversation
sondermanish
requested review from
sharat87,
abhvsn and
a team
as code owners
WalkthroughThis pull request introduces enhancements to the application JSON migration process within the Appsmith server. Key modifications include the addition of parameters to migration methods, improved handling of data source configurations, and updates to versioning. New helper classes and methods have been implemented to streamline migration logic, ensuring better compatibility and robustness. These changes aim to facilitate smoother migrations and improve the handling of application states during version transitions. Changes
Possibly related issues
Poem
Tip Announcements
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
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
|
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: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (8)
- app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaVersionsFallback.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/utils/JsonSchemaMigrationHelper.java (1 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java (2 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitServiceTest.java (3 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/migrations/JsonSchemaMigrationTest.java (2 hunks)
Additional comments not posted (13)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaVersionsFallback.java (1)
7-7: Approved server version increment.The update of
serverVersionfrom 9 to 10 is noted and approved. Please ensure that all dependent systems and migration scripts are updated accordingly to accommodate this version change.app/server/appsmith-server/src/test/java/com/appsmith/server/migrations/JsonSchemaMigrationTest.java (1)
100-100: Approved method signature change, suggest documenting new parameters.The updates to
migrateApplicationJsonToLatestSchemato include two additional parameters are approved. However, it's crucial to document the intended use of these parameters and ensure that they are adequately tested once their usage is implemented. Currently, passing them asnulldoes not test their functionality.Also applies to: 124-124
app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitServiceTest.java (3)
287-288: Approve the updated method signature usage.The test method
testAutoCommit_whenOnlyServerIsEligibleForMigration_commitSuccesscorrectly uses the updated method signature ofmigrateApplicationJsonToLatestSchemawith two additional string parameters. Ensure that these parameters are appropriately handled within the method definition.
575-576: Approve the updated method signature usage.The test method
testAutoCommit_whenServerIsRunningMigrationCallsAutocommitAgainOnSameBranch_ReturnsAutoCommitInProgresscorrectly uses the updated method signature ofmigrateApplicationJsonToLatestSchemawith two additional string parameters. Verify that these parameters are effectively utilized within the method definition.
649-650: Approve the updated method signature usage.The test method
testAutoCommit_whenServerIsRunningMigrationCallsAutocommitAgainOnDiffBranch_ReturnsAutoCommitLockedcorrectly uses the updated method signature ofmigrateApplicationJsonToLatestSchemawith two additional string parameters. It's crucial to check that these parameters are properly managed within the method itself.app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java (2)
1273-1294: Approve the addition ofdoesRestApiRequireMigration.The method
doesRestApiRequireMigrationis a useful addition for quickly determining if any actions in anApplicationJsonrequire migration due to missing data source configurations. Ensure that this method is thoroughly tested to confirm its accuracy in identifying actions that truly require migration.
1303-1325: Approve the addition ofsetDatasourceConfigDetailsInDefaultRestDatasourceForActions.The method
setDatasourceConfigDetailsInDefaultRestDatasourceForActionscorrectly handles the setting of data source configurations for actions with default REST data sources. Review the defaulting logic that sets the URL to an empty string to ensure it behaves as expected in scenarios where no valid URL is found.app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java (3)
9-9: Approved addition of JsonSchemaMigrationHelper.The addition of
JsonSchemaMigrationHelperas a dependency is a positive change, enhancing the handling of datasource configurations during migrations.
197-212: Approved addition of nonReactiveServerMigrationForImport method.The new method
nonReactiveServerMigrationForImportis a crucial addition for handling non-reactive migrations, especially in scenarios where datasource configurations are empty.It is recommended to conduct thorough testing of this method to ensure it handles all edge cases effectively.
66-106: Approved changes to method signature with suggestions for verification.The addition of
baseApplicationIdandbranchNameto the methodmigrateApplicationJsonToLatestSchemaenhances its functionality by making the migration process more context-aware.Please ensure that these new parameters are integrated correctly across all calls to this method in the system to avoid any potential issues.
Run the following script to verify the method usage:
Verification successful
Ensure comprehensive testing of new parameters in
migrateApplicationJsonToLatestSchema.The method
migrateApplicationJsonToLatestSchemahas been updated to includebaseApplicationIdandbranchName, and these changes are reflected in both the main codebase and test files. It is crucial to verify that all scenarios, including those where these parameters might benull, are thoroughly tested and managed to prevent potential issues.
- Confirm that all calls to this method in the codebase are updated to match the new signature.
- Ensure that the logic within the method can handle
nullvalues appropriately, as seen in the test cases.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `migrateApplicationJsonToLatestSchema` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type java -A 5 $'migrateApplicationJsonToLatestSchema'Length of output: 11513
app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java (2)
445-445: Method signature change correctly reflected in tests.The update to
migrateApplicationJsonToLatestSchemato include two additional string parameters is correctly reflected in the test cases. This change allows for more detailed testing scenarios, aligning with the PR's objectives to enhance the migration process.
577-579: Appropriate use ofMockito.anyString()for flexible testing.The use of
Mockito.anyString()in the test cases formigrateApplicationJsonToLatestSchemais appropriate. It allows the tests to cover a range of input scenarios, ensuring that the method is robust against various string inputs.app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (1)
365-366: Enhanced functionality with new parameters in migration method.The addition of
baseArtifactIdandbranchNameas parameters tomigrateApplicationJsonToLatestSchemaenhances the migration process by utilizing additional contextual information. This change is consistent with the PR's objectives and is correctly implemented in the methodreconstructArtifactExchangeJsonFromFilesInRepository.
| public Mono<ApplicationJson> addDatasourceConfigurationToDefaultRestApiActions( | ||
| String baseApplicationId, String branchName, ApplicationJson applicationJson) { | ||
|
|
||
| if (!StringUtils.hasText(baseApplicationId) || !StringUtils.hasText(branchName)) { | ||
| MigrationHelperMethods.migrateApplicationJsonToVersionTen(applicationJson, Map.of()); | ||
| return Mono.just(applicationJson); | ||
| } | ||
|
|
||
| Mono<Application> applicationMono = applicationService | ||
| .findByBranchNameAndBaseApplicationId(branchName, baseApplicationId, null) | ||
| .cache(); | ||
|
|
||
| return applicationMono | ||
| .flatMap(branchedApplication -> { | ||
| return newActionService | ||
| .findAllByApplicationIdAndViewMode( | ||
| branchedApplication.getId(), Boolean.FALSE, Optional.empty(), Optional.empty()) | ||
| .filter(action -> { | ||
| if (action.getUnpublishedAction() == null | ||
| || action.getUnpublishedAction().getDatasource() == null) { | ||
| return false; | ||
| } | ||
|
|
||
| boolean reverseFlag = StringUtils.hasText(action.getUnpublishedAction() | ||
| .getDatasource() | ||
| .getId()) | ||
| || !PluginConstants.DEFAULT_REST_DATASOURCE.equals(action.getUnpublishedAction() | ||
| .getDatasource() | ||
| .getName()); | ||
|
|
||
| return !reverseFlag; | ||
| }) | ||
| .collectMap(NewAction::getGitSyncId); | ||
| }) | ||
| .map(newActionMap -> { | ||
| MigrationHelperMethods.migrateApplicationJsonToVersionTen(applicationJson, newActionMap); | ||
| return applicationJson; | ||
| }) | ||
| .switchIfEmpty(Mono.defer(() -> Mono.just(applicationJson).map(applicationJson1 -> { | ||
| MigrationHelperMethods.migrateApplicationJsonToVersionTen(applicationJson, Map.of()); | ||
| return applicationJson; | ||
| }))); | ||
| } |
There was a problem hiding this comment.
Well-structured migration helper method, but consider enhancing documentation.
The method addDatasourceConfigurationToDefaultRestApiActions is well-implemented with appropriate checks and reactive patterns. However, the logic within the flatMap operation is quite complex, involving filtering and mapping over actions. It would be beneficial to add more detailed comments explaining the purpose and functionality of these operations to aid future maintainability.
| public static void migrateApplicationJsonToVersionTen( | ||
| ApplicationJson applicationJson, Map<String, NewAction> defaultDatasourceActionMap) { | ||
| List<NewAction> actionList = applicationJson.getActionList(); | ||
| if (CollectionUtils.isNullOrEmpty(actionList)) { | ||
| return; | ||
| } | ||
|
|
||
| for (NewAction action : actionList) { | ||
| if (action.getUnpublishedAction() == null | ||
| || action.getUnpublishedAction().getDatasource() == null) { | ||
| continue; | ||
| } | ||
|
|
||
| // Idea is to add datasourceConfiguration to existing DEFAULT_REST_DATASOURCE apis, | ||
| // for which the datasource configuration is missing | ||
| // the url would be set to empty string as right url is not present over here. | ||
| Datasource actionDatasource = action.getUnpublishedAction().getDatasource(); | ||
| if (actionDatasource.getDatasourceConfiguration() == null | ||
| && !org.springframework.util.StringUtils.hasText(actionDatasource.getId()) | ||
| && PluginConstants.DEFAULT_REST_DATASOURCE.equals(actionDatasource.getName())) { | ||
| setDatasourceConfigDetailsInDefaultRestDatasourceForActions(action, defaultDatasourceActionMap); | ||
| } | ||
| } |
There was a problem hiding this comment.
Approve the addition of migrateApplicationJsonToVersionTen.
The method migrateApplicationJsonToVersionTen is well-implemented to ensure that actions with default REST data sources are correctly configured during the migration process. Consider adding unit tests to cover scenarios where actions do not meet the criteria for migration to ensure that they are not inadvertently modified.
Would you like me to help in writing these unit tests?
| // All migration above version 9 is reactive | ||
| // TODO: make import flow migration reactive | ||
| return Mono.just(migrateServerSchema(appJson)) | ||
| .flatMap(migratedApplicationJson -> { |
There was a problem hiding this comment.
Can we make this an additional systematic path similar to the json file migrations? This is a legitimate case of a new data point that needs to be added to the repository, and, although is expected fewer times than file migrations, should be accounted for as well.
There was a problem hiding this comment.
So what you mean is that let's add a reactive migration set separately itself?
| * @param applicationJson | ||
| * @return | ||
| */ | ||
| private ApplicationJson nonReactiveServerMigrationForImport(ApplicationJson applicationJson) { |
There was a problem hiding this comment.
Why is this a separate method?
| } | ||
|
|
||
| Datasource actionDatasource = action.getUnpublishedAction().getDatasource(); | ||
| if (actionDatasource.getDatasourceConfiguration() == null |
There was a problem hiding this comment.
Would we need this to run for ds config with empty url as well? For apps like Akhil's that got affected by the previous version?
There was a problem hiding this comment.
we can run for that as well, but do we have any unintended consequences?
There was a problem hiding this comment.
Triggering it again for something that already exists on git is going to only get the correct value from DB. The extent of unintended consequence would be in case the user was supposed to truly see that as an uncommitted change but we went ahead and forcefully committed it. We can live with that, considering that at the current moment the data is already lost for the user, and there is no "right version" on the repo, except in their history.
Failed server tests
|
Failed server tests
|
… datasources for git connected app. (#36203) ## 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, @tag.ImportExport" ### 🔍 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/10789950948> > Commit: 69729ce > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10789950948&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git, @tag.ImportExport` > Spec: > <hr>Tue, 10 Sep 2024 10:07:42 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 application JSON migration process to support additional contextual parameters, improving accuracy and relevance. - Introduced a new helper class to facilitate datasource configuration during migrations. - **Bug Fixes** - Improved handling of incompatible schemas during migration, ensuring robust error management. - **Tests** - Adjusted test cases to accommodate changes in method signatures for migration processes, ensuring continued functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
… datasources for git connected app. (appsmithorg#36203) ## 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, @tag.ImportExport" ### 🔍 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/10789950948> > Commit: 69729ce > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10789950948&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git, @tag.ImportExport` > Spec: > <hr>Tue, 10 Sep 2024 10:07:42 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 application JSON migration process to support additional contextual parameters, improving accuracy and relevance. - Introduced a new helper class to facilitate datasource configuration during migrations. - **Bug Fixes** - Improved handling of incompatible schemas during migration, ensuring robust error management. - **Tests** - Adjusted test cases to accommodate changes in method signatures for migration processes, ensuring continued functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fixes #
Issue Numberor
Fixes
Issue URLWarning
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, @tag.ImportExport"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10789950948
Commit: 69729ce
Cypress dashboard.
Tags:
@tag.Git, @tag.ImportExportSpec:
Tue, 10 Sep 2024 10:07:42 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests