[jira] Created: (LANG-505) Rewrite StringEscapeUtils

Rewrite StringEscapeUtils
-------------------------

                 Key: LANG-505
                 URL: https://issues.apache.org/jira/browse/LANG-505
             Project: Commons Lang
          Issue Type: Task
            Reporter: Henri Yandell
             Fix For: 3.0


I think StringEscapeUtils needs a strong rewrite. For each escape method (and unescape) there tend to be three or four types of escaping happening. So not being able to define which set of three or four apply is a pain point (and cause of bug reports due to different desired features).

We should be offering basic functionality, but also allowing people to say "escape(Escapers.BASIC_XML, Escapers.LOW_UNICODE, Escapers.HIGH_UNICODE)".

Also should delete escapeSql; it's a bad one imo. Dangerous in that it will lead people to not use PreparedStatement and given it only escapes ', it's not much use. Especially as different dialects escape that in different ways.

Opening this ticket for discussion.



--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/jira/browse/LANG-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12710287#action_12710287 ]

Henri Yandell commented on LANG-505:
------------------------------------

A generic solution that's pluggable gets a bit interesting - do you assume that a single unicode codepoint maps to 1..n unicode codepoints, or should you be able to escape multiple codepoints? Unescaping is n codepoints to 1 codepoint. This makes it harder to have N plugins all working together. It becomes a translator, unescaping and escaping the same algorithm.

My first pass was on the 1..n. API is:

{code:java}
    // Note it's a FilterWriter, so 'out' is part of the class. Probably change that and put Writer in the API.
    public abstract boolean escape(int codepoint) throws IOException;
{code}

The boolean is returned to say whether that codepoint was successfully translated or not. This is needed for AggregateEscapers to work and so codepoint skipping can occur.

It leads to a top level use of:

{code:java}
    public void escapeJava(String input, Writer out) throws IOException {
        AggregateEscaper escapers = new AggregateEscaper(
            new EscapeBasedOnLookup(out,
                      new String[][] {
                            {"\"", "\\\""},
                            {"\\", "\\\\"}
                      }),
            new EscapeLowAsciiAsUnicode(out),
            new EscapeNonAsciiAsUnicode(out)
        );
           
        escape(input, escapers);
    }
{code}

It's easy to see from there how a user might modify what they view this to be. They could write an escapeC quite easily etc. However - it's 1..n, and unescaping isn't handled. The 1..n escape algorithm looks like:

{code:java}
    public void escape(String str, CharEscaper escaper) throws IOException {
        if (str == null) {
            return;
        }
        if (escaper == null) {
            throw new IllegalArgumentException("The CharEscaper must not be null. " +
                                               "Use NullEscaper if you expected this to mean a no-operation");
        }
        int sz = str.length();
        for (int i = 0; i < sz; i++) {
            int c = Character.codePointAt(str, i);
            boolean success = escaper.escape(c);

            // contract with escapers is that they have to understand codepoints and they just took care of a surrogate pair
            if(success && c >= 0x010000 && i < sz - 1) {
                i++;
            }
        }
    }
{code}

As I said - 1..n is the problem. Parsing needs to happen a character at a time, and each Escaper needs to be offered the choice to take control of the flow. Some options - thinking out loud:

* Escapers need to be passed in the equivalent of a C pointer String. A CharSequence would be good, but calling the subSequence method all the time might not be performant. Probably best to pass down the index and the CharSequence down. Making the whole thing String rather than char based - probably better anyway as that saves having to think in terms of codepoints.
* Escapers are asked whether they want to make a change first, then if they return true they are called again to make the change and they return an index increment for the driving loop to make based on the number of characters they consumed.

So:

{code:java}
    public abstract boolean isEscapable(int index, CharSequence input);
    public abstract int escape(int index, CharSequence input, Writer out) throws IOException;
{code}

Needs another code pass. Anyway - I think our current code is screwed up enough to warrant a deeper implementation.

