CLOUDSTACK-9132: API createVolume takes empty string for name parameter

[nitin-maharana]

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 a condition to check in case of empty string. If the name is an empty string, it generates a random name for the volume. Made the name field optional in UI as well as in API.

[nitin-maharana]

Reference(4.6) : #1206

[koushik-das]

@nitin-maharana HypervisorUtilsTest test have failed, please do a force push again.
Code changes LGTM

[nitin-maharana]

Sure @koushik-das. Thanks.

[rafaelweingartner]

@nitin-maharana, thanks for the update, I will just call your attention to a few points:

First of all, why did you open a new PR? I know you closed the other one, but IMHO it makes my job harder. The other PR had all of our conversions; you should have kept working there. It would be easier to track everything that was discussed if everything was at the same PR. Now please, keep with this one, no need to come back to the one you already closed or to open a new one after my suggestions.

I noticed that you extract the code to a method as I suggested and created some test cases. You also add the “required = true” as Daan suggested; that is good.

Now about the method “getVolumeNameFromCommand” you created. Please note that // is not the proper way to create a Java doc, please replace the // by the proper mark of Java docs

/** 
* this is a java doc
**/

/*
*This is a block comment
*/

// this is a line comment

About the Java doc I would also encourage you to detail a little bit more the method such as:

/** 
* This method retrieves the volume name from the CreateVolumeCmd object.
* If the method CreateVolumeCmd#getVolumeName() returns null, empty or blank, It will be generated a random name using getRandomVolumeName().
**/

I do not know the proper notation of java doc to make reference to java classes and methods, so you will need to google it.

Now about the tests, please remove those comments over method names. I believe they are not needed as the test method name is self-explained.

About the “docs.js”, please improve the sentence you wrote.
Something like this would be better: 'Enter a unique volume name; if a name is not provided, a random name will be created'

Now, I draw your attention again to the idea of sticking to the same PR were we started reviewing and chatting. If another reviewer comes to help with this new PR, she/he would not know about the blank case I told you in the other PR. The reviewer might even give a LGTM without even noticing that this PR is incomplete.

Having said that, could you add a test case to check the case in which users enter blank strings in the volume name? And of course, fix that in the code; otherwise, the test methods would not work.

[nitin-maharana]

@rafaelweingartner : Thanks for reviewing the change.

The reason to close the old PR is because it was merging with branch 4.6.
I started this PR a long days back when everything from 4.6 being merged with master.
But now I dont know the same is happening or not.

I saw most of the recent PRs are created with merging request to master. So, I opened a new PR with master. But I have given a reference to my old PR. So that reviewers can see the conversation.

About the code change, if we will make "required=true", there will be a mismatch in logic of UI and API. In UI, the field is optional. I will modify the comments section. Thanks for suggesting.

[rafaelweingartner]

@nitin-maharana I did not notice that the other PR was to 4.6, I thought it was to master.
I would just ask you to use a Javadoc block over “getVolumeNameFromCommand” instead of a commenting block.

At line 487, it is good your code, but we can improve it. The code you wrote is the same as using “org.apache.commons.lang.StringUtils.isBlank(str)”; therefore, I believe you should use the method from “StringUtils” from Apache commons lang.

I also recommend you to remove the comments of lines: 362, 369, 376 and 383, those tests names are intuitive, so there is no need to add comment there.

[nitin-maharana]

@rafaelweingartner : I have updated the branch. Can you please review the change. Thanks.

[rafaelweingartner]

@nitin-maharana now almost everything is ok.
There is only one problem. the "org.apache.commons.lang.StringUtils.isBlank" also returns true if the value is null or empty. Therefore, you can remove the "userSpecifiedName == null || userSpecifiedName.isEmpty() " conditions.

[nitin-maharana]

@rafaelweingartner : Thats cool. I went through the detail of isBlank, but could not catch. Thank you.
I have updated.

[rafaelweingartner]

@nitin-maharana now the code is perfect.
I can give a LGTM now.

Sorry if I bothered you with a lot of changes.

[nitin-maharana]

Thanks a lot @rafaelweingartner , I learned many things.

[nitin-maharana]

cc @remibergsma @bhaisaab

[yadvr]

LGTM

[nitin-maharana]

@remibergsma : Shall I make this one against 4.7. I mean, should I make all pending PRs against 4.7?
Is it not going to be merged in master now?

[remibergsma]

@nitin-maharana Any bug fix should be against 4.7, any new feature against master. Bug fixes will be forward merged to master after it is merged to 4.7.

[asfbot]

Daan Hoogland on dev@cloudstack.apache.org replies:
Remi, If a fix can be made agains 4.6, let's do that. Merging forward is no
biggy and we'll be helping more people this way.

[nitin-maharana]

@remibergsma : My new PRs are failing in 4.7. With the same change it was successful in master. I think there is some issue with the branch.

[remibergsma]

@nitin-maharana See thread on dev-list: the errors are due to quota plugin failing since Jan 1st 2016. Once this is resolved, you can rebase.

@DaanHoogland While that is true, there'll be no more 4.6 release so why bother. There really is no reason not to upgrade to 4.7 when people are on 4.6. It's a 5min task and doesn't require new systemvm template.

[DaanHoogland]

@remibergsma There are internal political issues that people may face with certain upgrade, be they with or without reason.

[nitin-maharana]

Closing this PR as made a new PR #1319 (Against 4.7 which will be merged in master later).