From 71261b1e6e5dae84c3e69a345451bb3b551a4afe Mon Sep 17 00:00:00 2001 From: Manish Kumar <107841575+sondermanish@users.noreply.github.com> Date: Mon, 16 Sep 2024 13:24:08 +0530 Subject: [PATCH] chore: autocommit migration for annotation and embedded datasource changes. (#36261) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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" ### :mag: Cypress test results > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: > Commit: 0013cdec8894922d3cae386a8d8d7b8aebc3837d > Workflow: `PR Automation test suite` > Tags: `@tag.Git` > Spec: `` >
Mon, 16 Sep 2024 06:20:40 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## 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. --- .../models/DatasourceConfiguration.java | 2 + .../migrations/JsonSchemaMigration.java | 52 +++++++++++-------- .../JsonSchemaVersionsFallback.java | 2 +- .../migrations/MigrationHelperMethods.java | 50 ++++++++++++++---- .../utils/JsonSchemaMigrationHelper.java | 29 ++++++++--- 5 files changed, 95 insertions(+), 40 deletions(-) diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java index 5a81b66b730..637879eb75b 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java @@ -34,8 +34,10 @@ public class DatasourceConfiguration implements AppsmithDomain { @JsonView({Views.Public.class, FromRequest.class}) AuthenticationDTO authentication; + @JsonView({Views.Public.class, FromRequest.class}) SSHConnection sshProxy; + @JsonView({Views.Public.class, FromRequest.class}) Boolean sshProxyEnabled; @JsonView({Views.Public.class, FromRequest.class, Git.class}) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java index 03d543dd3d0..badec22d19e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java @@ -67,28 +67,38 @@ public Mono migrateApplicationJsonToLatestSchema( // TODO: make import flow migration reactive return Mono.just(migrateServerSchema(appJson)) .flatMap(migratedApplicationJson -> { - if (migratedApplicationJson.getServerSchemaVersion() == 9 - && Boolean.TRUE.equals(MigrationHelperMethods.doesRestApiRequireMigration( - migratedApplicationJson))) { - return jsonSchemaMigrationHelper - .addDatasourceConfigurationToDefaultRestApiActions( - baseApplicationId, branchName, migratedApplicationJson) - .map(applicationJsonWithMigration10 -> { - applicationJsonWithMigration10.setServerSchemaVersion(10); - return applicationJsonWithMigration10; - }); + // In Server version 9, there was a bug where the Embedded REST API datasource URL + // was not being persisted correctly. Once the bug was fixed, + // any previously uncommitted changes started appearing as uncommitted modifications + // in the apps. To automatically commit these changes + // (which were now appearing as uncommitted), a migration process was needed. + // This migration fetches the datasource URL from the database + // and serializes it in Git if the URL exists. + // If the URL is missing, it copies the empty datasource configuration + // if the configuration is present in the database. + // Otherwise, it leaves the configuration unchanged. + // Due to an update in the migration logic after version 10 was shipped, + // the entire migration process was moved to version 11. + // This adjustment ensures that the same operation can be + // performed again for the changes introduced in version 10. + if (migratedApplicationJson.getServerSchemaVersion() == 9) { + migratedApplicationJson.setServerSchemaVersion(10); + } + + if (migratedApplicationJson.getServerSchemaVersion() == 10) { + if (Boolean.TRUE.equals(MigrationHelperMethods.doesRestApiRequireMigration( + migratedApplicationJson))) { + return jsonSchemaMigrationHelper + .addDatasourceConfigurationToDefaultRestApiActions( + baseApplicationId, branchName, migratedApplicationJson); + } + + migratedApplicationJson.setServerSchemaVersion(11); } - migratedApplicationJson.setServerSchemaVersion(10); return Mono.just(migratedApplicationJson); }) .map(migratedAppJson -> { - if (applicationJson - .getServerSchemaVersion() - .equals(jsonSchemaVersions.getServerVersion())) { - return applicationJson; - } - applicationJson.setServerSchemaVersion(jsonSchemaVersions.getServerVersion()); return applicationJson; }); @@ -193,16 +203,14 @@ private ApplicationJson nonReactiveServerMigrationForImport(ApplicationJson appl switch (applicationJson.getServerSchemaVersion()) { case 9: + applicationJson.setServerSchemaVersion(10); + case 10: // this if for cases where we have empty datasource configs MigrationHelperMethods.migrateApplicationJsonToVersionTen(applicationJson, Map.of()); - applicationJson.setServerSchemaVersion(10); + applicationJson.setServerSchemaVersion(11); default: } - if (applicationJson.getServerSchemaVersion().equals(jsonSchemaVersions.getServerVersion())) { - return applicationJson; - } - applicationJson.setServerSchemaVersion(jsonSchemaVersions.getServerVersion()); return applicationJson; } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaVersionsFallback.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaVersionsFallback.java index 06518c18b4e..c85b95bed5d 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaVersionsFallback.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaVersionsFallback.java @@ -4,7 +4,7 @@ @Component public class JsonSchemaVersionsFallback { - private static final Integer serverVersion = 10; + private static final Integer serverVersion = 11; public static final Integer clientVersion = 1; public Integer getServerVersion() { diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java index 87e3d93e6ff..277a3ac5768 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java @@ -44,6 +44,8 @@ import java.util.regex.Pattern; import java.util.stream.Collectors; +import static com.appsmith.external.constants.PluginConstants.PackageName.GRAPHQL_PLUGIN; +import static com.appsmith.external.constants.PluginConstants.PackageName.REST_API_PLUGIN; import static com.appsmith.server.constants.ResourceModes.EDIT; import static com.appsmith.server.constants.ResourceModes.VIEW; import static org.springframework.data.mongodb.core.query.Criteria.where; @@ -1235,13 +1237,34 @@ public static void setThemeSettings(Application.ThemeSetting themeSetting) { } } - private static boolean conditionForDefaultRestDatasourceMigration(NewAction action) { + public static boolean conditionForDefaultRestDatasource(NewAction action) { + if (action.getUnpublishedAction() == null + || action.getUnpublishedAction().getDatasource() == null) { + return false; + } + Datasource actionDatasource = action.getUnpublishedAction().getDatasource(); + // probable check for the default rest datasource action is. + // it has no datasource id and action's plugin id is either rest-api or graphql plugin. + boolean probableCheckForDefaultRestDatasource = !org.springframework.util.StringUtils.hasText( + actionDatasource.getId()) + && (REST_API_PLUGIN.equals(action.getPluginId()) || GRAPHQL_PLUGIN.equals(action.getPluginId())); + // condition to check if the action is default rest datasource. // it has no datasource id and name is equal to DEFAULT_REST_DATASOURCE - boolean isActionDefaultRestDatasource = !org.springframework.util.StringUtils.hasText(actionDatasource.getId()) - && PluginConstants.DEFAULT_REST_DATASOURCE.equals(actionDatasource.getName()); + boolean certainCheckForDefaultRestDatasource = + !org.springframework.util.StringUtils.hasText(actionDatasource.getId()) + && PluginConstants.DEFAULT_REST_DATASOURCE.equals(actionDatasource.getName()); + + // Two separate types of checks over here, it's either the obvious certain way to identify or + // the likely chance that the datasource is present. + return certainCheckForDefaultRestDatasource || probableCheckForDefaultRestDatasource; + } + + private static boolean conditionForDefaultRestDatasourceMigration(NewAction action) { + boolean isActionDefaultRestDatasource = conditionForDefaultRestDatasource(action); + Datasource actionDatasource = action.getUnpublishedAction().getDatasource(); // condition to check if the action has missing url or has no config at all boolean isDatasourceConfigurationOrUrlMissing = actionDatasource.getDatasourceConfiguration() == null @@ -1322,18 +1345,25 @@ public static void setDatasourceConfigDetailsInDefaultRestDatasourceForActions( if (defaultDatasourceActionMap.containsKey(action.getGitSyncId())) { NewAction actionFromMap = defaultDatasourceActionMap.get(action.getGitSyncId()); + // NPE check to avoid migration failures + if (actionFromMap.getUnpublishedAction() == null + || actionFromMap.getUnpublishedAction().getDatasource() == null + || actionFromMap.getUnpublishedAction().getDatasource().getDatasourceConfiguration() == null) { + return; + } + + // set the datasource config in the json action only if the datasource config from db is not null, + // else it'll start to show as uncommited changes. DatasourceConfiguration datasourceConfigurationFromDBAction = actionFromMap.getUnpublishedAction().getDatasource().getDatasourceConfiguration(); - if (datasourceConfigurationFromDBAction != null) { - datasourceConfiguration.setUrl(datasourceConfigurationFromDBAction.getUrl()); - } - } + // At this point it's established that datasource config of db action is not null. + datasourceConfiguration.setUrl(datasourceConfigurationFromDBAction.getUrl()); + actionDatasource.setDatasourceConfiguration(datasourceConfiguration); - if (!org.springframework.util.StringUtils.hasText(datasourceConfiguration.getUrl())) { + } else { datasourceConfiguration.setUrl(""); + actionDatasource.setDatasourceConfiguration(datasourceConfiguration); } - - actionDatasource.setDatasourceConfiguration(datasourceConfiguration); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/utils/JsonSchemaMigrationHelper.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/utils/JsonSchemaMigrationHelper.java index 934bf79c6ed..1f1e27de8c7 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/utils/JsonSchemaMigrationHelper.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/utils/JsonSchemaMigrationHelper.java @@ -1,6 +1,8 @@ package com.appsmith.server.migrations.utils; import com.appsmith.external.constants.PluginConstants; +import com.appsmith.external.models.Datasource; +import com.appsmith.external.models.PluginType; import com.appsmith.server.applications.base.ApplicationService; import com.appsmith.server.domains.Application; import com.appsmith.server.domains.NewAction; @@ -51,14 +53,27 @@ public Mono addDatasourceConfigurationToDefaultRestApiActions( return false; } - boolean reverseFlag = StringUtils.hasText(action.getUnpublishedAction() - .getDatasource() - .getId()) - || !PluginConstants.DEFAULT_REST_DATASOURCE.equals(action.getUnpublishedAction() - .getDatasource() - .getName()); + Datasource actionDatasource = + action.getUnpublishedAction().getDatasource(); - return !reverseFlag; + // lenient probable check for the default rest datasource action is. + // As we don't have any harm in the allowing API actions present in db. + // it has no datasource id and action's plugin type is API + boolean probableCheckForDefaultRestDatasource = + !org.springframework.util.StringUtils.hasText(actionDatasource.getId()) + && PluginType.API.equals(action.getPluginType()); + + // condition to check if the action is default rest datasource. + // it has no datasource id and name is equal to DEFAULT_REST_DATASOURCE + boolean certainCheckForDefaultRestDatasource = + !org.springframework.util.StringUtils.hasText(actionDatasource.getId()) + && PluginConstants.DEFAULT_REST_DATASOURCE.equals( + actionDatasource.getName()); + + // Two separate types of checks over here, it's either the obvious certain way to + // identify or + // the likely chance that the datasource is present. + return certainCheckForDefaultRestDatasource || probableCheckForDefaultRestDatasource; }) .collectMap(NewAction::getGitSyncId); })