Clojure

clojure.string/replace behaves unexpectedly when \ or $ are part of the result string

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: Release 1.2, Release 1.3
  • Fix Version/s: Release 1.5
  • Component/s: None
  • Labels:
    None
  • Environment:
    HW/OS/SW indipendant - issue is part of java interface
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

clojure.string/replace uses javas replace function to do it's work, the replace function has the tricky habit of 'double evaluating' the replacement string (third argument). This means that every \ in the string (so i.e. "
") is evaluated by the java code behind replace.

Since this behavior isn't documented it can lead to confusing errors, for example (made up semi real world example to explain the issue):

(clojure.string/replace "c:/windows/" #"/" "\\")

This should replace all unix folder separators with windows separators, this crashes with a index out of bound exemption since java tries to evaluate "
" as a semi regexp.

or

(println (str "\"" (clojure.string/replace "my string with \\ and \" in it" #"[\"\\]" (fn [x] (str "\\" x))) "\""))

The expected result would be:

"my string with \\ and \" in it"

the actual result is:

"my string with \ and " in it"

This should return an 'escaped' string, it does not since the 'double evaluation' of the return string results in \\\\ being reduced to
again.

Attachments

  1. Text File
    0001-Add-tests-for-870.patch
    05/Jan/12 8:53 PM
    0.9 kB
    Alan Malloy
  2. Text File
    0002-Fix-870-by-using-quoteReplacement.patch
    05/Jan/12 8:53 PM
    1 kB
    Alan Malloy
  3. Text File
    CLJ-753-870-905-combined-fix3.patch
    28/Feb/12 12:38 PM
    9 kB
    Andy Fingerhut
  4. Text File
    CLJ-753-870-905-combined-fix3.readme.txt
    28/Feb/12 12:38 PM
    3 kB
    Andy Fingerhut
  5. Text File
    clj-753-870-905-combined-fix4.patch
    15/Aug/12 11:19 AM
    9 kB
    Andy Fingerhut

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Heinz N. Gies added a comment -

A working code for the replace example is: 

(println (str "\"" (clojure.string/replace "my string with \\ and \" in it" #"[\"\\]" (fn [x] (str  "\\\\"  (if (= x "\\") "\\") x))) "\""))

which is horribly ugly since you need to hand escape the backslash or it crashes.

Show
Heinz N. Gies added a comment - A working code for the replace example is: 
(println (str "\"" (clojure.string/replace "my string with \\ and \" in it" #"[\"\\]" (fn [x] (str  "\\\\"  (if (= x "\\") "\\") x))) "\""))
which is horribly ugly since you need to hand escape the backslash or it crashes.
Hide
Permalink
Stuart Sierra added a comment -

Vetted on 1.4.0 master. Needs patch.

Show
Stuart Sierra added a comment - Vetted on 1.4.0 master. Needs patch.
Stuart Sierra made changes -
Field Original Value New Value
Patch Code and Test [ 10002 ]
Approval Vetted [ 10003 ]
Affects Version/s Release 1.3 [ 10038 ]
Fix Version/s Backlog [ 10035 ]
Hide
Permalink
Alan Malloy added a comment - - edited

No hand-escaping is necessary, just use #(java.util.regex.Matcher/quoteReplacement %).

Edit: I see, we're talking about when you use a function as a replacement, not a replacement string like "
x" - the function's output should not be interpreted as regex replacement, I agree. Looks like it just needs a small patch in clojure.string/replace-by.

Show
Alan Malloy added a comment - - edited No hand-escaping is necessary, just use #(java.util.regex.Matcher/quoteReplacement %). Edit: I see, we're talking about when you use a function as a replacement, not a replacement string like "
x" - the function's output should not be interpreted as regex replacement, I agree. Looks like it just needs a small patch in clojure.string/replace-by.
Hide
Permalink
Alan Malloy added a comment -

Patch and test for this issue.

Show
Alan Malloy added a comment - Patch and test for this issue.
Alan Malloy made changes -
Attachment 0002-Fix-870-by-using-quoteReplacement.patch [ 10761 ]
Attachment 0001-Add-tests-for-870.patch [ 10760 ]
Hide
Permalink
Andy Fingerhut added a comment -

CLJ-870-also-fix-replace-first.patch combines Alan Malloy's two patches, and adds the same change for replace-first. I like this change, too, and think replace and replace-first should be consistent with each other in this behavior.

Show
Andy Fingerhut added a comment - CLJ-870-also-fix-replace-first.patch combines Alan Malloy's two patches, and adds the same change for replace-first. I like this change, too, and think replace and replace-first should be consistent with each other in this behavior.
Andy Fingerhut made changes -
Attachment CLJ-870-also-fix-replace-first.patch [ 10762 ]
Hide
Permalink
Andy Fingerhut added a comment -

If there is any interest in a combined patch for this issue, CLJ-753 and CLJ-905 all in one, I'd be happy to merge them.

Show
Andy Fingerhut added a comment - If there is any interest in a combined patch for this issue, CLJ-753 and CLJ-905 all in one, I'd be happy to merge them.
Hide
Permalink
Andy Fingerhut added a comment -

Proposed combined patch for CLJ-753, CLJ-870, and CLJ-905, since they all affect the behavior of clojure.string/replace and replace-first.

Show
Andy Fingerhut added a comment - Proposed combined patch for CLJ-753, CLJ-870, and CLJ-905, since they all affect the behavior of clojure.string/replace and replace-first.
Andy Fingerhut made changes -
Attachment CLJ-753-870-905-combined-fix.patch [ 10768 ]
Stuart Sierra made changes -
Assignee Stuart Sierra [ stuart.sierra ]
Hide
Permalink
Stuart Sierra added a comment -

The 1/12/2012 combined patch is not in correct format. Please recreate, see http://clojure.org/patches for correct format.

Show
Stuart Sierra added a comment - The 1/12/2012 combined patch is not in correct format. Please recreate, see http://clojure.org/patches for correct format.
Hide
Permalink
Andy Fingerhut added a comment -

CLJ-753-870-905-combined-fix2.patch should have proper patch format.

Show
Andy Fingerhut added a comment - CLJ-753-870-905-combined-fix2.patch should have proper patch format.
Andy Fingerhut made changes -
Attachment CLJ-753-870-905-combined-fix2.patch [ 10801 ]
Andy Fingerhut made changes -
Patch Code and Test [ 10002 ]
Andy Fingerhut made changes -
Patch Code and Test [ 10002 ]
Andy Fingerhut made changes -
Patch Code and Test [ 10002 ]
Hide
Permalink
Andy Fingerhut added a comment - - edited

Attaching slightly improved patch CLJ-753-870-905-combined-fix3.patch, along with a README to explain in gory detail the behavior and performance changes to clojure.string/replace and clojure.string/replace-first, in case it would help anyone review the changes. Deleting my older patches.

Show
Andy Fingerhut added a comment - - edited Attaching slightly improved patch CLJ-753-870-905-combined-fix3.patch, along with a README to explain in gory detail the behavior and performance changes to clojure.string/replace and clojure.string/replace-first, in case it would help anyone review the changes. Deleting my older patches.
Andy Fingerhut made changes -
Attachment CLJ-753-870-905-combined-fix3.readme.txt [ 10972 ]
Attachment CLJ-753-870-905-combined-fix3.patch [ 10971 ]
Andy Fingerhut made changes -
Attachment CLJ-753-870-905-combined-fix2.patch [ 10801 ]
Andy Fingerhut made changes -
Attachment CLJ-753-870-905-combined-fix.patch [ 10768 ]
Andy Fingerhut made changes -
Attachment CLJ-870-also-fix-replace-first.patch [ 10762 ]
Rich Hickey made changes -
Fix Version/s Backlog [ 10035 ]
Fix Version/s Release 1.5 [ 10150 ]
Hide
Permalink
Aaron Bedra added a comment -

This is a small nit pick, but re-qr is a pretty non-descriptive name. Perhaps we could change it to re-quote-replacement?

Show
Aaron Bedra added a comment - This is a small nit pick, but re-qr is a pretty non-descriptive name. Perhaps we could change it to re-quote-replacement?
Aaron Bedra made changes -
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Hide
Permalink
Andy Fingerhut added a comment -

Patches clj-753-870-905-combined-fix4.patch dated Aug 15 2012 and the patch that Aaron reviewed, CLJ-753-870-905-combined-fix3.patch dated Feb 28 2012, are almost identical.

fix3 uses the name re-qr for a function, where fix4 uses the name re-quote-replacement. I have no strong preference for either of them.

Show
Andy Fingerhut added a comment - Patches clj-753-870-905-combined-fix4.patch dated Aug 15 2012 and the patch that Aaron reviewed, CLJ-753-870-905-combined-fix3.patch dated Feb 28 2012, are almost identical. fix3 uses the name re-qr for a function, where fix4 uses the name re-quote-replacement. I have no strong preference for either of them.
Andy Fingerhut made changes -
Attachment clj-753-870-905-combined-fix4.patch [ 11434 ]
Hide
Permalink
Andy Fingerhut added a comment -

Added patch addressing what I think is Aaron's reason for changing state to Incomplete, so as a result I am changing state back to its former Vetted state. If there is something else a non-screener should be doing to bring Incomplete tickets to the attention of screeners, please let me know, and I will document it here: http://dev.clojure.org/display/design/JIRA+workflow

Show
Andy Fingerhut added a comment - Added patch addressing what I think is Aaron's reason for changing state to Incomplete, so as a result I am changing state back to its former Vetted state. If there is something else a non-screener should be doing to bring Incomplete tickets to the attention of screeners, please let me know, and I will document it here: http://dev.clojure.org/display/design/JIRA+workflow
Andy Fingerhut made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]
Fogus made changes -
Assignee Stuart Sierra [ stuart.sierra ] Fogus [ fogus ]
Fogus made changes -
Assignee Fogus [ fogus ] Stuart Sierra [ stuart.sierra ]
Hide
Permalink
Stuart Sierra added a comment -

I will be looking at this.

Show
Stuart Sierra added a comment - I will be looking at this.
Hide
Permalink
Stuart Sierra added a comment -

Screened. This is an adequate fix for this particular problem, given the current API.

However, the long and complicated docstring proves that replace and replace-first should each be four separate functions:

code
replace-char [input char char]
replace-string [input string string]
replace-re [input pattern string]
replace-re-by [input pattern fn]
code

This is how these functions were originally written back in the days of clojure.contrib.str-utils. This would be a minor breaking change.

Show
Stuart Sierra added a comment - Screened. This is an adequate fix for this particular problem, given the current API. However, the long and complicated docstring proves that replace and replace-first should each be four separate functions: code replace-char [input char char] replace-string [input string string] replace-re [input pattern string] replace-re-by [input pattern fn] code This is how these functions were originally written back in the days of clojure.contrib.str-utils. This would be a minor breaking change.
Stuart Sierra made changes -
Waiting On richhickey
Approval Vetted [ 10003 ] Screened [ 10004 ]
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Closed [ 6 ]

People

Vote (2)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.