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 encoding of generated URLs #3138

Merged
merged 2 commits into from
May 31, 2024

Conversation

afullerx
Copy link
Contributor

@afullerx afullerx commented May 29, 2024

This PR attempts to resolve #3124 by encoding any special characters in URLs auto-generated from file names. This was done by adding urllib.parse.quote() to app.add_media_file() and app.add_static_file().

The only difficult question was whether or not to also encode caller-provided URLs. Since url_path must be supplied in unencoded form to work with the FastAPI decorator, there should be no risk of double-encoding. The only downside of encoding it is that if a user passes in an url_path that includes special characters and relies on the return value being the same unencoded string, it could break their code. 

Since this is a pretty narrow edge case, I opted for this behavior to keep the operation of the methods more consistent. If you'd rather go the more conservative route of just returning the url_path provided value unencoded, that's obviously a simple change. However, then the return value description would have to be more complicated to fully describe the behavior.

@afullerx afullerx changed the title add encoding of generated URLs Add encoding of generated URLs May 29, 2024
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request, @afullerx!

I wondered if we could only URL-encode the file.name. But as you mentioned, @self.get needs the unencoded path. So I guess your solution is pretty optimal.

Regarding the breaking change: I would call it a bug that the returned URL couldn't be used to access the file if it contained special characters, which is fixed with this PR. If you really need the original url_path, it makes little sense to rely on the return value.

@falkoschindler falkoschindler added this to the 1.4.27 milestone May 31, 2024
@falkoschindler falkoschindler added the bug Something isn't working label May 31, 2024
@falkoschindler falkoschindler merged commit 9599619 into zauberzeug:main May 31, 2024
1 check passed
@afullerx afullerx deleted the filename-url-encoding branch June 1, 2024 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Audio with hashtags in filenames are not loaded
2 participants