-
-
Notifications
You must be signed in to change notification settings - Fork 543
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
Fix sanic adapter casting files to io.BytesIO #3751
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Release type: patch | ||
|
||
Fix bug where files would be converted into io.BytesIO when using the sanic GraphQLView | ||
instead of using the sanic File type | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
from __future__ import annotations | ||
|
||
from io import BytesIO | ||
from typing import TYPE_CHECKING, Any, Optional, Union, cast | ||
|
||
from sanic.request import File | ||
|
@@ -10,7 +9,7 @@ | |
|
||
|
||
def convert_request_to_files_dict(request: Request) -> dict[str, Any]: | ||
"""Converts the request.files dictionary to a dictionary of BytesIO objects. | ||
"""Converts the request.files dictionary to a dictionary of sanic Request objects. | ||
|
||
`request.files` has the following format, even if only a single file is uploaded: | ||
|
||
|
@@ -29,12 +28,12 @@ def convert_request_to_files_dict(request: Request) -> dict[str, Any]: | |
if not request_files: | ||
return {} | ||
|
||
files_dict: dict[str, Union[BytesIO, list[BytesIO]]] = {} | ||
files_dict: dict[str, Union[File, list[File]]] = {} | ||
|
||
for field_name, file_list in request_files.items(): | ||
assert len(file_list) == 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: Consider handling multiple files per field name instead of asserting there will be exactly one The assertion could cause server crashes if multiple files are uploaded with the same field name. Consider returning a list of File objects when multiple files are present, matching the type hint |
||
|
||
files_dict[field_name] = BytesIO(file_list[0].body) | ||
files_dict[field_name] = file_list[0] | ||
|
||
return files_dict | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
from io import BytesIO | ||
from typing import cast | ||
|
||
import pytest | ||
from sanic_testing.testing import SanicTestClient | ||
|
||
import strawberry | ||
from sanic import Sanic | ||
from sanic.request import File | ||
from strawberry.file_uploads import Upload | ||
from strawberry.sanic import utils | ||
from strawberry.sanic.views import GraphQLView | ||
|
||
|
||
@strawberry.type | ||
class Query: | ||
@strawberry.field | ||
def index(self) -> str: | ||
return "Hello there" | ||
|
||
|
||
@strawberry.type | ||
class Mutation: | ||
@strawberry.mutation | ||
def file_upload(self, file: Upload) -> str: | ||
return cast(File, file).name | ||
|
||
|
||
@pytest.fixture | ||
def app(): | ||
sanic_app = Sanic("sanic_testing") | ||
|
||
sanic_app.add_route( | ||
GraphQLView.as_view( | ||
schema=strawberry.Schema(query=Query, mutation=Mutation), | ||
multipart_uploads_enabled=True, | ||
), | ||
"/graphql", | ||
) | ||
|
||
return sanic_app | ||
|
||
|
||
def test_file_cast(app: Sanic): | ||
"""Tests that the list of files in a sanic Request gets correctly turned into a dictionary""" | ||
file_name = "test.txt" | ||
|
||
file_content = b"Hello, there!." | ||
in_memory_file = BytesIO(file_content) | ||
in_memory_file.name = file_name | ||
|
||
form_data = { | ||
"operations": '{ "query": "mutation($file: Upload!){ fileUpload(file: $file) }", "variables": { "file": null } }', | ||
"map": '{ "file": ["variables.file"] }', | ||
} | ||
|
||
files = { | ||
"file": in_memory_file, | ||
} | ||
|
||
request, _ = cast(SanicTestClient, app.test_client).post( | ||
"/graphql", data=form_data, files=files | ||
) | ||
|
||
files = utils.convert_request_to_files_dict(request) # type: ignore | ||
file = files["file"] | ||
|
||
assert isinstance(file, File) | ||
assert file.name == file_name | ||
assert file.body == file_content | ||
|
||
|
||
def test_endpoint(app: Sanic): | ||
"""Tests that the graphql api correctly handles file upload and processing""" | ||
file_name = "test.txt" | ||
|
||
file_content = b"Hello, there!" | ||
in_memory_file = BytesIO(file_content) | ||
in_memory_file.name = file_name | ||
|
||
form_data = { | ||
"operations": '{ "query": "mutation($file: Upload!){ fileUpload(file: $file) }", "variables": { "file": null } }', | ||
"map": '{ "file": ["variables.file"] }', | ||
} | ||
|
||
files = { | ||
"file": in_memory_file, | ||
} | ||
|
||
_, response = cast(SanicTestClient, app.test_client).post( | ||
"/graphql", data=form_data, files=files | ||
) | ||
|
||
assert response.json["data"]["fileUpload"] == file_name # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: although a bug, someone using it might have gotten already used to it being
BytesIO
, so maybe we should mark this as a minor and mention that anyone relying on old behavior should adjust their code?What do you think @patrick91 ?