Consolidate validation of content (#2128)

* Consolidate validation of content

Previously we validated content outside of the content class via
calls to `is_valid_content` and some additional checks in page /
article generators (valid status).
This commit moves those checks all into content.valid() resulting
in a cleaner code structure.
This allows us to restructure how generators interact with content,
removing several old bugs in pelican (#1748, #1356, #2098).

- move verification function into content class
- move generator verifying content to contents class
- remove unused quote class
- remove draft class (no more rereading drafts)
- move auto draft status setter into Article.__init__
- add now parsing draft to basic test output
- remove problematic DEFAULT_STATUS setting
- add setter/getter for content.status
  removes need for lower() calls when verifying status

* expand c4b184fa32

Mostly implement feedback by @iKevinY.

* rename content.valid to content.is_valid
* rename valid_* functions to has_valid_*
* update tests and function calls in code accordingly

pelican/contents.py

@@ -138,14 +138,7 @@ class Content(object):
 
         # manage status
         if not hasattr(self, 'status'):
-            self.status = settings['DEFAULT_STATUS']
+            self.status = getattr(self, 'default_status', None)
-            if not settings['WITH_FUTURE_DATES'] and hasattr(self, 'date'):
-                if self.date.tzinfo is None:
-                    now = SafeDatetime.now()
-                else:
-                    now = SafeDatetime.utcnow().replace(tzinfo=pytz.utc)
-                if self.date > now:
-                    self.status = 'draft'
 
         # store the summary metadata if it is set
         if 'summary' in metadata:
@@ -156,13 +149,17 @@ class Content(object):
     def __str__(self):
         return self.source_path or repr(self)
 
-    def check_properties(self):
+    def _has_valid_mandatory_properties(self):
         """Test mandatory properties are set."""
         for prop in self.mandatory_properties:
             if not hasattr(self, prop):
-                raise NameError(prop)
+                logger.error(
+                    "Skipping %s: could not find information about '%s'",
+                    self, prop)
+                return False
+        return True
 
-    def valid_save_as(self):
+    def _has_valid_save_as(self):
         """Return true if save_as doesn't write outside output path, false
         otherwise."""
         try:
@@ -174,10 +171,35 @@ class Content(object):
         try:
             sanitised_join(output_path, self.save_as)
         except RuntimeError:  # outside output_dir
+            logger.error(
+                "Skipping %s: file %r would be written outside output path",
+                self,
+                self.save_as,
+            )
             return False
 
         return True
 
+    def _has_valid_status(self):
+        if hasattr(self, 'allowed_statuses'):
+            if self.status not in self.allowed_statuses:
+                logger.error(
+                    "Unknown status '%s' for file %s, skipping it.",
+                    self.status,
+                    self
+                )
+                return False
+
+        # if undefined we allow all
+        return True
+
+    def is_valid(self):
+        """Validate Content"""
+        # Use all() to not short circuit and get results of all validations
+        return all([self._has_valid_mandatory_properties(),
+                    self._has_valid_save_as(),
+                    self._has_valid_status()])
+
     @property
     def url_format(self):
         """Returns the URL, formatted with the proper values"""
@@ -194,8 +216,10 @@ class Content(object):
         })
         return metadata
 
-    def _expand_settings(self, key):
+    def _expand_settings(self, key, klass=None):
-        fq_key = ('%s_%s' % (self.__class__.__name__, key)).upper()
+        if not klass:
+            klass = self.__class__.__name__
+        fq_key = ('%s_%s' % (klass, key)).upper()
         return self.settings[fq_key].format(**self.url_format)
 
     def get_url_setting(self, key):
@@ -338,6 +362,15 @@ class Content(object):
         """Dummy function"""
         pass
 
+    @property
+    def status(self):
+        return self._status
+
+    @status.setter
+    def status(self, value):
+        # TODO maybe typecheck
+        self._status = value.lower()
+
     @property
     def url(self):
         return self.get_url_setting('url')
@@ -383,21 +416,36 @@ class Content(object):
 
 class Page(Content):
     mandatory_properties = ('title',)
+    allowed_statuses = ('published', 'hidden')
+    default_status = 'published'
     default_template = 'page'
 
 
-class Article(Page):
+class Article(Content):
     mandatory_properties = ('title', 'date', 'category')
+    allowed_statuses = ('published', 'draft')
+    default_status = 'published'
     default_template = 'article'
 
+    def __init__(self, *args, **kwargs):
+        super(Article, self).__init__(*args, **kwargs)
 
-class Draft(Page):
+        # handle WITH_FUTURE_DATES (designate article to draft based on date)
-    mandatory_properties = ('title', 'category')
+        if not self.settings['WITH_FUTURE_DATES'] and hasattr(self, 'date'):
-    default_template = 'article'
+            if self.date.tzinfo is None:
+                now = SafeDatetime.now()
+            else:
+                now = SafeDatetime.utcnow().replace(tzinfo=pytz.utc)
+            if self.date > now:
+                self.status = 'draft'
 
