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

chore: add postSaveHook #39306

Merged
merged 2 commits into from
Feb 18, 2025
Merged

chore: add postSaveHook #39306

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b1155a3
add postSaveHook
nsarupr Feb 17, 2025
260ad93
add metadata instead of individual fields for extensibility
nsarupr Feb 18, 2025
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
6 changes: 6 additions & 0 deletions ...ver/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,12 @@ public class DatasourceStorage extends GitSyncedDomain {
@Transient
Boolean isMock;

@Transient
String tenantId;

@Transient
String instanceId;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update prepareTransientFields to include new fields.

The new transient fields tenantId and instanceId should be initialized in the prepareTransientFields method to maintain consistency with other transient fields.

Apply this diff to update the method:

 public void prepareTransientFields(Datasource datasource) {
     this.datasourceId = datasource.getId();
     this.name = datasource.getName();
     this.pluginId = datasource.getPluginId();
     this.pluginName = datasource.getPluginName();
     this.workspaceId = datasource.getWorkspaceId();
     this.templateName = datasource.getTemplateName();
     this.isAutoGenerated = datasource.getIsAutoGenerated();
     this.isRecentlyCreated = datasource.getIsRecentlyCreated();
     this.isTemplate = datasource.getIsTemplate();
     this.isMock = datasource.getIsMock();
+    this.tenantId = datasource.getTenantId();
+    this.instanceId = datasource.getInstanceId();

     if (datasource.getInvalids() != null) {
         this.invalids.addAll(datasource.getInvalids());
     }
     this.gitSyncId = datasource.getGitSyncId();
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Transient
String tenantId;
@Transient
String instanceId;
public void prepareTransientFields(Datasource datasource) {
this.datasourceId = datasource.getId();
this.name = datasource.getName();
this.pluginId = datasource.getPluginId();
this.pluginName = datasource.getPluginName();
this.workspaceId = datasource.getWorkspaceId();
this.templateName = datasource.getTemplateName();
this.isAutoGenerated = datasource.getIsAutoGenerated();
this.isRecentlyCreated = datasource.getIsRecentlyCreated();
this.isTemplate = datasource.getIsTemplate();
this.isMock = datasource.getIsMock();
this.tenantId = datasource.getTenantId();
this.instanceId = datasource.getInstanceId();
if (datasource.getInvalids() != null) {
this.invalids.addAll(datasource.getInvalids());
}
this.gitSyncId = datasource.getGitSyncId();
}

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if DatasourceStorage should have these fields configured in the POJO. I understand that we need the fields instanceId and tenantId in our operations. But having these fields in the DatasourceStorage object seems awkward as it's violating the boundaries of our system architecture.

What do you think of instead introducing a map of parameters that can store this metadata that is parsed by the plugin itself? Would that be more extendable? Or do you think it'll lead to confusion and lack of clarity in the code?


public DatasourceStorage(
String datasourceId,
String environmentId,
Expand Down
7 changes: 7 additions & 0 deletions ...erver/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/PluginExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,13 @@ default Mono<DatasourceStorage> preDeleteHook(DatasourceStorage datasourceStorag
return Mono.just(datasourceStorage);
}

/**
* This function is being called as a hook after saving a datasource.
*/
default Mono<DatasourceStorage> postSaveHook(DatasourceStorage datasourceStorage) {
return Mono.just(datasourceStorage);
}

/**
* This function fetches the structure of the tables/collections in the datasource. It's used to make query creation
* easier for the user.
Expand Down
78 changes: 50 additions & 28 deletions ...th-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@
import com.appsmith.server.repositories.DatasourceRepository;
import com.appsmith.server.repositories.NewActionRepository;
import com.appsmith.server.services.AnalyticsService;
import com.appsmith.server.services.ConfigService;
import com.appsmith.server.services.DatasourceContextService;
import com.appsmith.server.services.FeatureFlagService;
import com.appsmith.server.services.SequenceService;
import com.appsmith.server.services.SessionUserService;
import com.appsmith.server.services.TenantService;
import com.appsmith.server.services.WorkspaceService;
import com.appsmith.server.solutions.DatasourcePermission;
import com.appsmith.server.solutions.EnvironmentPermission;
Expand Down Expand Up @@ -93,6 +95,8 @@ public class DatasourceServiceCEImpl implements DatasourceServiceCE {
private final RateLimitService rateLimitService;
private final FeatureFlagService featureFlagService;
private final ObservationRegistry observationRegistry;
private final TenantService tenantService;
private final ConfigService configService;

// Defines blocking duration for test as well as connection created for query execution
// This will block the creation of datasource connection for 5 minutes, in case of more than 3 failed connection
Expand All @@ -119,7 +123,9 @@ public DatasourceServiceCEImpl(
EnvironmentPermission environmentPermission,
RateLimitService rateLimitService,
FeatureFlagService featureFlagService,
ObservationRegistry observationRegistry) {
ObservationRegistry observationRegistry,
TenantService tenantService,
ConfigService configService) {

this.workspaceService = workspaceService;
this.sessionUserService = sessionUserService;
Expand All @@ -138,6 +144,8 @@ public DatasourceServiceCEImpl(
this.rateLimitService = rateLimitService;
this.featureFlagService = featureFlagService;
this.observationRegistry = observationRegistry;
this.tenantService = tenantService;
this.configService = configService;
}

@Override
Expand Down Expand Up @@ -182,6 +190,12 @@ private Mono<Datasource> createEx(
datasource.nullifyStorageReplicaFields();
Mono<Datasource> datasourceMono = Mono.just(datasource);

//
Mono<Tuple2<String, String>> tenantIdAndInstanceIdMonoCached = tenantService
.getDefaultTenantId()
.zipWith(configService.getInstanceId())
.cache();

// First check if this is an existing datasource or whether we need to create one
if (!hasText(datasource.getId())) {
// We need to create the datasource as well
Expand Down Expand Up @@ -222,34 +236,42 @@ private Mono<Datasource> createEx(
new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.DATASOURCE))));
}

return datasourceMono.flatMap(savedDatasource -> this.organiseDatasourceStorages(savedDatasource)
.flatMap(datasourceStorage -> {
// Make sure that we are creating entries only if the id is not already populated
if (hasText(datasourceStorage.getId())) {
return Mono.just(datasourceStorage);
}

return datasourceStorageService
.create(datasourceStorage, isDryOps)
.map(datasourceStorage1 -> {
if (datasourceStorageDryRunQueries != null && isDryOps) {
List<DatasourceStorage> datasourceStorages =
datasourceStorageDryRunQueries.get(SAVE);
if (datasourceStorages == null) {
datasourceStorages = new ArrayList<>();
return datasourceMono.zipWith(tenantIdAndInstanceIdMonoCached).flatMap(tuple2 -> {
Datasource savedDatasource = tuple2.getT1();
String tenantId = tuple2.getT2().getT1();
String instanceId = tuple2.getT2().getT2();
return this.organiseDatasourceStorages(savedDatasource)
.flatMap(datasourceStorage -> {
// Set the tenantId and instanceId
datasourceStorage.setTenantId(tenantId);
datasourceStorage.setInstanceId(instanceId);
// Make sure that we are creating entries only if the id is not already populated
if (hasText(datasourceStorage.getId())) {
return Mono.just(datasourceStorage);
}

return datasourceStorageService
.create(datasourceStorage, isDryOps)
.map(datasourceStorage1 -> {
if (datasourceStorageDryRunQueries != null && isDryOps) {
List<DatasourceStorage> datasourceStorages =
datasourceStorageDryRunQueries.get(SAVE);
if (datasourceStorages == null) {
datasourceStorages = new ArrayList<>();
}
datasourceStorages.add(datasourceStorage1);
datasourceStorageDryRunQueries.put(SAVE, datasourceStorages);
}
datasourceStorages.add(datasourceStorage1);
datasourceStorageDryRunQueries.put(SAVE, datasourceStorages);
}
return datasourceStorage1;
});
})
.map(datasourceStorageService::createDatasourceStorageDTOFromDatasourceStorage)
.collectMap(DatasourceStorageDTO::getEnvironmentId)
.map(savedStorages -> {
savedDatasource.setDatasourceStorages(savedStorages);
return savedDatasource;
}));
return datasourceStorage1;
});
})
.map(datasourceStorageService::createDatasourceStorageDTOFromDatasourceStorage)
.collectMap(DatasourceStorageDTO::getEnvironmentId)
.map(savedStorages -> {
savedDatasource.setDatasourceStorages(savedStorages);
return savedDatasource;
});
});
}

// this requires an EE override multiple environments
Expand Down
10 changes: 8 additions & 2 deletions ...mith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@
import com.appsmith.server.repositories.DatasourceRepository;
import com.appsmith.server.repositories.NewActionRepository;
import com.appsmith.server.services.AnalyticsService;
import com.appsmith.server.services.ConfigService;
import com.appsmith.server.services.DatasourceContextService;
import com.appsmith.server.services.FeatureFlagService;
import com.appsmith.server.services.SequenceService;
import com.appsmith.server.services.SessionUserService;
import com.appsmith.server.services.TenantService;
import com.appsmith.server.services.WorkspaceService;
import com.appsmith.server.solutions.DatasourcePermission;
import com.appsmith.server.solutions.EnvironmentPermission;
Expand Down Expand Up @@ -41,7 +43,9 @@ public DatasourceServiceImpl(
EnvironmentPermission environmentPermission,
RateLimitService rateLimitService,
FeatureFlagService featureFlagService,
ObservationRegistry observationRegistry) {
ObservationRegistry observationRegistry,
TenantService tenantService,
ConfigService configService) {

super(
repository,
Expand All @@ -60,6 +64,8 @@ public DatasourceServiceImpl(
environmentPermission,
rateLimitService,
featureFlagService,
observationRegistry);
observationRegistry,
tenantService,
configService);
}
}
15 changes: 14 additions & 1 deletion ...main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,16 @@ public Mono<DatasourceStorage> executePreSaveActions(DatasourceStorage datasourc
return pluginExecutorMono.flatMap(pluginExecutor -> pluginExecutor.preSaveHook(datasourceStorage));
}

public Mono<DatasourceStorage> executePostSaveActions(DatasourceStorage datasourceStorage) {
Mono<Plugin> pluginMono = pluginService.findById(datasourceStorage.getPluginId());
Mono<PluginExecutor> pluginExecutorMono = pluginExecutorHelper
.getPluginExecutor(pluginMono)
.switchIfEmpty(Mono.error(new AppsmithException(
AppsmithError.NO_RESOURCE_FOUND, FieldName.PLUGIN, datasourceStorage.getPluginId())));

return pluginExecutorMono.flatMap(pluginExecutor -> pluginExecutor.postSaveHook(datasourceStorage));
}

@Override
public Mono<DatasourceStorage> validateDatasourceStorage(DatasourceStorage datasourceStorage) {

Expand Down Expand Up @@ -242,7 +252,10 @@ private Mono<DatasourceStorage> validateAndSaveDatasourceStorageToRepository(
unsavedDatasourceStorage.updateForBulkWriteOperation();
return Mono.just(unsavedDatasourceStorage);
}
return repository.save(unsavedDatasourceStorage).thenReturn(unsavedDatasourceStorage);
return repository
.save(unsavedDatasourceStorage)
.then(this.executePostSaveActions(unsavedDatasourceStorage))
.thenReturn(unsavedDatasourceStorage);
Comment on lines +255 to +258
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider transaction boundary for save operation.

The save operation and post-save actions should be executed within a transaction to ensure atomicity. If the post-save action fails, the save should be rolled back.

Consider using Spring's @Transactional annotation or a transaction template to wrap both operations:

-return repository
-        .save(unsavedDatasourceStorage)
-        .then(this.executePostSaveActions(unsavedDatasourceStorage))
-        .thenReturn(unsavedDatasourceStorage);
+return transactionTemplate.execute(status -> {
+    return repository
+            .save(unsavedDatasourceStorage)
+            .then(this.executePostSaveActions(unsavedDatasourceStorage))
+            .thenReturn(unsavedDatasourceStorage);
+});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return repository
.save(unsavedDatasourceStorage)
.then(this.executePostSaveActions(unsavedDatasourceStorage))
.thenReturn(unsavedDatasourceStorage);
return transactionTemplate.execute(status -> {
return repository
.save(unsavedDatasourceStorage)
.then(this.executePostSaveActions(unsavedDatasourceStorage))
.thenReturn(unsavedDatasourceStorage);
});

});
}

Expand Down
Loading