Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SHIRO-812] Key value separator in config is broken with escape char #286

Merged
merged 1 commit into from
Apr 14, 2021

Conversation

fpapon
Copy link
Member

@fpapon fpapon commented Mar 13, 2021

No description provided.

@fpapon fpapon requested review from bmarwell and bdemers March 13, 2021 07:38
@fpapon
Copy link
Member Author

fpapon commented Mar 13, 2021

Fix this 0c424d3#commitcomment-47294324

@fpapon
Copy link
Member Author

fpapon commented Mar 14, 2021

I have one test failed but do we want this to be possible?

    @Test
    public void testSplitKeyValueEscapedEquals()  {
        String test = "Truth\\=Beauty";
        String[] kv = Ini.Section.splitKeyValue(test);
        assertEquals("Truth", kv[0]);
        assertEquals("Beauty", kv[1]);
    }

@bmarwell
Copy link
Contributor

bmarwell commented Mar 14, 2021

I have one test failed but do we want this to be possible?

    @Test
    public void testSplitKeyValueEscapedEquals()  {
        String test = "Truth\\=Beauty";
        String[] kv = Ini.Section.splitKeyValue(test);
        assertEquals("Truth", kv[0]);
        assertEquals("Beauty", kv[1]);
    }

For 2.0: no, we do not want to retain such irrational behaviour. For me, the equals sign is escaped.

@charliemblack
Copy link

I tested out the change with an LDAP integration I have with Apache Geode. The fix works like a champ! Thanks for doing this.

If I could request this to be back-ported to 1.7.x since that is what Apache Geode is on.

@bmarwell
Copy link
Contributor

bmarwell commented Apr 9, 2021

If I could request this to be back-ported to 1.7.x since that is what Apache Geode is on.

I am not sure we can backport this, as this is a breaking change. See #286 (comment).
OTOH, I cannot imagine someone has ever used a kv-pair like this intentionally.

@fpapon WDYT?

@bdemers
Copy link
Member

bdemers commented Apr 9, 2021

IIRC, @bmarwell added some tests with the current behavior in: b50b829

    public void testSplitKeyValueEscapedEquals()  {
        String test = "Truth\\=Beauty";
        String[] kv = Ini.Section.splitKeyValue(test);
        assertEquals("Truth", kv[0]);
        assertEquals("Beauty", kv[1]);
    }

The above was existing behavior. Though the more I look at it this must have been a side effect/bug. I cannot imagine why you would want this behavior, I would have expected Truth\\=Beauty to be evaluated to Truth\=Beauty with a null value, or possibly a runtime parsing error if the value is required.

I'm guessing this might have been the result of the key and value logic sharing the same escape logic.

TL;DR, I think this is a bug, and we should fix it in any version. (We do need to make this clear in the release notes, anyone caught by this should be able to process the INI file/string to strip out the odd \\= if needed.

Thoughts?

@bmarwell
Copy link
Contributor

bmarwell commented Apr 9, 2021

+1. Cherry pick into 1.7.x, remove the test. This behaviour can be restored easily by removing one backslash and does not look intentional.

@fpapon
Copy link
Member Author

fpapon commented Apr 9, 2021

Ok I will cherrypick to 1.7.x branch and add info in the release note.
Thanks for your feedback!

@fpapon fpapon merged commit d1d6f3e into apache:master Apr 14, 2021
@fpapon fpapon deleted the SHIRO-812 branch April 14, 2021 16:25
@todmorrison
Copy link

todmorrison commented Apr 14, 2021

This is a cleaner way and should fix up the "escaped escape-char" case (i.e. handle "//" as a single "/")

if (buildingKey) {
  // Given that isCharEscaped actually means isEscapeChar, the middle case is always true.
  // if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i) && !isCharEscaped(line, i-1)) {
     if (isKeyValueSeparatorChar(c) &&  !isCharEscaped(line, i-1)) {
         buildingKey = false;//now start building the value
     } else if (!isCharEscaped(line, i) || !isCharEscaped(line, i-1) ){    // Allow escaped escape character
          keyBuffer.append(c);
 }

@fpapon
Copy link
Member Author

fpapon commented Apr 16, 2021

@todmorrison thanks for your feedback, I will take a look.

@fpapon
Copy link
Member Author

fpapon commented Apr 20, 2021

@todmorrison I'm not sure to understand your code because our test failed. Did you take a look on our utests?

@todmorrison
Copy link

Hi @fpapon,

You're right. I partially amend my comment. The first part, changing:

if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i) && !isCharEscaped(line, i-1))

to

if (isKeyValueSeparatorChar(c) && !isCharEscaped(line, i-1))

is correct. The middle case is redundant since isKeyValueSeparatorChar(c) where c = line.charAt(i) means the character at i is "=" and certainly not "".

For the second part, (!isCharEscaped(line, i) || !isCharEscaped(line, i-1) , this fails the test for "Tru\th=Beauty" = "Truth", which still doesn't look right to me but my "fix" doesn't solve the problem either. Specifically, I feel that backslashes should be allowed in the key (and possibly other escape sequences), but this is harder than I originally thought. (A project for another day).

Anyway, I do feel my observation that the middle case in the first conditional is redundant is worth fixing. I withdraw the second change though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants