fix: removed datasourceContextCaching for RestAPI (#28160)
## Description > Rest Api used cached datasourceContext which was providing older tokens. Now when we look for datasourceContext we provide a new context. #### PR fixes following issue(s) Fixes #27699 #### Type of change - Bug fix (non-breaking change which fixes an issue) #### How Has This Been Tested? - [ ] Manual ## Checklist: #### Dev activity - [ ] My code follows the style guidelines of this project - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag #### QA activity: - [ ] [Speedbreak features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-) have been covered - [ ] Test plan covers all impacted features and [areas of interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-) - [ ] Test plan has been peer reviewed by project stakeholders and other QA members - [ ] Manually tested functionality on DP - [ ] We had an implementation alignment call with stakeholders post QA Round 2 - [ ] Cypress test cases have been added and approved by SDET/manual QA - [ ] Added `Test Plan Approved` label after Cypress tests were reviewed - [ ] Added `Test Plan Approved` label after JUnit tests were reviewed
This commit is contained in:
Manish Kumar
Trisha Anand
parent
b0e147fdbb
commit
6e2027f1b6
|
|
@ -10,6 +10,8 @@ public interface PluginConstants {
|
|||
String DYNAMO_PLUGIN = "dynamo-plugin";
|
||||
String AMAZON_S3_PLUGIN = "amazons3-plugin";
|
||||
String GOOGLE_SHEETS_PLUGIN = "google-sheets-plugin";
|
||||
String REST_API_PLUGIN = "restapi-plugin";
|
||||
String GRAPH_QL_PLUGIN = "graphql-plugin";
|
||||
}
|
||||
|
||||
public static final String DEFAULT_REST_DATASOURCE = "DEFAULT_REST_DATASOURCE";
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
package com.appsmith.server.services.ce;
|
||||
|
||||
import com.appsmith.external.constants.PluginConstants;
|
||||
import com.appsmith.external.dtos.ExecutePluginDTO;
|
||||
import com.appsmith.external.dtos.RemoteDatasourceDTO;
|
||||
import com.appsmith.external.exceptions.pluginExceptions.StaleConnectionException;
|
||||
|
|
@ -70,6 +71,7 @@ public class DatasourceContextServiceCEImpl implements DatasourceContextServiceC
|
|||
* value instead of creating a new connection for each subscription of the source publisher.
|
||||
*
|
||||
* @param datasourceStorage - datasource storage for which a new datasource context / connection needs to be created
|
||||
* @param plugin
|
||||
* @param pluginExecutor - plugin executor associated with the datasource's plugin
|
||||
* @param monitor - unique monitor object per datasource id. Lock is acquired on this monitor object.
|
||||
* @param datasourceContextIdentifier - key for the datasourceContextMaps.
|
||||
|
|
@ -78,6 +80,7 @@ public class DatasourceContextServiceCEImpl implements DatasourceContextServiceC
|
|||
*/
|
||||
public Mono<? extends DatasourceContext<?>> getCachedDatasourceContextMono(
|
||||
DatasourceStorage datasourceStorage,
|
||||
Plugin plugin,
|
||||
PluginExecutor<Object> pluginExecutor,
|
||||
Object monitor,
|
||||
DatasourceContextIdentifier datasourceContextIdentifier) {
|
||||
|
|
@ -114,7 +117,7 @@ public class DatasourceContextServiceCEImpl implements DatasourceContextServiceC
|
|||
|
||||
/* Create a fresh datasource context */
|
||||
DatasourceContext<Object> datasourceContext = new DatasourceContext<>();
|
||||
if (datasourceContextIdentifier.isKeyValid()) {
|
||||
if (datasourceContextIdentifier.isKeyValid() && shouldCacheContextForThisPlugin(plugin)) {
|
||||
/* For this datasource, either the context doesn't exist, or the context is stale. Replace (or add) with
|
||||
the new connection in the context map. */
|
||||
datasourceContextMap.put(datasourceContextIdentifier, datasourceContext);
|
||||
|
|
@ -138,13 +141,25 @@ public class DatasourceContextServiceCEImpl implements DatasourceContextServiceC
|
|||
datasourceContext)
|
||||
.cache(); /* Cache the value so that further evaluations don't result in new connections */
|
||||
|
||||
if (datasourceContextIdentifier.isKeyValid()) {
|
||||
if (datasourceContextIdentifier.isKeyValid() && shouldCacheContextForThisPlugin(plugin)) {
|
||||
datasourceContextMonoMap.put(datasourceContextIdentifier, datasourceContextMonoCache);
|
||||
}
|
||||
return datasourceContextMonoCache;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* determines whether we should cache context for given plugin
|
||||
* it gives false if plugin is rest-api or graph-ql
|
||||
* @param plugin
|
||||
* @return
|
||||
*/
|
||||
public boolean shouldCacheContextForThisPlugin(Plugin plugin) {
|
||||
// !(a || b) => (!a) & (!b)
|
||||
return !PluginConstants.PackageName.REST_API_PLUGIN.equals(plugin.getPackageName())
|
||||
&& !PluginConstants.PackageName.GRAPH_QL_PLUGIN.equals(plugin.getPackageName());
|
||||
}
|
||||
|
||||
public Mono<Object> updateDatasourceAndSetAuthentication(Object connection, DatasourceStorage datasourceStorage) {
|
||||
Mono<DatasourceStorage> datasourceStorageMono = Mono.just(datasourceStorage);
|
||||
if (connection instanceof UpdatableConnection updatableConnection) {
|
||||
|
|
@ -162,33 +177,38 @@ public class DatasourceContextServiceCEImpl implements DatasourceContextServiceC
|
|||
protected Mono<DatasourceContext<?>> createNewDatasourceContext(
|
||||
DatasourceStorage datasourceStorage, DatasourceContextIdentifier datasourceContextIdentifier) {
|
||||
log.debug("Datasource context doesn't exist. Creating connection.");
|
||||
Mono<Plugin> pluginMono = pluginService.findById(datasourceStorage.getPluginId());
|
||||
Mono<Plugin> pluginMono =
|
||||
pluginService.findById(datasourceStorage.getPluginId()).cache();
|
||||
|
||||
return pluginExecutorHelper.getPluginExecutor(pluginMono).flatMap(pluginExecutor -> {
|
||||
return pluginMono
|
||||
.zipWith(pluginExecutorHelper.getPluginExecutor(pluginMono))
|
||||
.flatMap(tuple2 -> {
|
||||
Plugin plugin = tuple2.getT1();
|
||||
PluginExecutor<Object> pluginExecutor = tuple2.getT2();
|
||||
|
||||
/**
|
||||
* Keep one monitor object against each datasource id. The synchronized method
|
||||
* `getCachedDatasourceContextMono` would then acquire lock on the monitor object which is unique
|
||||
* for each datasourceId hence ensuring that if competing threads want to create datasource context
|
||||
* on different datasource id then they are not blocked on each other and can run concurrently.
|
||||
* Only threads that want to create a new datasource context on the same datasource id would be
|
||||
* synchronized.
|
||||
*/
|
||||
Object monitor = new Object();
|
||||
if (datasourceContextIdentifier.isKeyValid()) {
|
||||
if (datasourceContextSynchronizationMonitorMap.get(datasourceContextIdentifier) == null) {
|
||||
synchronized (this) {
|
||||
datasourceContextSynchronizationMonitorMap.computeIfAbsent(
|
||||
datasourceContextIdentifier, k -> new Object());
|
||||
/**
|
||||
* Keep one monitor object against each datasource id. The synchronized method
|
||||
* `getCachedDatasourceContextMono` would then acquire lock on the monitor object which is unique
|
||||
* for each datasourceId hence ensuring that if competing threads want to create datasource context
|
||||
* on different datasource id then they are not blocked on each other and can run concurrently.
|
||||
* Only threads that want to create a new datasource context on the same datasource id would be
|
||||
* synchronized.
|
||||
*/
|
||||
Object monitor = new Object();
|
||||
if (datasourceContextIdentifier.isKeyValid()) {
|
||||
if (datasourceContextSynchronizationMonitorMap.get(datasourceContextIdentifier) == null) {
|
||||
synchronized (this) {
|
||||
datasourceContextSynchronizationMonitorMap.computeIfAbsent(
|
||||
datasourceContextIdentifier, k -> new Object());
|
||||
}
|
||||
}
|
||||
|
||||
monitor = datasourceContextSynchronizationMonitorMap.get(datasourceContextIdentifier);
|
||||
}
|
||||
}
|
||||
|
||||
monitor = datasourceContextSynchronizationMonitorMap.get(datasourceContextIdentifier);
|
||||
}
|
||||
|
||||
return getCachedDatasourceContextMono(
|
||||
datasourceStorage, pluginExecutor, monitor, datasourceContextIdentifier);
|
||||
});
|
||||
return getCachedDatasourceContextMono(
|
||||
datasourceStorage, plugin, pluginExecutor, monitor, datasourceContextIdentifier);
|
||||
});
|
||||
}
|
||||
|
||||
public boolean getIsStale(
|
||||
|
|
|
|||
|
|
@ -1,9 +1,14 @@
|
|||
package com.appsmith.server.services;
|
||||
|
||||
import com.appsmith.external.constants.PluginConstants;
|
||||
import com.appsmith.external.helpers.restApiUtils.connections.APIConnection;
|
||||
import com.appsmith.external.helpers.restApiUtils.connections.APIConnectionFactory;
|
||||
import com.appsmith.external.helpers.restApiUtils.connections.BearerTokenAuthentication;
|
||||
import com.appsmith.external.helpers.restApiUtils.connections.OAuth2ClientCredentials;
|
||||
import com.appsmith.external.models.ApiKeyAuth;
|
||||
import com.appsmith.external.models.AuthenticationDTO;
|
||||
import com.appsmith.external.models.BasicAuth;
|
||||
import com.appsmith.external.models.BearerTokenAuth;
|
||||
import com.appsmith.external.models.DBAuth;
|
||||
import com.appsmith.external.models.Datasource;
|
||||
import com.appsmith.external.models.DatasourceConfiguration;
|
||||
|
|
@ -146,6 +151,7 @@ public class DatasourceContextServiceTest {
|
|||
@WithUserDetails(value = "api_user")
|
||||
public void testDatasourceCache_afterDatasourceDeleted_doesNotReturnOldConnection() {
|
||||
// Never require the datasource connection to be stale
|
||||
Plugin emptyPlugin = new Plugin();
|
||||
doReturn(false).doReturn(false).when(datasourceContextService).getIsStale(any(), any());
|
||||
|
||||
MockPluginExecutor mockPluginExecutor = new MockPluginExecutor();
|
||||
|
|
@ -169,7 +175,7 @@ public class DatasourceContextServiceTest {
|
|||
Object monitor = new Object();
|
||||
// Create one instance of datasource connection
|
||||
Mono<DatasourceContext<?>> dsContextMono1 = datasourceContextService.getCachedDatasourceContextMono(
|
||||
datasourceStorage, spyMockPluginExecutor, monitor, datasourceContextIdentifier);
|
||||
datasourceStorage, emptyPlugin, spyMockPluginExecutor, monitor, datasourceContextIdentifier);
|
||||
|
||||
Datasource datasource = new Datasource();
|
||||
datasource.setId("id1");
|
||||
|
|
@ -197,7 +203,7 @@ public class DatasourceContextServiceTest {
|
|||
Mono<DatasourceContext<?>> dsContextMono2 = datasourceService
|
||||
.archiveById("id1")
|
||||
.flatMap(deleted -> datasourceContextService.getCachedDatasourceContextMono(
|
||||
datasourceStorage, spyMockPluginExecutor, monitor, datasourceContextIdentifier));
|
||||
datasourceStorage, emptyPlugin, spyMockPluginExecutor, monitor, datasourceContextIdentifier));
|
||||
|
||||
StepVerifier.create(dsContextMono1)
|
||||
.assertNext(dsContext1 -> {
|
||||
|
|
@ -349,7 +355,7 @@ public class DatasourceContextServiceTest {
|
|||
@WithUserDetails(value = "api_user")
|
||||
public void testCachedDatasourceCreate() {
|
||||
doReturn(false).doReturn(false).when(datasourceContextService).getIsStale(any(), any());
|
||||
|
||||
Plugin emptyPlugin = new Plugin();
|
||||
MockPluginExecutor mockPluginExecutor = new MockPluginExecutor();
|
||||
MockPluginExecutor spyMockPluginExecutor = spy(mockPluginExecutor);
|
||||
/* Return two different connection objects if `datasourceCreate` method is called twice */
|
||||
|
|
@ -369,11 +375,11 @@ public class DatasourceContextServiceTest {
|
|||
Object monitor = new Object();
|
||||
DatasourceContext<?> dsContext1 = (DatasourceContext<?>) datasourceContextService
|
||||
.getCachedDatasourceContextMono(
|
||||
datasourceStorage, spyMockPluginExecutor, monitor, datasourceContextIdentifier)
|
||||
datasourceStorage, emptyPlugin, spyMockPluginExecutor, monitor, datasourceContextIdentifier)
|
||||
.block();
|
||||
DatasourceContext<?> dsContext2 = (DatasourceContext<?>) datasourceContextService
|
||||
.getCachedDatasourceContextMono(
|
||||
datasourceStorage, spyMockPluginExecutor, monitor, datasourceContextIdentifier)
|
||||
datasourceStorage, emptyPlugin, spyMockPluginExecutor, monitor, datasourceContextIdentifier)
|
||||
.block();
|
||||
|
||||
/* They can only be equal if the `datasourceCreate` method was called only once */
|
||||
|
|
@ -388,6 +394,7 @@ public class DatasourceContextServiceTest {
|
|||
@Test
|
||||
@WithUserDetails(value = "api_user")
|
||||
public void testDatasourceCreate_withUpdatableConnection_recreatesConnectionAlways() {
|
||||
Plugin emptyPlugin = new Plugin();
|
||||
MockPluginExecutor mockPluginExecutor = new MockPluginExecutor();
|
||||
Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any())).thenReturn(Mono.just(mockPluginExecutor));
|
||||
|
||||
|
|
@ -452,7 +459,11 @@ public class DatasourceContextServiceTest {
|
|||
Object monitor = new Object();
|
||||
final DatasourceContext<?> dsc1 = (DatasourceContext) datasourceContextService
|
||||
.getCachedDatasourceContextMono(
|
||||
createdDatasourceStorage, spyMockPluginExecutor, monitor, datasourceContextIdentifier)
|
||||
createdDatasourceStorage,
|
||||
emptyPlugin,
|
||||
spyMockPluginExecutor,
|
||||
monitor,
|
||||
datasourceContextIdentifier)
|
||||
.block();
|
||||
assertNotNull(dsc1);
|
||||
assertTrue(dsc1.getConnection() instanceof UpdatableConnection);
|
||||
|
|
@ -461,7 +472,11 @@ public class DatasourceContextServiceTest {
|
|||
|
||||
final DatasourceContext<?> dsc2 = (DatasourceContext) datasourceContextService
|
||||
.getCachedDatasourceContextMono(
|
||||
createdDatasourceStorage, spyMockPluginExecutor, monitor, datasourceContextIdentifier)
|
||||
createdDatasourceStorage,
|
||||
emptyPlugin,
|
||||
spyMockPluginExecutor,
|
||||
monitor,
|
||||
datasourceContextIdentifier)
|
||||
.block();
|
||||
assertNotNull(dsc2);
|
||||
assertTrue(dsc2.getConnection() instanceof UpdatableConnection);
|
||||
|
|
@ -479,6 +494,7 @@ public class DatasourceContextServiceTest {
|
|||
public void testDatasourceContextIsInvalid_whenCachedDatasourceContextMono_isInErrorState() {
|
||||
doReturn(false).when(datasourceContextService).getIsStale(any(), any());
|
||||
|
||||
Plugin emptyPlugin = new Plugin();
|
||||
MockPluginExecutor mockPluginExecutor = new MockPluginExecutor();
|
||||
MockPluginExecutor spyMockPluginExecutor = spy(mockPluginExecutor);
|
||||
|
||||
|
|
@ -499,7 +515,7 @@ public class DatasourceContextServiceTest {
|
|||
|
||||
Mono<DatasourceContext<?>> failedDatasourceContextMono =
|
||||
datasourceContextService.getCachedDatasourceContextMono(
|
||||
datasourceStorage, spyMockPluginExecutor, monitor, datasourceContextIdentifier);
|
||||
datasourceStorage, emptyPlugin, spyMockPluginExecutor, monitor, datasourceContextIdentifier);
|
||||
|
||||
StepVerifier.create(failedDatasourceContextMono)
|
||||
.expectError(RuntimeException.class)
|
||||
|
|
@ -512,14 +528,14 @@ public class DatasourceContextServiceTest {
|
|||
/**
|
||||
* This test verifies that if a cached datasource context Mono goes to an error state, then that Mono is invalidated
|
||||
* and a new datasource context mono is created on calling
|
||||
* {@link com.appsmith.server.services.ce.DatasourceContextServiceCEImpl#getCachedDatasourceContextMono(DatasourceStorage, PluginExecutor, Object, DatasourceContextIdentifier)}
|
||||
* {@link com.appsmith.server.services.ce.DatasourceContextServiceCEImpl#getCachedDatasourceContextMono(DatasourceStorage, Plugin, PluginExecutor, Object, DatasourceContextIdentifier)}
|
||||
* and not fetched from the cache.
|
||||
*/
|
||||
@Test
|
||||
@WithUserDetails(value = "api_user")
|
||||
public void testNewDatasourceContextCreate_whenCachedDatasourceContextMono_isInErrorState() {
|
||||
doReturn(false).doReturn(false).when(datasourceContextService).getIsStale(any(), any());
|
||||
|
||||
Plugin emptyPlugin = new Plugin();
|
||||
MockPluginExecutor mockPluginExecutor = new MockPluginExecutor();
|
||||
MockPluginExecutor spyMockPluginExecutor = spy(mockPluginExecutor);
|
||||
|
||||
|
|
@ -541,13 +557,13 @@ public class DatasourceContextServiceTest {
|
|||
|
||||
Mono<DatasourceContext<?>> failedDatasourceContextMono =
|
||||
datasourceContextService.getCachedDatasourceContextMono(
|
||||
datasourceStorage, spyMockPluginExecutor, monitor, datasourceContextIdentifier);
|
||||
datasourceStorage, emptyPlugin, spyMockPluginExecutor, monitor, datasourceContextIdentifier);
|
||||
StepVerifier.create(failedDatasourceContextMono)
|
||||
.expectError(RuntimeException.class)
|
||||
.verify();
|
||||
|
||||
Mono<DatasourceContext<?>> validDatasourceContextMono = datasourceContextService.getCachedDatasourceContextMono(
|
||||
datasourceStorage, spyMockPluginExecutor, monitor, datasourceContextIdentifier);
|
||||
datasourceStorage, emptyPlugin, spyMockPluginExecutor, monitor, datasourceContextIdentifier);
|
||||
|
||||
StepVerifier.create(validDatasourceContextMono)
|
||||
.assertNext(validDatasourceContext ->
|
||||
|
|
@ -645,4 +661,66 @@ public class DatasourceContextServiceTest {
|
|||
})
|
||||
.verifyComplete();
|
||||
}
|
||||
|
||||
@Test
|
||||
@WithUserDetails(value = "api_user")
|
||||
public void verifyDatasourceContextHasRightCredentialsAfterVariableReplacement() {
|
||||
String datasourceId = "datasourceId";
|
||||
String environmentId = "environmentId";
|
||||
|
||||
Plugin restApiPlugin = pluginService
|
||||
.findByPackageName(PluginConstants.PackageName.REST_API_PLUGIN)
|
||||
.block();
|
||||
String primaryBearerToken = "bearerToken1";
|
||||
BearerTokenAuth authenticationDTO = new BearerTokenAuth();
|
||||
authenticationDTO.setBearerToken(primaryBearerToken);
|
||||
DatasourceConfiguration datasourceConfiguration = new DatasourceConfiguration();
|
||||
datasourceConfiguration.setAuthentication(authenticationDTO);
|
||||
|
||||
DatasourceStorage datasourceStorage = new DatasourceStorage();
|
||||
datasourceStorage.setDatasourceId(datasourceId);
|
||||
datasourceStorage.setEnvironmentId(environmentId);
|
||||
datasourceStorage.setDatasourceConfiguration(datasourceConfiguration);
|
||||
datasourceStorage.setPluginId(restApiPlugin.getId());
|
||||
datasourceStorage.setPluginName(restApiPlugin.getPluginName());
|
||||
|
||||
MockPluginExecutor mockPluginExecutor = new MockPluginExecutor();
|
||||
MockPluginExecutor spyMockPluginExecutor = spy(mockPluginExecutor);
|
||||
|
||||
Mockito.when(pluginExecutorHelper.getPluginExecutor(Mockito.any()))
|
||||
.thenReturn(Mono.just(spyMockPluginExecutor));
|
||||
|
||||
doReturn(APIConnectionFactory.createConnection(datasourceStorage.getDatasourceConfiguration()))
|
||||
.when(spyMockPluginExecutor)
|
||||
.datasourceCreate(any());
|
||||
|
||||
Mono<DatasourceContext<?>> datasourceContextMono =
|
||||
datasourceContextService.getDatasourceContext(datasourceStorage, restApiPlugin);
|
||||
StepVerifier.create(datasourceContextMono)
|
||||
.assertNext(datasourceContext -> {
|
||||
assertThat(datasourceContext.getConnection()).isInstanceOf(APIConnection.class);
|
||||
assertThat(((BearerTokenAuthentication) datasourceContext.getConnection()).getBearerToken())
|
||||
.isEqualTo(primaryBearerToken);
|
||||
})
|
||||
.verifyComplete();
|
||||
|
||||
String updatedBearerToken = "bearerToken2";
|
||||
BearerTokenAuth bearerTokenAuth = new BearerTokenAuth();
|
||||
bearerTokenAuth.setBearerToken(updatedBearerToken);
|
||||
datasourceStorage.getDatasourceConfiguration().setAuthentication(bearerTokenAuth);
|
||||
|
||||
doReturn(APIConnectionFactory.createConnection(datasourceStorage.getDatasourceConfiguration()))
|
||||
.when(spyMockPluginExecutor)
|
||||
.datasourceCreate(any());
|
||||
|
||||
Mono<DatasourceContext<?>> datasourceContextMono2 =
|
||||
datasourceContextService.getDatasourceContext(datasourceStorage, restApiPlugin);
|
||||
StepVerifier.create(datasourceContextMono)
|
||||
.assertNext(datasourceContext -> {
|
||||
assertThat(datasourceContext.getConnection()).isInstanceOf(APIConnection.class);
|
||||
assertThat(((BearerTokenAuthentication) datasourceContext.getConnection()).getBearerToken())
|
||||
.isEqualTo(updatedBearerToken);
|
||||
})
|
||||
.verifyComplete();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user