[GitHub] [tomee] rzo1 opened a new pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

classic Classic list List threaded Threaded
33 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 opened a new pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox

rzo1 opened a new pull request #763:
URL: https://github.com/apache/tomee/pull/763


   # 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
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rmannibucau commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox

rmannibucau commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784051271


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rzo1 commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784061648


   > 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.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rmannibucau commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rmannibucau commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784130369


   if incomplete noop should be done


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rzo1 commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784132571


   > if incomplete noop should be done
   
   Agreed.
   
   This behaviour is implemented in this PR.
   
   Before: } was replaced regardless of complete or not.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 edited a comment on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rzo1 edited a comment on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784132571


   > if incomplete noop should be done
   
   Agreed.
   
   This "noop" behaviour is implemented in this PR.
   
   Before: } was replaced regardless of complete or not.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rmannibucau commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rmannibucau commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784144567


   @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).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rzo1 commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784232901


   > @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.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rmannibucau commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rmannibucau commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784237920


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rzo1 commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784239645


   > @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?
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rmannibucau commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rmannibucau commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784241505


   @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 <foo value>.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.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rzo1 commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784267147


   > @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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rmannibucau commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rmannibucau commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784274948


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rzo1 commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784277046


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rzo1 commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784364672


   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`
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 edited a comment on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rzo1 edited a comment on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784364672


   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`
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 edited a comment on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rzo1 edited a comment on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784364672






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 edited a comment on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rzo1 edited a comment on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784364672


   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.**


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 edited a comment on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

rzo1 edited a comment on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784364672


   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?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] cesarhernandezgt commented on pull request #763: TOMEE-2968: Postgres connection error when a password contains "}"

GitBox
In reply to this post by GitBox

cesarhernandezgt commented on pull request #763:
URL: https://github.com/apache/tomee/pull/763#issuecomment-784381840


   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.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


12