From 089b46b7eb86cb55491d3c8e8bccb956b6dceeff Mon Sep 17 00:00:00 2001 From: winlu Date: Mon, 24 Jul 2017 19:01:14 +0200 Subject: [PATCH] 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 c4b184fa32f73a737ff9bade440ce0f7914dc350 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 | 108 +++++++++++------- pelican/generators.py | 54 +++------ pelican/settings.py | 1 - .../output/basic/drafts/a-draft-article.html | 69 +++++++++++ pelican/tests/test_contents.py | 16 +-- 5 files changed, 159 insertions(+), 89 deletions(-) create mode 100644 pelican/tests/output/basic/drafts/a-draft-article.html diff --git a/pelican/contents.py b/pelican/contents.py index 3d1128c9..15770fc8 100644 --- a/pelican/contents.py +++ b/pelican/contents.py @@ -138,14 +138,7 @@ class Content(object): # manage status if not hasattr(self, 'status'): - self.status = settings['DEFAULT_STATUS'] - 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' + self.status = getattr(self, 'default_status', None) # 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): - fq_key = ('%s_%s' % (self.__class__.__name__, key)).upper() + def _expand_settings(self, key, klass=None): + 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): - mandatory_properties = ('title', 'category') - default_template = 'article' + # handle WITH_FUTURE_DATES (designate article to draft based on date) + if not self.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' + # 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): - base_properties = ('author', 'date') + def _expand_settings(self, key): + 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 diff --git a/pelican/generators.py b/pelican/generators.py index f3590155..eb97c115 100644 --- a/pelican/generators.py +++ b/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) - if article_or_draft is None: - # TODO needs overhaul, maybe nomad for read_file - # solution, unified behaviour + article = self.get_cached_data(f, None) + if article is None: 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": - 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) - self.cache_data(f, article_or_draft) - - if article_or_draft.status.lower() == "published": - all_articles.append(article_or_draft) - else: - all_drafts.append(article_or_draft) - self.add_source_path(article_or_draft) + if article.status == "published": + all_articles.append(article) + elif article.status == "draft": + all_drafts.append(article) + self.add_source_path(article) 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): - 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) + if not page.is_valid(): 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) diff --git a/pelican/settings.py b/pelican/settings.py index 831e1851..d1249a24 100644 --- a/pelican/settings.py +++ b/pelican/settings.py @@ -126,7 +126,6 @@ DEFAULT_CONFIG = { 'FILENAME_METADATA': r'(?P\d{4}-\d{2}-\d{2}).*', 'PATH_METADATA': '', 'EXTRA_PATH_METADATA': {}, - 'DEFAULT_STATUS': 'published', 'ARTICLE_PERMALINK_STRUCTURE': '', 'TYPOGRIFY': False, 'TYPOGRIFY_IGNORE_TAGS': [], diff --git a/pelican/tests/output/basic/drafts/a-draft-article.html b/pelican/tests/output/basic/drafts/a-draft-article.html new file mode 100644 index 00000000..64d56ff9 --- /dev/null +++ b/pelican/tests/output/basic/drafts/a-draft-article.html @@ -0,0 +1,69 @@ + + + + + A draft article + + + + + + + + +
+
+
+

+ A draft article

+
+ +
+
+ + Published: + + +

In misc.

+ +

This is a draft article, it should live under the /drafts/ folder and not be +listed anywhere else.

+ +
+ +
+
+
+ +
+ + + + + \ No newline at end of file diff --git a/pelican/tests/test_contents.py b/pelican/tests/test_contents.py index 56928b81..d028c7a1 100644 --- a/pelican/tests/test_contents.py +++ b/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): - page.check_properties() - + self.assertFalse(page._has_valid_mandatory_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):