[SHIRO-530] INI parser does not properly handled backslashes at end o…

[bmarwell]

…f values

• Replace key-value-splitting with regex
• obscure disappearing escaping chars - please discuss!

Signed-off-by: Benjamin Marwell bmarwell@gmail.com

Now. If we would not want to break the existing (imho: obscure) way that shiro parses the shiro.ini file, we could introduce a parser strategy pattern.

But as the existing behaviour was not documented (other than tests), I would like to see a discussion about this.

Tests broken on purpose! See here why: SHIRO-530

[bmarwell]

TODO:

• document capture groups
• remove \\ from first capture group. It just makes no sense. Disappearing is unexpected. Or just let it disappear from the key only?
• to make it more similar to the existing behaviour, we could just strip backsashes from the key. final String key = matcher.group(1).replaceAll("\\\\", "");.
• Please explain this test (especially it's purpose). It makes no sense imho. This would be the only test failing then.

        test = "Truth\\=Beauty";
        kv = Ini.Section.splitKeyValue(test);
        assertEquals("Truth", kv[0]);
        assertEquals("Beauty", kv[1]);

[fpapon]

We have some users that are using backslash in the key:
https://issues.apache.org/jira/browse/SHIRO-684

[bmarwell]

?? Why introduce a new future? If you look at the tests, this was never supported and those always got removed.

[bdemers]

From SHIRO-530, it looks like we have:

example (as input)	result (as java string)
key=value\	key=value\n
key=value\\	key=value\\
key=value\\\	key=value\\
key=value\\\\	key=value\\\\

I would expect the result to be:

example (as input)	result (as java string)
key=value\	key=value\n
key=value\\	key=value\\
key=value\\\	key=value\\\n
key=value\\\\	key=value\\\\

As for "Truth\\=Beauty", I would expect that that to be some sort of error or at least a key with no value similar to:

[foobar]
one=one
justAKeyWithoutValue

I'm not saying this is the way it does function, or should just what I personally would expect. (not sure if that helps or hurts this conversation)

[bmarwell]

As for "Truth\\=Beauty", I would expect that that to be some sort of error or at least a key with no value similar to:

Thanks for the input @bdemers, I had the very same thoughts! The new force-pushed commit:

• Will throw an IllegalArgumentException for Key\\=Value.
• Will (as before!) remove all backslashes from the key.
• Will not (new behaviour) remove any backslashes from the value. The continuation backslash not included.

Actually, what @fpapon pointed out by posting the link: The XML config DOES allow backslashes in the key, where the Ini never allowed to do so. Please review my new commit which reflects these changes. I moved the tests a bit and commented them, so we can see what would change.
Please keep in mind: Neither the current nor the desired/expected behaviour is currently documented, not even in the code! It is just the tests!

[fpapon]

+1 for me, if we think it's not possible (ini specs) to have \ in the key, just throw an exception.
@bmhm Thanks for taking care of this ;)

[bmarwell]

I am not so sure about this commit. It would be possible, in theory, that someone misused the escaping backslashes (which disappeared from the value) as indendation:

key    =    value\
          \\1

In the old word, this was "key" => "value1", but in the new one it would be "value\\1".

But then, it was never documented.

[bmarwell]

Superseeded by #210