Use lia instead of our own adapters - #3967
Conversation
Reviewer's GuideThis PR replaces all custom HTTP request adapters across frameworks with the external lia library, re-exports lia’s HTTPException for streamlined error handling, updates ASGI and FastAPI adapter defaults, enhances ChannelsRequestAdapter, fixes multipart parsing logic, and adds lia-web as a new dependency. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
for more information, see https://pre-commit.ci
Apollo Federation Subgraph Compatibility Results
Learn more: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3967 +/- ##
==========================================
- Coverage 94.40% 94.31% -0.10%
==========================================
Files 528 528
Lines 34371 34154 -217
Branches 1803 1804 +1
==========================================
- Hits 32449 32213 -236
- Misses 1630 1648 +18
- Partials 292 293 +1 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #3967 will not alter performanceComparing Summary
|
535fa8e to
da7181f
Compare
787ce38 to
9a8f7aa
Compare
|
Thanks for adding the Here's a preview of the changelog: This release removes some internal code in favour of using an external dependency, Here's the tweet text: |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @patrick91 - I've reviewed your changes - here's some feedback:
- Import StarletteRequestAdapter from lia in the ASGI and FastAPI router modules to match the updated request_adapter_class and prevent NameError.
- Split out the ChannelsRequestAdapter enhancements (url and cookies parsing) into a separate pull request, as these are orthogonal to the lia adapter refactor.
- Verify that SyncHTTPRequestAdapter and AsyncHTTPRequestAdapter are properly aliased and re-exported from lia to maintain backward compatibility across existing code paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Import StarletteRequestAdapter from lia in the ASGI and FastAPI router modules to match the updated request_adapter_class and prevent NameError.
- Split out the ChannelsRequestAdapter enhancements (url and cookies parsing) into a separate pull request, as these are orthogonal to the lia adapter refactor.
- Verify that SyncHTTPRequestAdapter and AsyncHTTPRequestAdapter are properly aliased and re-exported from lia to maintain backward compatibility across existing code paths.
## Individual Comments
### Comment 1
<location> `strawberry/channels/handlers/http_handler.py:129` </location>
<code_context>
- def __init__(self, request: web.Request) -> None:
- self.request = request
-
- @property
- def query_params(self) -> QueryParams:
- return self.request.query.copy() # type: ignore[attr-defined]
</code_context>
<issue_to_address>
New url and cookies properties may not be consistently available across all request types.
These properties rely on certain fields and header formats that may not always be present, which could cause runtime errors. Please add error handling or validation to ensure robustness.
</issue_to_address>
### Comment 2
<location> `strawberry/channels/handlers/http_handler.py:173` </location>
<code_context>
- return self.request.form_data["files"]
+ return self.request.form_data.files
+
+ def get_form_data(self) -> FormData:
+ return self.request.form_data
</code_context>
<issue_to_address>
get_form_data is defined as a synchronous method, which may be inconsistent with async expectations.
If other adapters use async for get_form_data, consider updating this method to async for consistency, or verify that all usages support both sync and async implementations.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def get_form_data(self) -> FormData:
return self.request.form_data
=======
async def get_form_data(self) -> FormData:
return self.request.form_data
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @property | ||
| def url(self) -> str: | ||
| scheme = self.request.consumer.scope["scheme"] | ||
| host = self.headers.get("host", "localhost") | ||
| path = self.request.consumer.scope["path"] | ||
| query_string = self.request.consumer.scope["query_string"] | ||
| url = f"{scheme}://{host}{path}" | ||
| if query_string: | ||
| url += f"?{query_string.decode()}" | ||
| return url |
There was a problem hiding this comment.
suggestion (bug_risk): New url and cookies properties may not be consistently available across all request types.
These properties rely on certain fields and header formats that may not always be present, which could cause runtime errors. Please add error handling or validation to ensure robustness.
| def get_form_data(self) -> FormData: | ||
| return self.request.form_data |
There was a problem hiding this comment.
suggestion: get_form_data is defined as a synchronous method, which may be inconsistent with async expectations.
If other adapters use async for get_form_data, consider updating this method to async for consistency, or verify that all usages support both sync and async implementations.
| def get_form_data(self) -> FormData: | |
| return self.request.form_data | |
| async def get_form_data(self) -> FormData: | |
| return self.request.form_data |
| scheme = self.request.consumer.scope["scheme"] | ||
| host = self.headers.get("host", "localhost") | ||
| path = self.request.consumer.scope["path"] | ||
| query_string = self.request.consumer.scope["query_string"] | ||
| url = f"{scheme}://{host}{path}" | ||
| if query_string: | ||
| url += f"?{query_string.decode()}" | ||
| return url |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Move assignment closer to its usage within a block (
move-assign-in-block) - Use named expression to simplify assignment and conditional (
use-named-expression)
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR refactors Strawberry GraphQL's HTTP adapter infrastructure by replacing custom framework-specific adapters with standardized implementations from the external lia-web library. The change affects all major web framework integrations including Flask, Django, FastAPI, Sanic, Chalice, AioHTTP, Quart, Litestar, and ASGI.
The refactoring removes approximately 400+ lines of custom adapter code across multiple files and consolidates this functionality into the lia-web dependency (>=0.2.1). Each framework previously maintained its own HTTP request adapter class that handled query parameters, request bodies, form data, headers, and HTTP methods. These have been replaced with unified adapters from lia that provide the same interface but with standardized implementations.
Key architectural changes include:
- Dependency Management: Adds
lia-web (>=0.2.1)as a core dependency inpyproject.toml - Exception Handling: Migrates from custom
HTTPExceptiontolia's implementation across all view classes - Adapter Standardization: Replaces framework-specific adapters (e.g.,
FlaskHTTPRequestAdapter,DjangoHTTPRequestAdapter) withliaequivalents - Form Data Handling: Updates multipart form data access patterns from dictionary-style (
form_data['form']) to attribute-style (form_data.form) to matchlia'sFormDatastructure - Backward Compatibility: Maintains existing public APIs by preserving class names and interfaces while delegating implementation to
lia
The WebSocket adapters remain custom implementations since they contain GraphQL-specific subscription logic that's not covered by lia's HTTP abstractions. This refactoring allows Strawberry to focus on GraphQL-specific functionality rather than maintaining HTTP protocol handling details across multiple web frameworks.
Confidence score: 4/5
- This PR is generally safe to merge but requires thorough testing of framework integrations
- Score reflects the large scope of changes across critical HTTP handling code, though the external library approach reduces maintenance burden
- Pay close attention to form data handling changes and ensure lia-web dependency compatibility across all supported framework versions
16 files reviewed, 1 comment
| "typing-extensions>=4.5.0", | ||
| "python-dateutil~=2.7", | ||
| "packaging>=23", | ||
| "lia-web (>=0.2.1)", |
There was a problem hiding this comment.
style: The dependency syntax uses parentheses instead of standard quotes. While this works, the standard format would be 'lia-web>=0.2.1' for consistency with other dependencies.
|
👋 could this be notated as a breaking interface change in Strawberry's changelog? https://strawberry.rocks/docs/breaking-changes/ My project has a custom subclass of I ran into the following import error while upgrading, and didn't see mention of this anywhere in Strawberry's changelog (ultimately, I discovered the problem by looking at Strawberry's commit history and discovering this PR). Obviously this isn't a huge deal, but it might be nice to spell out this change for others who might encounter it in the future. |
|
@ryanpetrello sorry about that! would be ok with making a PR? otherwise I'll try over the weekend 😊 |
|
Hey, it happens 🤷 ! Overall, Strawberry has excellent documentation and ya'll are really great about communicating breaking changes (and this is a fairly minor one). If I wanted to open an MR, would I just add a version-specific update here? https://github.com/strawberry-graphql/strawberry/tree/main/docs/breaking-changes Similar to this? |
|
Here you go @patrick91 #3989 |
3 participants
Summary by Sourcery
Adopt lia as the external source for HTTP adapter implementations and exceptions by removing all custom adapter code, update imports and multipart logic accordingly, add ChannelsRequestAdapter enhancements, and bump dependencies with release documentation
New Features:
Enhancements:
Build:
Documentation: