Skip to content
This repository has been archived by the owner on Feb 7, 2019. It is now read-only.

Fix Windows file separator bug #14

Conversation

catalin-ursachi
Copy link

The following bit of code:

protected def traceFileName(dn:DN, opType:String) : String = {
    val fileName = dn.getRDNStrings().map( _.replaceAll(File.separator, "|")).reverse.mkString("/")
    fileName + "-" + System.currentTimeMillis.toString + "-" + opType + ".ldif"
  }

Appears to result in an error on Windows. Specifically, the problem is the following:

scala> "c\\programs\\ldap".replaceAll("\\", "x")
java.util.regex.PatternSyntaxException: Unexpected internal error near index 1
\
 ^
  at java.util.regex.Pattern.error(Pattern.java:1924)
  at java.util.regex.Pattern.compile(Pattern.java:1671)
  at java.util.regex.Pattern.<init>(Pattern.java:1337)
  at java.util.regex.Pattern.compile(Pattern.java:1022)
  at java.lang.String.replaceAll(String.java:2162)
  ... 33 elided

The first parameter of replaceAll is meant to be a Regex string. In Regex, a simple backslash must be double-escaped ("\\"); a simply-escaped backslash is a read as an escape character in Regex.

The following is a fix:

protected def traceFileName(dn:DN, opType:String) : String = {
    val fileName = dn.getRDNStrings().map( _.replaceAll("\\" + File.separator, "|")).reverse.mkString("/")
    fileName + "-" + System.currentTimeMillis.toString + "-" + opType + ".ldif"
  }

It works on Linux as well, as an escaped forward slash is simply a forward slash:

scala> "c/programs/ldap".replaceAll("\\/", "x")
res2: String = cxprogramsxldap

@catalin-ursachi catalin-ursachi changed the title Fixed Windows file separator bug Fix Windows file separator bug Jul 25, 2015
@fanf
Copy link
Member

fanf commented Feb 16, 2016

Wow, I'm totally sorry @catalin-ursachi, I completelly missed your PR! I opened ticket http://www.rudder-project.org/redmine/issues/7935 to track it on our side.

For the code itself, perhaps it could be neater to use Pattern.quote(File.separator), but thanks for pointing that out.

If you are still around after all that time, could you add in the commit message "Fixes #7935" so that the link with redmine is correctly done ?
And finally, if it's OK, could you target branches/rudder/2.11 in place of master, so that the fix happen in all our supported version (we have tools to up-merge easely).

Thanks again, and sorry again for the silence.

@fanf
Copy link
Member

fanf commented Mar 25, 2016

Superseeded by #15 which also target Rudder branch 2.11

@fanf fanf closed this Mar 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants