Skip to content

Commit 67721ad

Browse files
Pearl1594Pearl1594
committed
address review comments: extract common code, fix tests
1 parent 845a454 commit 67721ad

File tree

13 files changed

+152
-122
lines changed

13 files changed

+152
-122
lines changed

‎api/src/main/java/org/apache/cloudstack/api/command/admin/backup/UpdateBackupOfferingCmd.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import org.apache.cloudstack.backup.BackupManager;
3131
import org.apache.cloudstack.backup.BackupOffering;
3232
import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils;
33+
import org.apache.commons.collections.CollectionUtils;
3334
import org.apache.commons.lang3.StringUtils;
3435

3536
import com.cloud.exception.InvalidParameterValueException;
@@ -93,7 +94,7 @@ public Boolean getAllowUserDrivenBackups() {
9394
@Override
9495
public void execute() {
9596
try {
96-
if (StringUtils.isAllEmpty(getName(), getDescription()) && getAllowUserDrivenBackups() == null) {
97+
if (StringUtils.isAllEmpty(getName(), getDescription()) && getAllowUserDrivenBackups() == null && CollectionUtils.isEmpty(getDomainIds())) {
9798
throw new InvalidParameterValueException(String.format("Can't update Backup Offering [id: %s] because there are no parameters to be updated, at least one of the",
9899
"following should be informed: name, description or allowUserDrivenBackups.", id));
99100
}

‎api/src/main/java/org/apache/cloudstack/api/command/offering/DomainAndZoneIdResolver.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ default List<Long> resolveDomainIds(final String domainIds, final Long id, final
6464

6565
final Domain validDomain = base._entityMgr.findByUuid(Domain.class, trimmed);
6666
if (validDomain == null) {
67-
logger.warn("Invalid domain specified for {}: {}", type, trimmed);
67+
logger.warn("Invalid domain specified for {}", type);
6868
throw new InvalidParameterValueException("Failed to create " + type + " because invalid domain has been specified.");
6969
}
7070
validDomainIds.add(validDomain.getId());

‎api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,11 +62,13 @@ public class BackupOfferingResponse extends BaseResponse {
6262
private String zoneName;
6363

6464
@SerializedName(ApiConstants.DOMAIN_ID)
65-
@Param(description = "the domain ID(s) this disk offering belongs to. Ignore this information as it is not currently applicable.")
65+
@Param(description = "the domain ID(s) this disk offering belongs to. Ignore this information as it is not currently applicable.",
66+
since = "4.23.0")
6667
private String domainId;
6768

6869
@SerializedName(ApiConstants.DOMAIN)
69-
@Param(description = "the domain name(s) this disk offering belongs to. Ignore this information as it is not currently applicable.")
70+
@Param(description = "the domain name(s) this disk offering belongs to. Ignore this information as it is not currently applicable.",
71+
since = "4.23.0")
7072
private String domain;
7173

7274
@SerializedName(ApiConstants.CROSS_ZONE_INSTANCE_CREATION)

‎engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupOfferingDetailsDao.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,4 +28,5 @@ public interface BackupOfferingDetailsDao extends GenericDao<BackupOfferingDetai
2828
List<Long> findZoneIds(final long resourceId);
2929
String getDetail(Long backupOfferingId, String key);
3030
List<Long> findOfferingIdsByDomainIds(List<Long> domainIds);
31+
void updateBackupOfferingDetails(long backupOfferingId, List<Long> filteredDomainIds);
3132
}

‎engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupOfferingDetailsDaoImpl.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@
2121
import java.util.List;
2222
import java.util.stream.Collectors;
2323

24+
import com.cloud.utils.db.DB;
25+
import com.cloud.utils.db.SearchBuilder;
26+
import com.cloud.utils.db.SearchCriteria;
2427
import org.apache.cloudstack.api.ApiConstants;
2528
import org.apache.cloudstack.backup.BackupOfferingDetailsVO;
2629
import org.apache.cloudstack.resourcedetail.ResourceDetailsDaoBase;
@@ -73,4 +76,26 @@ public List<Long> findOfferingIdsByDomainIds(List<Long> domainIds) {
7376
Object[] dIds = domainIds.stream().map(s -> String.valueOf(s)).collect(Collectors.toList()).toArray();
7477
return findResourceIdsByNameAndValueIn("domainid", dIds);
7578
}
79+
80+
@DB
81+
@Override
82+
public void updateBackupOfferingDetails(long backupOfferingId, List<Long> filteredDomainIds) {
83+
SearchBuilder<BackupOfferingDetailsVO> sb = createSearchBuilder();
84+
List<BackupOfferingDetailsVO> detailsVO = new ArrayList<>();
85+
sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ);
86+
sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
87+
sb.done();
88+
SearchCriteria<BackupOfferingDetailsVO> sc = sb.create();
89+
sc.setParameters("offeringId", String.valueOf(backupOfferingId));
90+
sc.setParameters("detailName", ApiConstants.DOMAIN_ID);
91+
remove(sc);
92+
for (Long domainId : filteredDomainIds) {
93+
detailsVO.add(new BackupOfferingDetailsVO(backupOfferingId, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false));
94+
}
95+
if (!detailsVO.isEmpty()) {
96+
for (BackupOfferingDetailsVO detailVO : detailsVO) {
97+
persist(detailVO);
98+
}
99+
}
100+
}
76101
}

‎server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java‎

Lines changed: 9 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
import com.cloud.consoleproxy.ConsoleProxyManager;
5353
import com.cloud.network.router.VirtualNetworkApplianceManager;
5454
import com.cloud.storage.secondary.SecondaryStorageVmManager;
55+
import com.cloud.utils.DomainHelper;
5556
import com.cloud.vm.VirtualMachineManager;
5657
import org.apache.cloudstack.acl.RoleType;
5758
import org.apache.cloudstack.acl.SecurityChecker;
@@ -398,6 +399,8 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
398399
ClusterDao _clusterDao;
399400
@Inject
400401
AlertManager _alertMgr;
402+
@Inject
403+
DomainHelper domainHelper;
401404
List<SecurityChecker> _secChecker;
402405
List<ExternalProvisioner> externalProvisioners;
403406

@@ -3509,7 +3512,7 @@ protected ServiceOfferingVO createServiceOffering(final long userId, final boole
35093512
final boolean isCustomized, final boolean encryptRoot, Long vgpuProfileId, Integer gpuCount, Boolean gpuDisplay, final boolean purgeResources, Integer leaseDuration, VMLeaseManager.ExpiryAction leaseExpiryAction) {
35103513

35113514
// Filter child domains when both parent and child domains are present
3512-
List<Long> filteredDomainIds = filterChildSubDomains(domainIds);
3515+
List<Long> filteredDomainIds = domainHelper.filterChildSubDomains(domainIds);
35133516

35143517
// Check if user exists in the system
35153518
final User user = _userDao.findById(userId);
@@ -3898,7 +3901,7 @@ public ServiceOffering updateServiceOffering(final UpdateServiceOfferingCmd cmd)
38983901
final Account account = _accountDao.findById(user.getAccountId());
38993902

39003903
// Filter child domains when both parent and child domains are present
3901-
List<Long> filteredDomainIds = filterChildSubDomains(domainIds);
3904+
List<Long> filteredDomainIds = domainHelper.filterChildSubDomains(domainIds);
39023905
Collections.sort(filteredDomainIds);
39033906

39043907
List<Long> filteredZoneIds = new ArrayList<>();
@@ -4102,7 +4105,7 @@ protected DiskOfferingVO createDiskOffering(final Long userId, final List<Long>
41024105
}
41034106

41044107
// Filter child domains when both parent and child domains are present
4105-
List<Long> filteredDomainIds = filterChildSubDomains(domainIds);
4108+
List<Long> filteredDomainIds = domainHelper.filterChildSubDomains(domainIds);
41064109

41074110
// Check if user exists in the system
41084111
final User user = _userDao.findById(userId);
@@ -4378,7 +4381,7 @@ public DiskOffering updateDiskOffering(final UpdateDiskOfferingCmd cmd) {
43784381
final Account account = _accountDao.findById(user.getAccountId());
43794382

43804383
// Filter child domains when both parent and child domains are present
4381-
List<Long> filteredDomainIds = filterChildSubDomains(domainIds);
4384+
List<Long> filteredDomainIds = domainHelper.filterChildSubDomains(domainIds);
43824385
Collections.sort(filteredDomainIds);
43834386

43844387
List<Long> filteredZoneIds = new ArrayList<>();
@@ -7385,7 +7388,7 @@ public NetworkOfferingVO doInTransaction(final TransactionStatus status) {
73857388
}
73867389
if (offering != null) {
73877390
// Filter child domains when both parent and child domains are present
7388-
List<Long> filteredDomainIds = filterChildSubDomains(domainIds);
7391+
List<Long> filteredDomainIds = domainHelper.filterChildSubDomains(domainIds);
73897392
List<NetworkOfferingDetailsVO> detailsVO = new ArrayList<>();
73907393
for (Long domainId : filteredDomainIds) {
73917394
detailsVO.add(new NetworkOfferingDetailsVO(offering.getId(), Detail.domainid, String.valueOf(domainId), false));
@@ -7851,7 +7854,7 @@ public NetworkOffering updateNetworkOffering(final UpdateNetworkOfferingCmd cmd)
78517854
}
78527855

78537856
// Filter child domains when both parent and child domains are present
7854-
List<Long> filteredDomainIds = filterChildSubDomains(domainIds);
7857+
List<Long> filteredDomainIds = domainHelper.filterChildSubDomains(domainIds);
78557858
Collections.sort(filteredDomainIds);
78567859

78577860
List<Long> filteredZoneIds = new ArrayList<>();
@@ -8418,28 +8421,6 @@ private boolean checkOverlapPortableIpRange(final int regionId, final String new
84188421
return false;
84198422
}
84208423

8421-
private List<Long> filterChildSubDomains(final List<Long> domainIds) {
8422-
List<Long> filteredDomainIds = new ArrayList<>();
8423-
if (domainIds != null) {
8424-
filteredDomainIds.addAll(domainIds);
8425-
}
8426-
if (filteredDomainIds.size() > 1) {
8427-
for (int i = filteredDomainIds.size() - 1; i >= 1; i--) {
8428-
for (int j = i - 1; j >= 0; j--) {
8429-
if (_domainDao.isChildDomain(filteredDomainIds.get(i), filteredDomainIds.get(j))) {
8430-
filteredDomainIds.remove(j);
8431-
i--;
8432-
}
8433-
if (_domainDao.isChildDomain(filteredDomainIds.get(j), filteredDomainIds.get(i))) {
8434-
filteredDomainIds.remove(i);
8435-
break;
8436-
}
8437-
}
8438-
}
8439-
}
8440-
return filteredDomainIds;
8441-
}
8442-
84438424
protected void validateCacheMode(String cacheMode){
84448425
if(cacheMode != null &&
84458426
!Enums.getIfPresent(DiskOffering.DiskCacheMode.class,

‎server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java‎

Lines changed: 5 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@
6363
import com.cloud.network.element.NsxProviderVO;
6464
import com.cloud.network.rules.RulesManager;
6565
import com.cloud.network.vpn.RemoteAccessVpnService;
66+
import com.cloud.utils.DomainHelper;
6667
import com.cloud.vm.dao.VMInstanceDao;
6768
import com.google.common.collect.Sets;
6869
import org.apache.cloudstack.acl.ControlledEntity.ACLType;
@@ -285,6 +286,8 @@ public class VpcManagerImpl extends ManagerBase implements VpcManager, VpcProvis
285286
@Inject
286287
DomainDao domainDao;
287288
@Inject
289+
DomainHelper domainHelper;
290+
@Inject
288291
private AnnotationDao annotationDao;
289292
@Inject
290293
NetworkOfferingDao _networkOfferingDao;
@@ -636,7 +639,7 @@ public VpcOffering createVpcOffering(final String name, final String displayText
636639
}
637640

638641
// Filter child domains when both parent and child domains are present
639-
List<Long> filteredDomainIds = filterChildSubDomains(domainIds);
642+
List<Long> filteredDomainIds = domainHelper.filterChildSubDomains(domainIds);
640643

641644
final Map<Network.Service, Set<Network.Provider>> svcProviderMap = new HashMap<Network.Service, Set<Network.Provider>>();
642645
final Set<Network.Provider> defaultProviders = new HashSet<Network.Provider>();
@@ -1118,7 +1121,7 @@ private VpcOffering updateVpcOfferingInternal(long vpcOffId, String vpcOfferingN
11181121

11191122

11201123
// Filter child domains when both parent and child domains are present
1121-
List<Long> filteredDomainIds = filterChildSubDomains(domainIds);
1124+
List<Long> filteredDomainIds = domainHelper.filterChildSubDomains(domainIds);
11221125
Collections.sort(filteredDomainIds);
11231126

11241127
List<Long> filteredZoneIds = new ArrayList<>();
@@ -3658,30 +3661,6 @@ private boolean rollingRestartVpc(final Vpc vpc, final ReservationContext contex
36583661
return _ntwkMgr.areRoutersRunning(routerDao.listByVpcId(vpc.getId()));
36593662
}
36603663

3661-
private List<Long> filterChildSubDomains(final List<Long> domainIds) {
3662-
List<Long> filteredDomainIds = new ArrayList<>();
3663-
if (domainIds != null) {
3664-
filteredDomainIds.addAll(domainIds);
3665-
}
3666-
if (filteredDomainIds.size() > 1) {
3667-
for (int i = filteredDomainIds.size() - 1; i >= 1; i--) {
3668-
long first = filteredDomainIds.get(i);
3669-
for (int j = i - 1; j >= 0; j--) {
3670-
long second = filteredDomainIds.get(j);
3671-
if (domainDao.isChildDomain(filteredDomainIds.get(i), filteredDomainIds.get(j))) {
3672-
filteredDomainIds.remove(j);
3673-
i--;
3674-
}
3675-
if (domainDao.isChildDomain(filteredDomainIds.get(j), filteredDomainIds.get(i))) {
3676-
filteredDomainIds.remove(i);
3677-
break;
3678-
}
3679-
}
3680-
}
3681-
}
3682-
return filteredDomainIds;
3683-
}
3684-
36853664
protected boolean isGlobalAcl(Long aclVpcId) {
36863665
return aclVpcId != null && aclVpcId == 0;
36873666
}

‎server/src/main/java/com/cloud/utils/DomainHelper.java‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
package com.cloud.utils;
18+
19+
import java.util.ArrayList;
20+
import java.util.List;
21+
import java.util.Objects;
22+
23+
import javax.inject.Inject;
24+
25+
import org.springframework.stereotype.Component;
26+
27+
import com.cloud.domain.dao.DomainDao;
28+
29+
/**
30+
* Utility class for domain-related operations
31+
*/
32+
@Component
33+
public class DomainHelper {
34+
35+
@Inject
36+
private DomainDao domainDao;
37+
38+
/**
39+
* Filters out child/descendant domains when their parent/ancestor is also present in the list.
40+
* This prevents redundancy by keeping only the "top-level" domains.
41+
*
42+
* For example, if the input contains [Root, Child, Grandchild], only [Root] will be returned
43+
* because Child and Grandchild are descendants of Root.
44+
*
45+
* @param domainIds List of domain IDs to filter
46+
* @return Filtered list containing only domains that are not descendants of other domains in the list
47+
*/
48+
public List<Long> filterChildSubDomains(final List<Long> domainIds) {
49+
if (domainIds == null || domainIds.size() <= 1) {
50+
return domainIds == null ? new ArrayList<>() : new ArrayList<>(domainIds);
51+
}
52+
53+
final List<Long> result = new ArrayList<>();
54+
for (final Long candidate : domainIds) {
55+
boolean isDescendant = false;
56+
for (final Long other : domainIds) {
57+
if (Objects.equals(candidate, other)) {
58+
continue;
59+
}
60+
if (domainDao.isChildDomain(other, candidate)) {
61+
isDescendant = true;
62+
break;
63+
}
64+
}
65+
if (!isDescendant) {
66+
result.add(candidate);
67+
}
68+
}
69+
return result;
70+
}
71+
}
72+

0 commit comments

Comments
 (0)