Copilot commented on code in PR #14340:
URL: https://github.com/apache/cloudstack/pull/14340#discussion_r4257065453
##########
server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java:
##########
@@ -209,11 +209,21 @@ private String getTemplateStatus(TemplateJoinVO template)
{
return templateStatus.getStatus();
}
+ /**
+ * Lists the not bypassed store entries of a template, leaving out
store-less entries (store_id NULL)
+ * as created for Baremetal and External templates. These are returned
when storeIds is empty.
Review Comment:
The Javadoc is internally inconsistent: it says the method leaves out
store-less entries, but also says 'These are returned when storeIds is empty.'
Since the implementation always filters out store-less entries, update the
comment to reflect that store-less rows may be present in DAO results and are
intentionally excluded here (including when no storeIds are provided).
##########
server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java:
##########
@@ -532,7 +542,7 @@ public TemplateResponse newIsoResponse(TemplateJoinVO iso,
ResponseView view) {
isoResponse.setStatus(templateStatus);
}
isoResponse.setUrl(iso.getUrl());
- List<TemplateDataStoreVO> isosInStore =
_templateStoreDao.listByTemplateNotBypassed(iso.getId());
+ List<TemplateDataStoreVO> isosInStore =
listTemplatesInImageStores(iso.getId());
Review Comment:
The new behavior is exercised via direct tests of the helper, but the
regression risk is in the public response builders (`newTemplateResponse` /
`newIsoResponse`) where the NPE originally occurred. Add a regression test that
calls `newIsoResponse` (and/or `newTemplateResponse`) with a mocked DAO
returning a store-less `TemplateDataStoreVO` to ensure no NPE and that the
response remains correct.
##########
server/src/main/java/com/cloud/api/query/dao/TemplateJoinDaoImpl.java:
##########
@@ -209,11 +209,21 @@ private String getTemplateStatus(TemplateJoinVO template)
{
return templateStatus.getStatus();
}
+ /**
+ * Lists the not bypassed store entries of a template, leaving out
store-less entries (store_id NULL)
+ * as created for Baremetal and External templates. These are returned
when storeIds is empty.
+ */
+ protected List<TemplateDataStoreVO> listTemplatesInImageStores(long
templateId, Long... storeIds) {
+ return _templateStoreDao.listByTemplateNotBypassed(templateId,
storeIds).stream()
Review Comment:
Consider making the DAO call behavior explicit when no storeIds are provided
(e.g., branch on storeIds.length == 0 and call the no-filter DAO method if one
exists, or pass null intentionally). This avoids any ambiguity or future
divergence if the DAO implementation treats an empty array differently than 'no
store filter'.
--
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]
