From 410bd0e1b9d4a7fbd576db5f5c14fa2c6451640d Mon Sep 17 00:00:00 2001 From: osandamaleesha Date: Thu, 12 Sep 2024 13:45:41 +0530 Subject: [PATCH 01/10] Add timeout server configuratins to action execution --- .../action/execution/util/APIClient.java | 6 ++-- .../execution/util/ActionExecutorConfig.java | 35 +++++++++++++++++++ .../resources/identity.xml.j2 | 5 +++ ....identity.core.server.feature.default.json | 3 ++ 4 files changed, 46 insertions(+), 3 deletions(-) diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java index 75aaccc20a9..e75c5dc046d 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java @@ -54,9 +54,9 @@ public APIClient() { // todo: read connection configurations related to the http client of actions from the server configuration. // Initialize the http client. Set connection time out to 2s and read time out to 5s. - int readTimeout = 5000; - int connectionRequestTimeout = 2000; - int connectionTimeout = 2000; + int readTimeout = ActionExecutorConfig.getInstance().getHttpReadTimeout(); + int connectionRequestTimeout = ActionExecutorConfig.getInstance().getHttpConnectionRequestTimeout(); + int connectionTimeout = ActionExecutorConfig.getInstance().getHttpConnectionTimeout(); RequestConfig config = RequestConfig.custom() .setConnectTimeout(connectionTimeout) diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java index ff29ff9262f..14ca7c16833 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java @@ -44,6 +44,13 @@ public class ActionExecutorConfig { "Actions.ActionRequest.ExcludedHeaders.Header"; private static final String EXCLUDED_PARAMS_IN_ACTION_REQUEST_PROPERTY = "Actions.ActionRequest.ExcludedParameters.Parameter"; + private static final String HTTP_CONNECTION_TIMEOUT_PROPERTY = "Actions.HTTPConnections.HTTPConnectionTimeout"; + private static final String HTTP_READ_TIMEOUT_PROPERTY = "Actions.HTTPConnections.HTTPReadTimeout"; + private static final String HTTP_CONNECTION_REQUEST_TIMEOUT_PROPERTY = + "Actions.HTTPConnections.HTTPConnectionRequestTimeout"; + private static final int DEFAULT_HTTP_CONNECTION_TIMEOUT = 5000; + private static final int DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT = 2000; + private static final int DEFAULT_HTTP_READ_TIMEOUT = 2000; private ActionExecutorConfig() { @@ -73,6 +80,34 @@ public boolean isExecutionForActionTypeEnabled(ActionType actionType) { } } + public int getHttpConnectionTimeout() { + + return parseTimeoutConfig(HTTP_CONNECTION_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_TIMEOUT); + } + + public int getHttpConnectionRequestTimeout() { + + return parseTimeoutConfig(HTTP_CONNECTION_REQUEST_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT); + } + + public int getHttpReadTimeout() { + + return parseTimeoutConfig(HTTP_READ_TIMEOUT_PROPERTY, DEFAULT_HTTP_READ_TIMEOUT); + } + + private int parseTimeoutConfig(String timeoutTypeName, int defaultTimeout) { + + String timeoutPropertyValue = (String) IdentityConfigParser.getInstance().getConfiguration().get(timeoutTypeName); + if (StringUtils.isNotBlank(timeoutPropertyValue)) { + try { + return Integer.parseInt(timeoutPropertyValue); + } catch (Exception e) { + LOG.warn("Error occurred while parsing the '" + timeoutTypeName + "' property value in identity.xml.", e); + } + } + return defaultTimeout; + } + private boolean isActionTypeEnabled(String actionTypePropertyName) { boolean isActionTypeEnabled = false; diff --git a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 index 490feb4dcbc..fccd75b1d0f 100644 --- a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 +++ b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 @@ -2020,6 +2020,11 @@ + + {{actions.http_connections.connection_timeout}} + {{actions.http_connections.read_timeout}} + {{actions.http_connections.request_timeout}} + {{actions.maximum_actions_per_action_type}} diff --git a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json index babfa65ed47..efbd876e6b8 100644 --- a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json +++ b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json @@ -1571,6 +1571,9 @@ "on_demand_config.on_initial_use.enable_sms_otp_password_recovery_if_connector_enabled": false, + "actions.http_connections.connection_timeout": 5000, + "actions.http_connections.read_timeout": 2000, + "actions.http_connections.request_timeout": 2000, "actions.maximum_actions_per_action_type": 1, "actions.action_request.excluded_headers": [ "authorization", From 8c0743a10b5cd39ecb6cf5888eb93bc279c1648b Mon Sep 17 00:00:00 2001 From: osandamaleesha Date: Thu, 12 Sep 2024 14:45:13 +0530 Subject: [PATCH 02/10] Add a minor improvement --- .../action/execution/util/ActionExecutorConfig.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java index 14ca7c16833..85c3b376518 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java @@ -97,15 +97,16 @@ public int getHttpReadTimeout() { private int parseTimeoutConfig(String timeoutTypeName, int defaultTimeout) { - String timeoutPropertyValue = (String) IdentityConfigParser.getInstance().getConfiguration().get(timeoutTypeName); - if (StringUtils.isNotBlank(timeoutPropertyValue)) { + int timeoutPropertyValue = defaultTimeout; + String timeoutValue = (String) IdentityConfigParser.getInstance().getConfiguration().get(timeoutTypeName); + if (StringUtils.isNotBlank(timeoutValue)) { try { - return Integer.parseInt(timeoutPropertyValue); + timeoutPropertyValue = Integer.parseInt(timeoutValue); } catch (Exception e) { LOG.warn("Error occurred while parsing the '" + timeoutTypeName + "' property value in identity.xml.", e); } } - return defaultTimeout; + return timeoutPropertyValue; } private boolean isActionTypeEnabled(String actionTypePropertyName) { From 09868223356325dc47bf28b22495d9e35859ecf3 Mon Sep 17 00:00:00 2001 From: osandamaleesha Date: Thu, 12 Sep 2024 15:22:15 +0530 Subject: [PATCH 03/10] Add a value fix --- .../execution/util/ActionExecutorConfig.java | 14 +++++++------- ...arbon.identity.core.server.feature.default.json | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java index 85c3b376518..a7633a6ee7b 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java @@ -44,13 +44,13 @@ public class ActionExecutorConfig { "Actions.ActionRequest.ExcludedHeaders.Header"; private static final String EXCLUDED_PARAMS_IN_ACTION_REQUEST_PROPERTY = "Actions.ActionRequest.ExcludedParameters.Parameter"; - private static final String HTTP_CONNECTION_TIMEOUT_PROPERTY = "Actions.HTTPConnections.HTTPConnectionTimeout"; private static final String HTTP_READ_TIMEOUT_PROPERTY = "Actions.HTTPConnections.HTTPReadTimeout"; private static final String HTTP_CONNECTION_REQUEST_TIMEOUT_PROPERTY = "Actions.HTTPConnections.HTTPConnectionRequestTimeout"; - private static final int DEFAULT_HTTP_CONNECTION_TIMEOUT = 5000; + private static final String HTTP_CONNECTION_TIMEOUT_PROPERTY = "Actions.HTTPConnections.HTTPConnectionTimeout"; + private static final int DEFAULT_HTTP_READ_TIMEOUT = 5000; private static final int DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT = 2000; - private static final int DEFAULT_HTTP_READ_TIMEOUT = 2000; + private static final int DEFAULT_HTTP_CONNECTION_TIMEOUT = 2000; private ActionExecutorConfig() { @@ -80,9 +80,9 @@ public boolean isExecutionForActionTypeEnabled(ActionType actionType) { } } - public int getHttpConnectionTimeout() { + public int getHttpReadTimeout() { - return parseTimeoutConfig(HTTP_CONNECTION_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_TIMEOUT); + return parseTimeoutConfig(HTTP_READ_TIMEOUT_PROPERTY, DEFAULT_HTTP_READ_TIMEOUT); } public int getHttpConnectionRequestTimeout() { @@ -90,9 +90,9 @@ public int getHttpConnectionRequestTimeout() { return parseTimeoutConfig(HTTP_CONNECTION_REQUEST_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT); } - public int getHttpReadTimeout() { + public int getHttpConnectionTimeout() { - return parseTimeoutConfig(HTTP_READ_TIMEOUT_PROPERTY, DEFAULT_HTTP_READ_TIMEOUT); + return parseTimeoutConfig(HTTP_CONNECTION_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_TIMEOUT); } private int parseTimeoutConfig(String timeoutTypeName, int defaultTimeout) { diff --git a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json index efbd876e6b8..2823abb06cf 100644 --- a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json +++ b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json @@ -1571,9 +1571,9 @@ "on_demand_config.on_initial_use.enable_sms_otp_password_recovery_if_connector_enabled": false, - "actions.http_connections.connection_timeout": 5000, - "actions.http_connections.read_timeout": 2000, + "actions.http_connections.read_timeout": 5000, "actions.http_connections.request_timeout": 2000, + "actions.http_connections.connection_timeout": 2000, "actions.maximum_actions_per_action_type": 1, "actions.action_request.excluded_headers": [ "authorization", From e89ea4c2346f44139605a36f742988f5a0e4e84b Mon Sep 17 00:00:00 2001 From: osandamaleesha Date: Thu, 12 Sep 2024 23:36:52 +0530 Subject: [PATCH 04/10] Address reviewed changes --- .../execution/util/ActionExecutorConfig.java | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java index a7633a6ee7b..f3fe84af863 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java @@ -80,16 +80,34 @@ public boolean isExecutionForActionTypeEnabled(ActionType actionType) { } } + /** + * Retrieves the HTTP read timeout configuration. + * If the configuration value is invalid or missing, the default timeout value is parsed. + * + * @return The HTTP read timeout int value in milliseconds. + */ public int getHttpReadTimeout() { return parseTimeoutConfig(HTTP_READ_TIMEOUT_PROPERTY, DEFAULT_HTTP_READ_TIMEOUT); } + /** + * Retrieves the HTTP connection request timeout configuration. + * If the configuration value is invalid or missing, the default timeout value is parsed. + * + * @return The HTTP connection request timeout int value in milliseconds. + */ public int getHttpConnectionRequestTimeout() { return parseTimeoutConfig(HTTP_CONNECTION_REQUEST_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT); } + /** + * Retrieves the HTTP connection timeout configuration. + * If the configuration value is invalid or missing, the default timeout value is parsed. + * + * @return The HTTP connection timeout int value in milliseconds. + */ public int getHttpConnectionTimeout() { return parseTimeoutConfig(HTTP_CONNECTION_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_TIMEOUT); @@ -102,8 +120,11 @@ private int parseTimeoutConfig(String timeoutTypeName, int defaultTimeout) { if (StringUtils.isNotBlank(timeoutValue)) { try { timeoutPropertyValue = Integer.parseInt(timeoutValue); - } catch (Exception e) { - LOG.warn("Error occurred while parsing the '" + timeoutTypeName + "' property value in identity.xml.", e); + } catch (NumberFormatException e) { + if(LOG.isDebugEnabled()){ + LOG.debug(" " + timeoutTypeName + " property value must be an integer in identity.xml. " + + "Default property value is parsed ", e); + } } } return timeoutPropertyValue; From a47d1b96fcda675d6969dfb606a8bb0ab3a2b8c0 Mon Sep 17 00:00:00 2001 From: osandamaleesha Date: Thu, 12 Sep 2024 23:38:45 +0530 Subject: [PATCH 05/10] Add unit tests for ActionExecutorConfigTest class and update unit test environment --- .../ActionExecutorServiceImplTest.java | 6 ++++ .../action/execution/util/APIClientTest.java | 15 ++++++++++ .../util/ActionExecutorConfigTest.java | 28 +++++++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/ActionExecutorServiceImplTest.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/ActionExecutorServiceImplTest.java index 85d7fdc284e..99525657872 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/ActionExecutorServiceImplTest.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/ActionExecutorServiceImplTest.java @@ -48,6 +48,7 @@ import org.wso2.carbon.identity.action.execution.model.User; import org.wso2.carbon.identity.action.execution.model.UserStore; import org.wso2.carbon.identity.action.execution.util.APIClient; +import org.wso2.carbon.identity.action.execution.util.ActionExecutorConfig; import org.wso2.carbon.identity.action.execution.util.RequestFilter; import org.wso2.carbon.identity.action.management.ActionManagementService; import org.wso2.carbon.identity.action.management.exception.ActionMgtException; @@ -84,6 +85,7 @@ public class ActionExecutorServiceImplTest { private APIClient apiClient; @InjectMocks private ActionExecutorServiceImpl actionExecutorService; + private MockedStatic actionExecutorConfigStatic; private MockedStatic requestFilter; private MockedStatic actionExecutionRequestBuilderFactory; @@ -92,6 +94,9 @@ public class ActionExecutorServiceImplTest { @BeforeMethod public void setUp() throws Exception { + actionExecutorConfigStatic = mockStatic(ActionExecutorConfig.class); + ActionExecutorConfig actionExecutorConfig = mock(ActionExecutorConfig.class); + actionExecutorConfigStatic.when(ActionExecutorConfig::getInstance).thenReturn(actionExecutorConfig); MockitoAnnotations.openMocks(this); ActionExecutionServiceComponentHolder actionExecutionServiceComponentHolder = ActionExecutionServiceComponentHolder.getInstance(); @@ -110,6 +115,7 @@ public void tearDown() { requestFilter.close(); actionExecutionRequestBuilderFactory.close(); actionExecutionResponseProcessorFactory.close(); + actionExecutorConfigStatic.close(); } @Test diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/APIClientTest.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/APIClientTest.java index 8747394dac2..dbe1ab0423b 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/APIClientTest.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/APIClientTest.java @@ -30,7 +30,9 @@ import org.apache.http.impl.client.CloseableHttpClient; import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.MockedStatic; import org.mockito.MockitoAnnotations; +import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; @@ -46,6 +48,8 @@ import java.util.HashMap; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -66,16 +70,27 @@ public class APIClientTest { @Mock private StatusLine statusLine; + private MockedStatic actionExecutorConfigStatic; + @InjectMocks private APIClient apiClient; @BeforeMethod public void setUp() throws Exception { + actionExecutorConfigStatic = mockStatic(ActionExecutorConfig.class); + ActionExecutorConfig actionExecutorConfig = mock(ActionExecutorConfig.class); + actionExecutorConfigStatic.when(ActionExecutorConfig::getInstance).thenReturn(actionExecutorConfig); MockitoAnnotations.openMocks(this); setField(apiClient, "httpClient", httpClient); } + @AfterMethod + public void tearDown() { + + actionExecutorConfigStatic.close(); + } + @Test public void testCallAPIUnacceptableContentTypeForSuccessResponse() throws Exception { diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java index db686324915..901d350b939 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java @@ -39,6 +39,7 @@ import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; +import org.testng.Assert; public class ActionExecutorConfigTest { @@ -302,4 +303,31 @@ public void testGetExcludedParamsInActionRequestForEmptyConfigForAllAndDefinedTy actionExecutorConfig.getExcludedParamsInActionRequestForActionType(ActionType.PRE_ISSUE_ACCESS_TOKEN); assertEquals(excludedHeaders, Collections.emptySet()); } + + @Test + public void testGetHttpReadTimeout(){ + + Map configMap = new HashMap<>(); + configMap.put("Actions.HTTPConnections.HTTPReadTimeout", "5000"); + when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); + Assert.assertEquals(5000, actionExecutorConfig.getHttpReadTimeout()); + } + + @Test + public void testGetHttpConnectionRequestTimeout(){ + + Map configMap = new HashMap<>(); + configMap.put("Actions.HTTPConnections.HTTPConnectionRequestTimeout", "2000"); + when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); + Assert.assertEquals(2000, actionExecutorConfig.getHttpConnectionRequestTimeout()); + } + + @Test + public void testGetHttpConnectionTimeout(){ + + Map configMap = new HashMap<>(); + configMap.put("Actions.HTTPConnections.HTTPConnectionTimeout", "2000"); + when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); + Assert.assertEquals(2000, actionExecutorConfig.getHttpConnectionTimeout()); + } } From b3c446a1d49a4bfd2ccbe3b8b61623feda246ee1 Mon Sep 17 00:00:00 2001 From: osandamaleesha Date: Fri, 13 Sep 2024 00:05:48 +0530 Subject: [PATCH 06/10] Address checkstyles --- .../action/execution/util/ActionExecutorConfig.java | 2 +- .../action/execution/util/ActionExecutorConfigTest.java | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java index f3fe84af863..cabbeb84a3c 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java @@ -121,7 +121,7 @@ private int parseTimeoutConfig(String timeoutTypeName, int defaultTimeout) { try { timeoutPropertyValue = Integer.parseInt(timeoutValue); } catch (NumberFormatException e) { - if(LOG.isDebugEnabled()){ + if (LOG.isDebugEnabled()) { LOG.debug(" " + timeoutTypeName + " property value must be an integer in identity.xml. " + "Default property value is parsed ", e); } diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java index 901d350b939..1b7749ab3d4 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java @@ -21,6 +21,7 @@ import org.mockito.Mock; import org.mockito.MockedStatic; import org.mockito.MockitoAnnotations; +import org.testng.Assert; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -39,7 +40,6 @@ import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; -import org.testng.Assert; public class ActionExecutorConfigTest { @@ -305,7 +305,7 @@ public void testGetExcludedParamsInActionRequestForEmptyConfigForAllAndDefinedTy } @Test - public void testGetHttpReadTimeout(){ + public void testGetHttpReadTimeout() { Map configMap = new HashMap<>(); configMap.put("Actions.HTTPConnections.HTTPReadTimeout", "5000"); @@ -314,7 +314,7 @@ public void testGetHttpReadTimeout(){ } @Test - public void testGetHttpConnectionRequestTimeout(){ + public void testGetHttpConnectionRequestTimeout() { Map configMap = new HashMap<>(); configMap.put("Actions.HTTPConnections.HTTPConnectionRequestTimeout", "2000"); @@ -323,7 +323,7 @@ public void testGetHttpConnectionRequestTimeout(){ } @Test - public void testGetHttpConnectionTimeout(){ + public void testGetHttpConnectionTimeout() { Map configMap = new HashMap<>(); configMap.put("Actions.HTTPConnections.HTTPConnectionTimeout", "2000"); From 5f6e873c091b324a61192f2e25555d1675e2934c Mon Sep 17 00:00:00 2001 From: osandamaleesha Date: Fri, 13 Sep 2024 13:25:26 +0530 Subject: [PATCH 07/10] Address reviewed changes --- .../action/execution/util/APIClient.java | 8 +-- .../execution/util/ActionExecutorConfig.java | 49 ++++++++++++++----- .../util/ActionExecutorConfigTest.java | 39 ++++++++++++--- .../resources/identity.xml.j2 | 1 + ....identity.core.server.feature.default.json | 1 + 5 files changed, 77 insertions(+), 21 deletions(-) diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java index e75c5dc046d..b1cf59fbc06 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java @@ -54,9 +54,9 @@ public APIClient() { // todo: read connection configurations related to the http client of actions from the server configuration. // Initialize the http client. Set connection time out to 2s and read time out to 5s. - int readTimeout = ActionExecutorConfig.getInstance().getHttpReadTimeout(); - int connectionRequestTimeout = ActionExecutorConfig.getInstance().getHttpConnectionRequestTimeout(); - int connectionTimeout = ActionExecutorConfig.getInstance().getHttpConnectionTimeout(); + int readTimeout = ActionExecutorConfig.getInstance().getHttpReadTimeoutInMillis(); + int connectionRequestTimeout = ActionExecutorConfig.getInstance().getHttpConnectionRequestTimeoutInMillis(); + int connectionTimeout = ActionExecutorConfig.getInstance().getHttpConnectionTimeoutInMillis(); RequestConfig config = RequestConfig.custom() .setConnectTimeout(connectionTimeout) @@ -66,7 +66,7 @@ public APIClient() { .setRelativeRedirectsAllowed(false) .build(); PoolingHttpClientConnectionManager connectionManager = new PoolingHttpClientConnectionManager(); - connectionManager.setMaxTotal(20); + connectionManager.setMaxTotal(ActionExecutorConfig.getInstance().getHttpClientConnectionPoolSize()); httpClient = HttpClientBuilder.create().setDefaultRequestConfig(config).setConnectionManager(connectionManager) .build(); } diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java index cabbeb84a3c..febda78cb9c 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java @@ -48,9 +48,11 @@ public class ActionExecutorConfig { private static final String HTTP_CONNECTION_REQUEST_TIMEOUT_PROPERTY = "Actions.HTTPConnections.HTTPConnectionRequestTimeout"; private static final String HTTP_CONNECTION_TIMEOUT_PROPERTY = "Actions.HTTPConnections.HTTPConnectionTimeout"; - private static final int DEFAULT_HTTP_READ_TIMEOUT = 5000; - private static final int DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT = 2000; - private static final int DEFAULT_HTTP_CONNECTION_TIMEOUT = 2000; + private static final String HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY = "Actions.HTTPClientConnectionPoolSize"; + private static final int DEFAULT_HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY = 20; + private static final int DEFAULT_HTTP_READ_TIMEOUT_IN_MILLIS = 5000; + private static final int DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT_MILLIS = 2000; + private static final int DEFAULT_HTTP_CONNECTION_TIMEOUT_MILLIS = 2000; private ActionExecutorConfig() { @@ -80,15 +82,39 @@ public boolean isExecutionForActionTypeEnabled(ActionType actionType) { } } + /** + * Returns the HTTP client connection pool size based on the system configuration. + * + * @return The HTTP client connection pool size, or the default if the property is missing or invalid. + */ + public int getHttpClientConnectionPoolSize() { + + int poolSizePropertyValue = DEFAULT_HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY; + String poolSizeValue = (String) IdentityConfigParser.getInstance().getConfiguration(). + get(HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY); + if (StringUtils.isNotBlank(poolSizeValue)) { + try { + poolSizePropertyValue = Integer.parseInt(poolSizeValue); + } catch (NumberFormatException e) { + if (LOG.isDebugEnabled()) { + LOG.debug("Failed to read Http client connection pool size property in identity.xml." + + " Expects a number. Using the default value: " + + DEFAULT_HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY, e); + } + } + } + return poolSizePropertyValue; + } + /** * Retrieves the HTTP read timeout configuration. * If the configuration value is invalid or missing, the default timeout value is parsed. * * @return The HTTP read timeout int value in milliseconds. */ - public int getHttpReadTimeout() { + public int getHttpReadTimeoutInMillis() { - return parseTimeoutConfig(HTTP_READ_TIMEOUT_PROPERTY, DEFAULT_HTTP_READ_TIMEOUT); + return parseTimeoutConfig(HTTP_READ_TIMEOUT_PROPERTY, DEFAULT_HTTP_READ_TIMEOUT_IN_MILLIS); } /** @@ -97,9 +123,10 @@ public int getHttpReadTimeout() { * * @return The HTTP connection request timeout int value in milliseconds. */ - public int getHttpConnectionRequestTimeout() { + public int getHttpConnectionRequestTimeoutInMillis() { - return parseTimeoutConfig(HTTP_CONNECTION_REQUEST_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT); + return parseTimeoutConfig(HTTP_CONNECTION_REQUEST_TIMEOUT_PROPERTY, + DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT_MILLIS); } /** @@ -108,9 +135,9 @@ public int getHttpConnectionRequestTimeout() { * * @return The HTTP connection timeout int value in milliseconds. */ - public int getHttpConnectionTimeout() { + public int getHttpConnectionTimeoutInMillis() { - return parseTimeoutConfig(HTTP_CONNECTION_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_TIMEOUT); + return parseTimeoutConfig(HTTP_CONNECTION_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_TIMEOUT_MILLIS); } private int parseTimeoutConfig(String timeoutTypeName, int defaultTimeout) { @@ -122,8 +149,8 @@ private int parseTimeoutConfig(String timeoutTypeName, int defaultTimeout) { timeoutPropertyValue = Integer.parseInt(timeoutValue); } catch (NumberFormatException e) { if (LOG.isDebugEnabled()) { - LOG.debug(" " + timeoutTypeName + " property value must be an integer in identity.xml. " + - "Default property value is parsed ", e); + LOG.debug("Failed to read " + timeoutTypeName + " property in identity.xml." + + " Expects a number. Using the default value: " + defaultTimeout, e); } } } diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java index 1b7749ab3d4..80ac4d6ad28 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java @@ -305,29 +305,56 @@ public void testGetExcludedParamsInActionRequestForEmptyConfigForAllAndDefinedTy } @Test - public void testGetHttpReadTimeout() { + public void testGetHttpReadTimeoutInMillis() { Map configMap = new HashMap<>(); configMap.put("Actions.HTTPConnections.HTTPReadTimeout", "5000"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); - Assert.assertEquals(5000, actionExecutorConfig.getHttpReadTimeout()); + Assert.assertEquals(5000, actionExecutorConfig.getHttpReadTimeoutInMillis()); } @Test - public void testGetHttpConnectionRequestTimeout() { + public void testGetHttpReadTimeoutInMillisWithoutNumberFormat() { + + Map configMap = new HashMap<>(); + configMap.put("Actions.HTTPConnections.HTTPReadTimeout", "value"); + when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); + Assert.assertEquals(5000, actionExecutorConfig.getHttpReadTimeoutInMillis()); + } + + @Test + public void testGetHttpConnectionRequestTimeoutInMillis() { Map configMap = new HashMap<>(); configMap.put("Actions.HTTPConnections.HTTPConnectionRequestTimeout", "2000"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); - Assert.assertEquals(2000, actionExecutorConfig.getHttpConnectionRequestTimeout()); + Assert.assertEquals(2000, actionExecutorConfig.getHttpConnectionRequestTimeoutInMillis()); } @Test - public void testGetHttpConnectionTimeout() { + public void testGetHttpConnectionTimeoutInMillis() { Map configMap = new HashMap<>(); configMap.put("Actions.HTTPConnections.HTTPConnectionTimeout", "2000"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); - Assert.assertEquals(2000, actionExecutorConfig.getHttpConnectionTimeout()); + Assert.assertEquals(2000, actionExecutorConfig.getHttpConnectionTimeoutInMillis()); + } + + @Test + public void testGetHttpClientConnectionPoolSize() { + + Map configMap = new HashMap<>(); + configMap.put("Actions.HTTPClientConnectionPoolSize", "20"); + when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); + Assert.assertEquals(20, actionExecutorConfig.getHttpClientConnectionPoolSize()); + } + + @Test + public void testGetHttpClientConnectionPoolSizeWithoutNumberFormat() { + + Map configMap = new HashMap<>(); + configMap.put("Actions.HTTPClientConnectionPoolSize", "value"); + when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); + Assert.assertEquals(20, actionExecutorConfig.getHttpClientConnectionPoolSize()); } } diff --git a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 index fccd75b1d0f..fd9ec6dfc37 100644 --- a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 +++ b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 @@ -2025,6 +2025,7 @@ {{actions.http_connections.read_timeout}} {{actions.http_connections.request_timeout}} + {{actions.http_client_connection_pool_size}} {{actions.maximum_actions_per_action_type}} diff --git a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json index 2823abb06cf..c3701720b56 100644 --- a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json +++ b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json @@ -1574,6 +1574,7 @@ "actions.http_connections.read_timeout": 5000, "actions.http_connections.request_timeout": 2000, "actions.http_connections.connection_timeout": 2000, + "actions.http_client_connection_pool_size": 20, "actions.maximum_actions_per_action_type": 1, "actions.action_request.excluded_headers": [ "authorization", From d3435deafaa2baea562aae75e85180c9a5838a7e Mon Sep 17 00:00:00 2001 From: osandamaleesha Date: Wed, 18 Sep 2024 10:26:55 +0530 Subject: [PATCH 08/10] Address reviewed changes --- .../action/execution/util/APIClient.java | 2 +- .../execution/util/ActionExecutorConfig.java | 42 +++++++++---------- .../util/ActionExecutorConfigTest.java | 20 ++++----- .../resources/identity.xml.j2 | 12 +++--- ....identity.core.server.feature.default.json | 8 ++-- 5 files changed, 40 insertions(+), 44 deletions(-) diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java index b1cf59fbc06..a7ffd215690 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java @@ -66,7 +66,7 @@ public APIClient() { .setRelativeRedirectsAllowed(false) .build(); PoolingHttpClientConnectionManager connectionManager = new PoolingHttpClientConnectionManager(); - connectionManager.setMaxTotal(ActionExecutorConfig.getInstance().getHttpClientConnectionPoolSize()); + connectionManager.setMaxTotal(ActionExecutorConfig.getInstance().getHttpConnectionPoolSize()); httpClient = HttpClientBuilder.create().setDefaultRequestConfig(config).setConnectionManager(connectionManager) .build(); } diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java index febda78cb9c..ef1324e5bcf 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java @@ -44,15 +44,15 @@ public class ActionExecutorConfig { "Actions.ActionRequest.ExcludedHeaders.Header"; private static final String EXCLUDED_PARAMS_IN_ACTION_REQUEST_PROPERTY = "Actions.ActionRequest.ExcludedParameters.Parameter"; - private static final String HTTP_READ_TIMEOUT_PROPERTY = "Actions.HTTPConnections.HTTPReadTimeout"; + private static final String HTTP_READ_TIMEOUT_PROPERTY = "Actions.HTTPClient.HTTPReadTimeout"; private static final String HTTP_CONNECTION_REQUEST_TIMEOUT_PROPERTY = - "Actions.HTTPConnections.HTTPConnectionRequestTimeout"; - private static final String HTTP_CONNECTION_TIMEOUT_PROPERTY = "Actions.HTTPConnections.HTTPConnectionTimeout"; - private static final String HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY = "Actions.HTTPClientConnectionPoolSize"; - private static final int DEFAULT_HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY = 20; + "Actions.HTTPClient.HTTPConnectionRequestTimeout"; + private static final String HTTP_CONNECTION_TIMEOUT_PROPERTY = "Actions.HTTPClient.HTTPConnectionTimeout"; + private static final String HTTP_CONNECTION_POOL_SIZE_PROPERTY = "Actions.HTTPClient.HTTPConnectionPoolSize"; + private static final int DEFAULT_HTTP_CONNECTION_POOL_SIZE = 20; private static final int DEFAULT_HTTP_READ_TIMEOUT_IN_MILLIS = 5000; - private static final int DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT_MILLIS = 2000; - private static final int DEFAULT_HTTP_CONNECTION_TIMEOUT_MILLIS = 2000; + private static final int DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT_IN_MILLIS = 2000; + private static final int DEFAULT_HTTP_CONNECTION_TIMEOUT_IN_MILLIS = 2000; private ActionExecutorConfig() { @@ -83,24 +83,22 @@ public boolean isExecutionForActionTypeEnabled(ActionType actionType) { } /** - * Returns the HTTP client connection pool size based on the system configuration. + * Returns the HTTP connection pool size based on the system configuration. * - * @return The HTTP client connection pool size, or the default if the property is missing or invalid. + * @return The HTTP connection pool size, or the default if the property is missing or invalid. */ - public int getHttpClientConnectionPoolSize() { + public int getHttpConnectionPoolSize() { - int poolSizePropertyValue = DEFAULT_HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY; + int poolSizePropertyValue = DEFAULT_HTTP_CONNECTION_POOL_SIZE; String poolSizeValue = (String) IdentityConfigParser.getInstance().getConfiguration(). - get(HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY); + get(HTTP_CONNECTION_POOL_SIZE_PROPERTY); if (StringUtils.isNotBlank(poolSizeValue)) { try { poolSizePropertyValue = Integer.parseInt(poolSizeValue); } catch (NumberFormatException e) { - if (LOG.isDebugEnabled()) { - LOG.debug("Failed to read Http client connection pool size property in identity.xml." + - " Expects a number. Using the default value: " + - DEFAULT_HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY, e); - } + LOG.debug("Failed to read Http client connection pool size property in identity.xml." + + " Expects a number. Using the default value: " + + DEFAULT_HTTP_CONNECTION_POOL_SIZE, e); } } return poolSizePropertyValue; @@ -126,7 +124,7 @@ public int getHttpReadTimeoutInMillis() { public int getHttpConnectionRequestTimeoutInMillis() { return parseTimeoutConfig(HTTP_CONNECTION_REQUEST_TIMEOUT_PROPERTY, - DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT_MILLIS); + DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT_IN_MILLIS); } /** @@ -137,7 +135,7 @@ public int getHttpConnectionRequestTimeoutInMillis() { */ public int getHttpConnectionTimeoutInMillis() { - return parseTimeoutConfig(HTTP_CONNECTION_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_TIMEOUT_MILLIS); + return parseTimeoutConfig(HTTP_CONNECTION_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_TIMEOUT_IN_MILLIS); } private int parseTimeoutConfig(String timeoutTypeName, int defaultTimeout) { @@ -148,10 +146,8 @@ private int parseTimeoutConfig(String timeoutTypeName, int defaultTimeout) { try { timeoutPropertyValue = Integer.parseInt(timeoutValue); } catch (NumberFormatException e) { - if (LOG.isDebugEnabled()) { - LOG.debug("Failed to read " + timeoutTypeName + " property in identity.xml." + - " Expects a number. Using the default value: " + defaultTimeout, e); - } + LOG.debug("Failed to read " + timeoutTypeName + " property in identity.xml." + + " Expects a number. Using the default value: " + defaultTimeout, e); } } return timeoutPropertyValue; diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java index 80ac4d6ad28..6432e57c831 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java @@ -308,7 +308,7 @@ public void testGetExcludedParamsInActionRequestForEmptyConfigForAllAndDefinedTy public void testGetHttpReadTimeoutInMillis() { Map configMap = new HashMap<>(); - configMap.put("Actions.HTTPConnections.HTTPReadTimeout", "5000"); + configMap.put("Actions.HTTPClient.HTTPReadTimeout", "5000"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); Assert.assertEquals(5000, actionExecutorConfig.getHttpReadTimeoutInMillis()); } @@ -317,7 +317,7 @@ public void testGetHttpReadTimeoutInMillis() { public void testGetHttpReadTimeoutInMillisWithoutNumberFormat() { Map configMap = new HashMap<>(); - configMap.put("Actions.HTTPConnections.HTTPReadTimeout", "value"); + configMap.put("Actions.HTTPClient.HTTPReadTimeout", "value"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); Assert.assertEquals(5000, actionExecutorConfig.getHttpReadTimeoutInMillis()); } @@ -326,7 +326,7 @@ public void testGetHttpReadTimeoutInMillisWithoutNumberFormat() { public void testGetHttpConnectionRequestTimeoutInMillis() { Map configMap = new HashMap<>(); - configMap.put("Actions.HTTPConnections.HTTPConnectionRequestTimeout", "2000"); + configMap.put("Actions.HTTPClient.HTTPConnectionRequestTimeout", "2000"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); Assert.assertEquals(2000, actionExecutorConfig.getHttpConnectionRequestTimeoutInMillis()); } @@ -335,26 +335,26 @@ public void testGetHttpConnectionRequestTimeoutInMillis() { public void testGetHttpConnectionTimeoutInMillis() { Map configMap = new HashMap<>(); - configMap.put("Actions.HTTPConnections.HTTPConnectionTimeout", "2000"); + configMap.put("Actions.HTTPClient.HTTPConnectionTimeout", "2000"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); Assert.assertEquals(2000, actionExecutorConfig.getHttpConnectionTimeoutInMillis()); } @Test - public void testGetHttpClientConnectionPoolSize() { + public void testGetHttpConnectionPoolSize() { Map configMap = new HashMap<>(); - configMap.put("Actions.HTTPClientConnectionPoolSize", "20"); + configMap.put("Actions.HTTPClient.HTTPConnectionPoolSize", "20"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); - Assert.assertEquals(20, actionExecutorConfig.getHttpClientConnectionPoolSize()); + Assert.assertEquals(20, actionExecutorConfig.getHttpConnectionPoolSize()); } @Test - public void testGetHttpClientConnectionPoolSizeWithoutNumberFormat() { + public void testGetHttpConnectionPoolSizeWithoutNumberFormat() { Map configMap = new HashMap<>(); - configMap.put("Actions.HTTPClientConnectionPoolSize", "value"); + configMap.put("Actions.HTTPClient.HTTPConnectionPoolSize", "value"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); - Assert.assertEquals(20, actionExecutorConfig.getHttpClientConnectionPoolSize()); + Assert.assertEquals(20, actionExecutorConfig.getHttpConnectionPoolSize()); } } diff --git a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 index fd9ec6dfc37..cb471740c3e 100644 --- a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 +++ b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 @@ -2020,12 +2020,12 @@ - - {{actions.http_connections.connection_timeout}} - {{actions.http_connections.read_timeout}} - {{actions.http_connections.request_timeout}} - - {{actions.http_client_connection_pool_size}} + + {{actions.http_client.connection_timeout}} + {{actions.http_client.read_timeout}} + {{actions.http_client.request_timeout}} + {{actions.http_client.connection_pool_size}} + {{actions.maximum_actions_per_action_type}} diff --git a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json index c3701720b56..582a1d5b139 100644 --- a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json +++ b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json @@ -1571,10 +1571,10 @@ "on_demand_config.on_initial_use.enable_sms_otp_password_recovery_if_connector_enabled": false, - "actions.http_connections.read_timeout": 5000, - "actions.http_connections.request_timeout": 2000, - "actions.http_connections.connection_timeout": 2000, - "actions.http_client_connection_pool_size": 20, + "actions.http_client.connection_timeout": 2000, + "actions.http_client.read_timeout": 5000, + "actions.http_client.request_timeout": 2000, + "actions.http_client.connection_pool_size": 20, "actions.maximum_actions_per_action_type": 1, "actions.action_request.excluded_headers": [ "authorization", From edba811642bfb3eb64866ebfef8efd9d67ae484e Mon Sep 17 00:00:00 2001 From: osandamaleesha Date: Wed, 18 Sep 2024 15:19:22 +0530 Subject: [PATCH 09/10] Improve test case method names --- .../action/execution/util/ActionExecutorConfigTest.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java index 6432e57c831..421835fbec0 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java @@ -314,8 +314,9 @@ public void testGetHttpReadTimeoutInMillis() { } @Test - public void testGetHttpReadTimeoutInMillisWithoutNumberFormat() { + public void testGetHttpReadTimeoutInMillisForInvalidConfig() { + //If the server configuration value is not a number, the default http read timeout value of 5000 is parsed Map configMap = new HashMap<>(); configMap.put("Actions.HTTPClient.HTTPReadTimeout", "value"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); @@ -350,8 +351,9 @@ public void testGetHttpConnectionPoolSize() { } @Test - public void testGetHttpConnectionPoolSizeWithoutNumberFormat() { + public void testGetHttpConnectionPoolSizeForInvalidConfig() { + //If the server configuration value is not a number, the default http connection pool size value of 20 is parsed Map configMap = new HashMap<>(); configMap.put("Actions.HTTPClient.HTTPConnectionPoolSize", "value"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); From a8fffd8b4a562d08287595667140b676a673bf3b Mon Sep 17 00:00:00 2001 From: osandamaleesha Date: Thu, 19 Sep 2024 10:23:01 +0530 Subject: [PATCH 10/10] Add retry count server configurations and add test cases --- .../action/execution/util/APIClient.java | 2 +- .../execution/util/ActionExecutorConfig.java | 24 +++++++++++++++++++ .../action/execution/util/APIClientTest.java | 1 + .../util/ActionExecutorConfigTest.java | 19 +++++++++++++++ .../resources/identity.xml.j2 | 1 + ....identity.core.server.feature.default.json | 1 + 6 files changed, 47 insertions(+), 1 deletion(-) diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java index a7ffd215690..9d6e3926304 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java @@ -94,7 +94,7 @@ private void setRequestEntity(HttpPost httpPost, String jsonRequest, AuthMethods private ActionInvocationResponse executeRequest(HttpPost request) { int attempts = 0; - int retryCount = 2; // todo: read from server configurations + int retryCount = ActionExecutorConfig.getInstance().getHttpRequestRetryCount(); ActionInvocationResponse actionInvocationResponse = null; while (attempts < retryCount) { diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java index ef1324e5bcf..f8e4e7f4122 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java @@ -49,6 +49,8 @@ public class ActionExecutorConfig { "Actions.HTTPClient.HTTPConnectionRequestTimeout"; private static final String HTTP_CONNECTION_TIMEOUT_PROPERTY = "Actions.HTTPClient.HTTPConnectionTimeout"; private static final String HTTP_CONNECTION_POOL_SIZE_PROPERTY = "Actions.HTTPClient.HTTPConnectionPoolSize"; + private static final String HTTP_REQUEST_RETRY_COUNT_PROPERTY = "Actions.HTTPClient.HTTPRequestRetryCount"; + private static final int DEFAULT_HTTP_REQUEST_RETRY_COUNT = 2; private static final int DEFAULT_HTTP_CONNECTION_POOL_SIZE = 20; private static final int DEFAULT_HTTP_READ_TIMEOUT_IN_MILLIS = 5000; private static final int DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT_IN_MILLIS = 2000; @@ -82,6 +84,28 @@ public boolean isExecutionForActionTypeEnabled(ActionType actionType) { } } + /** + * Returns the HTTP request retry count based on the system configuration. + * + * @return The HTTP request retry count, or the default if the property is missing or invalid. + */ + public int getHttpRequestRetryCount() { + + int retryCountPropertyValue = DEFAULT_HTTP_REQUEST_RETRY_COUNT; + String retryCountValue = (String) IdentityConfigParser.getInstance().getConfiguration(). + get(HTTP_REQUEST_RETRY_COUNT_PROPERTY); + if (StringUtils.isNotBlank(retryCountValue)) { + try { + retryCountPropertyValue = Integer.parseInt(retryCountValue); + } catch (NumberFormatException e) { + LOG.debug("Failed to read Http request retry count property in identity.xml." + + " Expects a number. Using the default value: " + + DEFAULT_HTTP_REQUEST_RETRY_COUNT, e); + } + } + return retryCountPropertyValue; + } + /** * Returns the HTTP connection pool size based on the system configuration. * diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/APIClientTest.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/APIClientTest.java index dbe1ab0423b..eb2b0491ab0 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/APIClientTest.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/APIClientTest.java @@ -82,6 +82,7 @@ public void setUp() throws Exception { ActionExecutorConfig actionExecutorConfig = mock(ActionExecutorConfig.class); actionExecutorConfigStatic.when(ActionExecutorConfig::getInstance).thenReturn(actionExecutorConfig); MockitoAnnotations.openMocks(this); + when(actionExecutorConfig.getHttpRequestRetryCount()).thenReturn(2); setField(apiClient, "httpClient", httpClient); } diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java index 421835fbec0..4bfeb21ef8b 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java @@ -359,4 +359,23 @@ public void testGetHttpConnectionPoolSizeForInvalidConfig() { when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); Assert.assertEquals(20, actionExecutorConfig.getHttpConnectionPoolSize()); } + + @Test + public void testGetHttpRequestRetryCount() { + + Map configMap = new HashMap<>(); + configMap.put("Actions.HTTPClient.HTTPRequestRetryCount", "2"); + when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); + Assert.assertEquals(2, actionExecutorConfig.getHttpRequestRetryCount()); + } + + @Test + public void testGetHttpRequestRetryCountForInvalidConfig() { + + //If the server configuration value is not a number, the default http request retry count value of 2 is parsed + Map configMap = new HashMap<>(); + configMap.put("Actions.HTTPClient.HTTPRequestRetryCount", "value"); + when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); + Assert.assertEquals(2, actionExecutorConfig.getHttpRequestRetryCount()); + } } diff --git a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 index cb471740c3e..f044b6c75da 100644 --- a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 +++ b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 @@ -2025,6 +2025,7 @@ {{actions.http_client.read_timeout}} {{actions.http_client.request_timeout}} {{actions.http_client.connection_pool_size}} + {{actions.http_client.retry_count}} {{actions.maximum_actions_per_action_type}} diff --git a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json index 582a1d5b139..6dfbe477136 100644 --- a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json +++ b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json @@ -1575,6 +1575,7 @@ "actions.http_client.read_timeout": 5000, "actions.http_client.request_timeout": 2000, "actions.http_client.connection_pool_size": 20, + "actions.http_client.retry_count": 2, "actions.maximum_actions_per_action_type": 1, "actions.action_request.excluded_headers": [ "authorization",