JoaoJandre commented on code in PR #9107:
URL: https://github.com/apache/cloudstack/pull/9107#discussion_r1727039772
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -1211,132 +1215,142 @@ protected String validateConfigurationValue(final
String name, String value, fin
} else {
type = configuration.getType();
}
- //no need to validate further if a
- //config can have null value.
- String errMsg = null;
- try {
- if (type.equals(Integer.class)) {
- errMsg = "There was error in trying to parse value: " + value
+ ". Please enter a valid integer value for parameter " + name;
- Integer.parseInt(value);
- } else if (type.equals(Float.class)) {
- errMsg = "There was error in trying to parse value: " + value
+ ". Please enter a valid float value for parameter " + name;
- Float.parseFloat(value);
- } else if (type.equals(Long.class)) {
- errMsg = "There was error in trying to parse value: " + value
+ ". Please enter a valid long value for parameter " + name;
- Long.parseLong(value);
- }
- } catch (final Exception e) {
- // catching generic exception as some throws NullPointerException
and some throws NumberFormatExcpeion
- logger.error(errMsg);
- return errMsg;
+
+ boolean isTypeValid = validateValueType(value, type);
+ if (!isTypeValid) {
+ return String.format("Value [%s] is not a valid [%s].", value,
type);
}
- if (value == null) {
- if (type.equals(Boolean.class)) {
- return "Please enter either 'true' or 'false'.";
- }
- if (overprovisioningFactorsForValidation.contains(name)) {
- final String msg = "value cannot be null for the parameter " +
name;
- logger.error(msg);
- return msg;
- }
- return null;
+ return validateValueRange(name, value, type, configuration);
+ }
+
+ /**
+ * Returns whether a value is valid for a configuration of the provided
type.
+ * Valid configuration values are:
+ *
+ * <ul>
+ * <li>String: any value, including null;</li>
+ * <li>Character: any value, including null;</li>
+ * <li>Boolean: strings that contain "true" or "false"
(case-sensitive);</li>
+ * <li>Integer: strings that contain exactly an integer. Values that
contain a decimal aren't valid;</li>
+ * <li>Short, Long, Float and Double: strings that contain an integer
or a decimal.</li>
+ * </ul>
+ *
+ * If a type isn't listed here, then the value will be considered invalid.
+ * @param value value to validate.
+ * @param type type of the configuration.
+ * @return boolean indicating whether the value is valid.
+ */
+ protected boolean validateValueType(String value, Class<?> type) {
+ if (type == String.class || type == Character.class) {
+ return true;
}
- value = value.trim();
try {
- if (overprovisioningFactorsForValidation.contains(name) &&
Float.parseFloat(value) <= 0f) {
- final String msg = name + " should be greater than 0";
- logger.error(msg);
- throw new InvalidParameterValueException(msg);
+ if (type == Boolean.class) {
+ return value.equals("true") || value.equals("false");
+ } else if (type == Integer.class || type == Long.class || type ==
Short.class) {
+ Integer.parseInt(value);
Review Comment:
If the value is a long, will Integer.parseInt() be able to parse it?
Shouldn't we use Long.parseLong()?
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -1211,132 +1215,142 @@ protected String validateConfigurationValue(final
String name, String value, fin
} else {
type = configuration.getType();
}
- //no need to validate further if a
- //config can have null value.
- String errMsg = null;
- try {
- if (type.equals(Integer.class)) {
- errMsg = "There was error in trying to parse value: " + value
+ ". Please enter a valid integer value for parameter " + name;
- Integer.parseInt(value);
- } else if (type.equals(Float.class)) {
- errMsg = "There was error in trying to parse value: " + value
+ ". Please enter a valid float value for parameter " + name;
- Float.parseFloat(value);
- } else if (type.equals(Long.class)) {
- errMsg = "There was error in trying to parse value: " + value
+ ". Please enter a valid long value for parameter " + name;
- Long.parseLong(value);
- }
- } catch (final Exception e) {
- // catching generic exception as some throws NullPointerException
and some throws NumberFormatExcpeion
- logger.error(errMsg);
- return errMsg;
+
+ boolean isTypeValid = validateValueType(value, type);
+ if (!isTypeValid) {
+ return String.format("Value [%s] is not a valid [%s].", value,
type);
}
- if (value == null) {
- if (type.equals(Boolean.class)) {
- return "Please enter either 'true' or 'false'.";
- }
- if (overprovisioningFactorsForValidation.contains(name)) {
- final String msg = "value cannot be null for the parameter " +
name;
- logger.error(msg);
- return msg;
- }
- return null;
+ return validateValueRange(name, value, type, configuration);
+ }
+
+ /**
+ * Returns whether a value is valid for a configuration of the provided
type.
+ * Valid configuration values are:
+ *
+ * <ul>
+ * <li>String: any value, including null;</li>
+ * <li>Character: any value, including null;</li>
+ * <li>Boolean: strings that contain "true" or "false"
(case-sensitive);</li>
+ * <li>Integer: strings that contain exactly an integer. Values that
contain a decimal aren't valid;</li>
+ * <li>Short, Long, Float and Double: strings that contain an integer
or a decimal.</li>
+ * </ul>
+ *
+ * If a type isn't listed here, then the value will be considered invalid.
+ * @param value value to validate.
+ * @param type type of the configuration.
+ * @return boolean indicating whether the value is valid.
+ */
+ protected boolean validateValueType(String value, Class<?> type) {
+ if (type == String.class || type == Character.class) {
+ return true;
}
- value = value.trim();
try {
- if (overprovisioningFactorsForValidation.contains(name) &&
Float.parseFloat(value) <= 0f) {
- final String msg = name + " should be greater than 0";
- logger.error(msg);
- throw new InvalidParameterValueException(msg);
+ if (type == Boolean.class) {
+ return value.equals("true") || value.equals("false");
+ } else if (type == Integer.class || type == Long.class || type ==
Short.class) {
+ Integer.parseInt(value);
+ } else if (type == Float.class || type == Double.class) {
+ Double.parseDouble(value);
+ } else {
+ return false;
}
- } catch (final NumberFormatException e) {
- final String msg = "There was an error trying to parse the float
value for: " + name;
- logger.error(msg);
- throw new InvalidParameterValueException(msg);
+ return true;
+ } catch (NullPointerException | NumberFormatException e) {
+ return false;
}
+ }
- if (type.equals(Boolean.class)) {
- if (!(value.equals("true") || value.equals("false"))) {
- logger.error("Configuration variable " + name + " is expecting
true or false instead of " + value);
- return "Please enter either 'true' or 'false'.";
+ /**
+ * If the specified configuration contains a range, validates if the value
is in that range. If it doesn't contain
+ * a range, any value is considered valid.
+ * The value must be previously checked by `validateValueType` so there
aren't casting exceptions here.
+ * @param name name of the configuration.
+ * @param value value to validate.
+ * @param type type of the value.
+ * @param configuration if the configuration uses Config instead of
ConfigKey, the Config object; null otherwise.
+ * @return if the value is valid, returns null; if not, returns an error
message.
+ */
+ protected String validateValueRange(String name, String value, Class<?>
type, Config configuration) {
+ if (type.equals(Float.class)) {
+ Float val = Float.parseFloat(value);
+ if (overprovisioningFactorsForValidation.contains(name) && val <=
0f) {
+ return String.format("Value for configuration [%s] should be
greater than 0.", name);
+ } else if (weightBasedParametersForValidation.contains(name) &&
(val < 0f || val > 1f)) {
+ return String.format("Please enter a value between 0 and 1 for
the configuration parameter: [%s].", name);
}
- return null;
}
if (type.equals(Integer.class)) {
- try {
- final int val = Integer.parseInt(value);
- if (NetworkModel.MACIdentifier.key().equalsIgnoreCase(name)) {
- //The value need to be between 0 to 255 because the mac
generation needs a value of 8 bit
- //0 value is considered as disable.
- if(val < 0 || val > 255){
- throw new InvalidParameterValueException(name + "
value should be between 0 and 255. 0 value will disable this feature");
- }
+ int val = Integer.parseInt(value);
+ if (NetworkModel.MACIdentifier.key().equalsIgnoreCase(name)) {
+ // The value needs to be between 0 to 255 because the MAC
generation needs a value of 8 bits
+ // 0 is considered as disabled.
+ if (val < 0 || val > 255){
+ return String.format("[%s] value should be between 0 and
255. 0 value will disable this feature.", name);
}
-
- if
(UnmanagedVMsManager.ThreadsOnMSToImportVMwareVMFiles.key().equalsIgnoreCase(name)
||
UnmanagedVMsManager.ThreadsOnKVMHostToImportVMwareVMFiles.key().equalsIgnoreCase(name))
{
- if (val > 10) {
- throw new InvalidParameterValueException("Please enter
a value between 0 and 10 for the configuration parameter: " + name + ", -1 will
disable it");
- }
+ }
+ if
(UnmanagedVMsManager.ThreadsOnMSToImportVMwareVMFiles.key().equalsIgnoreCase(name)
||
+
UnmanagedVMsManager.ThreadsOnKVMHostToImportVMwareVMFiles.key().equalsIgnoreCase(name))
{
+ if (val > 10) {
+ return String.format("Please enter a value between 0 and
10 for the configuration parameter: [%s]. -1 will disable it.", name);
}
Review Comment:
```suggestion
UnmanagedVMsManager.ThreadsOnKVMHostToImportVMwareVMFiles.key().equalsIgnoreCase(name))
{
if (val < -2 || val > 10) {
return String.format("Please enter a value between 0 and
10 for the configuration parameter: [%s]. -1 will disable it.", name);
}
```
--
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]
