chore: autocommit migration for annotation and embedded datasource changes. #36261
Conversation
WalkthroughThe pull request introduces substantial modifications to the migration logic for application JSON schemas, specifically updating the handling of schema versions and datasource migrations. Key changes include restructuring the flow of the Changes
Possibly related PRs
Poem
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Additional comments not posted (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
...server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java
Show resolved
Hide resolved
![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 (2)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java (1)
40-40: Nice addition of thesshProxyEnabledfield! 🙌The
sshProxyEnabledfield of typeBooleanis a great complement to thesshProxyfield. It provides a convenient way to control whether the SSH proxy is enabled or not, allowing for more flexible configuration options.The use of the
@JsonViewannotation withViews.PublicandFromRequestensures that the field is properly included in the relevant JSON serialization views.As a small suggestion, consider adding a default value for the
sshProxyEnabledfield, such asfalse, to provide a clear initial state when the field is not explicitly set.@JsonView({Views.Public.class, FromRequest.class}) -Boolean sshProxyEnabled; +Boolean sshProxyEnabled = false;app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java (1)
Line range hint
1326-1368: Great job implementing thesetDatasourceConfigDetailsInDefaultRestDatasourceForActionsmethod! It provides a way to set the datasource configuration details for actions with default REST datasources.The method serves a specific purpose and handles different scenarios effectively:
- It takes a
NewActionobject and a map ofgitSyncIdtoNewActionas parameters.- It retrieves the action's unpublished action and datasource.
- It checks if the
defaultDatasourceActionMapcontains an entry for the action'sgitSyncId.- If an entry exists, it retrieves the corresponding action from the map and sets the datasource configuration URL from that action, if the datasource configuration is not null.
- If no entry exists or the datasource configuration from the map is null, it sets the URL to an empty string.
The method includes null checks to avoid potential null pointer exceptions and ensures that the datasource configuration is set only if it is not null to prevent uncommitted changes. This approach helps maintain data integrity and avoids unintended modifications.
To further improve the method, consider handling the case when the datasource configuration from the map is null. Instead of returning early, you could set the URL to an empty string, similar to the case when no entry exists in the map. This would ensure consistent behavior and avoid leaving the datasource configuration unset. Here's an example:
if (actionFromMap.getUnpublishedAction() == null || actionFromMap.getUnpublishedAction().getDatasource() == null || actionFromMap.getUnpublishedAction().getDatasource().getDatasourceConfiguration() == null) { datasourceConfiguration.setUrl(""); actionDatasource.setDatasourceConfiguration(datasourceConfiguration); return; }This way, the method would always set the datasource configuration, either with the URL from the map or an empty string, providing a more predictable outcome.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/utils/JsonSchemaMigrationHelper.java (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/utils/JsonSchemaMigrationHelper.java
Additional comments not posted (5)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java (1)
37-37: Great work adding thesshProxyfield! 👍The addition of the
sshProxyfield of typeSSHConnectionis a valuable enhancement to theDatasourceConfigurationclass. It allows the class to handle SSH connection configurations, which can be important for managing data sources securely.The use of the
@JsonViewannotation withViews.PublicandFromRequestensures that the field is properly included in the relevant JSON serialization views.Keep up the excellent work in expanding the functionality of the class!
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java (4)
1240-1263: Great job refactoring theconditionForDefaultRestDatasourcemethod! The changes make the code more robust and readable.The key improvements are:
- Adding null safety checks for the action's unpublished action and datasource to prevent potential null pointer exceptions.
- Splitting the logic to determine if an action is a default REST datasource into two separate checks:
- A certain check that verifies if the datasource ID is absent and the name matches
DEFAULT_REST_DATASOURCE.- A probable check that assesses whether the action's plugin ID corresponds to either the REST API or GraphQL plugin.
- Returning true if either the certain check or the probable check passes, providing flexibility in identifying default REST datasources.
These changes enhance the method's reliability and maintainability. Well done!
Line range hint
1265-1274: Excellent addition of theconditionForDefaultRestDatasourceMigrationmethod! It encapsulates the logic to determine if an action requires migration for a default REST datasource.The method serves a clear purpose and is implemented effectively:
- It calls the
conditionForDefaultRestDatasourcemethod to determine if the action is a default REST datasource.- It checks if the action's datasource configuration is missing or if the URL is not set.
- It returns true if the action is a default REST datasource and the datasource configuration or URL is missing.
The method follows a clear naming convention and is well-structured, making it easy to understand its functionality. This addition enhances the code's modularity and readability. Great work!
Line range hint
1277-1293: Great job implementing themigrateApplicationJsonToVersionTenmethod! It provides a clear and structured approach to migrate the application JSON to version 10.The method serves a specific purpose and is implemented effectively:
- It takes an
ApplicationJsonobject and a map ofgitSyncIdtoNewActionas parameters.- It iterates over the action list in the application JSON.
- For each action, it checks if the action requires migration using the
conditionForDefaultRestDatasourceMigrationmethod.- If the action requires migration, it calls the
setDatasourceConfigDetailsInDefaultRestDatasourceForActionsmethod to set the datasource configuration details.The method is well-organized and follows a logical flow, making it easy to understand and maintain. It leverages helper methods to keep the code modular and readable. This addition streamlines the migration process for version 10. Excellent work!
Line range hint
1295-1315: Excellent addition of thedoesRestApiRequireMigrationmethod! It provides a convenient way to determine if the application JSON requires migration for REST APIs.The method serves a clear purpose and is implemented effectively:
- It takes an
ApplicationJsonobject as a parameter.- It iterates over the action list in the application JSON.
- For each action, it checks if the action requires migration using the
conditionForDefaultRestDatasourceMigrationmethod.- If any action requires migration, it returns
true. Otherwise, it returnsfalse.The method is well-structured and follows a clear logic, making it easy to understand and use. It leverages the
conditionForDefaultRestDatasourceMigrationmethod to identify actions that require migration, promoting code reuse and modularity. This addition simplifies the process of checking if migration is needed for REST APIs. Great job!
sondermanish
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10847408262. |
|
Deploy-Preview-URL: https://ce-36261.dp.appsmith.com |
...server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java
Show resolved
Hide resolved
| applicationJsonWithMigration10.setServerSchemaVersion(10); | ||
| return applicationJsonWithMigration10; | ||
| }); | ||
| if (migratedApplicationJson.getServerSchemaVersion() == 9) { |
There was a problem hiding this comment.
@sondermanish Can you please add a comment to explain what we are doing here? I'm suggesting this because we are using magic numbers for version 9 and version 10. This is hard to understand for another reader.
There was a problem hiding this comment.
Added comments as suggested.
sondermanish
added
ok-to-test
…anges. (appsmithorg#36261) ## Description - Added autocommit migration to avoid uncommited changes on some of the applications. Fixes #`Issue Number` _or_ Fixes `Issue URL` ## Automation /ok-to-test tags="@tag.Git" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/10878546679> > Commit: 0013cde > Workflow: `PR Automation test suite` > Tags: `@tag.Git` > Spec: `` > <hr>Mon, 16 Sep 2024 06:20: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** - Updated the server version from 10 to 11, enhancing compatibility and functionality related to JSON schema handling. - Improved logic for default REST datasource migrations, making it more robust and accessible. - Added support for SSH connection configurations in datasource management. - **Bug Fixes** - Added null checks to prevent potential errors during datasource migrations, enhancing overall reliability. - **Refactor** - Streamlined filtering logic for actions, improving code readability and maintainability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fixes #
Issue Numberor
Fixes
Issue URLAutomation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10878546679
Commit: 0013cde
Cypress dashboard.
Tags:
@tag.GitSpec:
Mon, 16 Sep 2024 06:42:16 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor