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

Add custom exception to handle missing boundary error #1544

Closed

Conversation

opeco17
Copy link

@opeco17 opeco17 commented Mar 10, 2022

Closes #1542

Outline

As per #1542, I've added a custom exception (MissingBoundaryException) to handle boundary parameter related error

Background

When form data is missing boundary parameter, the following line causes the KeyError
https://github.com/encode/starlette/blob/master/starlette/formparsers.py#L162

In the following discussion, it has been decided to use custom exception instead of using KeyError directly
#1349 (comment)

I've added a custom exception (MissingBoundaryException) in this PR for that

@@ -154,12 +154,17 @@ def on_end(self) -> None:
self.messages.append(message)

async def parse(self) -> FormData:
from starlette.exceptions import MissingBoundaryException
Copy link
Author

Choose a reason for hiding this comment

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

To avoid a circular import error, I've added an import line in the function instead of top-level in the module

@opeco17 opeco17 changed the title Add custom exception to handle missing boundary error #1542 Add custom exception to handle missing boundary error Mar 10, 2022
@Kludex
Copy link
Member

Kludex commented Mar 28, 2022

I did some research about this PR, and to understand if the issue was reasonable.

Here's what I've found out:

On Starlette, we raise KeyError with "boundary key missing". Although this PR follows the issue that was raised, and it's actually an improvement, I think we should follow Django's lead, and also create a 400 response on this case.

@Kludex
Copy link
Member

Kludex commented Apr 15, 2022

As we are not following what the initial issue proposed, I'm closing this PR.

Sorry for wasting your time @opeco17.

@Kludex Kludex closed this Apr 15, 2022
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.

Raise custom Starlette Exception on missing "boundary"
2 participants