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

Setting SCRIPT_NAME environment variable when mounting Rack apps #87

Closed
stravid opened this issue Jan 25, 2016 · 6 comments
Closed

Setting SCRIPT_NAME environment variable when mounting Rack apps #87

stravid opened this issue Jan 25, 2016 · 6 comments
Assignees
Milestone

Comments

@stravid
Copy link
Contributor

stravid commented Jan 25, 2016

When I setup Sidekiq with Hanami I ran into an issue when mounting the Sidekiq web interface within an applications route or the container. All URLs for links and assets in Sidekiq were broken because they pointed to the root of my Hanami application even if I mounted it at /sidekiq for example.

Turns out Sidekiq uses SCRIPT_NAME to build its URLs and it seems that Hanami only sets it for Hanami apps and not Rack apps in general.

Reading the Rack specification I think Hanami should set SCRIPT_NAME accordingly. Rack::URLMap does it too for example.

The initial portion of the request URL's “path” that corresponds to the application object, so that the application knows its virtual “location”. This may be an empty string, if the application corresponds to the “root” of the server.

What do you think?

@jodosha
Copy link
Member

jodosha commented Feb 4, 2016

There is a PR by @cyphactor that should fix this problem directly into http_router (See joshbuddy/http_router#43).

If there isn't a quick merge and release of http_router, we can act now and port that patch here. To solve this kind of problems that we had in the past Hanami::Router wraps Hanami::Routing::HttpRouter which is a subclass of HttpRouter (from http_router gem).

@cyphactor It would be really helpful if you can port it here. Thank you very much!

@jodosha jodosha modified the milestone: v0.6.2 Feb 4, 2016
@drewdeponte
Copy link
Contributor

I still haven't seen any movement on the http_router PR (joshbuddy/http_router#43) sadly. Hence, I don't expect there to be any movement in terms of getting it merged in and released any time soon.

@jodosha I will try to port it to hanami-router today and get a PR up.

@jodosha jodosha modified the milestones: v0.6.2, next Feb 5, 2016
drewdeponte added a commit to Acornsgrow/router that referenced this issue Feb 9, 2016
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
in HttpRouter 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.

This change also includes a fix to handle maintaining URI encoded values
in PATH_INFO and SCRIPT_INFO. Currently, there is a problem where
because the method uses the request.path helper HttpRouter provides,
which URI.decodes the string, it screws up both the PATH_INFO and
SCRIPT_NAME. Therefore, I URI.encode it back appropriately before any
offsetting, or setting is done.

I have also added tests for each of these cases as I couldn't find any
tests covering this before.

This fixes issue hanami#87.
@jodosha
Copy link
Member

jodosha commented Feb 12, 2016

Closed by #91 Thanks to @cyphactor 👏

@jodosha jodosha closed this as completed Feb 12, 2016
@jodosha jodosha self-assigned this Feb 12, 2016
@jamesdabbs
Copy link

jamesdabbs commented Jul 26, 2016

Hey folks. I'm mucking around trying to get Hanami running on Rack 2 and ran into an issue with one of these tests that I thought would be good to bring up. Here's what I found -

At and around https://github.com/joshbuddy/http_router/blob/master/lib/http_router/node/path.rb#L32, the behavior of a Rack Request has changed. The salient difference is that in Rack < 2, request.rack_request.dup.env is the same object as request.rack_request.env (which begs the question - why the dup?), and so passing env on to the rewrites (which call env[STUFF] += ...) does actually update the initial request.env. Normally this doesn't matter because the new env is what's used going forward buuuuut it isn't the env that Rack::Test::Methods#last_request points you back to ... sooo https://github.com/hanami/router/blob/master/test/request_url_test.rb#L43 has the old un-rewritten request and env.

I took a swing at rewriting the test to capture its intent on my fork; does that seem reasonable? FWIW, I think this is a non-issue if this logic moves upstream to http_router.

Meta-level question: would y'all be up for bumping to rack ~> 2.0 (or >= 1.6?) so we could e.g. mount alongside a Rails 5.0 app? Happy to open a new issue to discuss if you'd like.

@jodosha
Copy link
Member

jodosha commented Aug 8, 2016

@jamesdabbs Thanks for having a look at this issue with Rack 2. The ideal scenario would be to support Rack >= 1.6 (so 2.0 is included).

The improved spec in your fork makes sense, but we should find a way to make it to pass for both Rack versions.

At this point backward compat is more important than Rack 2. This because we have more Ruby web apps still running on Rack 1.6 (see Rails 4, Hanami etc..) than Rack 2 (Rails 5).


If Rack 2 requires a deep intervention on http_router, this gonna be an issue because that gem is unmaintained. As post Hanami 1.0 task is to port hanami-router to roda.

@drewdeponte
Copy link
Contributor

@jamesdabbs when you say "request.rack_request.dup.env is the same object as request.rack_request.env" do they actually share the same object id?

Duping rack requests is a common way of protecting things to support multi-threading. It is also used at the rack middleware/app level for the same reason. I am not sure that is the reason in this particular case but possibility worth exploring.

http://stackoverflow.com/questions/23028226/rack-middleware-and-thread-safety

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

No branches or pull requests

5 participants