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

Problem with unicode characters #45

Open
pando85 opened this issue Jan 6, 2016 · 10 comments · May be fixed by #64
Open

Problem with unicode characters #45

pando85 opened this issue Jan 6, 2016 · 10 comments · May be fixed by #64

Comments

@pando85
Copy link

pando85 commented Jan 6, 2016

When I try to set up to sendfile with spanish characters (like: á, é, í, ó, ú...) using nginx backend, the link of sendfile response its: 404 not found.

@vstoykov
Copy link

I was trying Bulgarian characters (like: А, Б, В, Г, Д....) and everything works with Nginx backned under Python 3.

Can you investigate where exactly the things get broken.

@lilspikey
Copy link
Collaborator

I suspect it's more to do with how the default encoding on the server is setup etc.

@pando85
Copy link
Author

pando85 commented Feb 12, 2016

It could be related with server encoding, I don't remember right now and I can't tested, sorry.

@Proper-Job
Copy link
Contributor

The problem is that the URL that's handed to nginx via the X-Accel-Redirect header isn't quoted. If you use urllib.parse.quote() on the URL there're no problem.
I was going to fix this and add general Python 3 support next week. Is there a chance a PR with these changes will get reviewed and merged?

@lilspikey
Copy link
Collaborator

Possibly. I started a new job 3 months ago and it's been pretty intense...

@Proper-Job
Copy link
Contributor

@lilspikey that's not much to go on. What would it take for you to review a PR?
The main thing that we ran into (using Python3) is the quote issue. What if I made a PR for that and updated the tests accordingly. Would you be able to review that within the month of July?

@vstoykov
Copy link

I have a suggestion for generating the value for X-Accel-Redirect. Why not simply use Django's FileSystemStorage for generating the url and also validating the path to the file.

We can have a sendfile_storage which will be:

 sendfile_storage = FileSystemStorage(SENDFILE_ROOT, SENDFILE_URL)

And then calculating the internal URL will be just:

internal_url = sendfile_storage.url(filename)

I think that this will solve the problem with the encoding.

For more info https://docs.djangoproject.com/en/1.9/ref/files/storage/#the-filesystemstorage-class

@lilspikey
Copy link
Collaborator

@Proper-Job hopefully I could review it in July. If it's just the quoting, then it shouldn't be a large change, so should manage it. New job doesn't involve Django at all, so this would purely be done in personal time.

@vstoykov that looks like it might work, but for the sake of a small change it might be easier to just go with adding quoting for now.

@Proper-Job
Copy link
Contributor

I created a PR for this issue. I successfully tested it on nginx. #54

@willstott101
Copy link

Note that escaped URIs breaks support for Nginx versions older than 1.5.9 which isn't expecting them in the X-Accel-Redirect header.

I personally think this is fine seeming as it's either not-supporting a 3-year-old release of Nginx or not-supporting unicode file names. But I'm leaving this as a note for anyone noticing the breakage when upgrading sendfile.

adnrs96 added a commit to adnrs96/django-sendfile that referenced this issue Apr 12, 2017
In this commit we are adding support for Nginx version older than
1.5.9. This is achieved through conditionally quoting the URL's in
X-Accel-Redirect headers according to compatibility with Nginx.
Since newer versions of Nginx expect URL's in X-Accel-Redirect to
be quoted unlike the versions older that Nginx 1.5.9. In this commit
we start to check the Nginx version using 'nginx -v' and then caching
the result be making use of a decorator. Using this result we decide
whether the outgoing URL should be quoted or not.
Fixes: johnsensible#56, johnsensible#58. Closes johnsensible#45.
adnrs96 added a commit to adnrs96/django-sendfile that referenced this issue Apr 12, 2017
In this commit we are adding support for Nginx version older than
1.5.9. This is achieved through conditionally quoting the URL's in
X-Accel-Redirect headers according to compatibility with Nginx.
Since newer versions of Nginx expect URL's in X-Accel-Redirect to
be quoted unlike the versions older that Nginx 1.5.9. In this commit
we start to check the Nginx version using 'nginx -v' and then caching
the result be making use of a decorator. Using this result we decide
whether the outgoing URL should be quoted or not.
Fixes: johnsensible#56, johnsensible#58. Closes johnsensible#45.
adnrs96 added a commit to adnrs96/django-sendfile that referenced this issue Apr 13, 2017
In this commit we are adding support for Nginx version older than
1.5.9. This is achieved through conditionally quoting the URL's in
X-Accel-Redirect headers according to compatibility with Nginx.
Since newer versions of Nginx expect URL's in X-Accel-Redirect to
be quoted unlike the versions older that Nginx 1.5.9. From this commit
onwards we start to expect a 'NGINX_VERSION' config setting to be setup
in the django settings in order to facilitate decision making regarding
quoting URL's. If such a setting is not found we just assume Nginx version
to be greater then 1.5.9 and make decision accordingly and then cache the
decision by making use of a decorator. Using this result we decide whether
the outgoing URL should be quoted or not.
Fixes: johnsensible#56, johnsensible#58. Closes johnsensible#45.
adnrs96 added a commit to adnrs96/django-sendfile that referenced this issue Apr 13, 2017
In this commit we are adding support for Nginx version older than
1.5.9. This is achieved through conditionally quoting the URL's in
X-Accel-Redirect headers according to compatibility with Nginx.
Since newer versions of Nginx expect URL's in X-Accel-Redirect to
be quoted unlike the versions older that Nginx 1.5.9. From this commit
onwards we start to expect a 'NGINX_VERSION' config setting to be setup
in the django settings in order to facilitate decision making regarding
quoting URL's. If such a setting is not found we just assume Nginx version
to be greater then 1.5.9 and make decision accordingly and then cache the
decision by making use of a decorator. Using this result we decide whether
the outgoing URL should be quoted or not.
Fixes: johnsensible#56, johnsensible#58. Closes johnsensible#45.
adnrs96 added a commit to adnrs96/django-sendfile that referenced this issue Apr 13, 2017
In this commit we are adding support for Nginx version older than
1.5.9. This is achieved through conditionally quoting the URL's in
X-Accel-Redirect headers according to compatibility with Nginx.
Since newer versions of Nginx expect URL's in X-Accel-Redirect to
be quoted unlike the versions older that Nginx 1.5.9. From this commit
onwards we start to expect a 'NGINX_VERSION' config setting to be setup
in the django settings in order to facilitate decision making regarding
quoting URL's. If such a setting is not found we just assume Nginx version
to be greater then 1.5.9 and make decision accordingly and then cache the
decision by making use of a decorator. Using this result we decide whether
the outgoing URL should be quoted or not.
Fixes: johnsensible#56, johnsensible#58. Closes johnsensible#45.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants