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/12/29 08:18:53 UTC
[GitHub] [cloudstack] Pearl1594 opened a new pull request #5815: API: Fix listSSHKeyPairs API when listing all resources (listall=true & projectid=-1)
Pearl1594 opened a new pull request #5815:
URL: https://github.com/apache/cloudstack/pull/5815
### Description
This PR fixes the issue noticed when listing all ssh key pairs including ones owned by projects. Exception:
```
(primateqa) 🐱 > list sshkeypairs listall=true projectid=-1
🙈 Error: (HTTP 530, error code 4250) DB Exception on: com.mysql.cj.jdbc.ClientPreparedStatement: SELECT ssh_keypairs.id, ssh_keypairs.uuid, ssh_keypairs.account_id, ssh_keypairs.domain_id, ssh_keypairs.keypair_name, ssh_keypairs.fingerprint, ssh_keypairs.public_key FROM ssh_keypairs INNER JOIN account ON ssh_keypairs.account_id=account.id WH ORDER BY ssh_keypairs.id DESC LIMIT 0, 500
```
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ********************************************************************************* -->
<!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ********************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [X] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [X] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
#### Post Fix:
```
(localcloud) 🐱 > list sshkeypairs listall=true page=1 pagesize=20 filter=name,
{
"count": 1,
"sshkeypair": [
{
"name": "ssh"
}
]
}
(localcloud) 🐱 > list sshkeypairs listall=true projectid=-1 page=1 pagesize=20 filter=name,
{
"count": 2,
"sshkeypair": [
{
"name": "test"
},
{
"name": "ssh"
}
]
}
```
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
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
[GitHub] [cloudstack] nvazquez commented on pull request #5815: API: Fix listSSHKeyPairs API when listing all resources (listall=true & projectid=-1)
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5815:
URL: https://github.com/apache/cloudstack/pull/5815#issuecomment-1030739019
Thanks @rohityadavcloud I agree, @sureshanaparti please include it on 4.16.1 if necessary
--
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
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5815: API: Fix listSSHKeyPairs API when listing all resources (listall=true & projectid=-1)
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5815:
URL: https://github.com/apache/cloudstack/pull/5815#discussion_r776640457
##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -1293,7 +1294,8 @@ protected void addJoins(StringBuilder str, Collection<JoinBuilder<SearchCriteria
fromIndex += onClause.length();
}
- str.delete(str.length() - 4, str.length());
+ int diff = str.toString().trim().toLowerCase(Locale.ROOT).endsWith("where") ? 6 : 4;
Review comment:
@Pearl1594 I'd leave out the trim() it is not clear why the 6 or 4 like this (or add a comment, but I prefer the code to explain itself)
--
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
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5815: API: Fix listSSHKeyPairs API when listing all resources (listall=true & projectid=-1)
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5815:
URL: https://github.com/apache/cloudstack/pull/5815#discussion_r777312173
##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -1293,7 +1294,8 @@ protected void addJoins(StringBuilder str, Collection<JoinBuilder<SearchCriteria
fromIndex += onClause.length();
}
- str.delete(str.length() - 4, str.length());
+ int diff = str.toString().trim().toLowerCase(Locale.ROOT).endsWith("where") ? 6 : 4;
Review comment:
+1, add comment if code is not self-explanatory.
--
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
[GitHub] [cloudstack] Pearl1594 closed pull request #5815: API: Fix listSSHKeyPairs API when listing all resources (listall=true & projectid=-1)
Posted by GitBox <gi...@apache.org>.
Pearl1594 closed pull request #5815:
URL: https://github.com/apache/cloudstack/pull/5815
--
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
[GitHub] [cloudstack] sureshanaparti commented on pull request #5815: API: Fix listSSHKeyPairs API when listing all resources (listall=true & projectid=-1)
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5815:
URL: https://github.com/apache/cloudstack/pull/5815#issuecomment-1031061156
@Pearl1594 Is this PR good to go in 4.16.1? If so, please address outstanding comment and rebase with 4.16. Thanks.
--
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
[GitHub] [cloudstack] rohityadavcloud commented on a change in pull request #5815: API: Fix listSSHKeyPairs API when listing all resources (listall=true & projectid=-1)
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on a change in pull request #5815:
URL: https://github.com/apache/cloudstack/pull/5815#discussion_r776203127
##########
File path: framework/db/src/main/java/com/cloud/utils/db/GenericDaoBase.java
##########
@@ -1293,7 +1294,8 @@ protected void addJoins(StringBuilder str, Collection<JoinBuilder<SearchCriteria
fromIndex += onClause.length();
}
- str.delete(str.length() - 4, str.length());
+ int diff = str.toString().trim().toLowerCase(Locale.ROOT).endsWith("where") ? 6 : 4;
+ str.delete(str.length() - diff, str.length());
Review comment:
@Pearl1594 is there any unit test to check/cover this, could this cause a regression? Maybe explore a more localised fix that's limited to ssh-key based DaoImpl?
--
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
[GitHub] [cloudstack] weizhouapache commented on pull request #5815: API: Fix listSSHKeyPairs API when listing all resources (listall=true & projectid=-1)
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5815:
URL: https://github.com/apache/cloudstack/pull/5815#issuecomment-1004045292
@Pearl1594
I suggest to append 'WHERE' only when it is required, like code changes below
```
@@ -1262,10 +1262,11 @@ public abstract class GenericDaoBase<T, ID extends Serializable> extends Compone
@DB()
protected void addJoins(StringBuilder str, Collection<JoinBuilder<SearchCriteria<?>>> joins) {
+ boolean hasWhereClause = true;
int fromIndex = str.lastIndexOf("WHERE");
if (fromIndex == -1) {
fromIndex = str.length();
- str.append(" WHERE ");
+ hasWhereClause = false;
} else {
str.append(" AND ");
}
@@ -1287,13 +1288,20 @@ public abstract class GenericDaoBase<T, ID extends Serializable> extends Compone
.append(" ");
str.insert(fromIndex, onClause);
String whereClause = join.getT().getWhereClause();
- if ((whereClause != null) && !"".equals(whereClause)) {
+ if (!StringUtils.isEmpty(whereClause)) {
+ if (!hasWhereClause) {
+ str.append(" WHERE ");
+ hasWhereClause = true;
+ }
str.append(" (").append(whereClause).append(") AND");
}
fromIndex += onClause.length();
}
- str.delete(str.length() - 4, str.length());
+ if (hasWhereClause) {
+ // Remove ' AND' OR 'AND ' at the end of line
+ str.delete(str.length() - 4, str.length());
+ }
for (JoinBuilder<SearchCriteria<?>> join : joins) {
if (join.getT().getJoins() != null) {
```
--
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
[GitHub] [cloudstack] rohityadavcloud commented on pull request #5815: API: Fix listSSHKeyPairs API when listing all resources (listall=true & projectid=-1)
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on pull request #5815:
URL: https://github.com/apache/cloudstack/pull/5815#issuecomment-1029761245
This looks like a bugfix, should it be considered for 4.16.1 @sureshanaparti @nvazquez @DaanHoogland @weizhouapache
--
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