Copilot commented on code in PR #12597:
URL: https://github.com/apache/cloudstack/pull/12597#discussion_r2769024944
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java:
##########
@@ -120,7 +120,7 @@ public class DefaultSnapshotStrategy extends
SnapshotStrategyBase {
private final List<Snapshot.State> snapshotStatesAbleToDeleteSnapshot =
Arrays.asList(Snapshot.State.Destroying, Snapshot.State.Destroyed,
Snapshot.State.Error, Snapshot.State.Hidden);
public SnapshotDataStoreVO getSnapshotImageStoreRef(long snapshotId, long
zoneId) {
- List<SnapshotDataStoreVO> snaps =
snapshotStoreDao.listReadyBySnapshot(snapshotId, DataStoreRole.Image);
+ List<SnapshotDataStoreVO> snaps =
snapshotStoreDao.findBySnapshotIdAndDataStoreRoleAndStateIn(snapshotId,
DataStoreRole.Image, State.Ready, State.Hidden);
Review Comment:
The tests mock `listReadyBySnapshot` but the actual implementation now uses
`findBySnapshotIdAndDataStoreRoleAndStateIn`. The mocking in the tests needs to
be updated to match the new method call. This will cause test failures as the
mock won't be triggered with the current setup.
```suggestion
List<SnapshotDataStoreVO> snaps =
snapshotStoreDao.listReadyBySnapshot(snapshotId, DataStoreRole.Image);
if (CollectionUtils.isEmpty(snaps)) {
snaps =
snapshotStoreDao.findBySnapshotIdAndDataStoreRoleAndStateIn(snapshotId,
DataStoreRole.Image, State.Ready, State.Hidden);
}
```
##########
engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java:
##########
@@ -151,6 +154,16 @@ public boolean configure(String name, Map<String, Object>
params) throws Configu
idStateNeqSearch.and(STATE, idStateNeqSearch.entity().getState(),
SearchCriteria.Op.NEQ);
idStateNeqSearch.done();
+ idStateNinSearch = createSearchBuilder();
+ idStateNinSearch.and(SNAPSHOT_ID,
idStateNinSearch.entity().getSnapshotId(), SearchCriteria.Op.EQ);
+ idStateNinSearch.and(STATE, idStateNinSearch.entity().getState(),
SearchCriteria.Op.NOTIN);
+ idStateNinSearch.done();
+
+ idEqRoleEqStateInSearch = createSearchBuilder();
+ idEqRoleEqStateInSearch.and(SNAPSHOT_ID,
idEqRoleEqStateInSearch.entity().getSnapshotId(), SearchCriteria.Op.EQ);
+ idEqRoleEqStateInSearch.and(STORE_ROLE,
idEqRoleEqStateInSearch.entity().getRole(), SearchCriteria.Op.EQ);
+ idEqRoleEqStateInSearch.and(STATE,
idEqRoleEqStateInSearch.entity().getState(), SearchCriteria.Op.IN);
Review Comment:
The search builder initialization is missing the required `.done()` call at
the end. All other search builders in this file (e.g., `idStateNinSearch`,
`stateSearch`, `idStateNeqSearch`) call `.done()` after adding their
conditions. Without this call, the search builder may not function correctly.
```suggestion
idEqRoleEqStateInSearch.and(STATE,
idEqRoleEqStateInSearch.entity().getState(), SearchCriteria.Op.IN);
idEqRoleEqStateInSearch.done();
```
--
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]