> Rewrite StringEscapeUtils
> -------------------------
>
>                 Key: LANG-505
>                 URL: https://issues.apache.org/jira/browse/LANG-505
>             Project: Commons Lang
>          Issue Type: Task
>            Reporter: Henri Yandell
>             Fix For: 3.0
>
>
> I think StringEscapeUtils needs a strong rewrite. For each escape method (and unescape) there tend to be three or four types of escaping happening. So not being able to define which set of three or four apply is a pain point (and cause of bug reports due to different desired features).
> We should be offering basic functionality, but also allowing people to say "escape(Escapers.BASIC_XML, Escapers.LOW_UNICODE, Escapers.HIGH_UNICODE)".
> Also should delete escapeSql; it's a bad one imo. Dangerous in that it will lead people to not use PreparedStatement and given it only escapes ', it's not much use. Especially as different dialects escape that in different ways.
> Opening this ticket for discussion.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/jira/browse/LANG-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12716850#action_12716850 ]

Henri Yandell commented on LANG-505:
------------------------------------

Worked on this some more. The notion of passing in CharSequences works well, with a codepoint version to ease coding (and currently a char version but it may be pointless as codepoints feel like chars). I ignored the idea of having a query as to whether an escape will happen and then a second invocation to do the escape. It's not needed as yet.

escapeJava looks like:

{code:java}
    public static void escapeJava(String input, Writer out) throws IOException {
        AggregateEscaper escapers = new AggregateEscaper(
            new LookupTranslator(
                      new String[][] {
                            {"\"", "\\\""},
                            {"\\", "\\\\"}
                      }),
            new EscapeLowAsciiAsUnicode(),
            new EscapeNonAsciiAsUnicode()
        );
         
        escape(input, escapers, out);
    }
{code}

ie) despite the API change, much the same as the above. I went ahead and stopped the Escaper extending FilterWriter so that's why the out variable stops getting repeated everywhere.

The core algorithm itself now looks like:

{code:java}
    public static void escape(CharSequence input, CharSequenceEscaper escaper, Writer out) throws IOException {
        if (out == null) {
            throw new IllegalArgumentException("The Writer must not be null");
        }
        if (input == null) {
            return;
        }
        if (escaper == null) {
            throw new IllegalArgumentException("The CharSequenceEscaper must not be null. " +
                                               "Use NullEscaper if you expected this to mean a no-operation");
        }
        int sz = Character.codePointCount(input, 0, input.length());
        for (int i = 0; i < sz; i++) {

            // consumed is the number of codepoints consumed
            int consumed = escaper.escape(input, i, out);

            if(consumed == 0) {
                out.write( Character.toChars( Character.codePointAt(input, i) ) );
            } else {
                // contract with escapers is that they have to understand codepoints and they just took care of a surrogate pair
                for(int j=0; j<consumed; j++) {
                    if(i < sz - 2) {
                        // for loop will increment 1 anyway, so remove 1 from the end charCount
                        i += Character.charCount( Character.codePointAt(input, i) ) - 1;
                    }
                }
            }
        }
    }
{code}

This should be able to implement unescape as well as it is now a simple general purpose text translation API that can handle escaping n->m changes in characters. ie) a plugin can choose to consume 3 characters and turn them into 5, and vice versa.

> Rewrite StringEscapeUtils
> -------------------------
>
>                 Key: LANG-505
>                 URL: https://issues.apache.org/jira/browse/LANG-505
>             Project: Commons Lang
>          Issue Type: Task
>            Reporter: Henri Yandell
>             Fix For: 3.0
>
>
> I think StringEscapeUtils needs a strong rewrite. For each escape method (and unescape) there tend to be three or four types of escaping happening. So not being able to define which set of three or four apply is a pain point (and cause of bug reports due to different desired features).
> We should be offering basic functionality, but also allowing people to say "escape(Escapers.BASIC_XML, Escapers.LOW_UNICODE, Escapers.HIGH_UNICODE)".
> Also should delete escapeSql; it's a bad one imo. Dangerous in that it will lead people to not use PreparedStatement and given it only escapes ', it's not much use. Especially as different dialects escape that in different ways.
> Opening this ticket for discussion.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/jira/browse/LANG-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12717203#action_12717203 ]

Henri Yandell commented on LANG-505:
------------------------------------

unescapeJava was quite implementable - though I'm changing behaviour from passing on a last char being an escaping \ without a value to escape to it being removed (and in fact I think the correct behaviour would be an exception).

unescapeCsv + escapeCsv were hugely easy. Same for escapeJavaScript.

