Copilot commented on code in PR #11249:
URL: https://github.com/apache/cloudstack/pull/11249#discussion_r2217897798
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -5038,6 +5038,29 @@ public Vlan createVlanAndPublicIpRange(final long
zoneId, final long networkId,
final VlanVO vlan = commitVlanAndIpRange(zoneId, networkId,
physicalNetworkId, podId, startIP, endIP, vlanGateway, vlanNetmask, vlanId,
domain, vlanOwner, vlanIp6Gateway, vlanIp6Cidr,
ipv4, zone, vlanType, ipv6Range, ipRange, forSystemVms);
+ if (vlan != null) {
+ final NetworkVO networkVO = _networkDao.findById(networkId);
+ if (ipv4) {
+ String networkCidr = networkVO.getCidr();
+ String newCidr =
NetUtils.getCidrFromGatewayAndNetmask(vlanGateway, vlanNetmask);
+ String newNetworkCidr =
com.cloud.utils.StringUtils.updateCommaSeparatedStringWithValue(networkCidr,
newCidr, true);
+ networkVO.setCidr(newNetworkCidr);
+
+ String networkGateway = networkVO.getGateway();
+ String newNetworkGateway =
com.cloud.utils.StringUtils.updateCommaSeparatedStringWithValue(networkGateway,
vlanGateway, true);
+ networkVO.setGateway(newNetworkGateway);
+ } else if (ipv6) {
+ String networkIp6Cidr = networkVO.getIp6Cidr();
+ String newNetworkIp6Cidr =
com.cloud.utils.StringUtils.updateCommaSeparatedStringWithValue(networkIp6Cidr,
vlanIp6Cidr, true);
+ networkVO.setCidr(newNetworkIp6Cidr);
Review Comment:
The code is incorrectly setting IPv6 CIDR using setCidr() instead of
setIp6Cidr(). This will overwrite the IPv4 CIDR field with IPv6 data.
```suggestion
networkVO.setIp6Cidr(newNetworkIp6Cidr);
```
##########
engine/schema/src/main/resources/META-INF/db/schema-41930to41940.sql:
##########
@@ -0,0 +1,25 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+--;
+-- Schema upgrade from 4.19.3.0 to 4.19.4.0
+--;
+
+ALTER TABLE `cloud`.`networks` MODIFY COLUMN `cidr` varchar(255) DEFAULT NULL
COMMENT 'CloudStack managed vms get IP address from cidr.In general this cidr
also serves as the network CIDR. But in case IP reservation feature is being
used by a Guest network, networkcidr is the Effective network CIDR for that
network';
+ALTER TABLE `cloud`.`networks` MODIFY COLUMN `gateway` varchar(255) DEFAULT
NULL COMMENT 'gateway(s) for this network configuration';
+ALTER TABLE `cloud`.`networks` MODIFY COLUMN `ip6_cidr` varchar(1024) DEFAULT
NULL COMMENT 'IPv6 gateway(s) for this network';
+ALTER TABLE `cloud`.`networks` MODIFY COLUMN `ip6_gateway` varchar(1024)
DEFAULT NULL COMMENT 'IPv6 cidr(s) for this network';
Review Comment:
The comment for ip6_gateway column incorrectly states 'IPv6 cidr(s) for this
network' when it should describe IPv6 gateways, not CIDRs.
```suggestion
ALTER TABLE `cloud`.`networks` MODIFY COLUMN `ip6_gateway` varchar(1024)
DEFAULT NULL COMMENT 'IPv6 gateway(s) for this network';
```
##########
utils/src/main/java/com/cloud/utils/StringUtils.java:
##########
@@ -394,4 +397,52 @@ public static String rangeToNumbers(String
inputNumbersAndRanges) {
return result.toString();
}
+
+ /**
+ * Converts the comma separated numbers and ranges to numbers
+ * @param originalString the original string (can be null or empty)
containing list of comma separated values that has to be updated
+ * @param value the value to add to, or remove from the original string
+ * @param add if true, adds the input value; if false, removes it
+ * @return String containing the modified original string (or null if
empty)
+ */
+ public static String updateCommaSeparatedStringWithValue(String
originalString, String value, boolean add) {
+ if (org.apache.commons.lang3.StringUtils.isEmpty(value)) {
+ return originalString;
+ }
+
+ Set<String> values = new LinkedHashSet<>();
+
+ if (org.apache.commons.lang3.StringUtils.isNotEmpty(originalString)) {
+ values.addAll(Arrays.stream(originalString.split(","))
+ .map(String::trim)
+ .filter(s -> !s.isEmpty())
+ .collect(Collectors.toList()));
+ }
+
+ if (add) {
+ values.add(value);
+ } else {
+ values.remove(value);
+ }
+
+ return values.isEmpty() ? null : String.join(",", values);
+ }
+
+ /**
+ * Returns the first value from a comma-separated string.
+ * @param inputString the input string (can be null or empty) containing
list of comma separated values
+ * @return the first value, or null if none found
+ */
+ public static String getFirstValueFromCommaSeparatedString(String
inputString) {
+ if (org.apache.commons.lang3.StringUtils.isEmpty(inputString)) {
+ return inputString;
Review Comment:
[nitpick] The method returns the input string directly when it's null/empty,
but the return type suggests it should return null for empty input. Consider
returning null consistently for empty/null input.
```suggestion
if (org.apache.commons.lang3.StringUtils.isBlank(inputString)) {
return null;
```
##########
utils/src/main/java/com/cloud/utils/StringUtils.java:
##########
@@ -394,4 +397,52 @@ public static String rangeToNumbers(String
inputNumbersAndRanges) {
return result.toString();
}
+
+ /**
+ * Converts the comma separated numbers and ranges to numbers
+ * @param originalString the original string (can be null or empty)
containing list of comma separated values that has to be updated
+ * @param value the value to add to, or remove from the original string
+ * @param add if true, adds the input value; if false, removes it
+ * @return String containing the modified original string (or null if
empty)
+ */
+ public static String updateCommaSeparatedStringWithValue(String
originalString, String value, boolean add) {
+ if (org.apache.commons.lang3.StringUtils.isEmpty(value)) {
+ return originalString;
+ }
+
+ Set<String> values = new LinkedHashSet<>();
+
+ if (org.apache.commons.lang3.StringUtils.isNotEmpty(originalString)) {
+ values.addAll(Arrays.stream(originalString.split(","))
+ .map(String::trim)
+ .filter(s -> !s.isEmpty())
+ .collect(Collectors.toList()));
Review Comment:
Using Collectors.toList() and then addAll() is inefficient when the result
is going into a LinkedHashSet. Consider using Collectors.toCollection(() -> new
LinkedHashSet<>()) to collect directly into the set.
```suggestion
.collect(Collectors.toCollection(LinkedHashSet::new)));
```
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -6071,6 +6094,30 @@ public boolean deleteVlanIpRange(final
DeleteVlanIpRangeCmd cmd) {
private boolean deleteAndPublishVlanAndPublicIpRange(final long userId,
final long vlanDbId, final Account caller) {
VlanVO deletedVlan = deleteVlanAndPublicIpRange(userId, vlanDbId,
caller);
if (deletedVlan != null) {
+ final boolean ipv4 = deletedVlan.getVlanGateway() != null;
+ final boolean ipv6 = deletedVlan.getIp6Gateway() != null;
+ final long networkId = deletedVlan.getNetworkId();
+ final NetworkVO networkVO = _networkDao.findById(networkId);
+ if (ipv4) {
+ String networkCidr = networkVO.getCidr();
+ String cidrToRemove =
NetUtils.getCidrFromGatewayAndNetmask(deletedVlan.getVlanGateway(),
deletedVlan.getVlanNetmask());
+ String newNetworkCidr =
com.cloud.utils.StringUtils.updateCommaSeparatedStringWithValue(networkCidr,
cidrToRemove, false);
+ networkVO.setCidr(newNetworkCidr);
+
+ String networkGateway = networkVO.getGateway();
+ String newNetworkGateway =
com.cloud.utils.StringUtils.updateCommaSeparatedStringWithValue(networkGateway,
deletedVlan.getVlanGateway(), false);
+ networkVO.setGateway(newNetworkGateway);
+ } else if (ipv6) {
+ String networkIp6Cidr = networkVO.getIp6Cidr();
+ String newNetworkIp6Cidr =
com.cloud.utils.StringUtils.updateCommaSeparatedStringWithValue(networkIp6Cidr,
deletedVlan.getIp6Cidr(), false);
+ networkVO.setCidr(newNetworkIp6Cidr);
Review Comment:
The code is incorrectly setting IPv6 CIDR using setCidr() instead of
setIp6Cidr(). This will overwrite the IPv4 CIDR field with IPv6 data.
```suggestion
networkVO.setIp6Cidr(newNetworkIp6Cidr);
```
--
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: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
