Fix empty SCRIPT_NAME with partial match route bug #43
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Why you made the change: I made this change so that SCRIPT_NAME and PATH_INFO would be set appropriately in the scenario where a partial match route is used. How the change addresses the need: There is an issue with the current version of rewrite_partial_path_info where it does not set SCRIPT_NAME appropriately. When the request environments PATH_INFO is set, it causes the request.rack_request.path_info helper return the updated version, not the original. This is requst.rack_request.path_info eventually just reads the PATH_INFO value out of the request environment. As a side effect of this, the original code would always set the SCRIPT_NAME to "" because it would be slicing from request.rack_request.path_info[0, 0] in actuality. This change resolves this issue by first temporarily storing a dup of the original path_info and using that to determine the end index for the SCRIPT_NAME. This change also handles setting SCRIPT_NAME correctly for the somewhat unique but common case of matching the partial match exactly. I have added tests for each of these cases as I couldn't find any tests covering this before.
Fix empty SCRIPT_NAME with partial match route bug
Why you made the change: I did this so it was ancient, as well as because there was a missmatch between the version of rake specified and how it was used which was having problems. Upgrading to the latest version of rake resolves this issue so that `bundle exec rake` can successfully be run.
Why you made the change: I did this because travis ci was failing for weird reasons and figured maybe it had something to do with the default .travis.yml.
Why you made the change: I did this in hopes to avoid the `NoMethodError: undefined method `spec' for nil:NilClass` while using bundler 1.7.6 which is travis default bundler version.
Why you made the change: I did this because that is the default travis was testing against before so figured we shouldn't change it.
This reverts commit 14043cf.
This reverts commit 5170ae7.
|
@joshbuddy is there any chance you could review this PR as it fixes a bug in http_router? @jodosha do you have any pull with getting this in? It is the fix that resolves an actual bug in http_router, but also makes http://hanamirb.org work properly with http://sidekiq.org. |
I did this because sometimes the request path will be encoded. Since HTTP router unencodes the path, it loses the ability to correctly calculate the path info by performing math on the path length. This encodes the path info since it is used for correctly assigning the script name.
Add URI encoding for path info
|
@joshbuddy, please review this PR we need this on only for sidekiq ❤️💛💜💗💙 |
|
Will take a look!
… On Mar 1, 2017, at 14:44, Anton Davydov ***@***.***> wrote:
@joshbuddy, please review this PR we need this on only for sidekiq ❤️💛💜💗💙
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@joshbuddy you're my hero 🎉 P.S.: WDYT maybe it'll good to add someone from hanami core for helping with maintaining the project? Because we have some errors with this gem and we will be happy to fix it ASAP. /cc @jodosha |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why you made the change:
I made this change so that SCRIPT_NAME and PATH_INFO would be set
appropriately in the scenario where a partial match route is used.
How the change addresses the need:
There is an issue with the current version of rewrite_partial_path_info
where it does not set SCRIPT_NAME appropriately. When the request
environments PATH_INFO is set, it causes the request.rack_request.path_info
helper return the updated version, not the original. This is
requst.rack_request.path_info eventually just reads the PATH_INFO value
out of the request environment. As a side effect of this, the original
code would always set the SCRIPT_NAME to "" because it would be slicing
from request.rack_request.path_info[0, 0] in actuality.
This change resolves this issue by first temporarily storing a dup of
the original path_info and using that to determine the end index for the
SCRIPT_NAME. This change also handles setting SCRIPT_NAME correctly for
the somewhat unique but common case of matching the partial match
exactly.
I have added tests for each of these cases as I couldn't find any tests
covering this before.