matching routes with CompiledPrefix fails

[mwmitchell]

Hi,

This code throws an exception. Is this kind of (prefix) route supported?

(bidi/match-route
 (bidi/compile-route
  [["/base/path/" [#".+" :id]] {:get :match!}])
 "/base/path/1"
 :request-method :get)

java.lang.IllegalArgumentException: No implementation of method: :segment-regex-group of protocol: #'bidi.bidi/PatternSegment found for class: bidi.bidi.CompiledPrefix core_deftype.clj:544 clojure.core/-cache-protocol-fn bidi.clj:69 bidi.bidi/eval13078[fn] core.clj:2557 clojure.core/map[fn] LazySeq.java:40 clojure.lang.LazySeq.sval LazySeq.java:49 clojure.lang.LazySeq.seq RT.java:484 clojure.lang.RT.seq core.clj:133 clojure.core/seq core.clj:2551 clojure.core/map[fn] LazySeq.java:40 clojure.lang.LazySeq.sval LazySeq.java:49 clojure.lang.LazySeq.seq RT.java:484 clojure.lang.RT.seq core.clj:133 clojure.core/seq protocols.clj:26 clojure.core.protocols/seq-reduce protocols.clj:53 clojure.core.protocols/fn protocols.clj:13 clojure.core.protocols/fn[fn] core.clj:6287 clojure.core/reduce bidi.clj:200 bidi.bidi/eval13310[fn] bidi.clj:148 bidi.bidi/eval13210[fn] bidi.clj:164 bidi.bidi/match-pair bidi.clj:286 bidi.bidi/match-route

[malcolmsparks]

Thanks for this. I've noticed exactly the same thing recently - it may be a regression or may never have worked. I thought the code should pass through the prefix route as is if it can't optimise it. I'll investigate.

[AlexBaranosky]

I"m seeing the same thing:

(def r ["/" {["sleep/" :foo] :bar}])

(bidi/match-route (bidi/compile-route r) "/sleep/50")

It looks like the where you extend-protocol PatternSegment CompiledRoute has no method for PatternSegment and boom. If I get time, I'll investigate :)

[AlexBaranosky]

This code added to the defrecord code for CompiledPrefix doesn't quite work:

PatternSegment
  (segment-regex-group [this] (segment-regex-grop regex))
  (param-key [_] (pattern-key regex))
  (transform-param [_] (transform-param regex))
  (matches? [this s] (matches? regex s))

(deftest compile-routes-test
  (let [route (compile-route ["/" {["sleep/" :foo] :bar}])]
    (is (= {:params {:foo "50"}
              :handler :bar}
           (match-route route "/sleep/50")))))

Any ideas?

[bobby]

I have also recently encountered this issue. If I have time, I may dig into it a bit.

Any update from @malcolmsparks?

[malcolmsparks]

@bobby @mwmitchell I've spent a good while looking at this today. Thanks for these reports. The problem is due to the fact that the compile-route function does not distinguish between Strings that are part of patterns, and Strings that are part of pattern vectors.

I've approached the problem in a different way, by using the same recursion technique used for resolve-handler and unresolve-handler. That way, the context of the String can be determined. That means that core.match no longer serves, so I've removed it.

I've just released 1.18.6. I don't think that compilation will work yet for ClojureScript, but I think the compilation matters more on the server, so I've released anyway.

Please try this out against your use-cases. The tests pass, but I'm interested to know whether this release solves things for your cases. I haven't yet determined the performance gain, but it should be in the same ball-park as before because all the main regular expressions are now pre-compiled, just as they are with Compojure.

[malcolmsparks]

I've just released 1.18.6. I don't think that compilation will work yet for ClojureScript, but I think the compilation matters more on the server, so I've released anyway.

It should in 1.18.7

[bobby]

Hi @malcolmsparks!

Bidi has been really helpful on my current project, so many thanks.

Also, thank you for working on this issue! It looks like the latest implementation of (compile-route) doesn't handle the case of representing nested routes as a vector of pairs (i.e. instead of a map when order of routes matters). Relevant chunk of stacktrace:

java.lang.IllegalArgumentException: No implementation of method: :compile-segment of protocol: #'bidi.bidi/Compilable found for class: clojure.lang.PersistentVector
            clojure.core/-cache-protocol-fn                  core_deftype.clj:  555
                   bidi.bidi/eval24722/fn/G                          bidi.clj:  380
                       clojure.core/mapv/fn                          core.clj: 6611
                                        ...                                        
                        clojure.core/reduce                          core.clj: 6513
                          clojure.core/mapv                          core.clj: 6611
                     bidi.bidi/eval24790/fn                          bidi.clj:  399
                   bidi.bidi/eval24722/fn/G                          bidi.clj:  380
                    bidi.bidi/compile-route                          bidi.clj:  386
                       clojure.core/mapv/fn                          core.clj: 6611
                  clojure.core.protocols/fn                     protocols.clj:   84
                clojure.core.protocols/fn/G                     protocols.clj:   13
                        clojure.core/reduce                          core.clj: 6514
                          clojure.core/mapv                          core.clj: 6611
                     bidi.bidi/eval24778/fn                          bidi.clj:  410
                   bidi.bidi/eval24722/fn/G                          bidi.clj:  380
                    bidi.bidi/compile-route                          bidi.clj:  386
                       clojure.core/mapv/fn                          core.clj: 6611
                  clojure.core.protocols/fn                     protocols.clj:   84
                clojure.core.protocols/fn/G                     protocols.clj:   13
                        clojure.core/reduce                          core.clj: 6514
                          clojure.core/mapv                          core.clj: 6611
                     bidi.bidi/eval24778/fn                          bidi.clj:  410
                   bidi.bidi/eval24722/fn/G                          bidi.clj:  380
                    bidi.bidi/compile-route                          bidi.clj:  386
                       clojure.core/mapv/fn                          core.clj: 6611
                  clojure.core.protocols/fn                     protocols.clj:   84
                clojure.core.protocols/fn/G                     protocols.clj:   13
                        clojure.core/reduce                          core.clj: 6514
                          clojure.core/mapv                          core.clj: 6611
                     bidi.bidi/eval24778/fn                          bidi.clj:  410
                   bidi.bidi/eval24722/fn/G                          bidi.clj:  380
                    bidi.bidi/compile-route                          bidi.clj:  386
                       clojure.core/mapv/fn                          core.clj: 6611
                                        ...                                        
                        clojure.core/reduce                          core.clj: 6513
                          clojure.core/mapv                          core.clj: 6611
                     bidi.bidi/eval24790/fn                          bidi.clj:  400
                   bidi.bidi/eval24722/fn/G                          bidi.clj:  380
                    bidi.bidi/compile-route                          bidi.clj:  386

I'll see if I can work up a pull request.

[bobby]

Err, with further testing, I've discovered that the nested routes isn't the problem. I'll work up a minimally reproducing case soon.

[malcolmsparks]

Hi @bobby,

Great to hear from you.

I did focus on the vector of pairs use-case, so it should work, so if it doesn't it's because of a mistake on my part. I will add some tests once I'm satisfied this is a viable design - dev feedback is usually a great indicator.

Hmm. Perhaps clojure.lang.APersistentVector should be clojure.lang.PersistentVector. I must confess I'm not always 100% sure which type to use with protocols.

If it helps, you can install bidi locally, with 'lein bidi install', and the tests are easily run with 'lein test'. The cljx stuff means you can't put it in your project's checkouts, but it should be easy enough to verify if this change fixes your problem (ok, enough teaching grandmother how to suck eggs here ;) Any failing tests or descriptions would be much appreciated. I think I need to have a test suite that tests compile-route against many possible cases.

Regards,

Malcolm

On 24 February 2015 at 17:43, Bobby Calderwood [email protected] wrote:

Err, with further testing, I've discovered that the nested routes isn't the problem. I'll work up a minimally reproducing case soon.

— Reply to this email directly or view it on GitHub https://github.com/juxt/bidi/issues/17#issuecomment-75806270.

[bobby]

Turns it is is the regex/keyword/uuid matching case that is the problem. Minimally reproducing case:

user> (def routes [["/" [keyword :id] ".html"] :int])
#'user/routes
user> (bidi/match-route routes "/foo.html")
{:handler :int, :route-params {:id :foo}}
user> (bidi/compile-route routes)
java.lang.IllegalArgumentException: No implementation of method: :compile-segment of protocol: #'bidi.bidi/Compilable found for class: clojure.lang.PersistentVector

[bobby]

@malcolmsparks Any ideas about the minimally reproducing case I posted a while back? The issue seems to be around the nested [keyword :id] vector.

[bobby]

@malcolmsparks bump. Would you like me to take a crack at this? I may have time this weekend. If so, should we re-open the issue?

[malcolmsparks]

Yes please. I've been really busy lately and it would be really helpful.

If we had a default implementation bidi.bidi/Compilable that returned the identity, wouldn't that work?

"To define a default implementation of protocol (for other than nil) just use Object" -- http://clojure.org/protocols

Perhaps you've tried already, but if not it's worth a go.

[malcolmsparks]

Just checked, we already have that.

user> (def routes [["/" [keyword :id] ".html"] :int])
#'user/routes
user> (bidi/match-route routes "/foo.html")
{:handler :int, :route-params {:id :foo}}
user> (bidi/compile-route routes)
[[#"\Q/\E" [#<core$keyword clojure.core$keyword@147eac39> :id] #"\Q.html\E"] :int]
user> (bidi/match-route (bidi/compile-route routes) "/foo.html")
{:handler :int, :route-params {:id :foo}}
user> 

I think the problem is fixed. Can you confirm?

[bobby]

I can confirm that compiled routes under the case above works as of 1.18.9 (which doesn't appear in Releases, btw). Looks like it was fixed sometime between the releases of the Feb 24th and now. Thanks, and sorry for the churn!