HTML and XML left, while I'm left wondering if all I'm doing is writing a very dumb Regexp engine and it would be better to replace the entire StringEscapeUtils with a RegexUtils that defines various standard Regexs and lets you build your own. While huge numbers of non-grouping alternation might be considered poor performance in a regex, I doubt it's any worse than my own code.

Will attach code for people's perusal.

> Rewrite StringEscapeUtils
> -------------------------
>
>                 Key: LANG-505
>                 URL: https://issues.apache.org/jira/browse/LANG-505
>             Project: Commons Lang
>          Issue Type: Task
>            Reporter: Henri Yandell
>             Fix For: 3.0
>
>
> I think StringEscapeUtils needs a strong rewrite. For each escape method (and unescape) there tend to be three or four types of escaping happening. So not being able to define which set of three or four apply is a pain point (and cause of bug reports due to different desired features).
> We should be offering basic functionality, but also allowing people to say "escape(Escapers.BASIC_XML, Escapers.LOW_UNICODE, Escapers.HIGH_UNICODE)".
> Also should delete escapeSql; it's a bad one imo. Dangerous in that it will lead people to not use PreparedStatement and given it only escapes ', it's not much use. Especially as different dialects escape that in different ways.
> Opening this ticket for discussion.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

     [ https://issues.apache.org/jira/browse/LANG-505?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Henri Yandell updated LANG-505:
-------------------------------

    Attachment: LANG-505.diff

Attaching CharSequenceEscaper and various other changes to StringEscapeUtils to turn it into a system whereby the user can pick and choose what 'escapeXyz' means for them.

Still a work in progress. The XML/HTML parts need implementing, and javadoc/cleanup needs to happen. Mostly attaching to determine if it's a valuable direction or if the future is regexps.

> Rewrite StringEscapeUtils
> -------------------------
>
>                 Key: LANG-505
>                 URL: https://issues.apache.org/jira/browse/LANG-505
>             Project: Commons Lang
>          Issue Type: Task
>            Reporter: Henri Yandell
>             Fix For: 3.0
>
>         Attachments: LANG-505.diff
>
>
> I think StringEscapeUtils needs a strong rewrite. For each escape method (and unescape) there tend to be three or four types of escaping happening. So not being able to define which set of three or four apply is a pain point (and cause of bug reports due to different desired features).
> We should be offering basic functionality, but also allowing people to say "escape(Escapers.BASIC_XML, Escapers.LOW_UNICODE, Escapers.HIGH_UNICODE)".
> Also should delete escapeSql; it's a bad one imo. Dangerous in that it will lead people to not use PreparedStatement and given it only escapes ', it's not much use. Especially as different dialects escape that in different ways.
> Opening this ticket for discussion.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

     [ https://issues.apache.org/jira/browse/LANG-505?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Henri Yandell updated LANG-505:
-------------------------------

    Attachment: LANG-505.diff

Attaching latest work. StringEscapeUtils is now completely on the new escaper system in this and I mostly just need to javadoc and strip out debugging to have a like for like replacement. The only functional change is that a '\' on its own at the end of a Java file will be removed not kept.

> Rewrite StringEscapeUtils
> -------------------------
>
>                 Key: LANG-505
>                 URL: https://issues.apache.org/jira/browse/LANG-505
>             Project: Commons Lang
>          Issue Type: Task
>            Reporter: Henri Yandell
>             Fix For: 3.0
>
>         Attachments: LANG-505.diff, LANG-505.diff
>
>
> I think StringEscapeUtils needs a strong rewrite. For each escape method (and unescape) there tend to be three or four types of escaping happening. So not being able to define which set of three or four apply is a pain point (and cause of bug reports due to different desired features).
> We should be offering basic functionality, but also allowing people to say "escape(Escapers.BASIC_XML, Escapers.LOW_UNICODE, Escapers.HIGH_UNICODE)".
> Also should delete escapeSql; it's a bad one imo. Dangerous in that it will lead people to not use PreparedStatement and given it only escapes ', it's not much use. Especially as different dialects escape that in different ways.
> Opening this ticket for discussion.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/jira/browse/LANG-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12719235#action_12719235 ]

Henri Yandell commented on LANG-505:
------------------------------------

Also needs some potential deleting (RegexpReplacer may not be worth having as it's not used), renaming of files and methods as we dicker design, refactoring - for example so that people don't get the EntityArray.BASIC_ESCAPE String[][] but instead get LookupTranslator objects such as Escapers.BASIC_XML and Unescapers.BASIC_XML.

> Rewrite StringEscapeUtils
> -------------------------
>
>                 Key: LANG-505
>                 URL: https://issues.apache.org/jira/browse/LANG-505
>             Project: Commons Lang
>          Issue Type: Task
>            Reporter: Henri Yandell
>             Fix For: 3.0
>
>         Attachments: LANG-505.diff, LANG-505.diff
>
>
> I think StringEscapeUtils needs a strong rewrite. For each escape method (and unescape) there tend to be three or four types of escaping happening. So not being able to define which set of three or four apply is a pain point (and cause of bug reports due to different desired features).
> We should be offering basic functionality, but also allowing people to say "escape(Escapers.BASIC_XML, Escapers.LOW_UNICODE, Escapers.HIGH_UNICODE)".
> Also should delete escapeSql; it's a bad one imo. Dangerous in that it will lead people to not use PreparedStatement and given it only escapes ', it's not much use. Especially as different dialects escape that in different ways.
> Opening this ticket for discussion.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/jira/browse/LANG-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12719236#action_12719236 ]

Henri Yandell commented on LANG-505:
------------------------------------

And to remove Entities.java and escapeSql, and add new API for things like HTML3.2 etc.

> Rewrite StringEscapeUtils
> -------------------------
>
>                 Key: LANG-505
>                 URL: https://issues.apache.org/jira/browse/LANG-505
>             Project: Commons Lang
>          Issue Type: Task
>            Reporter: Henri Yandell
>             Fix For: 3.0
>
>         Attachments: LANG-505.diff, LANG-505.diff
>
>
> I think StringEscapeUtils needs a strong rewrite. For each escape method (and unescape) there tend to be three or four types of escaping happening. So not being able to define which set of three or four apply is a pain point (and cause of bug reports due to different desired features).
> We should be offering basic functionality, but also allowing people to say "escape(Escapers.BASIC_XML, Escapers.LOW_UNICODE, Escapers.HIGH_UNICODE)".
> Also should delete escapeSql; it's a bad one imo. Dangerous in that it will lead people to not use PreparedStatement and given it only escapes ', it's not much use. Especially as different dialects escape that in different ways.
> Opening this ticket for discussion.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/jira/browse/LANG-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12722504#action_12722504 ]

Henri Yandell commented on LANG-505:
------------------------------------

escapeSql is gone btw.

> Rewrite StringEscapeUtils
> -------------------------
>
>                 Key: LANG-505
>                 URL: https://issues.apache.org/jira/browse/LANG-505
>             Project: Commons Lang
>          Issue Type: Task
>            Reporter: Henri Yandell
>             Fix For: 3.0
>
>         Attachments: LANG-505.diff, LANG-505.diff
>
>
> I think StringEscapeUtils needs a strong rewrite. For each escape method (and unescape) there tend to be three or four types of escaping happening. So not being able to define which set of three or four apply is a pain point (and cause of bug reports due to different desired features).
> We should be offering basic functionality, but also allowing people to say "escape(Escapers.BASIC_XML, Escapers.LOW_UNICODE, Escapers.HIGH_UNICODE)".
> Also should delete escapeSql; it's a bad one imo. Dangerous in that it will lead people to not use PreparedStatement and given it only escapes ', it's not much use. Especially as different dialects escape that in different ways.
> Opening this ticket for discussion.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/jira/browse/LANG-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12722981#action_12722981 ]

Henri Yandell commented on LANG-505:
------------------------------------

Committed as r787560. Leaving this issue open so people can beat me up about it :)

> Rewrite StringEscapeUtils
> -------------------------
>
>                 Key: LANG-505
>                 URL: https://issues.apache.org/jira/browse/LANG-505
>             Project: Commons Lang
>          Issue Type: Task
>            Reporter: Henri Yandell
>             Fix For: 3.0
>
>         Attachments: LANG-505.diff, LANG-505.diff
>
>
> I think StringEscapeUtils needs a strong rewrite. For each escape method (and unescape) there tend to be three or four types of escaping happening. So not being able to define which set of three or four apply is a pain point (and cause of bug reports due to different desired features).
> We should be offering basic functionality, but also allowing people to say "escape(Escapers.BASIC_XML, Escapers.LOW_UNICODE, Escapers.HIGH_UNICODE)".
> Also should delete escapeSql; it's a bad one imo. Dangerous in that it will lead people to not use PreparedStatement and given it only escapes ', it's not much use. Especially as different dialects escape that in different ways.
> Opening this ticket for discussion.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/jira/browse/LANG-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12723925#action_12723925 ]

Henri Yandell commented on LANG-505:
------------------------------------

Performance an issue however as expected. StringEscapeUtilsTest has moved from 0.4 seconds to 1.9 seconds.

Presumably this is going to be because the LookupTranslator sits on top of arrays and not the IntHashMap. Also presumably there is some small cost to having more objects at play here.

> Rewrite StringEscapeUtils
> -------------------------
>
>                 Key: LANG-505
>                 URL: https://issues.apache.org/jira/browse/LANG-505
>             Project: Commons Lang
>          Issue Type: Task
>            Reporter: Henri Yandell
>             Fix For: 3.0
>
>         Attachments: LANG-505.diff, LANG-505.diff
>
>
> I think StringEscapeUtils needs a strong rewrite. For each escape method (and unescape) there tend to be three or four types of escaping happening. So not being able to define which set of three or four apply is a pain point (and cause of bug reports due to different desired features).
> We should be offering basic functionality, but also allowing people to say "escape(Escapers.BASIC_XML, Escapers.LOW_UNICODE, Escapers.HIGH_UNICODE)".
> Also should delete escapeSql; it's a bad one imo. Dangerous in that it will lead people to not use PreparedStatement and given it only escapes ', it's not much use. Especially as different dialects escape that in different ways.
> Opening this ticket for discussion.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

    [ https://issues.apache.org/jira/browse/LANG-505?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12725491#action_12725491 ]

Henri Yandell commented on LANG-505:
------------------------------------

Performance fixed. At least the speed is back down to 0.5 seconds, which is close enough for now.

> Rewrite StringEscapeUtils
> -------------------------
>
>                 Key: LANG-505
>                 URL: https://issues.apache.org/jira/browse/LANG-505
>             Project: Commons Lang
>          Issue Type: Task
>            Reporter: Henri Yandell
>             Fix For: 3.0
>
>         Attachments: LANG-505.diff, LANG-505.diff
>
>
> I think StringEscapeUtils needs a strong rewrite. For each escape method (and unescape) there tend to be three or four types of escaping happening. So not being able to define which set of three or four apply is a pain point (and cause of bug reports due to different desired features).
> We should be offering basic functionality, but also allowing people to say "escape(Escapers.BASIC_XML, Escapers.LOW_UNICODE, Escapers.HIGH_UNICODE)".
> Also should delete escapeSql; it's a bad one imo. Dangerous in that it will lead people to not use PreparedStatement and given it only escapes ', it's not much use. Especially as different dialects escape that in different ways.
> Opening this ticket for discussion.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

     [ https://issues.apache.org/jira/browse/LANG-505?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Henri Yandell closed LANG-505.
------------------------------

    Resolution: Fixed

Rewrite complete. LANG-515 to define how the escaping/unbehaviour logic should behave.

> Rewrite StringEscapeUtils
> -------------------------
>
>                 Key: LANG-505
>                 URL: https://issues.apache.org/jira/browse/LANG-505
>             Project: Commons Lang
>          Issue Type: Task
>            Reporter: Henri Yandell
>             Fix For: 3.0
>
>         Attachments: LANG-505.diff, LANG-505.diff
>
>
> I think StringEscapeUtils needs a strong rewrite. For each escape method (and unescape) there tend to be three or four types of escaping happening. So not being able to define which set of three or four apply is a pain point (and cause of bug reports due to different desired features).
> We should be offering basic functionality, but also allowing people to say "escape(Escapers.BASIC_XML, Escapers.LOW_UNICODE, Escapers.HIGH_UNICODE)".
> Also should delete escapeSql; it's a bad one imo. Dangerous in that it will lead people to not use PreparedStatement and given it only escapes ', it's not much use. Especially as different dialects escape that in different ways.
> Opening this ticket for discussion.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.