Can we not test instead for an empty array and return the default value? This way we properly reuse the getTrimmedStrings function thats already present, instead of directly calling out the StringUtils one.

Harsh J
added a comment - 24/Nov/11 16:46 Can we not test instead for an empty array and return the default value? This way we properly reuse the getTrimmedStrings function thats already present, instead of directly calling out the StringUtils one.

Can we not test instead for an empty array and return the default value? This way we properly reuse the getTrimmedStrings function thats already present, instead of directly calling out the StringUtils one.

This also should work.

This problem exists in 0.20-security as well. Would love it if you can post a patch for that as well?

In 20Security205, getStrings itself will return null if collection is empty.

Uma Maheswara Rao G
added a comment - 24/Nov/11 18:02
Can we not test instead for an empty array and return the default value? This way we properly reuse the getTrimmedStrings function thats already present, instead of directly calling out the StringUtils one.
This also should work.
This problem exists in 0.20-security as well. Would love it if you can post a patch for that as well?
In 20Security205, getStrings itself will return null if collection is empty.
Collection<String> values = getStringCollection(str);
if(values.size() == 0)
{
return null;
}
I dont see problem here. Am i missing some thing here?

Not setting a value for a configuration parameter is different from setting it to "". Hence when the parameter is not set, the default value should be returned. When the parameter is set (including "") then the specified value should be returned and not the default. Hence a simple check before invoking getTrimmedStrings() should suffice.

Amar Kamat
added a comment - 25/Nov/11 04:35 Not setting a value for a configuration parameter is different from setting it to "". Hence when the parameter is not set, the default value should be returned. When the parameter is set (including "") then the specified value should be returned and not the default. Hence a simple check before invoking getTrimmedStrings() should suffice.

Uma Maheswara Rao G
added a comment - 25/Nov/11 04:46 Yes Amar,
My first pacth should adderss your comment. But i used directly StringUtils api directly. So, that i could avaoid two times get invokation.
But coming to harsh comment, i thought to raise your point. But i could'nt find any usecase for setting "" for classnames. Do we have some use cases? If so, we can prefer 1st patch itself.
Thanks
Uma

get call will be executed multiple times if i use getTrimmedStrings(name).
Because we should call get once for validating null and then call getTrimmedStrings. This api again will call get and passed to StringUtils.getTrimmedStrings. That is the reason i used directly StringUtils.getTrimmedStrings. do you agree with this?

i will update the patch with other comments. Can you please confirm above one.

Uma Maheswara Rao G
added a comment - 08/Dec/11 16:24 Hi Amar, Thanks for the review.
1. Instead of
String[] classnames = StringUtils.getTrimmedStrings(valueString);
use
String[] classnames = getTrimmedStrings(name);
get call will be executed multiple times if i use getTrimmedStrings(name).
Because we should call get once for validating null and then call getTrimmedStrings. This api again will call get and passed to StringUtils.getTrimmedStrings. That is the reason i used directly StringUtils.getTrimmedStrings. do you agree with this?
i will update the patch with other comments. Can you please confirm above one.
Regards,
Uma

This is due to formatter. I am usiing hadoop formatter only. Manually i changed it.

3. Instead of assertTrue, you can use assertEquals()
Yes, assertEquals will give the values in trace when it is failing. But i included the value in error message.Since they are premitives i used assertTrue.
Now i changed it to assertEquals.

4. Kindly add a testcase for a value of "". The output should be an empty array.
Added test for "".

Uma Maheswara Rao G
added a comment - 08/Dec/11 18:03 Thanks Amar for taking a look again.
Ok, it is better than get. its little lighter
Updated the patch by addressing the comments.
kindly use
Class<?>[] classes =
config.getClasses("testClassName", Configuration.class);
This is due to formatter. I am usiing hadoop formatter only. Manually i changed it.
3. Instead of assertTrue, you can use assertEquals()
Yes, assertEquals will give the values in trace when it is failing. But i included the value in error message.Since they are premitives i used assertTrue.
Now i changed it to assertEquals.
4. Kindly add a testcase for a value of "". The output should be an empty array.
Added test for "".
Amar, can you please take a look?
Thanks
Uma