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

backends-nginx: Add support for Nginx versions older than 1.5.9. #64

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adnrs96
Copy link

@adnrs96 adnrs96 commented Apr 12, 2017

In this PR 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.
This will not break Unicode support as revealed from my investigation. I tried out Django-sendfile 0.3.11 with Nginx 1.4.6. This leads to 404's which are resolved when we stop quoting the URL's. It might be worthwhile mentioning after making such a change filenames such as á.txt and Б.txt were still functional and correctly recognised.
Fixes: #56, #58. Closes #45.

@EvaSDK
Copy link

EvaSDK commented Apr 12, 2017

Wouldn't it be simpler to have a configuration parameter for this ? Trying to get nginx version with subprocess when you might not be able to run it or not be on the same machine at all (think docker, etc) seems like over-complication.

@adnrs96
Copy link
Author

adnrs96 commented Apr 13, 2017

Indeed it would be simpler @EvaSDK . Having a configuration parameter for this was the second thing in my mind. Didn't thought of Docker use case otherwise it would have become the first. Workflow for that would be something like this:

  • Developer has to set NGINX_VERSION parameter in django configuration with other settings like backends.
  • If the NGINX_VERSION is found to be set, it is used to decide whether the URL's should be quoted or not. The decision is cached in variable as is done currently.
  • If NGINX_VERSION setting is not found to be set, we assume that NGINX_VERSION is greater than 1.5.9 and thus decision is made and cached. Doing this has a benefit that people using django-sendfile currently will not be forced to make any changes on there part if everything is working fine. On other hands developers with older Nginx can use configuration setting to make Django-sendfile work with there project.

If community feels good with this change I can update the PR. Just leave me a comment regarding this.

@adnrs96 adnrs96 force-pushed the master branch 2 times, most recently from 4488e52 to d69f33f Compare April 13, 2017 01:35
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
Copy link
Author

adnrs96 commented Apr 13, 2017

I figured it would be better to have a updated commit for community to review than just a workflow, so I have updated the commit. Also added instructions in Readme regarding the same.

@EvaSDK
Copy link

EvaSDK commented Apr 13, 2017

Looks much better to me.

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.

Problem with unicode characters
2 participants