Do not treat handlers actions that have not been implemented as a dispatch failure

[mishaschwartz]

If a handler raises a NotImplementedError for a given action, Magpie will consider that a webhook failure.

This is misleading because the webhook didn't actually fail, there was simply nothing to do since the action isn't implemented.

This PR changes the webhook dispatcher to treat handler actions that raise NotImplementedErrors as a noop instead of as an error.

For example:

• when a user is created, the Nginx.user_created handler is triggered, this method raises a NotImplementedError which is reported to Magpie as a webhook failure.
• Admins who are then looking at the user status think that there is a cowbird error and that the user was not properly created. In fact, the user was properly created since all webhooks that are implemented ran with no errors.

Other possible solutions to this problem:

• instead of raising a NotImplementedError, concrete implementations of the Handler class can simply pass instead
• instead of raising a NotImplementedError, abstract methods in the Handler class can simply pass instead (all concrete implementations can then optionally override these methods if needed)

[fmigneault]

I'd argue that if a webhook was configured to point at some action leading to an NotImplementedError, this is potentially not the intended action/resolution.

I would prefer the other proposed solution that the concrete implementation does an explicit no-op. It could also log that it was a no-op, so there is some traceability of the invoked handlers and actions performed.

The NotImplementedError are there really to raise that missing implementation, which was identified here because it was not silenced.

[mishaschwartz]

@fmigneault

Alright, I've updated the concrete classes. Let me know what you think.

Also: is debug an appropriate log level?

[fmigneault]

Oh wow. I didn't realize there were this many still unimplemented.

debug an appropriate log level

Mixed answer.

I'd say the ones with FIXME should probably be WARN since they should really be implemented at some point to "fix" misfired events to retry via Cowbird's API.

The ones under Magpie should probably be ERROR and even leave the intended raise NotImplementedError, since I don't see why Magpie user create/delete events triggered by Magpie would send a webhook to Cowbird. That is almost certainly a misconfiguration.

The others with INFO, since it can make sense that they would have "nothing to do", depending on the integration each service/event combination implies with the others, but we should still inform about the events happening so it's easy to tail the Cowbird logs and inspect the operations performed, without need to change the default logging level.

Personally, I would leave DEBUG only for cases where debugging is involved, such as intermediate steps and such.

[mishaschwartz]

I'll update the log levels to your suggestions.

leave the intended raise NotImplementedError, since I don't see why Magpie user create/delete events triggered by Magpie would send a webhook to Cowbird. That is almost certainly a misconfiguration.

Ok but I don't know how you can configure this so that Magpie user/delete doesn't trigger when a user is created/deleted. Is it possible to enable a handler only for certain events but not others?

If not, then we'll end up with the exact situation that we're trying to avoid... an error is raised every time a user is created or deleted and Magpie thinks that the webhook failed.

[fmigneault]

@mishaschwartz
Good point. If you can validate that the current configuration when creating a Magpie user ends up calling Cowbird's Magpie handler "on user create" event, then using an appropriate WARN message indicating why it is discarded instead of the raise NotImplementedError is adequate.

[mishaschwartz]

Yes I can validate that this is the case.

From the cowbird logs when a user is created:

[2024-12-18 18:05:46,960] INFO       [ThreadPoolExecutor-0_0][cowbird.api.webhooks.views] Dispatching event [Handler) -> None] for handler [Magpie].
[2024-12-18 18:05:46,960] ERROR      [ThreadPoolExecutor-0_0][cowbird.api.webhooks.views] Exception raised while handling event [Handler) -> None] for handler [Magpie] : [NotImplementedError()].
Traceback (most recent call last):
  File "/opt/local/src/cowbird/cowbird/api/webhooks/views.py", line 35, in dispatch
    handler_fct(handler)
  File "/opt/local/src/cowbird/cowbird/api/webhooks/views.py", line 66, in handler_fct
    handler.user_created(user_name=user_name)
  File "/opt/local/src/cowbird/cowbird/handlers/impl/magpie.py", line 293, in user_created
    raise NotImplementedError
NotImplementedError

[fmigneault]

https://github.com/Ouranosinc/cowbird/tree/2.5.0 is pushed with the merged PR
the docker should be ready soon enough