TOMEE-2968: Postgres connection error when a password contains "}"

[rzo1]

What does this PR do?

• Adds unit tests to reproduce TOMEE-2968 (also affects 7.0.x and 7.1.x)
• Single opening curly braces are stripped away in property values as we treat them as suffix placeholders (even in the case, we did not have an opening curly brace). PR proposes a related fix.

References

• https://issues.apache.org/jira/browse/TOMEE-2968
• https://markmail.org/message/vxmm7t5sl33zr4wq

[rmannibucau]

Hi @rzo1 , why not just escaping the character (think it is with '$' IIRC)?

[rzo1]

Hi @rzo1 , why not just escaping the character (think it is with '$' IIRC)?

Thought about it - current code will simply replace the closing bracket (even if we escape it).

Escaping with $ is only done for the opening version, so atm it boils down to PREFIX="${" and SUFFIX="}" to support property substitutions like "${foo}---${bar}".

The question is, if we should replace closing / opening brackets by empty string in the first place, if the suffix/prefix environment is not complete / closed.

[rmannibucau]

if incomplete noop should be done

[rzo1]

if incomplete noop should be done

Agreed.

This "noop" behaviour is implemented in this PR.

Before: } was replaced regardless of complete or not.

[rmannibucau]

@rzo1 so maybe add a test mixing both with ${foo}}, $${{foo}}, ${${bar}}, ${$${bar}} and so on, this is what this PR can wrongly handle (ie both at the same time and nested case).

[rzo1]

@rzo1 so maybe add a test mixing both with ${foo}}, $${{foo}}, ${${bar}}, ${$${bar}} and so on, this is what this PR can wrongly handle (ie both at the same time and nested case).

Thanks - I added some more tests mixing different cases / types of placeholders including the escape mechanism such as $$ to skip placeholder substitution.

[rmannibucau]

@rzo1 👍 , if you fix the regression of the default interpolation (noValueFound test and nested cases) it is good for me.

[rzo1]

@rzo1 , if you fix the regression of the default interpolation (noValueFound test and nested cases) it is good for me.

To see, if I get you right:

In case, no key is defined, we should resolve to the name of the key rather than doing a noop, ie ${v} should resolve to v if no key for v is defined?

[rmannibucau]

@rzo1 yes, this enables for simple configuration to use some default as override key (quite useful for hosts for example) but also to nested keys more readably than with defaults - and was from the time defaults were not supported) so ${${foo}.bar} would evaluate foo.bar if foo is missing (else .bar). Side note: if it helps, fork commons-lang3 code, openejb-core shoudln't depend on it anyway and code is quite small - https://github.com/apache/openwebbeans-meecrowave/blob/master/meecrowave-core/src/main/java/org/apache/meecrowave/lang/Substitutor.java.

[rzo1]

@rzo1 yes, this enables for simple configuration to use some default as override key (quite useful for hosts for example) but also to nested keys more readably than with defaults - and was from the time defaults were not supported) so ${${foo}.bar} would evaluate foo.bar if foo is missing (else .bar). Side note: if it helps, fork commons-lang3 code, openejb-core shoudln't depend on it anyway and code is quite small - https://github.com/apache/openwebbeans-meecrowave/blob/master/meecrowave-core/src/main/java/org/apache/meecrowave/lang/Substitutor.java.

Regression should be fixed now - at least, if I understood it correctly.

openejb-core has some more dependencies towards lang3

• org.apache.commons.lang3.StringUtils
• org.apache.commons.lang3.text.StrLookup;
• org.apache.commons.lang3.text.StrSubstitutor; (alternative: Substitutor from text - might be worth replacing it as it is deprecated...)
• org.apache.commons.lang3.builder.ToStringBuilder
• org.apache.commons.lang3.ClassUtils
• org.apache.commons.lang3.tuple.ImmutablePair

[rmannibucau]

@rzo1 doesnt if(raw.startsWith(ESCAPE_SEQUENCE)) return decryptIfNeeded(raw, acceptCharArray); skips too much? what about $${foo} ${bar}?

[rzo1]

Hmm yes - you are right. Will fix it soon.

[rzo1]

I think, we need to discuss, which one is the expected behaviour for $${foo}:

1. $${foo} -> $${foo} (noop)
2. $${foo} ->${foo} (escaped sequence, no replacement of placeholder (as mentioned in the docs of StrSubstitutor))
3. $${foo} -> foo

I guess, it is probably 2. or 3. - WDYT?

[cesarhernandezgt]

I think it will be 2. $${foo} ->${foo}
I'm catching up with this ticket.

As a side note, Jenkins Job was green, then a new build hanged the job so I cancel and after your upcoming push, I hope the status on the PR gets in sync.

[rzo1]

I think, that I have fixed the regression introduced while working on the escaped sequences.

The code should now reduce escaped placeholders according to 2.

Let's see, what the Jenkins Job will produce :)

[rzo1]

@cesarhernandezgt Can we manually re-trigger the Jenkins build or does it a scheduled build. I think the failing tests are unrelated.

[cesarhernandezgt]

@rzo1 I retrigger the build here: https://ci-builds.apache.org/job/Tomee/job/pull-request/81/

[rzo1]

@cesarhernandezgt Thanks - the build looks good now 🙃

edit: let's wait a few more days to see if we get additional feedback otherwise I will integrate it as is.