From c6b02f0e5ecbf1fa6b84ce22e9e03f40d1395540 Mon Sep 17 00:00:00 2001 From: Stuart Maxwell Date: Wed, 4 Dec 2024 19:52:55 +1300 Subject: [PATCH 1/5] Custom exception for PluginLoadError --- src/djpress/exceptions.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/djpress/exceptions.py b/src/djpress/exceptions.py index 98b8937..fcd458c 100644 --- a/src/djpress/exceptions.py +++ b/src/djpress/exceptions.py @@ -7,3 +7,7 @@ class PostNotFoundError(Exception): class PageNotFoundError(Exception): """Exception raised when the page is not found in the database.""" + + +class PluginLoadError(Exception): + """Raised when plugins cannot be loaded.""" From 22e3fede4da0dfa2263336d19025550655948270 Mon Sep 17 00:00:00 2001 From: Stuart Maxwell Date: Wed, 4 Dec 2024 19:53:22 +1300 Subject: [PATCH 2/5] Better error handling for plugins --- src/djpress/plugins.py | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/djpress/plugins.py b/src/djpress/plugins.py index a2ab7a4..2b41941 100644 --- a/src/djpress/plugins.py +++ b/src/djpress/plugins.py @@ -7,6 +7,7 @@ from django.utils.module_loading import import_string from djpress.conf import settings as djpress_settings +from djpress.exceptions import PluginLoadError # Hook definitions @@ -74,7 +75,16 @@ def run_hook(self, hook_name: Hooks, value: Any = None, *args: Any, **kwargs: An if hook_name in self.hooks: for callback in self.hooks[hook_name]: - value = callback(value, *args, **kwargs) + # Ruff warns us about performance issues running a try/except block in a loop, but I think this is + # acceptable since there won't be many hooks, and we don't want one crashing plugin to affect all the + # plugins. With this approach, if one fails, the `value` won't be modified and we'll continue to the + # next one. + try: + callback_value = callback(value, *args, **kwargs) + value = callback_value + except Exception: # noqa: BLE001, PERF203, S112 + # Continue with the next callback + continue return value @@ -93,12 +103,16 @@ def load_plugins(self) -> None: plugin_names: list = djpress_settings.PLUGINS plugin_settings: dict = djpress_settings.PLUGIN_SETTINGS - for plugin_path in plugin_names: - plugin_class = self._import_plugin_class(plugin_path) - plugin = self._instantiate_plugin(plugin_class, plugin_path, plugin_settings) - self.plugins.append(plugin) + try: + for plugin_path in plugin_names: + plugin_class = self._import_plugin_class(plugin_path) + plugin = self._instantiate_plugin(plugin_class, plugin_path, plugin_settings) + self.plugins.append(plugin) - self._loaded = True + self._loaded = True + except Exception as exc: + msg = f"Failed to load plugins: {exc}" + raise PluginLoadError(msg) from exc def _import_plugin_class(self, plugin_path: str) -> type: """Import the plugin class from either custom path or standard location. From df51766e28570e6c0cfcf0c5957143de1b0fda93 Mon Sep 17 00:00:00 2001 From: Stuart Maxwell Date: Wed, 4 Dec 2024 19:54:23 +1300 Subject: [PATCH 3/5] Run POST_SAVE_POST hook after transaction is committed --- src/djpress/models/post.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/djpress/models/post.py b/src/djpress/models/post.py index 6f4fdfd..6965fda 100644 --- a/src/djpress/models/post.py +++ b/src/djpress/models/post.py @@ -7,6 +7,7 @@ from django.core.exceptions import ValidationError from django.db import models from django.db.models import Max +from django.db.transaction import on_commit from django.utils import timezone from django.utils.text import slugify @@ -470,9 +471,9 @@ def save(self: "Post", *args, **kwargs) -> None: # noqa: ANN002, ANN003 self.full_clean() super().save(*args, **kwargs) - # If the post is a post and it's published, run the post_save_post hook + # If the post is a post and it's published, run the post_save_post hook after the transaction is committed if self.post_type == "post" and self.is_published: - registry.run_hook(Hooks.POST_SAVE_POST, self) + on_commit(lambda: registry.run_hook(Hooks.POST_SAVE_POST, self)) def clean(self) -> None: """Custom validation for the Post model.""" From 5e4a6c91570e7890b97764c6ce8bf89c56c89d5c Mon Sep 17 00:00:00 2001 From: Stuart Maxwell Date: Wed, 4 Dec 2024 20:09:14 +1300 Subject: [PATCH 4/5] Fix date formats in tests --- tests/test_views.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/test_views.py b/tests/test_views.py index 26b6574..88dd401 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -213,7 +213,7 @@ def test_date_archives_year_no_posts(client, test_post1): def test_date_archives_month(client, settings, test_post1): assert settings.DJPRESS_SETTINGS["ARCHIVE_PREFIX"] == "test-url-archives" url = get_archives_url(test_post1.date.year, test_post1.date.month) - assert url == f"/test-url-archives/{test_post1.date.year}/{test_post1.date.month}/" + assert url == f"/test-url-archives/{test_post1.date.year}/{test_post1.date.month:02}/" response = client.get(url) assert response.status_code == 200 @@ -223,7 +223,7 @@ def test_date_archives_month(client, settings, test_post1): settings.DJPRESS_SETTINGS["ARCHIVE_PREFIX"] = "" url = get_archives_url(test_post1.date.year, test_post1.date.month) - assert url == f"/{test_post1.date.year}/{test_post1.date.month}/" + assert url == f"/{test_post1.date.year}/{test_post1.date.month:02}/" response = client.get(url) assert response.status_code == 200 @@ -256,7 +256,7 @@ def test_date_archives_month_no_posts(client, test_post1): def test_date_archives_day(client, settings, test_post1): assert settings.DJPRESS_SETTINGS["ARCHIVE_PREFIX"] == "test-url-archives" url = get_archives_url(test_post1.date.year, test_post1.date.month, test_post1.date.day) - assert url == f"/test-url-archives/{test_post1.date.year}/{test_post1.date.month}/{test_post1.date.day}/" + assert url == f"/test-url-archives/{test_post1.date.year}/{test_post1.date.month:02}/{test_post1.date.day:02}/" response = client.get(url) assert response.status_code == 200 @@ -266,7 +266,7 @@ def test_date_archives_day(client, settings, test_post1): settings.DJPRESS_SETTINGS["ARCHIVE_PREFIX"] = "" url = get_archives_url(test_post1.date.year, test_post1.date.month, test_post1.date.day) - assert url == f"/{test_post1.date.year}/{test_post1.date.month}/{test_post1.date.day}/" + assert url == f"/{test_post1.date.year}/{test_post1.date.month:02}/{test_post1.date.day:02}/" response = client.get(url) assert response.status_code == 200 @@ -277,7 +277,7 @@ def test_date_archives_day(client, settings, test_post1): assert settings.DJPRESS_SETTINGS["POST_PREFIX"] == "test-posts" settings.DJPRESS_SETTINGS["POST_PREFIX"] = "{{ year }}/{{ month }}/{{ day }}" url = get_archives_url(test_post1.date.year, test_post1.date.month, test_post1.date.day) - assert url == f"/{test_post1.date.year}/{test_post1.date.month}/{test_post1.date.day}/" + assert url == f"/{test_post1.date.year}/{test_post1.date.month:02}/{test_post1.date.day:02}/" response = client.get(url) assert response.status_code == 200 @@ -301,7 +301,7 @@ def test_conflict_day_archives_and_single_post(client, settings, test_post1): settings.DJPRESS_SETTINGS["ARCHIVE_PREFIX"] = "" settings.DJPRESS_SETTINGS["POST_PREFIX"] = "{{ year }}/{{ month }}" url = get_archives_url(test_post1.date.year, test_post1.date.month, test_post1.date.day) - assert url == f"/{test_post1.date.year}/{test_post1.date.month}/{test_post1.date.day}/" + assert url == f"/{test_post1.date.year}/{test_post1.date.month:02}/{test_post1.date.day:02}/" response = client.get(url) assert response.status_code == 200 @@ -322,7 +322,7 @@ def test_conflict_month_archives_and_single_post(client, settings, test_post1): settings.DJPRESS_SETTINGS["ARCHIVE_PREFIX"] = "" settings.DJPRESS_SETTINGS["POST_PREFIX"] = "{{ year }}" url = get_archives_url(test_post1.date.year, test_post1.date.month) - assert url == f"/{test_post1.date.year}/{test_post1.date.month}/" + assert url == f"/{test_post1.date.year}/{test_post1.date.month:02}/" response = client.get(url) assert response.status_code == 200 From 8d605ea341e30b65e6ea6e349488618623d8e2a8 Mon Sep 17 00:00:00 2001 From: Stuart Maxwell Date: Wed, 4 Dec 2024 20:47:43 +1300 Subject: [PATCH 5/5] Add tests for coverage --- tests/test_plugins.py | 60 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 58 insertions(+), 2 deletions(-) diff --git a/tests/test_plugins.py b/tests/test_plugins.py index 82c1e3f..3ea4b65 100644 --- a/tests/test_plugins.py +++ b/tests/test_plugins.py @@ -1,6 +1,7 @@ import pytest from djpress.plugins import PluginRegistry, DJPressPlugin, Hooks from djpress.plugins import registry +from djpress.exceptions import PluginLoadError from djpress.conf import settings as djpress_settings @@ -72,8 +73,35 @@ def test_callback(content: str) -> str: return content + " modified" registry.register_hook(Hooks.PRE_RENDER_CONTENT, test_callback) - result = registry.run_hook(Hooks.PRE_RENDER_CONTENT, "test") - assert result == "test modified" + result = registry.run_hook(Hooks.PRE_RENDER_CONTENT, "test value") + assert result == "test value modified" + + +# Tests for hook execution (strict) +def test_run_hook_with_multiple_hooks(clean_registry): + """Test running hook with multiple hooks.""" + + def test_callback(content: str) -> str: + return content + " modified by test_callback" + + def second_test_callback(content: str) -> str: + return content + " modified by second test_callback" + + registry.register_hook(Hooks.PRE_RENDER_CONTENT, test_callback) + registry.register_hook(Hooks.PRE_RENDER_CONTENT, second_test_callback) + result = registry.run_hook(Hooks.PRE_RENDER_CONTENT, "test value") + assert result == "test value modified by test_callback modified by second test_callback" + + +def test_run_hook_with_exception(clean_registry): + """Test running hook with an exception doesn't modify the value.""" + + def test_callback(content: str) -> str: + raise ValueError("Test error") + + registry.register_hook(Hooks.PRE_RENDER_CONTENT, test_callback) + result = registry.run_hook(Hooks.PRE_RENDER_CONTENT, "test value") + assert result == "test value" def test_run_hook_with_string_fails(clean_registry): @@ -276,6 +304,34 @@ def mock_instantiate(self, plugin_class, path, settings): assert isinstance(registry.plugins[0], TestPlugin) +def test_load_plugins_exception(clean_registry, monkeypatch): + """Test plugin loading with exception.""" + + class TestPlugin(DJPressPlugin): + name = "test" + + def setup(self, registry): + pass + + # Mock the import and instantiate methods to return known values + def mock_import(self, path): + return TestPlugin + + def mock_instantiate(self, plugin_class, path, settings): + raise RuntimeError("Plugin setup failed") + + monkeypatch.setattr(PluginRegistry, "_import_plugin_class", mock_import) + monkeypatch.setattr(PluginRegistry, "_instantiate_plugin", mock_instantiate) + + # Set up test settings + monkeypatch.setattr(djpress_settings, "PLUGINS", ["test_plugin"]) + monkeypatch.setattr(djpress_settings, "PLUGIN_SETTINGS", {}) + + # Load plugins - will raise exception + with pytest.raises(PluginLoadError): + registry.load_plugins() + + def test_plugin_missing_name(): """Test plugin initialization with missing name."""