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

Make SCRIPT_NAME environ variable optional #149

Merged
merged 1 commit into from Dec 20, 2018

Conversation

Projects
None yet
3 participants
@AndreLouisCaron
Copy link
Contributor

AndreLouisCaron commented Dec 19, 2018

PEP 333 makes this CGI-style variable optional. See the the section titled "environ variables":

The following variables must be present, unless their value would be an empty string, in which case they may be omitted, except as otherwise noted below.

Some WSGI middleware does not set this environ variable. When combined with cheroot.wsgi.PathInfoDispatcher, there is a confusing KeyError exception that may not be correctly be translated to an HTTP 500 response.

This change makes the variable optional.

What kind of change does this PR introduce?

  • 🐞 bug fix
  • 🐣 feature
  • 📋 docs update
  • 📋 tests/coverage improvement
  • 📋 refactoring
  • 💥 other

📋 What is the related issue number (starting with #)

What is the current behavior? (You can also link to an open issue here)

A KeyError exception is raised when dispatching a request that does not set SCRIPT_NAME in the WSGI environ dictionary. Unless the PathInfoDispatch is wrapped into some other WSGI middleware, the exception may not be translated to an HTTP 500 error.

What is the new behavior (if this is a feature change)?

The request is routed as though SCRIPT_NAME contained an empty string.

📋 Other information:

See PEP 333 (and the quote above).

📋 Checklist:

  • I think the code is well written
  • I wrote good commit messages
  • I have squashed related commits together after the changes have been approved
  • Unit tests for the changes exist
  • Integration tests for the changes exist (if applicable)
  • I used the same coding conventions as the rest of the project
  • The new code doesn't generate linter offenses
  • Documentation reflects the changes
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences

This change is Reviewable

@AndreLouisCaron AndreLouisCaron force-pushed the AndreLouisCaron:optional-script-name branch from 1c8029d to aba54c9 Dec 19, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 19, 2018

Codecov Report

Merging #149 into master will decrease coverage by 0.35%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
- Coverage   74.26%   73.91%   -0.36%     
==========================================
  Files          22       23       +1     
  Lines        3447     3385      -62     
==========================================
- Hits         2560     2502      -58     
+ Misses        887      883       -4
Make SCRIPT_NAME environ variable optional
PEP 333 makes this CGI-style variable optional.  See the the section titled "environ variables":

> The following variables must be present, unless their value would be an empty string, in which case they may be omitted, except as otherwise noted below.

Some WSGI middleware does not set this environ variable.  When combined with `cheroot.wsgi.PathInfoDispatcher`, there is a confusing `KeyError` exception that may not be correctly be translated to an HTTP 500 response.

This change makes the variable optional.

@jaraco jaraco merged commit c4a60d6 into cherrypy:master Dec 20, 2018

5 of 6 checks passed

ci/circleci: macos-build Your tests failed on CircleCI
Details
LGTM analysis: Python No alert changes
Details
ci/circleci: linux-build Your tests passed on CircleCI!
Details
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@webknjaz

This comment has been minimized.

Copy link
Member

webknjaz commented Dec 20, 2018

@AndreLouisCaron thanks for this patch! 🎉

jaraco added a commit that referenced this pull request Dec 20, 2018

@AndreLouisCaron

This comment has been minimized.

Copy link
Contributor

AndreLouisCaron commented Dec 27, 2018

Out of curiosity, what is the usual release cycle? I'm not in a hurry to get a release, but I'd like to know when I should be able to upgrade to a version with this fix.

Thanks a bunch!

@webknjaz

This comment has been minimized.

Copy link
Member

webknjaz commented Dec 27, 2018

There's no regular release schedule, we cut releases once we feel like there's something worth releasing or if somebody asks to make it. Our release process is quite well automated so we just bump the version by pushing a new Git tag as per semver. After that, it gets published by Travis CI job.

@AndreLouisCaron

This comment has been minimized.

Copy link
Contributor

AndreLouisCaron commented Dec 27, 2018

Thanks for the info! I'll be back at work by Jan 7th, can you make a bug fix release happen by then, by any chance?

@webknjaz

This comment has been minimized.

Copy link
Member

webknjaz commented Dec 27, 2018

I've already triggered the process. I'll be released once https://travis-ci.org/cherrypy/cheroot/builds/472748498 finishes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment