Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

webhook: Add initial webhook plugin #39

Merged
merged 6 commits into from
Feb 7, 2024
Merged

webhook: Add initial webhook plugin #39

merged 6 commits into from
Feb 7, 2024

Conversation

jcharum
Copy link
Collaborator

@jcharum jcharum commented Feb 5, 2024

This initial plugin implementation is very simple. It does a bunch of simplifying things:

  • Assumes POST.
  • Assumes JSON request and response bodies.
  • body and return are Any, so we won't get much typing help.

This is good enough to be used by a workflow with the description:

Post a body of the form { "message": "Hello from Lutra and Zapier!"} to the webhook https://hooks.zapier.com/hooks/catch/<elided>. Allow only the message to be customizable.

@jcharum jcharum requested review from bozhi and jngiam February 5, 2024 22:24
@jcharum
Copy link
Collaborator Author

jcharum commented Feb 5, 2024

This mostly exists as a proof-of-concept that what should work, straightforward webhook calling, works. I don't personally have any use cases I want to build out for myself, but I'm happy to help extend this plugin in whatever way is useful.

import httpx


async def webhook_request(url: str, body: Any) -> Any:
Copy link
Collaborator

@bozhi bozhi Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why the body's type and return type is Any? I think the type checker will not catch any issues here and that seems a little problematic.

Copy link
Collaborator Author

@jcharum jcharum Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, body and the return value are arbitrary data structures that can be encoded to and decoded from JSON, respectively. There are enough trade-offs around what that type should even be that Python does not provide one in the type hints of the standard library. See the discussion in this issue. The final comment summarizes nuances around type-checking. Given the nuance, particularly around invariance and mutability, and the generality of webhook calling, it's not obvious to me what trade-off to make.

Similarly, the json parameter and return value of .json() are Any, presumably for the same reasons.

Furthermore, plugins do not yet support unions, as unions are generally very prone to introduce ambiguity in deserialization, and there is no mechanism to disambiguate. The one that exists for built-in actions is pretty fragile and requires some relatively careful thinking, so I am resisting adding support to plugins until we have a very very good reason.

Do you have an example of a problem case, as I think a worked example would be useful to explore these trade-offs?

Copy link
Collaborator

@jngiam jngiam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like zapier webhooks just use a secret url -- is that right?

@jcharum
Copy link
Collaborator Author

jcharum commented Feb 7, 2024

it seems like zapier webhooks just use a secret url -- is that right?

Yes. It's possible to make it a bit more secure by putting a condition on a field in the body in the zap.

@jcharum jcharum merged commit 2e4cad1 into main Feb 7, 2024
@jcharum jcharum deleted the jjc/webhook/init branch February 7, 2024 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants