Fix automatic webhooks

[siscia]

I want to ping back on #99 and #100 and #101

My expectations from reading the documentation was that if I imported from django_github_app.events.installation import gh then the handler would be automagically installed.

From that moment on, any new installation received over webhook would trigger the whole flow adding installation and repository.

I was not able to reproduce this.

I track down the issue to the GitHubRouter and how it interact with the WebhookViews.

My understanding is that the whole GitHubRouter is suppose to be a singleton in this architecture, and my lead here is the _routers defined as class member.

However, this doesn't quite work as expected. Mostly because the fetch method of the GitHubRouter completely ignores the _routers method and act only on itself.

This can be verified by the test: tests/test_routing.py::TestGitHubRouter::test_importing_module_add_handler added in this PR.

The test fail as it is.

The solution that I see would it be to update the GitHubRouter::fetch method.

This pass the new test.

However, the new tests fails when we run the whole battery of tests.
I trace this back to a failure when the new tests runs together with the tests/events/test_installation.py.

A dive a bit deeper and the failure happens whenever we import django_github_app.events.installation in the tests.
Didn't have time to investigate further.

[joshuadavidthomas]

My expectations from reading the documentation was that if I imported from django_github_app.events.installation import gh then the handler would be automagically installed.

Where are you importing this? In an AppConfig.ready() method, at module level in a view, or somewhere else?

The built-in handlers for installation events are automatically loaded by the library's own AppConfig.ready() - you shouldn't need to import them manually at all, provided you've installed the library in your settings.INSTALLED_APPS.

The pattern for custom handlers is documented here (see the AppConfig.ready() example near the end of that section).

All that said, if the docs gave you the impression you needed to manually import the built-in handlers, that's a gap worth addressing.

From that moment on, any new installation received over webhook would trigger the whole flow adding installation and repository.

I was not able to reproduce this.

Can you describe what you actually tried? What does your project structure look like, and what was the failure mode? (e.g., webhooks received but handlers not called, errors in logs, nothing happening, etc.)

My understanding is that the whole GitHubRouter is suppose to be a singleton in this architecture, and my lead here is the _routers defined as class member.

The _routers class variable tracks all router instances, but the current design isn't a singleton - handlers are copied into the view's router at construction time, similar to Django signals. The expectation is that handler modules are imported via AppConfig.ready() before the webhook view's router is created.

I'm open to reconsidering whether this is the right architecture. The signals-style pattern works, but it's clearly not obvious from the docs or API. This library emerged from an internal work project and I basically just went with the first obvious idea with an existing Django convention.

This can be verified by the test: tests/test_routing.py::TestGitHubRouter::test_importing_module_add_handler added in this PR.

The test fail as it is.

The test creates a new SyncWebhookView(), but view.router returns a module-level _router that was created when views.py was first imported - not a fresh router. Can you share more about your actual project setup where you ran into this issue? That would help me understand if there's a real-world scenario the current design doesn't handle well.

[joshuadavidthomas]

I should also note, this sounds related to #116, which I also haven't been able to reproduce or gotten any clarification from the issue reporter about. If you (or @jensenbox) could provide a minimal reproduction or more details about your Django project structure, that would really help me understand what's going wrong.

[joshuadavidthomas]

So, after poking around, writing a repro of my own from what I could glean from your PR description -- I think there is a timing issue related to the module-level _router and the fact that it's not a true singleton. You can see the fix in #132.

I'll cut a new release here at some point in the next little bit, if you could install and give that version a try (v0.10.0 since I had an existing new unreleased feature) and let me know! I'm going to go ahead and close this for the time being, feel free to re-open (or open an issue) if that doesn't fix your issue.