Copilot commented on code in PR #13932:
URL: https://github.com/apache/cloudstack/pull/13932#discussion_r4477145812
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java:
##########
@@ -296,7 +296,28 @@ protected void addExtraParamsToScript(String extraParams,
Script script) {
}
protected String encodeUsername(String username) {
- return URLEncoder.encode(username, Charset.defaultCharset());
+ return URLEncoder.encode(username,
StandardCharsets.UTF_8).replace("+", "%20");
+ }
+
+ private String encodePathSegment(String value) {
+ if (StringUtils.isBlank(value)) {
+ return value;
+ }
+ return URLEncoder.encode(value, StandardCharsets.UTF_8).replace("+",
"%20");
+ }
+
+ private String encodePathSegments(String value) {
+ if (StringUtils.isBlank(value)) {
+ return value;
Review Comment:
`StringUtils.isBlank` treats an all-whitespace value as blank and returns it
unchanged, so a valid inventory name such as `" "` (or a whitespace folder
segment in `path`) still puts a literal space into the URI. This bypasses the
percent-encoding that this helper is meant to provide; use a null/empty check
here and in `encodePathSegments` so whitespace-only values reach `URLEncoder`.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java:
##########
@@ -296,7 +296,28 @@ protected void addExtraParamsToScript(String extraParams,
Script script) {
}
protected String encodeUsername(String username) {
- return URLEncoder.encode(username, Charset.defaultCharset());
+ return URLEncoder.encode(username,
StandardCharsets.UTF_8).replace("+", "%20");
+ }
+
+ private String encodePathSegment(String value) {
+ if (StringUtils.isBlank(value)) {
+ return value;
+ }
+ return URLEncoder.encode(value, StandardCharsets.UTF_8).replace("+",
"%20");
+ }
+
+ private String encodePathSegments(String value) {
+ if (StringUtils.isBlank(value)) {
+ return value;
+ }
+ StringBuilder encoded = new StringBuilder();
+ for (String seg : value.split("/", -1)) {
+ if (encoded.length() > 0) {
+ encoded.append("/");
+ }
+ encoded.append(encodePathSegment(seg));
+ }
+ return encoded.toString();
Review Comment:
The new encoding behavior is not covered by the existing tests: the VDDK
tests use only simple names and do not assert the generated `-ic` URI. Please
add regression coverage for spaces, slash-preserving nested paths, and reserved
characters (including the OVA `vi://` path), otherwise a future change can
silently reintroduce the original import failure.
--
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]
