Add early backends support with requests-style API#13
Conversation
erikrose
left a comment
erikrose
left a comment
There was a problem hiding this comment.
You've been very patient, so I want to give you this half to chew on while I go back and do a separate review of just the requests stuff. Nice to see the backend exercise in there! It gave me an excuse to go learn about backends.
| def make_static_backend_request(backend_name: str, path: str) -> SimpleResponse: | ||
| """Make a request using a static backend (pre-configured in viceroy.toml). | ||
|
|
||
| Args: |
There was a problem hiding this comment.
I think the most common docstring convention is still Sphinx-style, and I do prefer it over Google-style because of familiarity, because it doesn't blow so many lines, and because Sphinx supports it without third-party add-ons. (I could see us wanting to expose Sphinx-generated reference docs if we lay much abstraction atop the WIT APIs.) You've already put a lot of Google-style ones into the codebase, so I don't think this PR is the right place to have a conversation about format, but let's do it sometime before we release.
erikrose
left a comment
erikrose
left a comment
There was a problem hiding this comment.
There, that should finish it off! Happy Halloween! :-D I enjoyed the requests façade. That's going to be satisfying to use.
Here we introduc two new examples; in the first we do some basic work direclty against the WIT bindings to feel out its edges and understand its behavior. In the second example, we use a newly introduced fastly_compute.requests module that, at this point, provides a synchronous interface for making backend requests that aims to be compatible with existing requests package usages (so users may be able to import our requests module as requets). A few other small changes have also been rolled into this commit.
- Remove blank lines and pass statements from exception classes - Fix Google-style docstring format by adding colons after exception classes - Remove nested imports, moving them to top-level imports - Remove boilerplate Returns sections that just repeat type annotations - Add notes about ignored kwargs parameters for requests compatibility - Remove unused pytest.mark.integration decorators and unused pytest imports - Fix integration marker configuration in pytest settings
- Make RequestException subclass IOError instead of Exception for better requests compatibility - Add self.request attribute to RequestException and HTTPError constructors - Reverse tuple order in BackendResolver.resolve() to (final_url, backend_name) for consistency with parameter order (url, backend)
- Create shared read_response_body utility to eliminate duplicate response reading logic - Factor out charset parsing into _parse_charset helper method - Remove duplicate find_free_port function from test_server.py - Factor out dynamic backend registration logic into _register_dynamic_backend helper - Simplify make_dynamic_post_request by using shared backend registration code
- Replace manual URL parsing with stdlib urllib.parse for better reliability - Use urlparse() instead of string manipulation for scheme/host/path extraction - Add proper URL validation for scheme and netloc requirements
- Add type hint set[str] for _dynamic_backends field - Improve docstring for _resolve_static_backend method with clearer explanation - Add better docstring for __call__ method explaining WSGI callability requirements
- Remove LocalTestServerConfig class and use direct parameters in LocalTestServer - Update LocalTestServer constructor to take host, port, and responses directly - Replace _running flag with server None checks for cleaner state management - Update set_up_backends method name for PEP8 compliance - Update all test files to use new LocalTestServer interface - Add comprehensive documentation for LocalTestServer parameters These changes improve the test server API by removing unnecessary abstraction and making the interface more direct and intuitive.
This was mostly a building block towards getting the requests support. Remove for now to reduce maint. burden, duplication, confusion.
The simple bit qualifier doesn't add any real meaning but mentioning backends does.
Previuosly, we were just using hardcoded values when setting up the hostcalls. Here we try to map the requests `timeout` kwarg where possible and allow for full configuration via `timeout_config` on requests.
|
I'm working through the feedback yesterday/today -- I started to do some exploration to get better specific exceptions. Working against viceroy for backends, however, things are a bit challenging at the moment as Viceroy ends up spitting out a generic error without details for most things at present. This appears to align with this note in the initial impl https://github.com/fastly/Viceroy/blob/edd8497064561b865b5570b8a02bad6b139add3b/src/component/compute/http_req.rs#L66-L72 |
This is challenging to test at present and there aren't test cases added as Viceroy currently maps everything to a generic error so this is a best effort to try to put in place a framework that could be closer to correct and more fine-grained than what was present previously.
posborne
force-pushed
the
posborne/backend-requests
branch
from
10a9617 to
3ef02b9
Compare
I'm familiar with snapshot testing from the rust world using the insta library. Here we introduce syrupy to do snapshot testing to assert that we get back the responses we want. For now, this is only applied to the backend tests which needed some updates as it is. The general flow is to add tests and then to run `make test-update-snapshots` (or run the syrupy command directly) in order to write out new snapshots (which should be reviewed).
|
I'm going to close this PR for now; I've updated enough (based on comments and otherwise) that I think a fresh PR makes sense (will open once enough other changes land to stage another one). |
2 participants
Here we introduce two new examples; in the first we do some basic work
direclty against the WIT bindings to feel out its edges and understand
its behavior. In the second example, we use a newly introduced
fastly_compute.requests module that, at this point, provides a
synchronous interface for making backend requests that aims to be
compatible with existing requests package usages (so users may
be able to import our requests module as requets).
A few other small changes have also been rolled into this commit along with a fair bit of code to aid in testing backends by using either a local server or fastly's http-me.
Some of this is LLM generated but after a brief review with some fixes, most of it seems like an OK starting point. The biggest thing I think we need to sort through and change (either now or deferred) is to improve the exception handling code.
The WIT bindings bubble a lot of things to a fairly monolithic exception with description and I think we have some information loss on specific types of errors and it will take some detailed work (and ideally test cases) to ensure that we give good exception data for each of those cases [to match requests for those code paths]. I'm kind of OK with deferring that detail work past this POC phase given that I think it will be fairly time consuming.