Copilot commented on code in PR #14255:
URL: https://github.com/apache/cloudstack/pull/14255#discussion_r4317982780
##########
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 now selects `v.name` and `v.uuid` while grouping by only `b.id,
nbsr.volume_id`. Depending on database settings/engine (e.g., strict
`ONLY_FULL_GROUP_BY` semantics), this can cause view creation/query failures or
non-deterministic results. Consider adding `v.name` and `v.uuid` to the `GROUP
BY`, or aggregating them (e.g., `MAX(v.uuid)` / `MAX(v.name)`) since they’re
functionally tied to `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:
`dataResource[item]` can be `null` for non-Root Admin users (per the
intended behavior on the API side). Calling `Object.values(null)` will throw
and can break the entire details tab rendering. Guard the block to only render
when `dataResource[item]` is truthy, or default to an empty object/array before
calling `Object.values`.
##########
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:
The description has a grammatical error: change “The backups paths” to “The
backup paths”.
##########
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:
Changing the interface method signature is a source/binary breaking change
for any out-of-tree `BackupManager` implementations (plugins, forks). To
preserve compatibility, consider keeping the original method signature as an
overload (or a Java default method) delegating to the new method with a safe
default for `isCallerRootAdmin`.
##########
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">
Review Comment:
Using the array index as the Vue `key` can cause incorrect re-rendering if
the object entries change order or items are inserted/removed. Prefer a stable
key (e.g., the volume UUID from the map key) by iterating over
`Object.entries(...)` and using the entry key as `:key`.
##########
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:
When `listBackups` returns many results for a Root Admin on KBOSS, this adds
a per-backup DAO call (`internalBackupJoinDao.listById(...)`), which can become
an N+1 query pattern. Consider fetching joins in bulk at the list-call level
(e.g., DAO method that accepts multiple backup IDs and returns grouped
results), or adding an optional preloaded map into `createBackupResponse` to
avoid repeated DB round trips.
--
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]
