Rework webhook#856
Open
dmamelin wants to merge 8 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR grew out of the discussion on #843: the consensus there was to split
@webhook_handlerout as a new decorator (rather than overloading@webhook_triggerwith asets_http_response_codeflag), keep@webhook_triggerbackward compatible, and integrate the new decorator tightly with the existing decorator pipeline instead of growing a parallel codepath. This PR is one alternative implementation; see "Relationship to #852" below for how it differs from @JayNewstrom's follow-up.Status codes generated by the decorator itself:
Kwargs (
webhook_id,str_expr,local_only,methods,kwargs) match@webhook_trigger;timeoutis new. Only one handler per webhook_id.Changes to
@webhook_triggermethods=["GET", "POST"]andmethods=["POST", "GET"]are equivalent.{"POST", "PUT"}.@webhook_triggeron the same webhook_id with differentlocal_onlyormethodsnow raise at registration. Same webhook_id with matching settings is still allowed (the documented multi-trigger case).Infrastructure changes
CallResultHandlerDecoratorgainshandle_call_exception(data, exc)andhandle_call_canceled(data). Defaults forward tohandle_call_result(data, None). This makes the success / exception / canceled trichotomy a property of the protocol rather than something each result handler has to bolt on.@webhook_handleroverrides both to surface 500/503.DecoratorManager.safe_await(coro)— awaits a coroutine and routes exceptions throughhandle_exceptionrather than propagating. Used wherever a buggy extension point shouldn't break sibling work.AutoKwargsDecoratornow collects annotations across the MRO (needed by@webhook_handler, which inherits via a mixin chain).Comparison with #852
None/int/Response. This PR also handlesstr(text body),bytes(raw body),dict/list(JSON body), and(int_status, body) tuples.intdeliberately not supported. 852 mapsintto a status code with an empty body, soreturn 200andreturn 42both succeed (silently dropping any "data" interpretation). This PR rejects bareintand returns500with a warning —return 200, Noneand (status, body) tuples are the explicit way to set a status.The cost is one extra character; the benefit is that ambiguous returns don't silently do the wrong thing.handle_call_exceptionto CallResultHandlerDecorator. 852 then handles cancellation by passingCancelledErrorthrough the same hook, plus a notified flag and a try/except/finally in_callto make sure handlers get exactly one notification. This PR uses straight if/else paths in _call and a separatehandle_call_canceledhook, so cancellation stays a distinct event (503 vs. 500) without flags or finally blocks.PyscriptFixture
A new pytest fixture (tests/conftest.py) that replaces the ad-hoc setup_script + notify_q + MockOpen boilerplate. I've been mulling this fixture for a while — early drafts have been sitting on a personal branch for some time without a concrete use case to anchor them. The webhook work finally provides that anchor: enough new tests, with non-trivial assertions on values and exceptions, to justify landing the fixture with real examples rather than as a speculative cleanup PR.
Tests now read like:
Setup:
pyscript.start(source)— single-file shortcut, mounts ashello.pyunder the pyscript folder.add_file(name, content)/add_files({...})for multi-file layouts.globandisfilemocks are wired automatically.pyscript.config,pyscript.yaml_config— mutate beforestart().pyscript.now— singledatetimeorlist(consumed sequentially as pyscript readsdt_now).Observation:
pyscript.done/pyscript.done2—asyncio.Queuefed automatically bypyscript.done = ...writes in scripts.wait_done(expected)/wait_done2(expected)— await with 4s default timeout. Non-string expected is compared afterast.literal_eval.pyscript.exceptions— fed by a monkeypatch ofAstEval.log_exception.wait_exception(expected_type, match=...)— type + substring match.Strict teardown: asserts the queues(
done,done2,exceptions) are empty at end of test. Tests that intentionally produce values or errors must consume them explicitly — silent passes that masked a missing assertion are caught immediately.The fixture was designed with the existing tests in mind. All of them can be migrated to it. This should improve convenience, readability, and safety thanks to strict teardown.