+        # if we are a draft and there is no date provided, set max datetime
+        if not hasattr(self, 'date') and self.status == 'draft':
+            self.date = SafeDatetime.max
 
-class Quote(Page):
+    def _expand_settings(self, key):
-    base_properties = ('author', 'date')
+        klass = 'article' if self.status == 'published' else 'draft'
+        return super(Article, self)._expand_settings(key, klass)
 
 
 @python_2_unicode_compatible
@@ -482,25 +530,3 @@ class Static(Page):
 
         self.override_save_as = new_save_as
         self.override_url = new_url
-
-
-def is_valid_content(content, f):
-    try:
-        content.check_properties()
-    except NameError as e:
-        logger.error(
-            "Skipping %s: could not find information about '%s'",
-            f, six.text_type(e))
-        return False
-
-    if not content.valid_save_as():
-        logger.error(
-            "Skipping %s: file %r would be written outside output path",
-            f,
-            content.save_as,
-        )
-        # Note: future code might want to use a result variable instead, to
-        # allow showing multiple error messages at once.
-        return False
-
-    return True

pelican/generators.py

@@ -19,7 +19,7 @@ import six
 
 from pelican import signals
 from pelican.cache import FileStampDataCacher
-from pelican.contents import Article, Draft, Page, Static, is_valid_content
+from pelican.contents import Article, Page, Static
 from pelican.readers import Readers
 from pelican.utils import (DateFormatter, copy, mkdir_p, posixize_path,
                            process_translations, python_2_unicode_compatible)
@@ -509,12 +509,10 @@ class ArticlesGenerator(CachingGenerator):
         for f in self.get_files(
                 self.settings['ARTICLE_PATHS'],
                 exclude=self.settings['ARTICLE_EXCLUDES']):
-            article_or_draft = self.get_cached_data(f, None)
+            article = self.get_cached_data(f, None)
-            if article_or_draft is None:
+            if article is None:
-                # TODO needs overhaul, maybe nomad for read_file
-                # solution, unified behaviour
                 try:
