Copilot commented on code in PR #14255:
URL: https://github.com/apache/cloudstack/pull/14255#discussion_r4317823784
##########
api/src/main/java/org/apache/cloudstack/backup/BackupManager.java:
##########
@@ -259,7 +259,7 @@ public interface BackupManager extends BackupService,
Configurable, PluggableSer
String getBackupNameFromVM(VirtualMachine vm);
- BackupResponse createBackupResponse(Backup backup, Boolean listVmDetails);
+ BackupResponse createBackupResponse(Backup backup, Boolean listVmDetails,
boolean isCallerRootAdmin);
Review Comment:
The PR is marked as non-breaking, but changing a public interface method
signature is a compile-time breaking change for any other `BackupManager`
implementors (including out-of-tree plugins). To keep compatibility, consider
retaining the original method and adding an overload (or a `default` method)
that delegates to the new signature.
##########
engine/schema/src/main/resources/META-INF/db/views/cloud.internal_backup_view.sql:
##########
@@ -37,11 +37,14 @@ SELECT b.id,
MAX(CASE WHEN bd.name = 'current' THEN bd.value END) current,
COALESCE(MAX(CASE WHEN bd.name = 'isolated' THEN bd.value END),
'false') isolated,
nbpr.volume_id,
+ v.name as volume_name,
+ v.uuid as volume_uuid,
nbsr.path image_store_path
FROM backups b
LEFT JOIN backup_details bd ON b.id = bd.backup_id
LEFT JOIN backup_offering bo ON b.backup_offering_id = bo.id
LEFT JOIN internal_backup_store_ref nbsr ON b.id = nbsr.backup_id
LEFT JOIN internal_backup_pool_ref nbpr ON nbpr.volume_id = nbsr.volume_id
+LEFT JOIN volumes v ON v.id = nbsr.volume_id
WHERE bo.provider='kboss'
GROUP BY b.id, nbsr.volume_id;
Review Comment:
This view selects `v.name`/`v.uuid` but does not aggregate them or include
them in `GROUP BY`. On MySQL with `ONLY_FULL_GROUP_BY` this will fail view
creation. Fix by adding `v.name, v.uuid` to the `GROUP BY`, or by aggregating
them (e.g., `MAX(v.name)`, `MAX(v.uuid)`) since they’re functionally dependent
on `volume_id`.
##########
ui/src/components/view/DetailsTab.vue:
##########
@@ -68,6 +68,11 @@
<span v-else>{{ volume.type }} - {{ volume.path }}</span> ({{
parseFloat(volume.size / (1024.0 * 1024.0 * 1024.0)).toFixed(1) }} GiB)
</div>
</div>
+ <div v-else-if="$route.meta.name === 'backup' && item ===
'backuppaths'">
+ <div v-for="(backupVolumeInfo, idx) in
Object.values(dataResource[item])" :key="idx">
+ <span>{{ backupVolumeInfo.volumeName }} - {{
backupVolumeInfo.backupPath }}</span>
+ </div>
+ </div>
Review Comment:
`Object.values(dataResource[item])` will throw if `dataResource.backuppaths`
is `null/undefined` (which is expected for non-Root Admin callers per this PR).
Add a safe fallback in the template (e.g., iterate over
`Object.values(dataResource[item] || {})`) and/or hide the `backuppaths` detail
row entirely when it’s not present to avoid runtime rendering errors for
regular users.
##########
api/src/main/java/org/apache/cloudstack/api/response/helper/BackupVolumeNameAndPath.java:
##########
@@ -0,0 +1,45 @@
+// 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.
+
+package org.apache.cloudstack.api.response.helper;
+
+public class BackupVolumeNameAndPath {
+ private String volumeName;
+ private String backupPath;
+
+ public BackupVolumeNameAndPath(String volumeName, String backupPath) {
+ this.volumeName = volumeName;
+ this.backupPath = backupPath;
+ }
Review Comment:
This class is used as a nested API response type but doesn’t include
CloudStack-style response metadata annotations (`@SerializedName` / `@Param`).
Depending on how responses are serialized/documented, this can lead to
inconsistent field names and missing docs in generated API descriptions.
Consider annotating fields (and aligning names to API conventions, e.g.,
`volumename`/`backuppath`) or using an established response object pattern.
##########
api/src/main/java/org/apache/cloudstack/api/response/BackupResponse.java:
##########
@@ -99,6 +100,10 @@ public class BackupResponse extends BaseResponse {
@Param(description = "Backup offering name")
private String backupOfferingName;
+ @SerializedName(ApiConstants.BACKUP_PATHS)
+ @Param(description = "The backups paths in the secondary storage")
+ private Map<String, BackupVolumeNameAndPath> backupPaths;
Review Comment:
Grammar nit: the description reads \"The backups paths\"; it should be \"The
backup paths\" (or \"Backup paths\") for clarity.
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -2677,6 +2681,17 @@ public BackupResponse createBackupResponse(Backup
backup, Boolean listVmDetails)
response.setToCheckpointId(backup.getToCheckpointId());
}
+ if (offering != null &&
KBOSS_BACKUP_PROVIDER.equals(offering.getProvider()) && isCallerRootAdmin) {
+ List<InternalBackupJoinVO> backupJoins =
internalBackupJoinDao.listById(backup.getId());
+ Map<String, BackupVolumeNameAndPath> backupVolumePaths = new
HashMap<>();
+
+ backupJoins.forEach(b -> {
+ backupVolumePaths.put(b.getVolumeUuid(), new
BackupVolumeNameAndPath(b.getVolumeName(), b.getImageStorePath()));
+ });
+
+ response.setBackupPaths(backupVolumePaths);
+ }
Review Comment:
`createBackupResponse` is called in a loop for `listBackups`, and this adds
a DAO call per backup (N+1 query pattern) for Root Admin KBOSS listings. If
large backup lists are common, consider fetching joins in bulk (e.g., DAO
method by `backupIds`) in `ListBackupsCmd`/manager and passing a prebuilt
mapping into response generation to avoid per-row DB access.
--
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]
