[rfc][icedtea-web] fix itweb-settings set command to allow duplicate strings

[Jie Kang]

----- Original Message -----
> Hello,
> 
> 
> > > Hello,
> > > 
> > > > > Hello,
> > > > > 
> > > > > This patch fixes the issue where the itweb-settings Set would not
> > > > > allow
> > > > > the
> > > > > key and value to be the same value.
> > > > > Such as set blah blah. This was due to the isKey() method using
> > > > > .indexOf()
> > > > > which would always return the index of the first "blah" registering
> > > > > two
> > > > > keys
> > > > > with no values. The fix is to just alternate between the two parts of
> > > > > the
> > > > > if
> > > > > statement. It does this by using null for key and checking when it is
> > > > > null,
> > > > > to assign a key. When a value is assigned and written then the key is
> > > > > assigned back to null.
> > > > 
> > > > Hello,
> > > > 
> > > > Thanks for the fix :)
> > > > 
> > > > Nits below:
> > > > 
> > > > -            if (isKey(arg)) {
> > > > +            if (key == null) {
> > > > 
> > > > I think you should still make sure that the argument is actually a key
> > > > before
> > > > assigning it. So something like:
> > > > 
> > > > if (key == null && isKey(arg)) { [...] }
> > > 
> > > Hmm, I don't agree with this.
> > > 
> > > Seems redundant to me and I think it would introduce a new bug at least
> > > using
> > > the current isKey() implementation.
> > > 
> > > The current isKey would look at the args index in order to find out
> > > whether
> > > it was even or odd in the order to help
> > > decide whether it was a key or value. The current for loop has this order
> > > from args and so when it runs all we need to do
> > > is alternate the first part of the if and the else part which the current
> > > patch I sent does with the null.
> > > 
> > > The bug that I believe would result in the case for example:
> > > 
> > > set blah blue blue red
> > > 
> > > 1. blah: key is null and index is 0 so enters Key Assigning Part
> > > 2. blue: key is blah and index is 1 of blue so enters Value Assigning
> > > Part
> > > 3. blue: key is null and index found is 1 of blue so enters Value
> > > Assigning
> > > Part with key null and value blue
> > > 
> > > Now I suppose I can rewrite isKey to keep track of the last index which I
> > > considered in the first place,
> > > but since the for loop goes over the terms in the order we want (as isKey
> > > and
> > > args get the params in the same way)
> > > All that is needed is to alternate between the first if and the else
> > > part.
> > > For me it seems redundant,
> > > but perhaps I am looking at this wrong.
> > 
> > 
> > Okay. I didn't really look at what isKey() does. I just assumed that only
> > certain inputs were accepted as 'keys' and isKey() would check that. But it
> > looks like you can put anything as a key so that suggestion doesn't make
> > sense.
> > 
> > 
> > 
> > Patch looks fine to me. My one concern that you can optionally address is:
> > It's not very obvious the way it's coded now that you're iterating over the
> > args in key value pairs. Can you maybe restructure it or add comments to
> > make it more clear?
> 
> Yeah, I have rewritten it with a boolean, hopefully this is much clearer.
> 
> > Also, what happens on input like:
> > 
> > 'set a b c'
> > 
> > Does 'c' get ignored?
> 
> The OptionParser throws an UnevenParameterException (Added test)
> 
> > How about 'set a b\ c'? Ie. can users add spaces to their key/value? I
> > think
> > these would be some good test cases.
> 
> Hmm, I am having issues writing a test in this case, since adding "\ " is an
> illegal escape
> character in java it seems, and adding a space in the String[] of args, I
> can't seem to simulate it

I think you can do a test with:

+        String[] args = {
+                "set", "key", "value1 value2"

to simulate. 

Rest of the patch seems good to me :)


Regards,

> "a", "b ", "c" throws UnevenParameterException and "a", "b", " c", "d"
> results in "a=b" and " c=d" in
> the deployment properties files.
> 
> From testing of itweb-settings it becomes 'a=b c'.
> 
> I have attached a patch that adds unit tests for the set command in
> CommandLine.
> 
> Thank you,
> Lukasz Dracz
> 
> 
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern