You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/08/27 15:39:24 UTC
[GitHub] [cloudstack] ravening commented on a change in pull request #5307: Filter disk / service offerings by domain at DB level
ravening commented on a change in pull request #5307:
URL: https://github.com/apache/cloudstack/pull/5307#discussion_r697529920
##########
File path: engine/schema/src/main/java/com/cloud/service/dao/ServiceOfferingDetailsDaoImpl.java
##########
@@ -67,4 +71,23 @@ public String getDetail(Long serviceOfferingId, String key) {
}
return detailValue;
}
+
+ @Override
+ public List<Long> findOfferingIdsByDomainIds(List<Long> domainIds) {
+ Object[] dIds = domainIds.stream().map(s -> String.valueOf(s)).collect(Collectors.toList()).toArray();
Review comment:
better to mention String[] rather than Object[] since you are explicitly getting string value?
##########
File path: server/src/main/java/com/cloud/api/query/QueryManagerImpl.java
##########
@@ -2824,75 +2826,42 @@
sc.addAnd("zoneId", SearchCriteria.Op.SC, zoneSC);
}
- // FIXME: disk offerings should search back up the hierarchy for
- // available disk offerings...
- /*
- * sb.addAnd("domainId", sb.entity().getDomainId(),
- * SearchCriteria.Op.EQ); if (domainId != null) {
- * SearchBuilder<DomainVO> domainSearch =
- * _domainDao.createSearchBuilder(); domainSearch.addAnd("path",
- * domainSearch.entity().getPath(), SearchCriteria.Op.LIKE);
- * sb.join("domainSearch", domainSearch, sb.entity().getDomainId(),
- * domainSearch.entity().getId()); }
- */
-
- // FIXME: disk offerings should search back up the hierarchy for
- // available disk offerings...
- /*
- * if (domainId != null) { sc.setParameters("domainId", domainId); //
- * //DomainVO domain = _domainDao.findById((Long)domainId); // // I want
- * to join on user_vm.domain_id = domain.id where domain.path like
- * 'foo%' //sc.setJoinParameters("domainSearch", "path",
- * domain.getPath() + "%"); // }
- */
-
- Pair<List<DiskOfferingJoinVO>, Integer> result = _diskOfferingJoinDao.searchAndCount(sc, searchFilter);
- // Remove offerings that are not associated with caller's domain
- if (account.getType() != Account.ACCOUNT_TYPE_ADMIN && CollectionUtils.isNotEmpty(result.first())) {
- ListIterator<DiskOfferingJoinVO> it = result.first().listIterator();
- while (it.hasNext()) {
- DiskOfferingJoinVO offering = it.next();
- if(!Strings.isNullOrEmpty(offering.getDomainId())) {
- boolean toRemove = true;
- String[] domainIdsArray = offering.getDomainId().split(",");
- for (String domainIdString : domainIdsArray) {
- Long dId = Long.valueOf(domainIdString.trim());
- if (isRecursive) {
- if (_domainDao.isChildDomain(account.getDomainId(), dId)) {
- toRemove = false;
- break;
- }
- } else {
- if (_domainDao.isChildDomain(dId, account.getDomainId())) {
- toRemove = false;
- break;
- }
- }
- }
- if (toRemove) {
- it.remove();
- }
- }
+ // Filter offerings that are not associated with caller's domain
+ // Fetch the offering ids from the details table since theres no smart way to filter them in the join ... yet!
+ Account caller = CallContext.current().getCallingAccount();
+ if (caller.getType() != Account.ACCOUNT_TYPE_ADMIN) {
+ Domain callerDomain = _domainDao.findById(caller.getDomainId());
+ List<Long> domainIds = _domainDao.getDomainParentIds(callerDomain.getId())
+ .stream().collect(Collectors.toList());
+ if (isRecursive) {
+ List<Long> childrenIds = _domainDao.getDomainChildrenIds(callerDomain.getPath());
+ if (childrenIds != null && !childrenIds.isEmpty())
+ domainIds.addAll(childrenIds);
Review comment:
proper indentation here?
##########
File path: server/src/main/java/com/cloud/api/query/QueryManagerImpl.java
##########
@@ -2824,75 +2826,42 @@
sc.addAnd("zoneId", SearchCriteria.Op.SC, zoneSC);
}
- // FIXME: disk offerings should search back up the hierarchy for
- // available disk offerings...
- /*
- * sb.addAnd("domainId", sb.entity().getDomainId(),
- * SearchCriteria.Op.EQ); if (domainId != null) {
- * SearchBuilder<DomainVO> domainSearch =
- * _domainDao.createSearchBuilder(); domainSearch.addAnd("path",
- * domainSearch.entity().getPath(), SearchCriteria.Op.LIKE);
- * sb.join("domainSearch", domainSearch, sb.entity().getDomainId(),
- * domainSearch.entity().getId()); }
- */
-
- // FIXME: disk offerings should search back up the hierarchy for
- // available disk offerings...
- /*
- * if (domainId != null) { sc.setParameters("domainId", domainId); //
- * //DomainVO domain = _domainDao.findById((Long)domainId); // // I want
- * to join on user_vm.domain_id = domain.id where domain.path like
- * 'foo%' //sc.setJoinParameters("domainSearch", "path",
- * domain.getPath() + "%"); // }
- */
-
- Pair<List<DiskOfferingJoinVO>, Integer> result = _diskOfferingJoinDao.searchAndCount(sc, searchFilter);
- // Remove offerings that are not associated with caller's domain
- if (account.getType() != Account.ACCOUNT_TYPE_ADMIN && CollectionUtils.isNotEmpty(result.first())) {
- ListIterator<DiskOfferingJoinVO> it = result.first().listIterator();
- while (it.hasNext()) {
- DiskOfferingJoinVO offering = it.next();
- if(!Strings.isNullOrEmpty(offering.getDomainId())) {
- boolean toRemove = true;
- String[] domainIdsArray = offering.getDomainId().split(",");
- for (String domainIdString : domainIdsArray) {
- Long dId = Long.valueOf(domainIdString.trim());
- if (isRecursive) {
- if (_domainDao.isChildDomain(account.getDomainId(), dId)) {
- toRemove = false;
- break;
- }
- } else {
- if (_domainDao.isChildDomain(dId, account.getDomainId())) {
- toRemove = false;
- break;
- }
- }
- }
- if (toRemove) {
- it.remove();
- }
- }
+ // Filter offerings that are not associated with caller's domain
+ // Fetch the offering ids from the details table since theres no smart way to filter them in the join ... yet!
+ Account caller = CallContext.current().getCallingAccount();
+ if (caller.getType() != Account.ACCOUNT_TYPE_ADMIN) {
+ Domain callerDomain = _domainDao.findById(caller.getDomainId());
+ List<Long> domainIds = _domainDao.getDomainParentIds(callerDomain.getId())
+ .stream().collect(Collectors.toList());
+ if (isRecursive) {
+ List<Long> childrenIds = _domainDao.getDomainChildrenIds(callerDomain.getPath());
+ if (childrenIds != null && !childrenIds.isEmpty())
+ domainIds.addAll(childrenIds);
}
- }
- return new Pair<>(result.first(), result.second());
- }
- private List<ServiceOfferingJoinVO> filterOfferingsOnCurrentTags(List<ServiceOfferingJoinVO> offerings, ServiceOfferingVO currentVmOffering) {
- if (currentVmOffering == null) {
- return offerings;
- }
- List<String> currentTagsList = StringUtils.csvTagsToList(currentVmOffering.getTags());
+ List<Long> ids = _diskOfferingDetailsDao.findOfferingIdsByDomainIds(domainIds);
+
+ if (ids == null || ids.isEmpty()) {
Review comment:
if else part can be refactored as follows as most of it contains common code
```
SearchBuilder<DiskOfferingJoinVO> sb = _diskOfferingJoinDao.createSearchBuilder();
sb.or("domainId", sb.entity().getDomainId(), Op.NULL);
SearchCriteria<DiskOfferingJoinVO> scc = sb.create();
sc.addAnd("domainId", SearchCriteria.Op.SC, scc);
if (ids != null && !ids.isEmpty()) {
sb.and("id", sb.entity().getId(), Op.IN);
scc.setParameters("id", ids.toArray());
}
sb.done();
```
##########
File path: engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/dao/DiskOfferingDetailsDaoImpl.java
##########
@@ -66,4 +71,25 @@ public String getDetail(Long diskOfferingId, String key) {
}
return detailValue;
}
-}
\ No newline at end of file
+
+ @Override
+ public List<Long> findOfferingIdsByDomainIds(List<Long> domainIds) {
Review comment:
@davidjumani This part is duplicated code right? this can be in a common place for both service and disk offering
##########
File path: server/src/main/java/com/cloud/api/query/QueryManagerImpl.java
##########
@@ -3064,39 +3033,72 @@
sc.addAnd("cpuspeedconstraints", SearchCriteria.Op.SC, cpuSpeedSearchCriteria);
}
- Pair<List<ServiceOfferingJoinVO>, Integer> result = _srvOfferingJoinDao.searchAndCount(sc, searchFilter);
-
- //Couldn't figure out a smart way to filter offerings based on tags in sql so doing it in Java.
- List<ServiceOfferingJoinVO> filteredOfferings = filterOfferingsOnCurrentTags(result.first(), currentVmOffering);
- // Remove offerings that are not associated with caller's domain
- if (caller.getType() != Account.ACCOUNT_TYPE_ADMIN && CollectionUtils.isNotEmpty(filteredOfferings)) {
- ListIterator<ServiceOfferingJoinVO> it = filteredOfferings.listIterator();
- while (it.hasNext()) {
- ServiceOfferingJoinVO offering = it.next();
- if(!Strings.isNullOrEmpty(offering.getDomainId())) {
- boolean toRemove = true;
- String[] domainIdsArray = offering.getDomainId().split(",");
- for (String domainIdString : domainIdsArray) {
- Long dId = Long.valueOf(domainIdString.trim());
- if (isRecursive) {
- if (_domainDao.isChildDomain(caller.getDomainId(), dId)) {
- toRemove = false;
- break;
- }
- } else {
- if (_domainDao.isChildDomain(dId, caller.getDomainId())) {
- toRemove = false;
- break;
- }
- }
- }
- if (toRemove) {
- it.remove();
- }
+ // Filter offerings that are not associated with caller's domain
+ // Fetch the offering ids from the details table since theres no smart way to filter them in the join ... yet!
+ if (caller.getType() != Account.ACCOUNT_TYPE_ADMIN) {
+ Domain callerDomain = _domainDao.findById(caller.getDomainId());
+ List<Long> domainIds = _domainDao.getDomainParentIds(callerDomain.getId())
+ .stream().collect(Collectors.toList());
+ if (isRecursive) {
+ List<Long> childrenIds = _domainDao.getDomainChildrenIds(callerDomain.getPath());
+ if (childrenIds != null && !childrenIds.isEmpty())
+ domainIds.addAll(childrenIds);
+ }
+
+ List<Long> ids = _srvOfferingDetailsDao.findOfferingIdsByDomainIds(domainIds);
Review comment:
Most of this is duplicated code. can be extracted to a single function.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org