From 6e2027f1b6ffeedbfbf194ee503947467bbbc8ec Mon Sep 17 00:00:00 2001 From: Manish Kumar <107841575+sondermanish@users.noreply.github.com> Date: Wed, 18 Oct 2023 12:52:40 +0530 Subject: [PATCH] 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 --- .../external/constants/PluginConstants.java | 2 + .../ce/DatasourceContextServiceCEImpl.java | 70 +++++++----- .../DatasourceContextServiceTest.java | 102 +++++++++++++++--- 3 files changed, 137 insertions(+), 37 deletions(-) diff --git a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/PluginConstants.java b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/PluginConstants.java index 3d12fee22c..883835c55a 100644 --- a/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/PluginConstants.java +++ b/app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/PluginConstants.java @@ -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"; diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java index 5594ad7cb6..1b24142b42 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java @@ -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> getCachedDatasourceContextMono( DatasourceStorage datasourceStorage, + Plugin plugin, PluginExecutor pluginExecutor, Object monitor, DatasourceContextIdentifier datasourceContextIdentifier) { @@ -114,7 +117,7 @@ public class DatasourceContextServiceCEImpl implements DatasourceContextServiceC /* Create a fresh datasource context */ DatasourceContext 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 updateDatasourceAndSetAuthentication(Object connection, DatasourceStorage datasourceStorage) { Mono datasourceStorageMono = Mono.just(datasourceStorage); if (connection instanceof UpdatableConnection updatableConnection) { @@ -162,33 +177,38 @@ public class DatasourceContextServiceCEImpl implements DatasourceContextServiceC protected Mono> createNewDatasourceContext( DatasourceStorage datasourceStorage, DatasourceContextIdentifier datasourceContextIdentifier) { log.debug("Datasource context doesn't exist. Creating connection."); - Mono pluginMono = pluginService.findById(datasourceStorage.getPluginId()); + Mono 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 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( diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceContextServiceTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceContextServiceTest.java index e5e93b5e64..717a795037 100644 --- a/app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceContextServiceTest.java +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceContextServiceTest.java @@ -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> 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> 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> 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> failedDatasourceContextMono = datasourceContextService.getCachedDatasourceContextMono( - datasourceStorage, spyMockPluginExecutor, monitor, datasourceContextIdentifier); + datasourceStorage, emptyPlugin, spyMockPluginExecutor, monitor, datasourceContextIdentifier); StepVerifier.create(failedDatasourceContextMono) .expectError(RuntimeException.class) .verify(); Mono> 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> 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> datasourceContextMono2 = + datasourceContextService.getDatasourceContext(datasourceStorage, restApiPlugin); + StepVerifier.create(datasourceContextMono) + .assertNext(datasourceContext -> { + assertThat(datasourceContext.getConnection()).isInstanceOf(APIConnection.class); + assertThat(((BearerTokenAuthentication) datasourceContext.getConnection()).getBearerToken()) + .isEqualTo(updatedBearerToken); + }) + .verifyComplete(); + } }