You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by nitin-maharana <gi...@git.apache.org> on 2015/12/10 07:25:17 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
GitHub user nitin-maharana opened a pull request:
https://github.com/apache/cloudstack/pull/1206
CLOUDSTACK-9132: API createVolume takes empty string for name parameter
Steps to Reproduce:
================
Create a volume using createVolume API where parameter name is empty.
It creates a volume with empty name.
But the name parameter is mandatory.(Issue)
Expected Behaviour:
================
It shouldn't create a volume with an empty name. Error should be returned.
Solution:
=======
Added the validation check for volume name.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nitin-maharana/CloudStack CloudStack-Nitin15
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1206.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1206
----
commit 4a5993b5d4693fcdb7017503d5776d66104e643f
Author: Nitin Kumar Maharana <ni...@gmail.com>
Date: 2015-12-09T11:50:48Z
CLOUDSTACK-9132: API createVolume takes empty string for name parameter
Added the validation check for volume name.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-166556945
Made a new PR https://github.com/apache/cloudstack/pull/1273 with master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164366344
API with non-empty name:
====================
http://10.102.192.122:8080/client/api?command=createVolume&response=json&name=TempVolume&zoneId=18b1cb1a-bb8b-4e3d-aab8-a48475ed3c60&diskOfferingId=dc8a8d64-34a6-481e-99f9-89a49ee94824&size=1&_=1450077422731
API with empty name:
=================
http://10.102.192.122:8080/client/api?command=createVolume&response=json&name=&zoneId=18b1cb1a-bb8b-4e3d-aab8-a48475ed3c60&diskOfferingId=dc8a8d64-34a6-481e-99f9-89a49ee94824&size=1&_=1450077422731
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by priyankparihar <gi...@git.apache.org>.
Github user priyankparihar commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164412418
@nitin-maharana Your point looks good. Both(UI and API) should be consistent.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164344691
@DaanHoogland : Yes, your idea also looks good. Previously, it was generating a random value only in case of NULL. But if we pass an empty string, it was creating a volume with the empty name. I will modify the condition and update the PR. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-165076449
@rafaelweingartner : Thanks for the suggestion. I will follow the same to write test cases for this.
@DaanHoogland : Should we make the Name field as optional in UI as well as API?
Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-165757054
cc @koushik-das @kishankavala
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164415638
I don't agree, for two reasons
1. making the field required constitutes a backwards incompatibility of the API
2. we should use as much reasonable defaults as posible as to not burdon the user with decision they don't strictly have to make.
I do agree with the present implementation as can be seen in the diff at this moment:
```
- if (userSpecifiedName == null) {
+ if (userSpecifiedName == null || userSpecifiedName.isEmpty()) {
```
Though the screenshots are not real tests I can live with them for this change: LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164482127
@nitin-maharana I do not agree with you. Take a look at the size of the method “allocVolume”, there are more than 100 lines, that is terrible to write a test case. We should start creating little chunks of code, test them as unit, and then we can write integration tests.
Something like this would do:
protected String getVolumeNameFromCommand(CreateVolumeCmd cmd){
String userSpecifiedName = cmd.getVolumeName();
if (userSpecifiedName == null || userSpecifiedName.isEmpty()) {
userSpecifiedName = getRandomVolumeName();
}
return userSpecifiedName;
}
After that, you can write test cases to check if the method “getVolumeNameFromCommand” is doing its job as expected. That means, one test to check if it calls “getRandomVolumeName” and returns the value of that call when “cmd.getVolumeName()” returns null and other when it returns empty.
I would also add a third test case to check if the method cmd.getVolumeName() returns something that is not empty, if that value is properly returned.
Having said that, I would also ask, is “ ” (string with a lot of space empty for you?) Because, for me empty is “”, this “ ” is blank, and this way we have another test case to write a make sure everything is proper coded.
For Java, empty is "", which means if cmd.getVolumeName() returns " ", that is the value that will be used.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164187066
@nitin-maharana I don't agree we should allow the user to get a default name if (s)he doesn't care. I'd say let the name be the uuid-generated one when empty (as well as when null).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164411409
@DaanHoogland : In UI, we put the field as required field, so user has to give some name to create a volume. It cannot be null or blank. But according the change, the API doesn't support the logic of UI. Either we should make the field as optional in UI or we should throw an error in API when the name is null or empty.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana closed the pull request at:
https://github.com/apache/cloudstack/pull/1206
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-165081512
@nitin-maharana that would be my preference. In the UI a pop-up could explain the consequence of not entering a name.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164417605
Hi @nitin-maharana I agree with @DaanHoogland, we should maintain backward compatibility. I almost liked you code, I would just suggest you extracting it to a method such as getVolumeName or getCreateVolumeNameIfNeeded (sorry for the lack of creativity, today I am out of inspiration).
After you extracted the code to a method, you should be able to write at least two test cases to make sure it is working as expected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9132: API createVolume takes e...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164475760
@DaanHoogland : You mean the name should be an optional field? If that is the case, we should make the field as optional in UI as well as in API.
@rafaelweingartner : The volume name is either collected from the UI or we have a method in API called getRandomVolumeName() which sets a random value. I think we don't need to extract further.
The above method sets a value.
Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---