-
-
Notifications
You must be signed in to change notification settings - Fork 951
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
Add request_max_size option #2175
Changes from 1 commit
3b23283
d12f18e
1882aa1
7423965
2f0083b
cae3aaa
c49e492
d269fd5
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 | ||
---|---|---|---|---|
|
@@ -61,6 +61,7 @@ def __init__( | |||
on_startup: typing.Optional[typing.Sequence[typing.Callable]] = None, | ||||
on_shutdown: typing.Optional[typing.Sequence[typing.Callable]] = None, | ||||
lifespan: typing.Optional[Lifespan["AppType"]] = None, | ||||
request_max_size: int = 2621440, # 2.5mb | ||||
) -> None: | ||||
# The lifespan context function is a newer style that replaces | ||||
# on_startup / on_shutdown handlers. Use one or the other, not both. | ||||
|
@@ -78,6 +79,7 @@ def __init__( | |||
) | ||||
self.user_middleware = [] if middleware is None else list(middleware) | ||||
self.middleware_stack: typing.Optional[ASGIApp] = None | ||||
self.request_max_size = request_max_size | ||||
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. It might be a good idea to start making these attributes private. 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. Currently, all members of Starlette are public. I would not like to introduce a new style here. 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. Not true: starlette/starlette/datastructures.py Line 61 in 2168e47
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. I am referring to the 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. Fair enough. I still think a lot of those shouldn't have been public in the first place and just because they are doesn't mean we should make this public as well. |
||||
|
||||
def build_middleware_stack(self) -> ASGIApp: | ||||
debug = self.debug | ||||
|
@@ -117,6 +119,7 @@ def url_path_for(self, __name: str, **path_params: typing.Any) -> URLPath: | |||
|
||||
async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: | ||||
scope["app"] = self | ||||
scope["request_max_size"] = self.request_max_size | ||||
alex-oleshkevich marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
if self.middleware_stack is None: | ||||
self.middleware_stack = self.build_middleware_stack() | ||||
await self.middleware_stack(scope, receive, send) | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,7 @@ | |
from starlette.middleware import Middleware | ||
from starlette.requests import Request | ||
from starlette.responses import PlainTextResponse, RedirectResponse | ||
from starlette.types import ASGIApp, Lifespan, Receive, Scope, Send | ||
from starlette.types import ASGIApp, Lifespan, Message, Receive, Scope, Send | ||
from starlette.websockets import WebSocket, WebSocketClose | ||
|
||
|
||
|
@@ -205,12 +205,14 @@ def __init__( | |
methods: typing.Optional[typing.List[str]] = None, | ||
name: typing.Optional[str] = None, | ||
include_in_schema: bool = True, | ||
request_max_size: typing.Optional[int] = None, | ||
) -> None: | ||
assert path.startswith("/"), "Routed paths must start with '/'" | ||
self.path = path | ||
self.endpoint = endpoint | ||
self.name = get_name(endpoint) if name is None else name | ||
self.include_in_schema = include_in_schema | ||
self.request_max_size = request_max_size | ||
|
||
endpoint_handler = endpoint | ||
while isinstance(endpoint_handler, functools.partial): | ||
|
@@ -273,7 +275,25 @@ async def handle(self, scope: Scope, receive: Receive, send: Send) -> None: | |
) | ||
await response(scope, receive, send) | ||
else: | ||
await self.app(scope, receive, send) | ||
bytes_read = 0 | ||
max_body_size = self.request_max_size or scope.get("request_max_size") | ||
|
||
async def receive_wrapper() -> Message: | ||
nonlocal bytes_read | ||
message = await receive() | ||
if message["type"] != "http.request" or max_body_size is None: | ||
return message | ||
|
||
body = message.get("body", b"") | ||
bytes_read += len(body) | ||
if bytes_read > max_body_size: | ||
raise HTTPException( | ||
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. Is the exception good enough? 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. Yeah fine by me 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. Please follow the same logic as above. if "app" in scope:
raise HTTPException(status_code=413, "Request Entity Too Large")
else:
response = PlainTextResponse("Request Entity Too Large", status_code=404)
alex-oleshkevich marked this conversation as resolved.
Show resolved
Hide resolved
|
||
status_code=413, detail="Request Entity Too Large" | ||
) | ||
|
||
return message | ||
|
||
await self.app(scope, receive_wrapper, send) | ||
|
||
def __eq__(self, other: typing.Any) -> bool: | ||
return ( | ||
|
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.
Like in Django
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.
Can you permalink where you got this from in a comment below here just for the record? I'd also be okay with a code comment.
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.
https://github.com/django/django/blob/main/django/conf/global_settings.py#L310
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.
What does flask do?
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.
@Kludex Flask depends on the configuration option or on 'content-length' header. Otherwise, reads nothing.
https://flask.palletsprojects.com/en/2.3.x/config/#MAX_CONTENT_LENGTH
https://flask.palletsprojects.com/en/2.3.x/patterns/fileuploads/#improving-uploads
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.
PHP has 8mb
https://github.com/php/php-src/blob/165a4e53da819d377734eb2286df1b54709d18f2/php.ini-production#L713