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

Jump to bottom

chore: autocommit migration for annotation and embedded datasource changes. #36261

Merged
merged 4 commits into from
Sep 16, 2024
Merged

chore: autocommit migration for annotation and embedded datasource changes. #36261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d7544bf
added autocommit migration nuances
sondermanish Sep 11, 2024
32187de
added changes
sondermanish Sep 11, 2024
49bcb9e
changed db action filtering logic
sondermanish Sep 12, 2024
0013cde
added comment as per suggestion
sondermanish Sep 16, 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
2 changes: 2 additions & 0 deletions ...psmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand Down
52 changes: 30 additions & 22 deletions ...ver/appsmith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaMigration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,28 +67,38 @@ public Mono<ApplicationJson> 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) {
Copy link
Member

@mohanarpit mohanarpit Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments as suggested.

migratedApplicationJson.setServerSchemaVersion(10);
}

if (migratedApplicationJson.getServerSchemaVersion() == 10) {
sagar-qa007 marked this conversation as resolved.
Show resolved Hide resolved
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;
});
Expand Down Expand Up @@ -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())) {
sondermanish marked this conversation as resolved.
Show resolved Hide resolved
return applicationJson;
}

applicationJson.setServerSchemaVersion(jsonSchemaVersions.getServerVersion());
return applicationJson;
}
Expand Down
2 changes: 1 addition & 1 deletion ...smith-server/src/main/java/com/appsmith/server/migrations/JsonSchemaVersionsFallback.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
50 changes: 40 additions & 10 deletions .../appsmith-server/src/main/java/com/appsmith/server/migrations/MigrationHelperMethods.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}
29 changes: 22 additions & 7 deletions ...-server/src/main/java/com/appsmith/server/migrations/utils/JsonSchemaMigrationHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -51,14 +53,27 @@ public Mono<ApplicationJson> 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);
})
Expand Down
Loading