This commit allows to escape property placeholders to later processed by user. PropertyPlaceholderHelper allows to set custom escape character. "\" is used as the default one.

Closes gh-9628

Comment From: snicoll

Thanks for the PR. I've played with the code a bit and already polished quite a bit so I'll probably cherry-pick if you have time to update it. That doesn't work for default value having a property placeholder in it, something like:

${my.prop:\\${my.fallback}}

if my.prop isn't available, it's trying to resolve my.fallback rather than using ${my.fallback} as default value. Would you have time to add more tests and fix this? Please do so in a separate commit as I'll integrate that in what I've already polished.

Comment From: bdshadow

@snicoll yes, i will fix it thank you for the review

Comment From: snicoll

One thing I had in mind would be to have a data structure that represents a bit of text and some extra information. You could then handle those cases:

  • It's just text and you output as is
  • It's a placeholder and it needs to be resolved

Perhaps with a method that needs to "emit" the value and does the replacement by calling a function. I haven't really looked at the problem in details but it looks like we may remove the escape character "too soon" and we end up in another branch that is not aware that it should be escaped. Having a data structure may also simplify a bit of the code.

Food for thoughts, and thanks for looking at it!

Comment From: bdshadow

Tried for some time to solve it. The code really becomes ugly. The problem with the existing mr is indeed that the escape character is removed "too soon". While trying to fix it, I've come up with some more tests, which I'm not sure how they should work:

p1=abc
p2=\${undefined:${p1}}

p3=\${p1}
p4=\${undefined:${p3}}

p5=abc\${

What should p2, p4 and p5 be resolved to? Should p2 become ${undefined:abc}? Do we want to resolve nested placeholders inside an escaped placeholder? If yes, then p4 must become ${undefined:${p1}}? A separate case is p5. Should it become "abc${"? Or should it be treated as not a placeholder and stay the same"abc\${"? Probably these are questions to ask in the initial ticket

Comment From: snicoll

It's fine we can continue here as we're trying to figure out the best way to move forward with the code. Thanks for the follow-up and I am not surprised things get messy with corner cases.

To answer your questions:

  • p2=\${undefined:${p1}} should be ${undefined:${p1}}as whatever that's inside the prefix/suffix that's escaped should be rendered as is.
  • \${undefined:${p3}} same ${undefined:${p3}}
  • abc\${ is a good question. It should be rendered as abc\${ I guess since it's not a valid expression.

I don't know if the above helps or makes it even worse. Let me know :)

Comment From: bdshadow

Finally, i've updated it :) Actually, i added 2 commits. The first one just fixes the problem we discussed above. In the second one, i tried to introduce a data structure, which you mentioned. I'm not sure if it really helps, but at least it lowers the complexity of the code, which sonar was complaining about. I tried to add 2 classes Property and Placeholder, but in my opinion, it led to more complexity as Placeholder can become a Property and there were unnecessary conversions back and forward all the time. I also thought about some static initializers like `Property.of(...), maybe it can simplify reading the code too. Looking forward to your opinion and am ready to polish it further.

Comment From: snicoll

@bdshadow I want to acknowledge that I've seen this. Getting the espacing to work is quite involved as we both feared. If nobody beats me to it, I'll need some quality time to review and provide more feedback if needed. That won't happen before we start 6.2 in anger in a month or so.

Comment From: bdshadow

Thank you @snicoll. Totally understand. Looking forward to it

Comment From: snicoll

It turns out quite tricky and, while I don't have a definitive implementation for this, I did rewrite the parser from scratch as I felt this was the only way to address this issue and other issues reported against it. Thanks for the PR and your efforts.