groovy
  1. groovy
  2. GROOVY-399

Groovy Template quote escape problem

    Details

    • Type: Bug Bug
    • Status: Open Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Templating
    • Labels:
      None
    • Number of attachments :
      3

      Description

      The SimpleTemplateEngine got a problem with escaping quotes inside GStrings.

      "this" is tricky: ${hello + " " + hello}
      

      will be parsed to

      out.print("\"this\" is tricky: ${hello + \" \" + hello}");
      

      which is a problem for the current lexer as a GString must be a valid groovy statement.

      1. patch.txt
        1.0 kB
        Thomas Heller
      2. test.tar.gz
        11 kB
        Niall Gallagher
      3. test-closure.tar.gz
        13 kB
        Niall Gallagher

        Issue Links

          Activity

          Hide
          Thomas Heller added a comment -

          patch diff to be applied from groovy/groovy-core

          Show
          Thomas Heller added a comment - patch diff to be applied from groovy/groovy-core
          Hide
          Chris Poirier added a comment -

          Thomas: I'm not clear on the purpose of that test – that GString expression will be expanded during initial parsing (and will fail, due to the first backslash). Is that what you wanted? If not, you need to escape it:
          String source = "\"this\" is tricky: \$

          {hello + \" \" + hello}

          ";

          Show
          Chris Poirier added a comment - Thomas: I'm not clear on the purpose of that test – that GString expression will be expanded during initial parsing (and will fail, due to the first backslash). Is that what you wanted? If not, you need to escape it: String source = "\"this\" is tricky: \$ {hello + \" \" + hello} ";
          Hide
          Thomas Heller added a comment -

          If you have a Groovy Template (from groovy.text.SimpleTemplateEngine) which contains (in a File for example)

          "this" is tricky: $

          {hello + " " + hello}

          it will be parsed by the Template impl to create to stated source which is causing an exception is you pass it into the groovy compiler.

          Now this creates:

          out.print("\"this\" is tricky: ${hello + " " + hello}

          ");

          which is not a valid groovy statement (as of now). I think it should be but a GString is expected to be a plain groovy statement so

          out.print("\"this\" is tricky: $

          {hello + " " + hello}

          ");

          would be correct. I think the lexer needs to handle that since the first statement is correct java while the second is not.

          Show
          Thomas Heller added a comment - If you have a Groovy Template (from groovy.text.SimpleTemplateEngine) which contains (in a File for example) "this" is tricky: $ {hello + " " + hello} it will be parsed by the Template impl to create to stated source which is causing an exception is you pass it into the groovy compiler. Now this creates: out.print("\"this\" is tricky: ${hello + " " + hello} "); which is not a valid groovy statement (as of now). I think it should be but a GString is expected to be a plain groovy statement so out.print("\"this\" is tricky: $ {hello + " " + hello} "); would be correct. I think the lexer needs to handle that since the first statement is correct java while the second is not.
          Hide
          Chris Poirier added a comment -

          > I think the lexer needs to handle that since the first statement is correct java while the second is not.

          Yes, because regular Java doesn't do GStrings.... In order for GString expressions to be truly useful, they have to host real Groovy code, like closures, possibly with nesting strings, possibly with nested GString expressions. How do you handle backslashes in nested GStrings? The answer is not to try. I think the handling currently in place is the correct thing to do. Double quotes delimit a nested language.

          So can I assume that the intention of the attached test is to break the build?

          Show
          Chris Poirier added a comment - > I think the lexer needs to handle that since the first statement is correct java while the second is not. Yes, because regular Java doesn't do GStrings.... In order for GString expressions to be truly useful, they have to host real Groovy code, like closures, possibly with nesting strings, possibly with nested GString expressions. How do you handle backslashes in nested GStrings? The answer is not to try. I think the handling currently in place is the correct thing to do. Double quotes delimit a nested language. So can I assume that the intention of the attached test is to break the build?
          Hide
          Thomas Heller added a comment -

          Its not intended to break the build, its intended to show a bug.

          I'm quite sure that you will see something like that in templates and either the lexer or the groovy template engine need to handle this case.

          I inlined the template text in this testcase to not depend on external files etc. But you are welcome to move the example

          $

          {hello + " " + hello}

          into a file and let it run from there.

          I know that

          $

          {hello} ${hello}

          is a better solution. But since GStrings are plain groovy statements its perfectly fine the run the "broken" example.

          Show
          Thomas Heller added a comment - Its not intended to break the build, its intended to show a bug. I'm quite sure that you will see something like that in templates and either the lexer or the groovy template engine need to handle this case. I inlined the template text in this testcase to not depend on external files etc. But you are welcome to move the example $ {hello + " " + hello} into a file and let it run from there. I know that $ {hello} ${hello} is a better solution. But since GStrings are plain groovy statements its perfectly fine the run the "broken" example.
          Hide
          Chris Poirier added a comment -

          Agreed. The SimpleTemplateEngine needs to be fixed.

          Show
          Chris Poirier added a comment - Agreed. The SimpleTemplateEngine needs to be fixed.
          Hide
          Guillaume Laforge added a comment -

          Sam, it looks like a problem for you.

          Show
          Guillaume Laforge added a comment - Sam, it looks like a problem for you.
          Hide
          Thomas Heller added a comment -

          A little more obvious problem:

          $

          {someObj.withAMethod("hi")}

          won't work when used inside templates.

          Show
          Thomas Heller added a comment - A little more obvious problem: $ {someObj.withAMethod("hi")} won't work when used inside templates.
          Hide
          Niall Gallagher added a comment -

          I have implemented a solution to this problem. The solution uses a cascading translator that converts

          $

          { hello + " " + hello}

          to

          <?= hello + " " + hello ?>

          and finally, to

          out.println( hello + " " + hello );

          which is a valid Groovy statement. I would like to contribute and/or maintain this code for Groovy.

          Show
          Niall Gallagher added a comment - I have implemented a solution to this problem. The solution uses a cascading translator that converts $ { hello + " " + hello} to <?= hello + " " + hello ?> and finally, to out.println( hello + " " + hello ); which is a valid Groovy statement. I would like to contribute and/or maintain this code for Groovy.
          Hide
          Guillaume Laforge added a comment -

          Niall, attach your patch to one of the issues, and we'll have a look at it.

          Show
          Guillaume Laforge added a comment - Niall, attach your patch to one of the issues, and we'll have a look at it.
          Hide
          Niall Gallagher added a comment -

          The attached file is not exactly a patch but rather a rework of the parsing done for the templates. If it is reasonable at this stage to port the GStringTemplateEngine and SimpleTemplateEngine to this framework then I would be happy to do it.

          Attached you will find some test drivers and an example template with some "tricky" expressions. Don't know what licence to attach to these?

          Show
          Niall Gallagher added a comment - The attached file is not exactly a patch but rather a rework of the parsing done for the templates. If it is reasonable at this stage to port the GStringTemplateEngine and SimpleTemplateEngine to this framework then I would be happy to do it. Attached you will find some test drivers and an example template with some "tricky" expressions. Don't know what licence to attach to these?
          Hide
          Niall Gallagher added a comment -

          Whoops! in haste I did not attach a parser for the closure used by the GStringTemplateEngine. I have attached it on this upload. It produces an identical script to the one produced by the current GStringTemplateEngine, except it does not escape quotes within $

          {..}

          expressions. The following will generate the Groovy script from a template.

          java TestCascade template.groovy

          Show
          Niall Gallagher added a comment - Whoops! in haste I did not attach a parser for the closure used by the GStringTemplateEngine. I have attached it on this upload. It produces an identical script to the one produced by the current GStringTemplateEngine, except it does not escape quotes within $ {..} expressions. The following will generate the Groovy script from a template. java TestCascade template.groovy
          Hide
          Guillaume Laforge added a comment -

          Oh Template and Groovlet master!!!

          Show
          Guillaume Laforge added a comment - Oh Template and Groovlet master!!!
          Hide
          Christian Stein added a comment -

          Looking at http://jira.codehaus.org/browse/GROOVY-399 - we should write the test cases asap - before refactoring the whole parser stuff. After a first glimpse, it could help with all those char-by-char parser short-comings.

          Show
          Christian Stein added a comment - Looking at http://jira.codehaus.org/browse/GROOVY-399 - we should write the test cases asap - before refactoring the whole parser stuff. After a first glimpse, it could help with all those char-by-char parser short-comings.
          Hide
          Paul King added a comment -

          OK, I made a few changes to the existing code base so that the ugly problems with quotes inside (for now simple) GStrings goes away. Not as good as the other proposals listed here for a re-write but at least gets rid of the ugly cases for now.

          I also tried grafting in the current antlr GString parsing into the template codebase and made some progress but backed out as it was going to take a lot more time to finish than I have right now.

          Show
          Paul King added a comment - OK, I made a few changes to the existing code base so that the ugly problems with quotes inside (for now simple) GStrings goes away. Not as good as the other proposals listed here for a re-write but at least gets rid of the ugly cases for now. I also tried grafting in the current antlr GString parsing into the template codebase and made some progress but backed out as it was going to take a lot more time to finish than I have right now.

            People

            • Assignee:
              Unassigned
              Reporter:
              Thomas Heller
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated: