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

Linear router fails to match route/req when it ends with a trailing slash #538

Open
mpenet opened this Issue Oct 11, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@mpenet

mpenet commented Oct 11, 2017

Description

Linear router fails to match route when it ends with a trailing slash, other router types are working as expected.

Expected Behavior

It should match with all router types the same way, preferably the way map-tree and prefix tree routers match now.

Actual Behavior

It doesn't match with linear-search router

Steps to reproduce

(require '[io.pedestal.http.route.definition.table :as table])
(require '[io.pedestal.http.route.map-tree :as map-tree])
(require '[io.pedestal.http.route.linear-search :as linear])
(require '[io.pedestal.http.route.prefix-tree :as prefix])
(require '[io.pedestal.http.route.router :as router])

(def rt (table/table-routes [["/foo/bar/" :get (constantly "") :route-name ::foobar]]))
(def req  {:path-info "/foo/bar/" :request-method :get})

;; map-tree works as expected
(-> rt map-tree/router  (router/find-route req))
;; => {:path "/foo/bar/", :method :get, :path-re #"/\Qfoo\E/\Qbar\E", :path-parts ["foo" "bar"], :interceptors [#Interceptor{:name }], :route-name :boot.user/foobar, :path-params [], :io.pedestal.http.route.prefix-tree/satisfies-constraints? #function[clojure.core/constantly/fn--5396]}

;; prefix too 
(-> rt prefix/router  (router/find-route req))
;; => {:path "/foo/bar/", :method :get, :path-re #"/\Qfoo\E/\Qbar\E", :path-parts ["foo" "bar"], :interceptors [#Interceptor{:name }], :route-name :boot.user/foobar, :path-params {}, :io.pedestal.http.route.prefix-tree/satisfies-constraints? #function[clojure.core/constantly/fn--5396]}

;; linear fails
(-> rt linear/router  (router/find-route req))
;; => nil

Pedestal version

0.5.3

@mpenet

This comment has been minimized.

Show comment
Hide comment
@mpenet

mpenet Oct 11, 2017

Another quirk from linear when the request path is not valid and it ends up matching anyway:

(require '[io.pedestal.http.route.definition.table :as table])
(require '[io.pedestal.http.route.map-tree :as map-tree])
(require '[io.pedestal.http.route.linear-search :as linear])
(require '[io.pedestal.http.route.prefix-tree :as prefix])
(require '[io.pedestal.http.route.router :as router])

(def rt (table/table-routes [["/foo/bar/" :get (constantly "") :route-name ::foobar]]))
(def req  {:path-info "/foo/bar" :request-method :get})

;; map-tree works as expected
(-> rt map-tree/router  (router/find-route req))
;; => nil

;; prefix too 
(-> rt prefix/router  (router/find-route req))
;; => nil

;; linear matches
(-> rt linear/router  (router/find-route req))
;; => {:path "/foo/bar/", :method :get, :path-re #"/\Qfoo\E/\Qbar\E", :path-parts ["foo" "bar"], :interceptors [#Interceptor{:name }], :route-name :boot.user/foobar, :path-params {}, :matcher #function[io.pedestal.http.route.linear-search/matcher/fn--19001]}

My opinion is that they should all match the same way, preferably without doing any magic regarding trailing slashes as this is more a concern for the developer than for the router, especially if the route lib wants to be protocol agnostic and truly standalone.

Route is an impressive library, thanks for your work on it.
Maybe it would benefit from some exposure to potential uses in other contexts, as it is not something obvious to the outsider.

mpenet commented Oct 11, 2017

Another quirk from linear when the request path is not valid and it ends up matching anyway:

(require '[io.pedestal.http.route.definition.table :as table])
(require '[io.pedestal.http.route.map-tree :as map-tree])
(require '[io.pedestal.http.route.linear-search :as linear])
(require '[io.pedestal.http.route.prefix-tree :as prefix])
(require '[io.pedestal.http.route.router :as router])

(def rt (table/table-routes [["/foo/bar/" :get (constantly "") :route-name ::foobar]]))
(def req  {:path-info "/foo/bar" :request-method :get})

;; map-tree works as expected
(-> rt map-tree/router  (router/find-route req))
;; => nil

;; prefix too 
(-> rt prefix/router  (router/find-route req))
;; => nil

;; linear matches
(-> rt linear/router  (router/find-route req))
;; => {:path "/foo/bar/", :method :get, :path-re #"/\Qfoo\E/\Qbar\E", :path-parts ["foo" "bar"], :interceptors [#Interceptor{:name }], :route-name :boot.user/foobar, :path-params {}, :matcher #function[io.pedestal.http.route.linear-search/matcher/fn--19001]}

My opinion is that they should all match the same way, preferably without doing any magic regarding trailing slashes as this is more a concern for the developer than for the router, especially if the route lib wants to be protocol agnostic and truly standalone.

Route is an impressive library, thanks for your work on it.
Maybe it would benefit from some exposure to potential uses in other contexts, as it is not something obvious to the outsider.

@ohpauleez

This comment has been minimized.

Show comment
Hide comment
@ohpauleez

ohpauleez Oct 11, 2017

Member

Thanks for the report @mpenet! I'll take a look at this today.

We already have a collection of tests around similar trailing-slash behavior, so once I dig in I'll report back here. I agree with your assessment and your general opinion around the expectations across multiple routers- the only thing that might prevent that is legacy behavior in the linear router (which would be illustrated in the tests), but I don't think that's the case here.

Member

ohpauleez commented Oct 11, 2017

Thanks for the report @mpenet! I'll take a look at this today.

We already have a collection of tests around similar trailing-slash behavior, so once I dig in I'll report back here. I agree with your assessment and your general opinion around the expectations across multiple routers- the only thing that might prevent that is legacy behavior in the linear router (which would be illustrated in the tests), but I don't think that's the case here.

@ohpauleez

This comment has been minimized.

Show comment
Hide comment
@ohpauleez

ohpauleez Oct 11, 2017

Member

A quick triage: There is a small gap in the test suite for this particular case, so I'll add that to ensure we defend against regressions.

The linear router works against a series of predicate functions that match various things (host, scheme, constraints, etc.). The path matching portion works against the :path-re regular expression, which gets calculated in path-regex and doesn't include trailing slashes. Historically folks who really needed this worked around it with an interceptor that modified the incoming path-info of a request to drop the trailing slash.

Originally, I think this was done to discourage route-tables with trailing slashes. In the new router implementations (and in url-for), trailing-slashes are discouraged but allowed and behave as expected.

It seems like the linear router was never updated to preserve this behavior and to avoid a breaking change. At this point, I think most Pedestal services are on prefix or map tree routers, and potentially we want to make this breaking change. @mtnygard Any thoughts?

Member

ohpauleez commented Oct 11, 2017

A quick triage: There is a small gap in the test suite for this particular case, so I'll add that to ensure we defend against regressions.

The linear router works against a series of predicate functions that match various things (host, scheme, constraints, etc.). The path matching portion works against the :path-re regular expression, which gets calculated in path-regex and doesn't include trailing slashes. Historically folks who really needed this worked around it with an interceptor that modified the incoming path-info of a request to drop the trailing slash.

Originally, I think this was done to discourage route-tables with trailing slashes. In the new router implementations (and in url-for), trailing-slashes are discouraged but allowed and behave as expected.

It seems like the linear router was never updated to preserve this behavior and to avoid a breaking change. At this point, I think most Pedestal services are on prefix or map tree routers, and potentially we want to make this breaking change. @mtnygard Any thoughts?

@ohpauleez

This comment has been minimized.

Show comment
Hide comment
@ohpauleez

ohpauleez Oct 11, 2017

Member

I'd also love to see anyone else from the user community weigh in-

Are there folks using the linear router that would be broken if the change was made?

Member

ohpauleez commented Oct 11, 2017

I'd also love to see anyone else from the user community weigh in-

Are there folks using the linear router that would be broken if the change was made?

@mpenet

This comment has been minimized.

Show comment
Hide comment
@mpenet

mpenet Oct 18, 2017

Thanks for looking into this.
That is a blocking issue for us, we re trying to port a legacy app to pedestal route to allow a migration path down the road to the other routers once a grace period passes, but we would need to rely on ordering of routes given how they are structured atm. And we do have routes with trailing slashes mixed into that.

mpenet commented Oct 18, 2017

Thanks for looking into this.
That is a blocking issue for us, we re trying to port a legacy app to pedestal route to allow a migration path down the road to the other routers once a grace period passes, but we would need to rely on ordering of routes given how they are structured atm. And we do have routes with trailing slashes mixed into that.

@ohpauleez

This comment has been minimized.

Show comment
Hide comment
@ohpauleez

ohpauleez May 2, 2018

Member

Circling back- Let's make this breaking change. Users have had enough time to upgrade to other router implementations (and most new Pedestal services are using the new routers). This change will bring behavior and expectations inline for all routers.

Member

ohpauleez commented May 2, 2018

Circling back- Let's make this breaking change. Users have had enough time to upgrade to other router implementations (and most new Pedestal services are using the new routers). This change will bring behavior and expectations inline for all routers.

@ohpauleez ohpauleez added the primed label May 2, 2018

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