Issue 4488 - Add support for JSON page front matter

cms/utils.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-present eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 import re
+import json
 
 __all__ = [
     'get_page_params',
     'process_page',
     'split_head_body',
     'extract_page_metadata'
 ]
 
 
 def split_head_body(html):
@@ skipping 31 matching lines @@
     ----------
     source: str
         Source text of the page.
 
     Returns
     -------
     (dict, str)
         Metadata of the page, remaining source text without metadata.
 
     """
-    metadata = {}
-    lines = source.splitlines(True)
-    for i, line in enumerate(lines):
-        if line.strip() in {'<!--', '-->'}:
-            lines[i] = ''
-            continue
-        if not re.search(r'^\s*[\w\-]+\s*=', line):
-            break
-        name, value = line.split('=', 1)
-        value = value.strip()
-        if value.startswith('[') and value.endswith(']'):
-            value = [element.strip() for element in value[1:-1].split(',')]
-        lines[i] = '\n'
-        metadata[name.strip()] = value
-    return metadata, ''.join(lines)
+    m = re.search(r'^\s*<!--\s*(.*?)-->', source, re.S)
+    text = m.group(1) if m else source
+
+    decoder = json.JSONDecoder()
+    try:
+        metadata, length = decoder.raw_decode(text)
+    except ValueError:
+        metadata = None
+
+    if not isinstance(metadata, dict):
+        metadata = {}
+        length = 0
+        for line in text.splitlines(True):
+            if not re.search(r'^\s*[\w\-]+\s*=', line):
+                break
+            name, value = line.split('=', 1)
+            value = value.strip()
+            if value.startswith('[') and value.endswith(']'):
+                value = [element.strip() for element in value[1:-1].split(',')]
+            metadata[name.strip()] = value
+            length += len(line)
+
+    # Need to preserve line numbers for jinja2 tracebacks
+    cutoff = m.end() if m else length
+    return metadata, '\n' * source.count('\n', 0, cutoff) + source[cutoff:]

[Sebastian Noack, 2018/03/26 02:57:48]

I just noticed one more edge case, if there is an HTML comment at the beginning
of the file, but it doesn't contain any metadata, we would still strip the
comment. This could be accounted for with following addition to the logic here:

  if length > 0:
      cutoff = m.end() if m else length
      source = '\n' * source.count('\n', 0, cutoff) + source[cutoff:]

  return metadata, source

[Vasily Kuznetsov, 2018/03/26 09:24:28]

Good catch, Sebastian! It probably makes sense to add a test for this too, to
avoid future regressions.

[Jon Sonesen, 2018/03/28 03:01:36]

Perhaps a test case should be added as well to cover this?

[rosie, 2018/03/31 21:09:28]

I've implemented Sebastian's suggestion and it works as expected, but while
trying to implement a test case for this, I ran into two issues.

1) A file with no specified template uses the default.tmpl to generate the page,
but the default.tmpl file is blank, so a blank page is generated. I'm not sure
if this is the intended behavior for this test site.

2) The sitemap.tmpl file seems to only include pages that have some metadata
when creating the sitemap output, so this new test case would never show up on
the sitemap. This may be out of scope for the current issue.

[Sebastian Noack, 2018/03/31 22:07:35]

So default.tmpl is empty, and empty.tmpl is not. That looks like a mistake, or
at very least it's extremely confusing. But I agree that cleaning this up seems
out of scope of this change.
Apparently the get_pages_metadata() function called in sitemap.tmpl explicitly
ignores any pages that don't define any custom metadata:
https://hg.adblockplus.org/cms/file/b683e321a9d0/cms/converters.py#l477
I cannot tell if that is intended or not, but at very least the comment there
gives no sufficient explanation.

[Vasily Kuznetsov, 2018/04/03 14:29:19]

I suppose we can say that it ended up being so for historical reasons :) Anyway,
apart from being confusing this seems to make testing of this case unnecessarily
complicated. I think the best approach would be to swap the contents of
`default.html` and `empty.tmpl` and adjust the rest of the test suite to make
sure the output doesn't change. https://issues.adblockplus.org/ticket/6546 (and
corresponding review https://codereview.adblockplus.org/29741581) do this, so
perhaps we can just land that before this change.
This looks like a bug to me. I've raised it in the review
(https://codereview.adblockplus.org/29472555/) but then it seems we've forgot
about it. I now created https://issues.adblockplus.org/ticket/6545 to fix this,
but as far as I can see this doesn't block this issue.

[rosie, 2018/04/18 02:35:32]

Acknowledged.

 
 
 def get_page_params(source, locale, page, format=None, site_url_override=None,
                     localized_string_callback=None):
     from cms.converters import converters
 
     # Guess page format if omitted, but default to Markdown for friendlier exceptions
     if format is None:
         for format in converters.iterkeys():
             if source.has_page(page, format):
@@ skipping 60 matching lines @@
     return params
 
 
 def process_page(source, locale, page, format=None, site_url_override=None,
                  localized_string_callback=None):
     from cms.converters import TemplateConverter
 
     params = get_page_params(source, locale, page, format, site_url_override,
                              localized_string_callback)
     return TemplateConverter(*params['templatedata'], params=params)()