-                    article_or_draft = self.readers.read_file(
+                    article = self.readers.read_file(
                         base_path=self.path, path=f, content_class=Article,
                         context=self.context,
                         preread_signal=signals.article_generator_preread,
@@ -528,34 +526,17 @@ class ArticlesGenerator(CachingGenerator):
                     self._add_failed_source_path(f)
                     continue
 
-                if not is_valid_content(article_or_draft, f):
+                if not article.is_valid():
                     self._add_failed_source_path(f)
                     continue
 
-                if article_or_draft.status.lower() == "published":
+                self.cache_data(f, article)
-                    pass
-                elif article_or_draft.status.lower() == "draft":
-                    article_or_draft = self.readers.read_file(
-                        base_path=self.path, path=f, content_class=Draft,
-                        context=self.context,
-                        preread_signal=signals.article_generator_preread,
-                        preread_sender=self,
-                        context_signal=signals.article_generator_context,
-                        context_sender=self)
-                else:
-                    logger.error(
-                        "Unknown status '%s' for file %s, skipping it.",
-                        article_or_draft.status, f)
-                    self._add_failed_source_path(f)
-                    continue
 
-                self.cache_data(f, article_or_draft)
+            if article.status == "published":
-
+                all_articles.append(article)
-            if article_or_draft.status.lower() == "published":
+            elif article.status == "draft":
-                all_articles.append(article_or_draft)
+                all_drafts.append(article)
-            else:
+            self.add_source_path(article)
-                all_drafts.append(article_or_draft)
-            self.add_source_path(article_or_draft)
 
         self.articles, self.translations = process_translations(
             all_articles,
@@ -634,22 +615,15 @@ class PagesGenerator(CachingGenerator):
                     self._add_failed_source_path(f)
                     continue
 
-                if not is_valid_content(page, f):
+                if not page.is_valid():
-                    self._add_failed_source_path(f)
-                    continue
-
-                if page.status.lower() not in ("published", "hidden"):
-                    logger.error(
-                        "Unknown status '%s' for file %s, skipping it.",
-                        page.status, f)
                     self._add_failed_source_path(f)
                     continue
 
                 self.cache_data(f, page)
 
-            if page.status.lower() == "published":
+            if page.status == "published":
                 all_pages.append(page)
-            elif page.status.lower() == "hidden":
+            elif page.status == "hidden":
                 hidden_pages.append(page)
             self.add_source_path(page)
 

pelican/settings.py

@@ -126,7 +126,6 @@ DEFAULT_CONFIG = {
     'FILENAME_METADATA': r'(?P<date>\d{4}-\d{2}-\d{2}).*',
     'PATH_METADATA': '',
     'EXTRA_PATH_METADATA': {},
-    'DEFAULT_STATUS': 'published',
     'ARTICLE_PERMALINK_STRUCTURE': '',
     'TYPOGRIFY': False,
     'TYPOGRIFY_IGNORE_TAGS': [],

pelican/tests/output/basic/drafts/a-draft-article.html

@@ -0,0 +1,69 @@
+<!DOCTYPE html>
+<html lang="en">
+<head>
+        <meta charset="utf-8" />
+        <title>A draft article</title>
+        <link rel="stylesheet" href="/theme/css/main.css" />
+        <link href="/feeds/all.atom.xml" type="application/atom+xml" rel="alternate" title="A Pelican Blog Atom Feed" />
+
+        <!--[if IE]>
+            <script src="https://html5shiv.googlecode.com/svn/trunk/html5.js"></script>
+        <![endif]-->
+</head>
+
+<body id="index" class="home">
+        <header id="banner" class="body">
+                <h1><a href="/">A Pelican Blog </a></h1>
+                <nav><ul>
+                    <li><a href="/tag/oh.html">Oh Oh Oh</a></li>
+                    <li><a href="/override/">Override url/save_as</a></li>
+                    <li><a href="/pages/this-is-a-test-page.html">This is a test page</a></li>
+                    <li><a href="/category/bar.html">bar</a></li>
+                    <li><a href="/category/cat1.html">cat1</a></li>
+                    <li class="active"><a href="/category/misc.html">misc</a></li>
+                    <li><a href="/category/yeah.html">yeah</a></li>
+                </ul></nav>
+        </header><!-- /#banner -->
+<section id="content" class="body">
+  <article>
+    <header>
+      <h1 class="entry-title">
+        <a href="/drafts/a-draft-article.html" rel="bookmark"
+           title="Permalink to A draft article">A draft article</a></h1>
+    </header>
+
+    <div class="entry-content">
+<footer class="post-info">
+        <abbr class="published" title="9999-12-31T23:59:59.999999">
+                Published: 
+        </abbr>
+
+<p>In <a href="/category/misc.html">misc</a>.</p>
+
+</footer><!-- /.post-info -->      <p>This is a draft article, it should live under the /drafts/ folder and not be
+listed anywhere else.</p>
+
+    </div><!-- /.entry-content -->
+
+  </article>
+</section>
+        <section id="extras" class="body">
+                <div class="social">
+                        <h2>social</h2>
+                        <ul>
+                            <li><a href="/feeds/all.atom.xml" type="application/atom+xml" rel="alternate">atom feed</a></li>
+
+                        </ul>
+                </div><!-- /.social -->
+        </section><!-- /#extras -->
+
+        <footer id="contentinfo" class="body">
+                <address id="about" class="vcard body">
+                Proudly powered by <a href="http://getpelican.com/">Pelican</a>, which takes great advantage of <a href="http://python.org">Python</a>.
+                </address><!-- /#about -->
+
+                <p>The theme is by <a href="http://coding.smashingmagazine.com/2009/08/04/designing-a-html-5-layout-from-scratch/">Smashing Magazine</a>, thanks!</p>
+        </footer><!-- /#contentinfo -->
+
+</body>
+</html>

pelican/tests/test_contents.py

@@ -69,11 +69,13 @@ class TestPage(LoggedTestCase):
     def test_mandatory_properties(self):
         # If the title is not set, must throw an exception.
         page = Page('content')
-        with self.assertRaises(NameError):
+        self.assertFalse(page._has_valid_mandatory_properties())
-            page.check_properties()
+        self.assertLogCountEqual(
-
+                count=1,
+                msg="Skipping .*: could not find information about 'title'",
+                level=logging.ERROR)
         page = Page('content', metadata={'title': 'foobar'})
-        page.check_properties()
+        self.assertTrue(page._has_valid_mandatory_properties())
 
     def test_summary_from_metadata(self):
         # If a :summary: metadata is given, it should be used
@@ -503,7 +505,7 @@ class TestArticle(TestPage):
         article_kwargs['metadata']['slug'] = '../foo'
         article_kwargs['settings'] = settings
         article = Article(**article_kwargs)
-        self.assertFalse(article.valid_save_as())
+        self.assertFalse(article._has_valid_save_as())
 
     def test_valid_save_as_detects_breakout_to_root(self):
         settings = get_settings()
@@ -511,7 +513,7 @@ class TestArticle(TestPage):
         article_kwargs['metadata']['slug'] = '/foo'
         article_kwargs['settings'] = settings
         article = Article(**article_kwargs)
-        self.assertFalse(article.valid_save_as())
+        self.assertFalse(article._has_valid_save_as())
 
     def test_valid_save_as_passes_valid(self):
         settings = get_settings()
@@ -519,7 +521,7 @@ class TestArticle(TestPage):
         article_kwargs['metadata']['slug'] = 'foo'
         article_kwargs['settings'] = settings
         article = Article(**article_kwargs)
-        self.assertTrue(article.valid_save_as())
+        self.assertTrue(article._has_valid_save_as())
 
 
 class TestStatic(LoggedTestCase):