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

Should fnhouse look at the :path-info key? #32

Open
gfredericks opened this issue Sep 3, 2015 · 6 comments
Open

Should fnhouse look at the :path-info key? #32

gfredericks opened this issue Sep 3, 2015 · 6 comments

Comments

@gfredericks
Copy link
Contributor

We just converted a bunch of routes from compojure to fnhouse, and noticed that they didn't work when deployed to a jboss context (with a route prefix).

As best as I can tell, the request coming in from the jboss machinery has at least three interesting keys:

{...
 :context "/my-prefix"
 :uri "/my-prefix/and/the/path"
 :path-info "/and/the/path"
 ...}

It seems that compojure was able to use the :path-info key when available, but fnhouse does not.

The ring spec does not mention :path-info or :context, so I can't say how standard these are.

Obviously there are some easy workarounds here, but I was wondering if fnhouse ought to support this somehow.

@w01fe
Copy link
Member

w01fe commented Sep 3, 2015

Thanks for the report.

My inclination is that this is better handled in middleware around the fnhouse root handler than within fnhouse itself. That said, I'm not familiar with jboss; does that seem reasonable, or do you have an argument for why fnhouse should know about this?

@gfredericks
Copy link
Contributor Author

I think the argument would only make sense if :path-info is the standard way to communicate "your app has been deployed in some kind of container with a path prefix and here is the unprefixed path`.

It does feel pretty middlewarey to me too though, or even like maybe things like jboss should use :uri for the shortened path and put the full thing somewhere else.

I wonder if @tobias has any context (ha!) on this.

@w01fe
Copy link
Member

w01fe commented Sep 3, 2015

Sounds reasonable to me -- let us know if you figure out the answer :) Thanks!

@jcrossley3
Copy link

Hi guys. @tobias referred me. This is not a jboss issue; it's a servlet issue. Neither :context nor :path-info are a part of the ring spec because they're not a part of the http spec. However, they are both part of the servlet spec, and both ring-core and compojure honor them. IMO, any routing library that expects to be deployed in a servlet container (jboss is but one) should honor them. That's not to say they can't be "honored" via middleware, of course.

@gfredericks
Copy link
Contributor Author

thanks @jcrossley3

Another detail, @w01fe, is that (after glancing at the source code) I believe compojure uses :path-info internally for its own route-prefix mechanism, so that jboss-things and expressions like (context "/foo/:bar" ...) are indistinguishable from the individual handler's perspective.

Fnhouse I think just applies prefixes in a different fashion, using nss->handlers-fn.

In any case I'll leave the final judgement up to you. I'm currently using this middleware, is this what you had in mind or would it be different somehow?

(defn- wrap-fnhouse-path-info-hack
  "Copies the :path-info key of the request to :uri when present, so
  that our routes work when deployed to JBOSS.

  See https://github.com/Prismatic/fnhouse/issues/32 for details."
  [handler]
  (fn [request]
    (-> request
        (cond-> (contains? request :path-info)
          (assoc :uri (:path-info request)
                 ::orig-uri (:uri request)))
        (handler))))

@w01fe
Copy link
Member

w01fe commented Sep 4, 2015

Thanks for the context, @jcrossley3 -- that's very helpful.

@gfredericks yep, that looks like what I had in mind.

I guess I don't see any harm in concatenating the path-info here, since it seems like probably the least surprising option:

https://github.com/Prismatic/fnhouse/blob/master/src/fnhouse/routes.clj#L129

Or as an alternative, we could add your middleware to the middleware.clj file so it's ready-at-hand.

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

No branches or pull requests

3 participants