GutoVeronezi commented on code in PR #10505:
URL: https://github.com/apache/cloudstack/pull/10505#discussion_r3011514729
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaSummaryCmd.java:
##########
@@ -16,61 +16,80 @@
//under the License.
package org.apache.cloudstack.api.command;
-import com.cloud.user.Account;
import com.cloud.utils.Pair;
+
+import org.apache.cloudstack.api.ACL;
import org.apache.cloudstack.api.APICommand;
import org.apache.cloudstack.api.ApiConstants;
import org.apache.cloudstack.api.BaseListCmd;
import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.response.AccountResponse;
import org.apache.cloudstack.api.response.DomainResponse;
-import org.apache.cloudstack.api.response.ListResponse;
import org.apache.cloudstack.api.response.QuotaResponseBuilder;
import org.apache.cloudstack.api.response.QuotaSummaryResponse;
-import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.api.response.ProjectResponse;
+import org.apache.cloudstack.api.response.ListResponse;
+import org.apache.cloudstack.quota.QuotaAccountStateFilter;
+import org.apache.cloudstack.quota.QuotaService;
+import org.apache.commons.lang3.ObjectUtils;
import java.util.List;
import javax.inject.Inject;
-@APICommand(name = "quotaSummary", responseObject =
QuotaSummaryResponse.class, description = "Lists balance and quota usage for
all Accounts", since = "4.7.0", requestHasSensitiveInfo = false,
responseHasSensitiveInfo = false,
- httpMethod = "GET")
+@APICommand(name = "quotaSummary", responseObject =
QuotaSummaryResponse.class, description = "Lists Quota balance summary of
Accounts.", since = "4.7.0",
Review Comment:
```suggestion
@APICommand(name = "quotaSummary", responseObject =
QuotaSummaryResponse.class, description = "Lists Quota balance summary of
accounts and projects.", since = "4.7.0",
```
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java:
##########
@@ -42,20 +42,24 @@
import javax.inject.Inject;
+import com.cloud.domain.Domain;
import com.cloud.exception.PermissionDeniedException;
+import com.cloud.projects.dao.ProjectDao;
import com.cloud.user.User;
import com.cloud.user.UserVO;
import com.cloud.utils.DateUtil;
import com.cloud.utils.exception.CloudRuntimeException;
import org.apache.cloudstack.api.ApiErrorCode;
import org.apache.cloudstack.api.ServerApiException;
+
Review Comment:
Is this necessary?
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaSummaryResponse.java:
##########
@@ -30,40 +29,48 @@
public class QuotaSummaryResponse extends BaseResponse {
@SerializedName("accountid")
- @Param(description = "Account ID")
+ @Param(description = "Account's ID")
private String accountId;
@SerializedName("account")
- @Param(description = "Account name")
+ @Param(description = "Account's name")
private String accountName;
@SerializedName("domainid")
- @Param(description = "Domain ID")
+ @Param(description = "Domain's ID")
private String domainId;
@SerializedName("domain")
- @Param(description = "Domain name")
- private String domainName;
+ @Param(description = "Domain's path")
+ private String domainPath;
@SerializedName("balance")
- @Param(description = "Account balance")
+ @Param(description = "Account's balance")
private BigDecimal balance;
@SerializedName("state")
- @Param(description = "Account state")
+ @Param(description = "Account's state")
private State state;
+ @SerializedName("domainremoved")
+ @Param(description = "If the domain is removed or not", since = "4.21.0")
Review Comment:
```suggestion
@Param(description = "If the domain is removed or not", since = "4.23.0")
```
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java:
##########
@@ -180,75 +194,108 @@ public QuotaTariffResponse
createQuotaTariffResponse(QuotaTariffVO tariff, boole
}
@Override
- public Pair<List<QuotaSummaryResponse>, Integer>
createQuotaSummaryResponse(final String accountName, final Long domainId) {
- List<QuotaSummaryResponse> result = new
ArrayList<QuotaSummaryResponse>();
+ public Pair<List<QuotaSummaryResponse>, Integer>
createQuotaSummaryResponse(QuotaSummaryCmd cmd) {
+ Account caller = CallContext.current().getCallingAccount();
- if (accountName != null && domainId != null) {
- Account account = _accountDao.findActiveAccount(accountName,
domainId);
- QuotaSummaryResponse qr = getQuotaSummaryResponse(account);
- result.add(qr);
+ if
(!accountTypesThatCanListAllQuotaSummaries.contains(caller.getType()) ||
!cmd.isListAll()) {
+ return getQuotaSummaryResponse(cmd.getEntityOwnerId(), null, null,
cmd);
}
- return new Pair<>(result, result.size());
+ return getQuotaSummaryResponseWithListAll(cmd, caller);
}
- @Override
- public Pair<List<QuotaSummaryResponse>, Integer>
createQuotaSummaryResponse(Boolean listAll) {
- return createQuotaSummaryResponse(listAll, null, null, null);
+ protected Pair<List<QuotaSummaryResponse>, Integer>
getQuotaSummaryResponseWithListAll(QuotaSummaryCmd cmd, Account caller) {
+ Long domainId = cmd.getDomainId();
+ if (domainId != null) {
+ DomainVO domain = domainDao.findByIdIncludingRemoved(domainId);
+ if (domain == null) {
+ throw new InvalidParameterValueException(String.format("Domain
[%s] does not exist.", domainId));
+ }
+ }
+
+ String domainPath = getDomainPathByDomainIdForDomainAdmin(caller);
+
+ Long accountId = cmd.getEntityOwnerId();
+ if (accountId == -1) {
+ // No specific account was provided
Review Comment:
I think this comment is not necessary.
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaSummaryResponse.java:
##########
@@ -73,9 +80,17 @@ public class QuotaSummaryResponse extends BaseResponse {
@Param(description = "If the account has the quota config enabled")
private boolean quotaEnabled;
- public QuotaSummaryResponse() {
- super();
- }
+ @SerializedName("projectname")
+ @Param(description = "Name of the project", since = "4.21.0")
+ private String projectName;
+
+ @SerializedName("projectid")
+ @Param(description = "Project's id", since = "4.21.0")
Review Comment:
```suggestion
@Param(description = "Project's id", since = "4.23.0")
```
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java:
##########
@@ -180,75 +194,108 @@ public QuotaTariffResponse
createQuotaTariffResponse(QuotaTariffVO tariff, boole
}
@Override
- public Pair<List<QuotaSummaryResponse>, Integer>
createQuotaSummaryResponse(final String accountName, final Long domainId) {
- List<QuotaSummaryResponse> result = new
ArrayList<QuotaSummaryResponse>();
+ public Pair<List<QuotaSummaryResponse>, Integer>
createQuotaSummaryResponse(QuotaSummaryCmd cmd) {
+ Account caller = CallContext.current().getCallingAccount();
- if (accountName != null && domainId != null) {
- Account account = _accountDao.findActiveAccount(accountName,
domainId);
- QuotaSummaryResponse qr = getQuotaSummaryResponse(account);
- result.add(qr);
+ if
(!accountTypesThatCanListAllQuotaSummaries.contains(caller.getType()) ||
!cmd.isListAll()) {
+ return getQuotaSummaryResponse(cmd.getEntityOwnerId(), null, null,
cmd);
}
- return new Pair<>(result, result.size());
+ return getQuotaSummaryResponseWithListAll(cmd, caller);
}
- @Override
- public Pair<List<QuotaSummaryResponse>, Integer>
createQuotaSummaryResponse(Boolean listAll) {
- return createQuotaSummaryResponse(listAll, null, null, null);
+ protected Pair<List<QuotaSummaryResponse>, Integer>
getQuotaSummaryResponseWithListAll(QuotaSummaryCmd cmd, Account caller) {
+ Long domainId = cmd.getDomainId();
+ if (domainId != null) {
+ DomainVO domain = domainDao.findByIdIncludingRemoved(domainId);
+ if (domain == null) {
+ throw new InvalidParameterValueException(String.format("Domain
[%s] does not exist.", domainId));
+ }
+ }
+
+ String domainPath = getDomainPathByDomainIdForDomainAdmin(caller);
+
+ Long accountId = cmd.getEntityOwnerId();
+ if (accountId == -1) {
+ // No specific account was provided
+ accountId = cmd.isListAll() ? null : caller.getAccountId();
+ }
+
+ return getQuotaSummaryResponse(accountId, domainId, domainPath, cmd);
}
- @Override
- public Pair<List<QuotaSummaryResponse>, Integer>
createQuotaSummaryResponse(Boolean listAll, final String keyword, final Long
startIndex, final Long pageSize) {
- List<QuotaSummaryResponse> result = new
ArrayList<QuotaSummaryResponse>();
- Integer count = 0;
- if (listAll) {
- Filter filter = new Filter(AccountVO.class, "accountName", true,
startIndex, pageSize);
- Pair<List<AccountVO>, Integer> data =
_accountDao.findAccountsLike(keyword, filter);
- count = data.second();
- for (final AccountVO account : data.first()) {
- QuotaSummaryResponse qr = getQuotaSummaryResponse(account);
- result.add(qr);
- }
- } else {
- Pair<List<QuotaAccountVO>, Integer> data =
quotaAccountDao.listAllQuotaAccount(startIndex, pageSize);
- count = data.second();
- for (final QuotaAccountVO quotaAccount : data.first()) {
- AccountVO account = _accountDao.findById(quotaAccount.getId());
- if (account == null) {
- continue;
- }
- QuotaSummaryResponse qr = getQuotaSummaryResponse(account);
- result.add(qr);
- }
+ /**
+ * Retrieves the domain path of the caller's domain (if the caller is
Domain Admin) for filtering in the quota summary query.
+ * @return null if the caller is an Admin or the domain path of the
caller's domain if the caller is a Domain Admin.
+ * @throws InvalidParameterValueException if it cannot find the domain.
+ */
+ protected String getDomainPathByDomainIdForDomainAdmin(Account caller) {
+ if (caller.getType() != Account.Type.DOMAIN_ADMIN) {
+ return null;
+ }
+
+ Long domainId = caller.getDomainId();
+ Domain domain = domainDao.findById(domainId);
+ _accountMgr.checkAccess(caller, domain);
+
+ if (domain == null) {
+ throw new InvalidParameterValueException(String.format("Domain ID
[%s] is invalid.", domainId));
}
- return new Pair<>(result, count);
+
+ return domain.getPath();
}
- protected QuotaSummaryResponse getQuotaSummaryResponse(final Account
account) {
- Calendar[] period = _statement.getCurrentStatementTime();
-
- if (account != null) {
- QuotaSummaryResponse qr = new QuotaSummaryResponse();
- DomainVO domain = _domainDao.findById(account.getDomainId());
- BigDecimal curBalance =
_quotaBalanceDao.lastQuotaBalance(account.getAccountId(),
account.getDomainId(), period[1].getTime());
- BigDecimal quotaUsage =
_quotaUsageDao.findTotalQuotaUsage(account.getAccountId(),
account.getDomainId(), null, period[0].getTime(), period[1].getTime());
-
- qr.setAccountId(account.getUuid());
- qr.setAccountName(account.getAccountName());
- qr.setDomainId(domain.getUuid());
- qr.setDomainName(domain.getName());
- qr.setBalance(curBalance);
- qr.setQuotaUsage(quotaUsage);
- qr.setState(account.getState());
- qr.setStartDate(period[0].getTime());
- qr.setEndDate(period[1].getTime());
- qr.setCurrency(QuotaConfig.QuotaCurrencySymbol.value());
-
qr.setQuotaEnabled(QuotaConfig.QuotaAccountEnabled.valueIn(account.getId()));
- qr.setObjectName("summary");
- return qr;
- } else {
- return new QuotaSummaryResponse();
+ protected Pair<List<QuotaSummaryResponse>, Integer>
getQuotaSummaryResponse(Long accountId, Long domainId, String domainPath,
QuotaSummaryCmd cmd) {
+ if (accountId != null && accountId == -1) {
+ // Either no specific account as provided, or list all is disabled
Review Comment:
I think this comment could be a JavaDoc.
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaResponseBuilderImpl.java:
##########
@@ -121,7 +129,7 @@ public class QuotaResponseBuilderImpl implements
QuotaResponseBuilder {
@Inject
private QuotaCreditsDao quotaCreditsDao;
@Inject
- private QuotaUsageDao _quotaUsageDao;
+ private QuotaUsageDao quotaUsageDao;
Review Comment:
Refactoring/renaming would be better done in separate/specific PRs to reduce
the number of lines to review in this context.
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaSummaryResponse.java:
##########
@@ -30,40 +29,48 @@
public class QuotaSummaryResponse extends BaseResponse {
@SerializedName("accountid")
- @Param(description = "Account ID")
+ @Param(description = "Account's ID")
private String accountId;
@SerializedName("account")
- @Param(description = "Account name")
+ @Param(description = "Account's name")
private String accountName;
@SerializedName("domainid")
- @Param(description = "Domain ID")
+ @Param(description = "Domain's ID")
private String domainId;
@SerializedName("domain")
- @Param(description = "Domain name")
- private String domainName;
+ @Param(description = "Domain's path")
+ private String domainPath;
@SerializedName("balance")
- @Param(description = "Account balance")
+ @Param(description = "Account's balance")
private BigDecimal balance;
@SerializedName("state")
- @Param(description = "Account state")
+ @Param(description = "Account's state")
private State state;
+ @SerializedName("domainremoved")
+ @Param(description = "If the domain is removed or not", since = "4.21.0")
+ private boolean domainRemoved;
+
+ @SerializedName("accountremoved")
+ @Param(description = "If the account is removed or not", since = "4.21.0")
Review Comment:
```suggestion
@Param(description = "If the account is removed or not", since =
"4.23.0")
```
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaSummaryResponse.java:
##########
@@ -73,9 +80,17 @@ public class QuotaSummaryResponse extends BaseResponse {
@Param(description = "If the account has the quota config enabled")
private boolean quotaEnabled;
- public QuotaSummaryResponse() {
- super();
- }
+ @SerializedName("projectname")
+ @Param(description = "Name of the project", since = "4.21.0")
+ private String projectName;
+
+ @SerializedName("projectid")
+ @Param(description = "Project's id", since = "4.21.0")
+ private String projectId;
+
+ @SerializedName("projectremoved")
+ @Param(description = "Whether the project is removed or not", since =
"4.21.0")
Review Comment:
```suggestion
@Param(description = "Whether the project is removed or not", since =
"4.23.0")
```
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/response/QuotaSummaryResponse.java:
##########
@@ -73,9 +80,17 @@ public class QuotaSummaryResponse extends BaseResponse {
@Param(description = "If the account has the quota config enabled")
private boolean quotaEnabled;
- public QuotaSummaryResponse() {
- super();
- }
+ @SerializedName("projectname")
+ @Param(description = "Name of the project", since = "4.21.0")
Review Comment:
```suggestion
@Param(description = "Name of the project", since = "4.23.0")
```
##########
plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaSummaryCmd.java:
##########
@@ -16,61 +16,80 @@
//under the License.
package org.apache.cloudstack.api.command;
-import com.cloud.user.Account;
import com.cloud.utils.Pair;
+
+import org.apache.cloudstack.api.ACL;
import org.apache.cloudstack.api.APICommand;
import org.apache.cloudstack.api.ApiConstants;
import org.apache.cloudstack.api.BaseListCmd;
import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.response.AccountResponse;
import org.apache.cloudstack.api.response.DomainResponse;
-import org.apache.cloudstack.api.response.ListResponse;
import org.apache.cloudstack.api.response.QuotaResponseBuilder;
import org.apache.cloudstack.api.response.QuotaSummaryResponse;
-import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.api.response.ProjectResponse;
+import org.apache.cloudstack.api.response.ListResponse;
+import org.apache.cloudstack.quota.QuotaAccountStateFilter;
+import org.apache.cloudstack.quota.QuotaService;
+import org.apache.commons.lang3.ObjectUtils;
import java.util.List;
import javax.inject.Inject;
-@APICommand(name = "quotaSummary", responseObject =
QuotaSummaryResponse.class, description = "Lists balance and quota usage for
all Accounts", since = "4.7.0", requestHasSensitiveInfo = false,
responseHasSensitiveInfo = false,
- httpMethod = "GET")
+@APICommand(name = "quotaSummary", responseObject =
QuotaSummaryResponse.class, description = "Lists Quota balance summary of
Accounts.", since = "4.7.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
httpMethod = "GET")
public class QuotaSummaryCmd extends BaseListCmd {
+ @Inject
+ QuotaResponseBuilder quotaResponseBuilder;
+
+ @Inject
+ QuotaService quotaService;
+
+ @ACL
+ @Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID,
entityType = AccountResponse.class, description = "ID of the account for which
balance will be listed. Can not be specified with projectId.", since = "4.23.0")
+ private Long accountId;
+
+ @ACL
@Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING,
required = false, description = "Optional, Account Id for which statement needs
to be generated")
private String accountName;
+ @ACL
@Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID,
required = false, entityType = DomainResponse.class, description = "Optional,
If domain Id is given and the caller is domain admin then the statement is
generated for domain.")
private Long domainId;
- @Parameter(name = ApiConstants.LIST_ALL, type = CommandType.BOOLEAN,
required = false, description = "Optional, to list all Accounts irrespective of
the quota activity")
+ @Parameter(name = ApiConstants.LIST_ALL, type = CommandType.BOOLEAN,
description = "False (default) lists the Quota balance summary for calling
Account. True lists balance summary for " +
Review Comment:
It is important to mention here that the default changes to `true` if the
domain ID is informed.
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -3886,6 +3888,48 @@ public Long finalizeAccountId(final String accountName,
final Long domainId, fin
return null;
}
+ @Override
+ public Long finalizeAccountId(Long accountId, String accountName, Long
domainId, Long projectId) {
+ if (projectId != null) {
+ if (ObjectUtils.anyNotNull(accountId, accountName)) {
+ throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
"Project and account can not be specified together.");
+ }
+ return getActiveProjectAccountByProjectId(projectId);
+ }
+ if (accountId != null) {
+ if (getActiveAccountById(accountId) != null) {
+ return accountId;
+ }
+ throw new InvalidParameterValueException(String.format("Unable to
find account with ID [%s].", accountId));
+ }
+
+ if (accountName == null && domainId == null) {
+ throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
String.format("Either %s or %s is required.", ApiConstants.ACCOUNT_ID,
ApiConstants.PROJECT_ID));
Review Comment:
```suggestion
throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
String.format("Either %s or %s must be informed.", ApiConstants.ACCOUNT_ID,
ApiConstants.PROJECT_ID));
```
--
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]
