Skip to content

Conversation

@gi0baro
Copy link
Contributor

@gi0baro gi0baro commented Aug 22, 2024

Summary

As per title, this adds support for ASGI pathsend extension on FileResponse class for servers implementing it.

This is a re-work of #2435 combined with parts of #2616, as discussed in #2613 with @Kludex.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@gi0baro gi0baro force-pushed the asgi-pathsend-rework branch 2 times, most recently from 2bbda0a to ddfc5c0 Compare August 22, 2024 14:24
@gi0baro gi0baro force-pushed the asgi-pathsend-rework branch 2 times, most recently from 9286c83 to 4631efb Compare September 2, 2024 16:36
@gi0baro gi0baro requested review from Kludex and adriangb September 2, 2024 16:42
@Kludex Kludex assigned Kludex and unassigned gi0baro Sep 23, 2024
@gi0baro gi0baro force-pushed the asgi-pathsend-rework branch 2 times, most recently from fe45c1b to 6b658e8 Compare October 3, 2024 10:04
@gi0baro
Copy link
Contributor Author

gi0baro commented Oct 3, 2024

@Kludex I just rebased from current master for your convenience

@gi0baro
Copy link
Contributor Author

gi0baro commented Jan 9, 2025

@Kludex what's your current take on this? Should I rebase so it can be merged, or should we just forget about it?

@Kludex
Copy link
Owner

Kludex commented Jan 10, 2025

@Kludex what's your current take on this? Should I rebase so it can be merged, or should we just forget about it?

I want this to get in, I want to get back to this soon. I can rebase myself.

Sorry the delay here.

Copy link
Owner

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been going for long - Sorry about it.

The only blocker here is deciding what to do about the changes on the BaseHTTPMiddleware - but I guess that's really the point of this PR. Would it make sense to remove the extension from the scope?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is altering the scope["extensions"] in the application side allowed? Would it be bad to remove the http.response.httpsend from the BaseHTTPMiddleware? It's a question. I'm not sure what would be best.

I would like to avoid adding complexity to the BaseHTTPMiddleware.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how altering the scope would help in avoiding changes on BaseHTTPMiddleware.

To my perspective, the only way to avoid these lines https://github.com/encode/starlette/pull/2671/files#diff-931c53548e8908544bd0818ee1a8d5f1ada4f07430a439e9322ab369eb40a8acR161-R165 is to re-implement the whole streaming wrapper, which doesn't sound less complex to me tbh..
Maybe we could try to avoid the response re-wrap in _StreamingResponse depending on the context? Probably I don't have the needed to knowledge of Starlette internals to propose such a change though :(

@gi0baro gi0baro force-pushed the asgi-pathsend-rework branch 3 times, most recently from 8d68730 to d1dbb45 Compare January 13, 2025 14:22
@gi0baro
Copy link
Contributor Author

gi0baro commented Jan 13, 2025

@Kludex not sure why the coverage fails after the last rebase..

@Kludex
Copy link
Owner

Kludex commented Jan 13, 2025

@Kludex not sure why the coverage fails after the last rebase..

Because we recently set branch = True on coverage.

@gi0baro gi0baro force-pushed the asgi-pathsend-rework branch from d1dbb45 to 6d23fba Compare March 5, 2025 11:49
@gi0baro
Copy link
Contributor Author

gi0baro commented Mar 5, 2025

@Kludex sorry for the long delay.

This is now rebased on latest master, and tests/CI is finally green. Let me know if any additional edits are required for merge.

@gi0baro gi0baro requested a review from Kludex March 5, 2025 11:52
@Kludex
Copy link
Owner

Kludex commented May 29, 2025

Sorry the delay. Gradly you caught me at PyCon IT. 😅

@Kludex Kludex enabled auto-merge (squash) May 29, 2025 13:16
@Kludex Kludex merged commit af1c709 into Kludex:master May 29, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants