Copilot commented on code in PR #11770:
URL: https://github.com/apache/cloudstack/pull/11770#discussion_r2489911401
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -982,6 +982,19 @@ private void
updateCustomDisplayNameOnHypervisorsList(String previousValue, Stri
}
}
+ protected String getNormalizedEmptyValueForConfig(final String name, final
String inputValue,
+ final Long configStorageId) {
+ String value = inputValue.trim();
Review Comment:
Missing null check for `inputValue`. If `inputValue` is null, calling
`trim()` will throw a NullPointerException. Based on the method signature and
test cases, the method should handle null inputs gracefully.
```suggestion
String value = StringUtils.defaultString(inputValue).trim();
```
##########
server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java:
##########
@@ -1072,4 +1072,42 @@ public void
serviceOfferingExternalDetailsNeedUpdateReturnsFalseWhenDetailsMatch
Assert.assertFalse(result);
}
+
+ @Test
+ public void
normalizedEmptyValueForConfigReturnsTrimmedValueWhenInputIsValid() {
+ String result =
configurationManagerImplSpy.getNormalizedEmptyValueForConfig("someConfig", "
validValue ", null);
+ Assert.assertEquals("validValue", result);
+ }
+
+ @Test
+ public void
normalizedEmptyValueForConfigReturnsNullWhenInputIsNullAndNoConfigStorageId() {
+ String result =
configurationManagerImplSpy.getNormalizedEmptyValueForConfig("someConfig",
"null", null);
+ Assert.assertNull(result);
+ }
+
+ @Test
+ public void
normalizedEmptyValueForConfigReturnsEmptyStringWhenInputIsNullAndConfigStorageIdProvided()
{
+ String result =
configurationManagerImplSpy.getNormalizedEmptyValueForConfig("someConfig",
"null", 123L);
+ Assert.assertEquals("", result);
+ }
+
+ @Test
+ public void
normalizedEmptyValueForConfigReturnsEmptyStringWhenKeyTypeIsStringAndInputIsEmpty()
{
+ ConfigKey<String> mockKey = Mockito.mock(ConfigKey.class);
+ Mockito.when(mockKey.type()).thenReturn(String.class);
+ Mockito.doReturn(mockKey).when(configDepot).get("someConfig");
Review Comment:
The mock is being set on `configDepot` directly, but the production code
uses `_configDepot` (with underscore prefix). This test will fail because the
spy instance `configurationManagerImplSpy` uses the `_configDepot` field which
is injected via `@InjectMocks`. The mock should be set on
`configurationManagerImplSpy._configDepot` instead, like other tests in this
file (see lines 379, 387, 394, etc.).
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/config/UpdateCfgCmd.java:
##########
@@ -161,15 +161,13 @@ public void execute() {
* Sets the configuration value in the response. If the configuration is
in the `Hidden` or `Secure` categories, the value is encrypted before being set
in the response.
* @param response to be set with the configuration `cfg` value
* @param cfg to be used in setting the response value
- * @return the response with the configuration's value
*/
- public ConfigurationResponse setResponseValue(ConfigurationResponse
response, Configuration cfg) {
+ public void setResponseValue(ConfigurationResponse response, Configuration
cfg) {
+ String value = cfg.getValue();
if (cfg.isEncrypted()) {
Review Comment:
Missing null check for `cfg.getValue()`. If `cfg.getValue()` returns null
and `cfg.isEncrypted()` is true, `DBEncryptionUtil.encrypt(null)` will be
called which may cause issues. Consider checking if value is null before
encryption, as shown in the test case on line 76.
```suggestion
if (cfg.isEncrypted() && value != null) {
```
##########
server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java:
##########
@@ -1072,4 +1072,42 @@ public void
serviceOfferingExternalDetailsNeedUpdateReturnsFalseWhenDetailsMatch
Assert.assertFalse(result);
}
+
+ @Test
+ public void
normalizedEmptyValueForConfigReturnsTrimmedValueWhenInputIsValid() {
+ String result =
configurationManagerImplSpy.getNormalizedEmptyValueForConfig("someConfig", "
validValue ", null);
+ Assert.assertEquals("validValue", result);
+ }
+
+ @Test
+ public void
normalizedEmptyValueForConfigReturnsNullWhenInputIsNullAndNoConfigStorageId() {
+ String result =
configurationManagerImplSpy.getNormalizedEmptyValueForConfig("someConfig",
"null", null);
+ Assert.assertNull(result);
+ }
+
+ @Test
+ public void
normalizedEmptyValueForConfigReturnsEmptyStringWhenInputIsNullAndConfigStorageIdProvided()
{
+ String result =
configurationManagerImplSpy.getNormalizedEmptyValueForConfig("someConfig",
"null", 123L);
+ Assert.assertEquals("", result);
+ }
+
+ @Test
+ public void
normalizedEmptyValueForConfigReturnsEmptyStringWhenKeyTypeIsStringAndInputIsEmpty()
{
+ ConfigKey<String> mockKey = Mockito.mock(ConfigKey.class);
+ Mockito.when(mockKey.type()).thenReturn(String.class);
+ Mockito.doReturn(mockKey).when(configDepot).get("someConfig");
+
+ String result =
configurationManagerImplSpy.getNormalizedEmptyValueForConfig("someConfig", "",
null);
+ Assert.assertEquals("", result);
+ }
+
+ @Test
+ public void
normalizedEmptyValueForConfigReturnsNullWhenKeyTypeIsNotStringAndInputIsEmpty()
{
+ ConfigKey<Integer> mockKey = Mockito.mock(ConfigKey.class);
+ Mockito.when(mockKey.type()).thenReturn(Integer.class);
+ Mockito.doReturn(mockKey).when(configDepot).get("someConfig");
Review Comment:
The mock is being set on `configDepot` directly, but the production code
uses `_configDepot` (with underscore prefix). This test will fail because the
spy instance `configurationManagerImplSpy` uses the `_configDepot` field which
is injected via `@InjectMocks`. The mock should be set on
`configurationManagerImplSpy._configDepot` instead, like other tests in this
file (see lines 379, 387, 394, etc.).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
