Copilot commented on code in PR #13783:
URL: https://github.com/apache/cloudstack/pull/13783#discussion_r3714956027
##########
plugins/api/discovery/src/test/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImplTest.java:
##########
@@ -107,6 +107,7 @@ public void
getCmdRequestMapAddsParamsWithExposedAndIncludedInApiDocAnnotations(
Mockito.when(parameterMock.name()).thenReturn("paramName");
Mockito.when(parameterMock.since()).thenReturn("");
Mockito.when(parameterMock.entityType()).thenReturn(new
Class[]{Object.class});
+ Mockito.when(parameterMock.allowedValues()).thenReturn(new String[]{});
Review Comment:
Discovery now conditionally emits `allowedvalues`, but the test setup only
stubs the empty case and doesn’t assert the new behavior. Add/extend a test to
cover (1) non-empty `allowedValues` is serialized into the parameter response,
and (2) empty `allowedValues` results in the field being absent (or null) as
intended.
##########
api/src/main/java/org/apache/cloudstack/api/command/user/ipv6/CreateIpv6FirewallRuleCmd.java:
##########
@@ -79,7 +79,14 @@ public class CreateIpv6FirewallRuleCmd extends
BaseAsyncCreateCmd {
@Parameter(name = ApiConstants.NETWORK_ID, type = CommandType.UUID,
entityType = NetworkResponse.class, description = "The Network of the Instance
the Ipv6 firewall rule will be created for", required = true)
private Long networkId;
- @Parameter(name = ApiConstants.TRAFFIC_TYPE, type = CommandType.STRING,
description = "The traffic type for the Ipv6 firewall rule, can be ingress or
egress, defaulted to ingress if not specified")
+ @Parameter(
+ name = ApiConstants.TRAFFIC_TYPE,
+ type = CommandType.STRING,
+ description = "The traffic type for the Ipv6 firewall rule, can be ingress
or egress, defaulted to ingress if not specified",
+ allowedValues = {
+ "Ingress",
+ "Egress"
+ })
Review Comment:
The `allowedValues` casing (`Ingress`/`Egress`) conflicts with the parameter
description (`ingress`/`egress`). This can mislead clients and break
validation/autocomplete if the API expects lowercase. Align `allowedValues`
with the actual accepted values (or update the description to match).
##########
plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java:
##########
@@ -236,10 +237,19 @@ protected ApiDiscoveryResponse getCmdRequestMap(Class<?>
cmdClass, APICommand ap
paramResponse.setSince(parameterAnnotation.since());
}
paramResponse.setRelated(parameterAnnotation.entityType()[0].getName());
- if (parameterAnnotation.authorized() != null) {
-
paramResponse.setAuthorizedRoleTypes(Arrays.asList(parameterAnnotation.authorized()));
- }
- response.addParam(paramResponse);
+
+ String[] allowedValues =
parameterAnnotation.allowedValues();
+ if (allowedValues.length > 0) {
+ paramResponse.setAllowedValues(
+
Collections.unmodifiableList(Arrays.asList(allowedValues))
+ );
+ }
Review Comment:
`parameterAnnotation.allowedValues()` is assumed non-null; while real Java
annotations won’t return null, tests (and any mocked `Parameter`) can, which
would NPE on `allowedValues.length`. Add a null-check (treat null as empty)
before accessing `.length`.
##########
plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java:
##########
@@ -236,10 +237,19 @@ protected ApiDiscoveryResponse getCmdRequestMap(Class<?>
cmdClass, APICommand ap
paramResponse.setSince(parameterAnnotation.since());
}
paramResponse.setRelated(parameterAnnotation.entityType()[0].getName());
- if (parameterAnnotation.authorized() != null) {
-
paramResponse.setAuthorizedRoleTypes(Arrays.asList(parameterAnnotation.authorized()));
- }
- response.addParam(paramResponse);
+
+ String[] allowedValues =
parameterAnnotation.allowedValues();
+ if (allowedValues.length > 0) {
+ paramResponse.setAllowedValues(
+
Collections.unmodifiableList(Arrays.asList(allowedValues))
+ );
Review Comment:
If the project’s Java baseline permits, prefer constructing an immutable
list without wrapping `Arrays.asList(...)` (e.g., using standard library
immutable list creation) to reduce verbosity and avoid the extra wrapper. If
constrained to older Java, consider at least keeping the list creation on one
line for readability.
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/CreateVPCOfferingCmd.java:
##########
@@ -92,10 +92,15 @@ public class CreateVPCOfferingCmd extends
BaseAsyncCreateCmd {
@Parameter(name = ApiConstants.SERVICE_CAPABILITY_LIST, type =
CommandType.MAP, description = "Desired service capabilities as part of VPC
offering", since = "4.4")
private Map<String, List<String>> serviceCapabilityList;
- @Parameter(name = ApiConstants.INTERNET_PROTOCOL,
- type = CommandType.STRING,
- description = "The internet protocol of the offering. Options are
IPv4 and dualstack. Default is IPv4. dualstack will create an offering that
supports both IPv4 and IPv6",
- since = "4.17.0")
+ @Parameter(
+ name = ApiConstants.INTERNET_PROTOCOL,
+ type = CommandType.STRING,
+ description = "The internet protocol of the offering. Options are IPv4
and dualstack. Default is IPv4. dualstack will create an offering that supports
both IPv4 and IPv6",
+ since = "4.17.0",
+ allowedValues = {
+ "IPv4",
+ "DualStack"
+ })
Review Comment:
The description documents the option as `dualstack`, but `allowedValues`
lists `DualStack`. To avoid confusing API discovery consumers, make the
description and `allowedValues` consistent (using the exact accepted token(s)).
--
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]
