Issue 4488 - Add support for JSON page front matter

cms/utils.py

Index: cms/utils.py
===================================================================
--- a/cms/utils.py
+++ b/cms/utils.py
@@ -14,6 +14,7 @@
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 import re
+import json
 
 __all__ = [
     'get_page_params',
@@ -65,21 +66,31 @@
         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,