Skip to content
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

Move exception handling logic to Route #2026

Merged
merged 31 commits into from
Jun 7, 2023
Merged

Conversation

adriangb
Copy link
Member

@adriangb adriangb commented Feb 6, 2023

Fixes #493, partially resolves #1692 (comment), resolves #1649 (comment)

By moving handling of exceptions to the Route level we can use the same Request/WebSocket object for the endpoint and exception handlers.

See #2020 for some more background

@Kludex Kludex mentioned this pull request Feb 14, 2023
8 tasks
@Kludex Kludex requested a review from a team March 5, 2023 15:16
@Kludex
Copy link
Member

Kludex commented Mar 5, 2023

I'm positive towards this PR. Is any other member that wants to check it? @encode/maintainers

@adriangb
Copy link
Member Author

@tomchristie it would be great to get your eyes on this since no one has really changed this part of the codebase since you originally wrote it

@Kludex Kludex mentioned this pull request Mar 16, 2023
11 tasks
Copy link
Member

@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.

Are you strong about this code structure?

Do you mind if we maintain the exceptions.py and create a _<some_good_name_at_root_level>? I feel like importing wrap_app_handling_exceptions from middleware/exceptions is not very logic. 🤔

@encode/maintainers If someone is against this idea, or has any concerns regarding this, or want more context, anything... Please let us know. Otherwise, I'll get this merged end of next week.

@adriangb
Copy link
Member Author

adriangb commented Apr 14, 2023

this does not solve the problem when someone wants to access request.body in other middleware. This issue is still there, isn't it? This solution (#1519) is more straightforward as fixes the issue everywhere.

#1692 also resolves it in a way that compliments this PR, and without breaking other things.

@adriangb adriangb requested a review from a team May 4, 2023 18:59
@adriangb
Copy link
Member Author

adriangb commented May 4, 2023

Folks, it would be great to get some more input/review here.

@Kludex
Copy link
Member

Kludex commented Jun 2, 2023

The last bits here:

What's our plan regarding the ExceptionMiddleware? Are we going to remove it at some point?

Also, should we update the documentation: https://www.starlette.io/exceptions/#errors-and-handled-exceptions ?

Copy link
Member

@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.

Do you mind if we change the structure of the files around (I can change it)?

I'd prefer to not remove the file exceptions.py, and just create a top level _exception_handler.py.

@adriangb
Copy link
Member Author

adriangb commented Jun 6, 2023

Sure go for it

@Kludex
Copy link
Member

Kludex commented Jun 7, 2023

@adriangb I didn't change the documentation, but I think we should do something there for 1.0.

Also, I applied this commit: 3fb1118 (#2026). If you didn't like it, let me know, so I can revert it. 👍

@Kludex Kludex merged commit e99738b into encode:master Jun 7, 2023
@adriangb adriangb deleted the cleanup-move-exc branch June 7, 2023 06:29
adriangb added a commit to adriangb/starlette that referenced this pull request Jun 9, 2023
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
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.

Question about Request.Body()
3 participants