diff --git a/CLAUDE.md b/CLAUDE.md index 0bb74af..8d06a39 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,9 +24,11 @@ uv build ### Core Components **octohook/__init__.py** - Entry point providing: -- `load_hooks(modules)` - Recursively imports modules to register webhook handlers -- `model_overrides` - Global dict for extending/replacing model classes -- Exports: `hook`, `handle_webhook`, `parse`, `WebhookEvent`, `WebhookEventAction` +- `setup(modules, model_overrides)` - Configures octohook by loading webhook handlers and registering model overrides. Validates that model overrides inherit from base classes. Raises on import errors. Always calls reset() first to clear existing state. +- `reset()` - Clears all registered hooks, imported modules, and model overrides. Returns octohook to unconfigured state. +- `_model_overrides` - Internal dict for extending/replacing model classes (private, set via setup()) +- `OctohookConfigError` - Exception raised for configuration errors +- Exports: `hook`, `handle_webhook`, `parse`, `setup`, `reset`, `WebhookEvent`, `WebhookEventAction`, `OctohookConfigError` **octohook/decorators.py** - Decorator system (`_WebhookDecorator` class): - `@hook(event, actions, repositories, debug)` - Registers functions as webhook handlers @@ -63,7 +65,7 @@ Handler resolution order: ### Model Override System -Users can extend/replace models via `model_overrides`: +Users can extend/replace models via `setup()`: ```python from octohook.models import PullRequest @@ -72,10 +74,13 @@ class MyPullRequest(PullRequest): def custom_method(self): pass -octohook.model_overrides = {PullRequest: MyPullRequest} +octohook.setup( + modules=["hooks"], + model_overrides={PullRequest: MyPullRequest} +) ``` -When any model is instantiated, `BaseGithubModel.__new__` checks `model_overrides` and substitutes the custom class. This allows adding custom methods/properties to GitHub objects without modifying octohook source. +When any model is instantiated, `BaseGithubModel.__new__` checks `_model_overrides` and substitutes the custom class. This allows adding custom methods/properties to GitHub objects without modifying octohook source. ### Payload Inconsistencies @@ -91,3 +96,5 @@ Octohook uses `Optional` types extensively and the `_optional()` helper to handl - Test fixtures in `tests/fixtures/complete/` contain real GitHub webhook payloads - Hook tests verify that the decorator system correctly routes events to handlers - Model tests verify parsing and URL interpolation +- `tests/conftest.py` provides an autouse fixture that calls `reset()` before/after each test for isolation +- The autouse fixture also clears test module imports from `sys.modules` to ensure decorators re-register on each test diff --git a/README.md b/README.md index 3399415..aa66ce7 100644 --- a/README.md +++ b/README.md @@ -64,7 +64,7 @@ def webhook(): ### @hook Alternatively, you can also let `octohook` do the heavy lifting of finding and executing the appropriate handlers for any given webhook. -The `@hook` decorator takes in four parameters, the `WebhookEvent`, a list of `WebhookEventAction`s, an optional list of repositories and a `debug` flag (defaults to `False`). +The `@hook` decorator takes in four parameters, the `WebhookEvent`, a list of `WebhookEventAction`s, an optional list of repositories and a `debug` flag (defaults to `False`). Any function this decorator is applied to is invoked whenever you receive an event with the specified `WebhookEvent` and a listed `WebhookEventAction`. @@ -82,7 +82,7 @@ def work(event: PullRequestEvent): `work()` is automatically called with the parsed `PullRequestEvent` anytime you receive a webhook event with `X-Github-Event: pull_request` and it has any of the `created` or `edited` actions. -If you don't specify a list of actions, then the function is invoked for _any_ action. For some events like `Push`, which do not have an `action`, take care not to specify any actions in the decorator. +If you don't specify a list of actions, then the function is invoked for _any_ action. For some events like `Push`, which do not have an `action`, take care not to specify any actions in the decorator. #### hooks/do_something.py ```python @@ -105,37 +105,39 @@ import octohook app = Flask(__name__) -octohook.load_hooks(["hooks"]) +# Load webhook handlers from the hooks module +octohook.setup(modules=["hooks"]) @app.route('/webhook', methods=['POST']) def webhook(): github_event = request.headers.get('X-GitHub-Event') - + octohook.handle_webhook(event_name=github_event, payload=request.json) return Response("OK", status=200) ``` -`handle_hooks` goes through all the handlers sequentially and blocks till everything is done. Any exceptions are logged to `logging.getLogger('octohook')`. You can configure the output stream of this logger to capture the logs. +`handle_webhook` goes through all the handlers sequentially and blocks till everything is done. Any exceptions are logged to `logging.getLogger('octohook')`. You can configure the output stream of this logger to capture the logs. ### Model Overrides -`octohook` provides a way to extend/modify the models being provided in the event object. `model_overrides` is a dictionary where you can map `octohook` models to your own. - +`octohook` provides a way to extend/modify the models being provided in the event object. ```python import octohook from octohook.models import PullRequest class MyPullRequest(PullRequest): - + def custom_work(self): pass -octohook.load_hooks(["module_a"]) -octohook.model_overrides = { - PullRequest: MyPullRequest -} +octohook.setup( + modules=["module_a"], + model_overrides={ + PullRequest: MyPullRequest + } +) ``` Now, everytime `octohook` attempts to initialize a `PullRequest` object, it will initialize `MyPullRequest` instead. @@ -145,5 +147,5 @@ Check the [test](tests/test_model_override.py) for example usage. **Note** - The class is initialized with the relevant `payload: dict` data from the incoming event payload. -- It is recommended you subclass the original model class, but it is not required. +- You must subclass the original model class - `setup()` validates this automatically. - Type hints are no longer reliable for the overridden classes. diff --git a/octohook/__init__.py b/octohook/__init__.py index 6225de7..4b7c2a4 100644 --- a/octohook/__init__.py +++ b/octohook/__init__.py @@ -2,7 +2,7 @@ from importlib import import_module from pathlib import Path from pkgutil import walk_packages -from typing import List +from typing import List, Optional, Dict, Type from .decorators import hook, handle_webhook from .events import parse, WebhookEvent, WebhookEventAction @@ -11,10 +11,11 @@ "events", "handle_webhook", "hook", - "load_hooks", "models", - "model_overrides", + "OctohookConfigError", "parse", + "reset", + "setup", "WebhookEvent", "WebhookEventAction", ] @@ -22,15 +23,15 @@ logger = logging.getLogger("octohook") _imported_modules = [] -model_overrides = {} +_model_overrides = {} -def _import_module(module: str) -> List[str]: - try: - imported = import_module(module) - except Exception as e: - logger.error("Failed to import module %s", module, exc_info=e) - return [] +class OctohookConfigError(Exception): + """Raised when octohook configuration is invalid.""" + pass + +def _import_module(module: str) -> List[str]: + imported = import_module(module) module_path = imported.__file__ if module_path.endswith("__init__.py"): @@ -47,14 +48,84 @@ def _import_module(module: str) -> List[str]: imported_modules.append(import_module(module_to_be_imported).__name__) return imported_modules -def load_hooks(modules: List[str]): + +def setup( + *, + modules: List[str], + model_overrides: Optional[Dict[Type, Type]] = None, +) -> None: """ - Iterates through the given modules recursively and imports all the modules to load hook information. + Configure octohook by loading webhook handlers and registering model overrides. + + This function clears any existing configuration via reset(), then recursively + imports the specified modules to discover and register all decorated webhook + handlers. - @param modules List of modules to be imported. The modules cannot be relative. For example, you may use - "module_a.module_b" or "module_a", but not ".module_a" or "module_a/module_b" + Args: + modules: List of fully-qualified module paths containing webhook handlers. + Modules are imported recursively. Cannot use relative imports. + model_overrides: Optional mapping of base model classes to custom subclasses. + All custom classes are validated to ensure they inherit from + the base class they override. + + Raises: + ImportError: If any specified module cannot be imported. + TypeError: If a model override is not a class or not a subclass of the base model. + + Example: + >>> import octohook + >>> from octohook.models import PullRequest + >>> + >>> class CustomPullRequest(PullRequest): + ... pass + >>> + >>> octohook.setup( + ... modules=["hooks.github", "hooks.slack"], + ... model_overrides={PullRequest: CustomPullRequest} + ... ) """ - global _imported_modules + global _imported_modules, _model_overrides + + reset() + + if model_overrides: + for base_class, override_class in model_overrides.items(): + if not isinstance(override_class, type): + raise TypeError( + f"Model override for {base_class.__name__} must be a class, " + f"got {type(override_class).__name__}" + ) + if not issubclass(override_class, base_class): + raise TypeError( + f"Model override {override_class.__name__} must be a subclass of " + f"{base_class.__name__}" + ) + + _model_overrides = model_overrides.copy() for module in modules: _imported_modules.extend(_import_module(module)) + + +def reset() -> None: + """ + Clear all octohook configuration and return to unconfigured state. + + Removes all registered webhook handlers, clears the list of imported modules, + and removes all model overrides. After calling reset(), setup() must be called + again before handling webhooks. + + This function is automatically called by setup() to ensure a clean configuration. + It can also be called directly to clear octohook state. + + Example: + >>> import octohook + >>> octohook.reset() + >>> octohook.setup(modules=["hooks"]) + """ + global _imported_modules, _model_overrides + from octohook.decorators import _decorator + + _decorator.handlers.clear() + _imported_modules.clear() + _model_overrides.clear() diff --git a/octohook/models.py b/octohook/models.py index e6c41c2..b1f206d 100644 --- a/octohook/models.py +++ b/octohook/models.py @@ -29,9 +29,9 @@ def _transform(url: str, local_variables: dict) -> str: class BaseGithubModel(ABC): def __new__(cls, *args, **kwargs): - from octohook import model_overrides + from octohook import _model_overrides - cls = model_overrides.get(cls) or cls + cls = _model_overrides.get(cls) or cls return object.__new__(cls) diff --git a/tests/conftest.py b/tests/conftest.py index 16d7d14..a189ecc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -9,9 +9,30 @@ from typing import List, Dict, Any import pytest +import octohook from octohook.decorators import _WebhookDecorator +@pytest.fixture(autouse=True) +def cleanup_test_modules(): + """ + Clean up test module imports after each test. + + Removes test hook modules from sys.modules to ensure decorators re-register + on each test. This is necessary because @hook decorators execute at import time, + and without cleanup, Python won't re-execute them when modules are imported again. + """ + import sys + + original_modules = set(sys.modules.keys()) + + yield + + for module_name in list(sys.modules.keys()): + if module_name not in original_modules and module_name.startswith("tests.hooks"): + sys.modules.pop(module_name, None) + + @pytest.fixture def isolated_decorator(): """ diff --git a/tests/test_decorator.py b/tests/test_decorator.py index af4e9b2..fcde6dc 100644 --- a/tests/test_decorator.py +++ b/tests/test_decorator.py @@ -1,10 +1,8 @@ -import sys - import pytest import octohook import octohook.decorators -from octohook import load_hooks +from octohook import setup from octohook.decorators import _WebhookDecorator from octohook.events import WebhookEvent, WebhookEventAction from tests.hooks import _tracker @@ -24,86 +22,57 @@ EXPECTED_HANDLE_HOOKS_ONLY = 10 # Total hooks excluding debug_hooks module -@pytest.fixture(autouse=True) -def clean_imported_modules(monkeypatch): - """ - Clean up module imports after each test to prevent test pollution. - - This fixture: - 1. Saves current state of imported modules - 2. Resets octohook._imported_modules before test - 3. Cleans up sys.modules after test - - Using autouse=True applies this to all tests in this file automatically. - """ - # Save current imported modules - original_modules = set(sys.modules.keys()) - - # Reset octohook's module tracking - monkeypatch.setattr(octohook, "_imported_modules", []) - - yield # Run the test - - # Clean up any modules imported during test - for module_name in list(sys.modules.keys()): - if module_name not in original_modules and module_name.startswith("tests.hooks"): - sys.modules.pop(module_name, None) - - # Reset module tracking again - octohook._imported_modules = [] - - -def test_load_hooks_calls_hook(mocker): +def test_setup_calls_hook(mocker): """ - Verify that load_hooks() correctly discovers and registers all hooks in test modules. + Verify that setup() correctly discovers and registers all hooks in test modules. The tests/hooks directory contains 22 total hooks across 6 files. This test - ensures all are discovered when load_hooks() is called with the parent module. + ensures all are discovered when setup() is called with the parent module. """ mock = mocker.patch("octohook.decorators.hook") - load_hooks(["tests.hooks"]) + setup(modules=["tests.hooks"]) assert mock.call_count == EXPECTED_TOTAL_HOOKS -def test_load_hooks_only_parses_specified_modules(mocker): +def test_setup_only_parses_specified_modules(mocker): """ - Verify that load_hooks() only loads hooks from specified modules. + Verify that setup() only loads hooks from specified modules. Tests that when a specific submodule is provided (e.g., tests.hooks.debug_hooks), only hooks from that module are loaded, not from sibling or parent modules. """ mock = mocker.patch("octohook.decorators.hook") - load_hooks(["tests.hooks.debug_hooks"]) + setup(modules=["tests.hooks.debug_hooks"]) assert mock.call_count == EXPECTED_DEBUG_HOOKS_ONLY -def test_load_hooks_parses_python_module(mocker): +def test_setup_parses_python_module(mocker): """ - Verify that load_hooks() can load a single Python module file. + Verify that setup() can load a single Python module file. - Tests that load_hooks() works with fully-qualified module paths pointing + Tests that setup() works with fully-qualified module paths pointing to individual Python files, not just packages. """ mock = mocker.patch("octohook.decorators.hook") - load_hooks(["tests.hooks.debug_hooks.label"]) + setup(modules=["tests.hooks.debug_hooks.label"]) assert mock.call_count == EXPECTED_DEBUG_HOOKS_ONLY -def test_load_hooks_parses_properly(mocker): +def test_setup_parses_properly(mocker): """ Verify that hooks are correctly organized in the three-level filtering structure. Tests the internal handler storage structure: event → action → repo → handlers. This ensures the filtering system (specific action + specific repo, wildcard action, - wildcard repo, etc.) is set up correctly after load_hooks() runs. + wildcard repo, etc.) is set up correctly after setup() runs. """ decorator = _WebhookDecorator() mocker.patch("octohook.decorators.hook", side_effect=decorator.webhook) - load_hooks(["tests.hooks"]) + setup(modules=["tests.hooks"]) handlers = decorator.handlers @@ -129,18 +98,18 @@ def test_load_hooks_parses_properly(mocker): assert len(review[WebhookEventAction.SUBMITTED]["doodla/octohook-playground2"]) == 1 -def test_calling_load_hooks_multiple_times_does_not_have_side_effects(mocker): +def test_calling_setup_multiple_times_does_not_have_side_effects(mocker): """ - Verify that calling load_hooks() multiple times doesn't register hooks multiple times. + Verify that calling setup() multiple times doesn't register hooks multiple times. - Tests that load_hooks() is idempotent - calling it repeatedly with the same + Tests that setup() is idempotent - calling it repeatedly with the same modules doesn't result in duplicate hook registrations or other side effects. """ mock = mocker.patch("octohook.decorators.hook") - load_hooks(["tests.hooks"]) - load_hooks(["tests.hooks"]) - load_hooks(["tests.hooks"]) + setup(modules=["tests.hooks"]) + setup(modules=["tests.hooks"]) + setup(modules=["tests.hooks"]) assert mock.call_count == EXPECTED_TOTAL_HOOKS @@ -159,7 +128,7 @@ def test_handle_hooks(mocker, fixture_loader): decorator = _WebhookDecorator() mocker.patch("octohook.decorators.hook", side_effect=decorator.webhook) - load_hooks(["tests.hooks.handle_hooks"]) + setup(modules=["tests.hooks.handle_hooks"]) # Expected hooks for each action (after removing inner/ directory) assertions = { @@ -239,7 +208,7 @@ def test_debug_hooks_are_handled(mocker, fixture_loader): decorator = _WebhookDecorator() mocker.patch("octohook.decorators.hook", side_effect=decorator.webhook) - load_hooks(["tests.hooks"]) + setup(modules=["tests.hooks"]) # LabelEvent has `debug=True`. Only debug hooks should be fired. label_events = fixture_loader.load("label") diff --git a/tests/test_edge_cases.py b/tests/test_edge_cases.py index a873fb5..45a7d1b 100644 --- a/tests/test_edge_cases.py +++ b/tests/test_edge_cases.py @@ -159,7 +159,7 @@ class MyUser(User): def custom_field(self): return "custom" - monkeypatch.setattr(octohook, "model_overrides", {User: MyUser}) + monkeypatch.setattr(octohook, "_model_overrides", {User: MyUser}) # Load fixture payload = fixture_loader.load("pull_request")[0] diff --git a/tests/test_model_override.py b/tests/test_model_override.py index b48fcac..f6d794c 100644 --- a/tests/test_model_override.py +++ b/tests/test_model_override.py @@ -20,7 +20,7 @@ def model_override(monkeypatch): the standard PullRequest class, allowing us to test the model override system. """ - monkeypatch.setattr(octohook, "model_overrides", { + monkeypatch.setattr(octohook, "_model_overrides", { PullRequest: MyPullRequest, }) return MyPullRequest diff --git a/tests/test_setup.py b/tests/test_setup.py new file mode 100644 index 0000000..d30d1e4 --- /dev/null +++ b/tests/test_setup.py @@ -0,0 +1,147 @@ +""" +Tests for octohook.setup() and octohook.reset() functions. + +Verifies webhook handler loading, model override validation, and state management. +""" +import pytest + +import octohook +from octohook import setup, reset, OctohookConfigError +from octohook.models import PullRequest +from octohook.events import WebhookEvent, WebhookEventAction + + +class CustomPullRequest(PullRequest): + """Test custom model class.""" + @property + def custom_property(self): + return "custom" + + +class InvalidClass: + """Not a subclass of any model.""" + pass + + +def test_setup_loads_modules(): + """Verify that setup() loads hooks from specified modules.""" + setup(modules=["tests.hooks.handle_hooks"]) + + from octohook.decorators import _decorator + assert len(_decorator.handlers) > 0 + + +def test_setup_with_model_overrides(): + """Verify that setup() applies model overrides correctly.""" + setup( + modules=["tests.hooks.handle_hooks"], + model_overrides={PullRequest: CustomPullRequest} + ) + + assert octohook._model_overrides[PullRequest] == CustomPullRequest + + +def test_setup_validates_model_override_is_subclass(): + """Verify that setup() validates model overrides are subclasses.""" + with pytest.raises(TypeError, match="must be a subclass"): + setup( + modules=["tests.hooks.handle_hooks"], + model_overrides={PullRequest: InvalidClass} + ) + + +def test_setup_validates_model_override_is_class(): + """Verify that setup() validates model overrides are classes.""" + with pytest.raises(TypeError, match="must be a class"): + setup( + modules=["tests.hooks.handle_hooks"], + model_overrides={PullRequest: "not a class"} + ) + + +def test_setup_raises_on_invalid_module(): + """Verify that setup() raises ImportError for invalid modules.""" + with pytest.raises(ModuleNotFoundError): + setup(modules=["nonexistent.module"]) + + +def test_setup_multiple_calls_replaces_hooks(): + """Verify that calling setup() multiple times replaces hooks.""" + setup(modules=["tests.hooks.handle_hooks.label"]) + + from octohook.decorators import _decorator + first_count = len(_decorator.handlers) + + # Second call should reset and load new hooks + setup(modules=["tests.hooks.debug_hooks"]) + + second_count = len(_decorator.handlers) + + # Should have different hooks loaded (debug_hooks only has LABEL event) + assert WebhookEvent.LABEL in _decorator.handlers + + +def test_reset_clears_hooks(): + """Verify that reset() clears all registered hooks.""" + setup(modules=["tests.hooks.handle_hooks"]) + + from octohook.decorators import _decorator + assert len(_decorator.handlers) > 0 + + reset() + + assert len(_decorator.handlers) == 0 + + +def test_reset_clears_model_overrides(): + """Verify that reset() clears model overrides.""" + setup( + modules=["tests.hooks.handle_hooks"], + model_overrides={PullRequest: CustomPullRequest} + ) + + assert len(octohook._model_overrides) > 0 + + reset() + + assert len(octohook._model_overrides) == 0 + + +def test_reset_clears_imported_modules(): + """Verify that reset() clears imported modules tracking.""" + setup(modules=["tests.hooks.handle_hooks"]) + + assert len(octohook._imported_modules) > 0 + + reset() + + assert len(octohook._imported_modules) == 0 + + +def test_reset_allows_setup_again(): + """Verify that reset() allows setup() to be called again without warning.""" + setup(modules=["tests.hooks.handle_hooks"]) + + reset() + + # Should not warn since we explicitly reset + setup(modules=["tests.hooks.debug_hooks"]) + + from octohook.decorators import _decorator + assert len(_decorator.handlers) > 0 + + +def test_setup_and_handle_webhook(fixture_loader): + """Integration test: setup() -> handle_webhook().""" + from tests.hooks import _tracker + + setup(modules=["tests.hooks.handle_hooks"]) + + payloads = fixture_loader.load("label") + created_payload = [p for p in payloads if p["action"] == "created"][0] + + _tracker.reset() + octohook.handle_webhook("label", created_payload) + + calls = _tracker.get_calls() + assert len(calls) > 0