groovy
  1. groovy
  2. GROOVY-3248

String.replaceAll(String, Closure) should quote the replacement string

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.7, 1.6-rc-1
    • Fix Version/s: 1.6-rc-2, 1.5.8
    • Component/s: None
    • Labels:
      None
    • Number of attachments :
      0

      Description

      String.replaceAll(String, Closure) is passing the stringified closure result directly to Matcher.appendReplacement.

      The consequence is that the resulting string is interpreted as a "replacement string" which treats '$' and '\' specially so that group replacements can be done. For my purposes that definitely undesireable.

      The whole reason I see for the closure API is so that those substitutions are done in the closure and that the closure expects it's result string to be substituted literally.

      An example of the problem is like this:

      groovy> println 'x123z45'.replaceAll(/([^z]*)(z)/, { all, m, d ->  m })
      x12345
      
      groovy> println '$123z45'.replaceAll(/([^z]*)(z)/, { all, m, d ->  m })
      
      $1232345
      

      The fix is that quoteReplacement needs to be called on the result before appendReplacement is called. This is similar to the problem we had with minus.

      http://jira.codehaus.org/browse/GROOVY-1637

      Hmmm, there they avoided the problem that quoteReplacement is JDK 1.5 by not using replaceAll. Looks like we need to implement RegexUtils.quoteReplacement after all.

        Activity

        Hide
        Jim White added a comment -

        Fix committed to trunk cs14758.

        Show
        Jim White added a comment - Fix committed to trunk cs14758.
        Hide
        Jim White added a comment -

        Fix includes another subtle problem. The code was using String.valueOf on the result rather than InvokerHelper.toString. That gets into another whole can of worms, but considering it was the only such usage in DGM it is an obvious fix to make it like the rest of the code.

        Commited to trunk cs14762.

        Show
        Jim White added a comment - Fix includes another subtle problem. The code was using String.valueOf on the result rather than InvokerHelper.toString. That gets into another whole can of worms, but considering it was the only such usage in DGM it is an obvious fix to make it like the rest of the code. Commited to trunk cs14762.
        Hide
        Jim White added a comment -

        Fixed in 1.5 branch cs14763.

        Noticed that there are a lot more RegularExpressionsTests in trunk. They should probably be merged/converted to 1.5.

        Show
        Jim White added a comment - Fixed in 1.5 branch cs14763. Noticed that there are a lot more RegularExpressionsTests in trunk. They should probably be merged/converted to 1.5.
        Hide
        Jim White added a comment -

        Really fixed 1.5 now (but not the merge of the additional tests beyond the new one for replaceAll(String, Closure)).

        Also merged to 1.6.

        Show
        Jim White added a comment - Really fixed 1.5 now (but not the merge of the additional tests beyond the new one for replaceAll(String, Closure)). Also merged to 1.6.

          People

          • Assignee:
            Jim White
            Reporter:
            Jim White
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: