Address allowed values enum review feedback

Author: dheeraj12347

api/src/main/java/com/cloud/cpu/CPU.java

@@ -41,6 +41,11 @@ public String getType() {
             return type;
         }
 
+        @Override
+        public String toString() {
+            return type;
+        }
+
         public int getBits() {
             return bits;
         }

api/src/main/java/org/apache/cloudstack/api/command/user/iso/ListIsosCmd.java

@@ -90,7 +90,7 @@ public class ListIsosCmd extends BaseListTaggedResourcesCmd implements UserCmd {
 
     @Parameter(name = ApiConstants.ARCH,
         type = CommandType.STRING,
-        description = "the CPU arch of the ISO. Valid options are: x86_64, aarch64, s390x",
+        description = "the CPU arch of the ISO. Valid options are: i686, x86_64, aarch64, s390x",
         since = "4.20",
         allowedValueType = CPU.CPUArch.class)
     private String arch;

api/src/main/java/org/apache/cloudstack/api/command/user/template/ListTemplatesCmd.java

@@ -115,7 +115,7 @@ public class ListTemplatesCmd extends BaseListTaggedResourcesCmd implements User
 
     @Parameter(name = ApiConstants.ARCH,
            type = CommandType.STRING,
-           description = "the CPU arch of the template. Valid options are: x86_64, aarch64, s390x",
+           description = "the CPU arch of the template. Valid options are: i686, x86_64, aarch64, s390x",
            since = "4.20",
            allowedValueType = CPU.CPUArch.class)
     private String arch;

scripts/check_allowed_values.py

@@ -165,6 +165,18 @@ def extract_inner_enums(source):
         for constant in body.split(","):
             constant = constant.strip()
 
+            # Enum constants with a constructor argument, e.g.
+            # x86("i686", 32)
+            value_match = re.match(
+                r"[A-Za-z_][A-Za-z0-9_]*\s*\(\s*\"([^\"]+)\"",
+                constant,
+            )
+
+            if value_match:
+                constants.append(value_match.group(1))
+                continue
+
+            # Plain enum constants, e.g. LEAKS, ALL
             constant_match = re.match(
                 r"([A-Z][A-Z0-9_]*)\b",
                 constant,
@@ -178,6 +190,31 @@ def extract_inner_enums(source):
 
     return enums
 
+def build_enum_index():
+    enum_index = {}
+
+    for path in find_java_files():
+        source = path.read_text(encoding="utf-8")
+        enums = extract_inner_enums(source)
+
+        for enum_name, values in enums.items():
+            enum_index[enum_name] = values
+
+    return enum_index
+
+def extract_allowed_value_type(block):
+    match = re.search(
+        r"\ballowedValueType\s*=\s*([A-Za-z_][A-Za-z0-9_]*)(?:\.([A-Za-z_][A-Za-z0-9_]*))?\.class",
+        block,
+    )
+
+    if not match:
+        return None
+
+    if match.group(2):
+        return match.group(2)
+
+    return match.group(1)
 
 def extract_parameter_field_name(source, parameter_end):
     match = re.match(
@@ -205,22 +242,21 @@ def find_enum_for_parameter(source, parameter_end, field_name):
 
     return None
 
-def check_file(path):
+def check_file(path, enum_index=None):
     source = Path(path).read_text(encoding="utf-8")
     violations = []
-
     enums = extract_inner_enums(source)
 
+    if enum_index is None:
+        enum_index = {}
+
     for start, block in extract_parameter_blocks(source):
         if not is_string_parameter(block):
             continue
 
         if re.search(r"\ballowedValues\s*=", block):
             continue
 
-        if re.search(r"\ballowedValueType\s*=", block):
-            continue
-
         parameter_end = start + len(block)
         field_name = extract_parameter_field_name(
             source,
@@ -233,28 +269,51 @@ def check_file(path):
             field_name,
         )
 
+        if not enum_name:
+            enum_name = extract_allowed_value_type(block)
+
+        description = extract_description(block)
+        values = extract_valid_values(description) if description else []
+
+        enum_values = None
         if enum_name and enum_name in enums:
-            line = source[:start].count("\n") + 1
-            name = extract_parameter_name(block)
-
-            violations.append(
-                (
-                    str(path),
-                    line,
-                    enums[enum_name],
-                    name,
+            enum_values = enums[enum_name]
+        elif enum_name and enum_name in enum_index:
+            enum_values = enum_index[enum_name]
+
+        if enum_values is not None:
+
+            if re.search(r"\ballowedValueType\s*=", block):
+                if values and values != enum_values:
+                    line = source[:start].count("\n") + 1
+                    name = extract_parameter_name(block)
+
+                    violations.append(
+                        (
+                            str(path),
+                            line,
+                            enum_values,
+                            name,
+                        )
+                    )
+            else:
+                line = source[:start].count("\n") + 1
+                name = extract_parameter_name(block)
+
+                violations.append(
+                    (
+                        str(path),
+                        line,
+                        enum_values,
+                        name,
+                    )
                 )
-            )
 
             continue
 
-        description = extract_description(block)
-
-        if not description:
+        if re.search(r"\ballowedValueType\s*=", block):
             continue
 
-        values = extract_valid_values(description)
-
         if not values:
             continue
 
@@ -269,9 +328,10 @@ def check_file(path):
 
 def main():
     violations = []
+    enum_index = build_enum_index()
 
     for path in find_java_files():
-        violations.extend(check_file(path))
+        violations.extend(check_file(path, enum_index))
 
     if violations:
         print("Missing allowedValues annotations:")

scripts/test_check_allowed_values.py

@@ -281,6 +281,50 @@ def test_enum_parameter_with_allowed_value_type_is_not_reported(self):
         finally:
             path.unlink()
 
+    def test_external_enum_parameter_with_mismatched_description_is_reported(self):
+        enum_source = """
+        public class CPU {
+            public enum CPUArch {
+                x86("i686", 32),
+                amd64("x86_64", 64),
+                arm64("aarch64", 64),
+                s390x("s390x", 64);
+            }
+        }
+        """
+
+        command_source = """
+        public class TestCmd {
+            @Parameter(
+                name = "arch",
+                type = CommandType.STRING,
+                allowedValueType = CPU.CPUArch.class,
+                description = "the CPU arch. Valid options are: x86_64, aarch64, s390x"
+            )
+            private String arch;
+        }
+        """
+
+        with tempfile.NamedTemporaryFile(
+            mode="w",
+            suffix=".java",
+            encoding="utf-8",
+            delete=False,
+        ) as file:
+            file.write(command_source)
+            path = Path(file.name)
+
+        try:
+            enum_index = check_allowed_values.extract_inner_enums(enum_source)
+            violations = check_allowed_values.check_file(path, enum_index)
+
+            self.assertEqual(len(violations), 1)
+            self.assertEqual(
+                violations[0][2],
+                ["i686", "x86_64", "aarch64", "s390x"],
+            )
+        finally:
+            path.unlink()
 
 if __name__ == "__main__":
     unittest.main()

server/src/main/java/com/cloud/template/TemplateManagerImpl.java

@@ -2443,10 +2443,11 @@ public TemplateType validateTemplateType(BaseCmd cmd, boolean isAdmin, boolean i
         }
         if (newType != null) {
             try {
+                TemplateApiType.valueOf(newType.toUpperCase());
                 templateType = TemplateType.valueOf(newType.toUpperCase());
             } catch (IllegalArgumentException ex) {
                 throw new InvalidParameterValueException(String.format("Please specify a valid templatetype: %s",
-                        org.apache.commons.lang3.StringUtils.join(",", TemplateType.values())));
+                    org.apache.commons.lang3.StringUtils.join(",", TemplateApiType.values())));
             }
         }
         if (templateType != null) {