Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix matching multiline control lines in templates with CRLF line endings #346

Closed
wants to merge 2 commits into from

Conversation

@LordAro
Copy link
Contributor

@LordAro LordAro commented Nov 9, 2021

A missing '\' meant that it would actually allow

% if foo \r
   bar:

in a template file and not match if the file actually had a \r char

@zzzeek zzzeek requested a review from bourke Nov 9, 2021
@zzzeek
Copy link
Member

@zzzeek zzzeek commented Nov 9, 2021

hey there-

any chance you can provide a test case for this?

@LordAro
Copy link
Contributor Author

@LordAro LordAro commented Nov 9, 2021

Not easy to do reliably, as it would require specific line endings in the file under test - and git mangles those quite readily with autocrlf (which is my workaround for this in the meantime)

Also, I have no idea how to write mako files, I just came across this while trying to get another project that uses them to build

That said, it should be easy to come up with something - as best as I can tell it affects any multiline "control line" (% ... \) with a \r\n line ending

@zzzeek
Copy link
Member

@zzzeek zzzeek commented Nov 10, 2021

im not really sure I understand the problem.

why would someone write this?

% if foo \r
   bar:

or is the issue that it's a syntax error and it doesnt raise an error? I dont really know what we are trying to achieve here.

@LordAro
Copy link
Contributor Author

@LordAro LordAro commented Nov 11, 2021

Sorry, I phrased the bug report really unclearly. Without this change, a multiline conditional with Windows CRLF newlines such as:

% if foo and \
     bar:

will never work, as the \r (char 13) means that the multiline statement is not recognised properly, resulting in errors like:

CompileException: Fragment 'if (not property.is_dispatcher and \' is not a partial control statement in file 'C:/builds/dev/libadalang/venv/Lib/site-packages/langkit/templates/properties/def_ada.mako' at line: 315 char: 1

@zzzeek
Copy link
Member

@zzzeek zzzeek commented Nov 12, 2021

So this is why we'd' prefer you open an issue report first w/ a test case. So far I can't reproduce (edit: see below, got it), I've added this test:

diff --git a/test/test_template.py b/test/test_template.py
index f2ca6b4..32bab5f 100644
--- a/test/test_template.py
+++ b/test/test_template.py
@@ -32,6 +32,26 @@ class ctx:
         pass
 
 
+class MiscTest(TemplateTest):
+    def test_crlf_linebreaks(self):
+
+        crlf = """
+<%
+    foo = True
+    bar = True
+%>
+% if foo and \
+     bar:
+     foo and bar
+%endif
+"""
+        crlf = crlf.replace("\n", "\r\n")
+        breakpoint()
+        self._do_test(
+            Template(crlf),
+            "\r\n\r\n     foo and bar\r\n"
+        )
+
 class EncodingTest(TemplateTest):
     def test_escapes_html_tags(self):
         from mako.exceptions import html_error_template

no failure. note the string above that we are running as a template is:

'\r\n<%\r\n    foo = True\r\n    bar = True\r\n%>\r\n% if foo and      bar:\r\n     foo and bar\r\n%endif\r\n'

that's the pattern you're referring towards, right?

so to move forward here we need an example template that reproduces the error, so that a test can be added, otherwise the code we have here could be reverted one day and we'd not know it.

@zzzeek
Copy link
Member

@zzzeek zzzeek commented Nov 12, 2021

oh i did the test wrong, didnt get the backslash in, moment

@zzzeek
Copy link
Member

@zzzeek zzzeek commented Nov 12, 2021

hi -

I forgot to escape the string, now it reproduces. add this to your patch please:

diff --git a/test/test_template.py b/test/test_template.py
index f2ca6b4..2d3d9e4 100644
--- a/test/test_template.py
+++ b/test/test_template.py
@@ -32,6 +32,25 @@ class ctx:
         pass
 
 
+class MiscTest(TemplateTest):
+    def test_crlf_linebreaks(self):
+
+        crlf = r"""
+<%
+    foo = True
+    bar = True
+%>
+% if foo and \
+     bar:
+     foo and bar
+%endif
+"""
+        crlf = crlf.replace("\n", "\r\n")
+        self._do_test(
+            Template(crlf),
+            "\r\n\r\n     foo and bar\r\n"
+        )
+
 class EncodingTest(TemplateTest):
     def test_escapes_html_tags(self):
         from mako.exceptions import html_error_template

@LordAro
Copy link
Contributor Author

@LordAro LordAro commented Nov 13, 2021

Done.

@zzzeek zzzeek requested a review from sqla-tester Nov 13, 2021
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision e79ebab of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

@sqla-tester sqla-tester commented Nov 13, 2021

New Gerrit review created for change e79ebab: https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3307

@bourke bourke added this to Icebox in Mako prioritization Nov 17, 2021
@bourke bourke moved this from Icebox to Active in Mako prioritization Nov 17, 2021
@zzzeek
Copy link
Member

@zzzeek zzzeek commented Nov 17, 2021

the upcoming 1.2 release will leave Python 2 forever and is also not set to be released right now until we finish it up. i will try to get this published as 1.1.6 which will require some changelog tinkering.

@sqla-tester
Copy link
Collaborator

@sqla-tester sqla-tester commented Nov 17, 2021

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3307 has been merged. Congratulations! :)

@sqla-tester
Copy link
Collaborator

@sqla-tester sqla-tester commented Nov 17, 2021

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3308 has been merged. Congratulations! :)

Mako prioritization automation moved this from Active to Done Nov 17, 2021
sqlalchemy-bot pushed a commit that referenced this issue Nov 17, 2021
Fixed issue where control statements on multi lines with a backslash would
not parse correctly if the template itself contained CR/LF pairs as on
Windows. Pull request courtesy Charles Pigott.

A missing '\\' meant that it would actually allow

```
% if foo \r
   bar:
```

in a template file and not match if the file actually had a `\r` char

Closes: #346
Pull-request: #346
Pull-request-sha: e79ebab

Change-Id: I179bdd661cecb1ffb3cf262e31183c8e83d98f12
(cherry picked from commit 338b755)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants