Backend Requests Support via Requests-Compatible API Facade#27
Conversation
- Add LocalTestServer for httpbin-like backend testing - Enhance ViceroyTestBase with backend setup capabilities - Add utility functions for response body reading - Update build configuration and dependencies - Improve temporary file handling with NamedTemporaryFile - Add syrupy for snapshot testing. This provides the foundation for backend testing and includes utilities needed by the requests implementation.
- Implement fastly_compute.requests module with requests compatibility - Support for both static and dynamic backends - Advanced timeout configuration (connect, first_byte, between_bytes) - WIT error mapping to map to requests-compat exceptions (there are some known failures here due to how viceroy behaves). - requests-compatible response object with lazy loading - Add backend-requests example application - Comprehensive test suite with snapshot testing
erikrose
left a comment
erikrose
left a comment
There was a problem hiding this comment.
Just the usual spate of small things, plus a thought on future-proof API design before we get too deeply entrenched. This will be a great façade to have included.
| if not url: | ||
| return {"error": "url query parameter is required"} | ||
|
|
||
| backend = query.get("backend", None) |
There was a problem hiding this comment.
If query.get is anything like dict.get, the None is implied. But Bottle docs contradict themselves about whether the default is "" or None. :-(
There was a problem hiding this comment.
I'll have to retest after the break as I can't recall; I would expect it to be None based on normal dict get behavior, though this isn't a straight up dict so explicit could be argued as favorable here (to potentially abuse the zen of python).
| return { | ||
| "method": method, | ||
| "url": url, | ||
| "backend": kwargs.get("backend", None), |
| demo: The demo/endpoint name for this error | ||
| """ | ||
| exc_print = StringIO() | ||
| traceback.print_exc(file=exc_print) |
There was a problem hiding this comment.
We could use format_exc(), which directly returns a string, and save ourselves going in and out of a StringIO.
| # Backend-specific features | ||
| response = requests.get( | ||
| "/api/endpoint", | ||
| backend="my-backend" # Use specific static backend |
There was a problem hiding this comment.
Shall we merge this with the same example under "Basic Usage"?
| @classmethod | ||
| def teardown_class(cls): | ||
| """Clean up test server.""" | ||
| if hasattr(cls, "test_server"): |
There was a problem hiding this comment.
This might have been a workaround for a type checker complaint, but I'll confirm.
| line-ending = "auto" | ||
|
|
||
| [tool.ruff.lint.pydocstyle] | ||
| convention = "google" |
There was a problem hiding this comment.
Less common, less tool support. Somewhere, maybe on the last review of this, I had a better comment about the tradeoffs.
Aha, ran across it while looking for something else.
There was a problem hiding this comment.
Yeah, this carried over from the base PR. For now, I would like to keep it to avoid massive additional churn on this PR. We can maybe shortly do a revisit to convention and tool-assisted rework of existing docs in a separate PR.
Use traceback.format_exc() and remove redundant None defaults from dict.get() calls
BREAKING CHANGE: backend → fastly_backend, timeout_config → fastly_timeout Establishes clear naming convention for Fastly-specific parameters
Remove no-op assignment, simplify exception handling, and remove redundant checks
Remove redundant None from Any | None union
Reduce duplication in from_http_req_error() and from_wit_error()
Remove __init__ methods that only call super() with same signature
Allow exceptions to propagate for proper error handling and debugging
Make test_server variables private and remove unnecessary hasattr check
Previously we were doing a sanitize step that I do not believe is really necessary. We now use the lowered netloc directly for the dynamic backend name. Additionaly, a case related to an empty netloc was removed as it is not possible to hit.
- Flip ternary to avoid 'not' (clearer logic) - Remove redundant UnicodeError catch (subclass of ValueError) - Improve resolve_backend() docstring with clearer algorithm explanation
Merge Fastly-specific features section into Basic Usage for better flow. The examples now show a progression from simple to advanced usage.
erikrose
left a comment
erikrose
left a comment
There was a problem hiding this comment.
Other than this one thing, I think we're ready to rock!
| # Add custom headers | ||
| for name, value in headers.items(): | ||
| wit_request.insert_header(name, value.encode("utf-8")) | ||
| except ValueError as e: |
There was a problem hiding this comment.
I think we'd do ourselves a favor to tighten up the scope on such a generic error, instead putting this just around both calls to encode() if indeed that's all we expect to raise ValueError.
These changes remove some overly broad exceptions and restructure the core request logic in a few ways: - Remove insert a user-agent; I compared other SDKs and they do not add this, so we shouldn't either. - Don't override content-type if set explicitly. - Simplified Host header handling; there was a workaround that is now patched in viceroy and I think not necessary. - Reduced the sites where we perform hostcalls in request and made those sites be very specific with exception handling.
| headers = headers if headers is not None else {} | ||
| if fastly_backend is not None: | ||
| host_header = headers.pop("Host", url_parsed.netloc) | ||
| wit_request.insert_header("Host", host_header.encode("utf-8")) |
There was a problem hiding this comment.
Just a small note here, the commit message contradicts this slightly. I may revisit what we're doing here now that I landed a change for fastly/Viceroy#549, but for now I'm keeping this as-is for dynamic backends. I think we might not need to do it, but I also don't think it represents a problem either in this form.
|
|
||
| # Add custom headers | ||
| for name, value in headers.items(): | ||
| headers = headers if headers is not None else {} |
There was a problem hiding this comment.
You ought to be able to say headers or {} if you like, since {} is the only falsy value of a dict.
There was a problem hiding this comment.
It would be fine; I wrote it this way to avoid extra allocations in the case of headers already being {} as an empty dict is falsy.
2 participants
These changes are a second pass with significant rework based on #13. I believe I addressed all the comments from that PR and then some with significantly more work put into adding additional tests, a rewrite of almost all of the code, introduction of snapshot testing, a port of the changes to use the new wit interfaces which model backends as a resource, etc.