Type webhook methods #307
Type webhook methods #307
Conversation
| class WebhookModel(WorkOSModel, Generic[EventPayload]): | ||
| """Representation of an Webhook delivered via Webhook. | ||
| Attributes: | ||
| OBJECT_FIELDS (list): List of fields an Webhook is comprised of. | ||
| """ | ||
|
|
||
| id: str | ||
| data: EventPayload | ||
| created_at: str |
There was a problem hiding this comment.
Super-sad panda 😿
There is a difference between our webhooks and events. Each event contains object: 'event'. Our webhooks do not.
A mini future us project should be to add this object attribute to webhook bodies so that they're identical and we can remove this sort of duplication.
| import hashlib | ||
|
|
||
| WebhookPayload = Union[bytes, bytearray] | ||
| WebhookTypeAdapter: TypeAdapter[Webhook] = TypeAdapter(Webhook) |
There was a problem hiding this comment.
Webhook is a discriminated union, not a model. So we need to create a TypeAdapter to validate the incoming payload.
| Webhooks.verify_header(self, payload, sig_header, secret, tolerance) | ||
| event = json.loads(payload, object_pairs_hook=OrderedDict) |
There was a problem hiding this comment.
Got rid of this and just relying on pydantic's JSON parser (looks like jiter)
|
The webhooks tests still need fixing. The content of the webhook is incorrect, I need to replace it with a valid body and corresponding valid hash. |
|
@mattgd Updated tests! Ready for review now. |
| Attributes: | ||
| OBJECT_FIELDS (list): List of fields an Webhook is comprised of. |
There was a problem hiding this comment.
This comment can go since OBJECT_FIELDS doesn't exist anymore.
There was a problem hiding this comment.
Yeah, I plan to do a comment/docstring sweep across all the resources as a follow-up. Though, I haven't seen any place where these docstrings are surfaced to the developer. Do you know how they're usually used?
There was a problem hiding this comment.
I see them in my editor, albeit poorly formatted:
There was a problem hiding this comment.
Interesting. It seems like they're more useful the the actual SDK methods. Not to much for the resources/models. I have to dig into the packages to actually see the docstrings for our resources.
workos/webhooks.py
Outdated
| def verify_header( | ||
| self, | ||
| event_body: WebhookPayload, | ||
| event_signature: str, | ||
| secret: str, | ||
| tolerance: Optional[int] = None, | ||
| ): |
There was a problem hiding this comment.
Not a task for now, but does needing to wrap verify_header() in a try/except seem intuitive to you?
There was a problem hiding this comment.
Good question. It seems like the blessed path is to use verify_event (which actually shouldn't be called verify_event, it's really a deserialization). It makes sense for that to raise when the header can't be verified.
So what's the use case for using verify_header directly? Bypassing deserialization and parsing the webhook into a raw dict?
There was a problem hiding this comment.
I'm going to add a linear task to settle this.
There was a problem hiding this comment.
workos/webhooks.py
Outdated
| def verify_header( | ||
| self, | ||
| event_body: WebhookPayload, | ||
| event_signature: str, | ||
| secret: str, | ||
| tolerance: Optional[int] = None, | ||
| ): |
There was a problem hiding this comment.
We'll want an explicit -> None return here.
Description
Add type hints to
verify_event. Unfortunately, this lead to a bunch of duplication, comments inline.Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.