Make reverse routes always use the first matching route

[gmethvin]

Fixes #3050. The previous way of removing the extra routes would prioritize the last matching route as opposed to the first one. Now it should work the same as in 2.2, but without the warning.

[cchantep]

LGTM

[jroper]

No! This will create a conflict with what I'm working on!! sigh

[jroper]

Backported to 2.3.x b0388f9

[jroper]

I think this might have caused some problems. In playframework.com, we have the following routes:

GET      /documentation/latest                                      controllers.Documentation.latest(page = "Home")
GET      /documentation/latest/:page                                controllers.Documentation.latest(page)

Now this is generating this:

def latest(page:String): Call = {
   (page: @unchecked) match {
// @LINE:42
case (page)  =>
  import ReverseRouteContext.empty
  Call("GET", _prefix + { _defaultPrefix } + "documentation/latest/" + implicitly[PathBindable[String]].unbind("page", dynamicString(page)))

// @LINE:41
case (page) if page == "Home" =>
  implicit val _rrc = new ReverseRouteContext(Map(("page", "Home")))
  Call("GET", _prefix + { _defaultPrefix } + "documentation/latest")

   }
}

As you can see, the ordering there is reversed, and I get an unreachable code warning.

[jroper]

So I think the problem is here - groupBy returns a Map, so the order of the routes is lost. The right duplicate is selected, but the order of non duplicates is now random by hash code.

[gmethvin]

Ah, now it makes sense why we were using a ListMap. I think it would've been easier to just reverse the list before creating the ListMap.

I'll submit a PR to fix.

[jroper]

I've just cut 2.3.1, found this in a smoke test. I don't think it's a big enough regression to warrant immediately cutting a new release - it just means that at random, the a less specific route might be chosen, resulting in a cosmetically different URL - the URL produced will still cause the same action to be invoked. What do you think?

[jroper]

Or another way of looking at it, we've replaced one very minor reverse routes priority bug with another very minor reverse routes priority bug.

[gmethvin]

The functionality is the same but the warning is a bit annoying/confusing, even if the behavior is correct.

[fmw]

I would definitely appreciate a release for this fix, since the cosmetic difference in the generated URL can be problematic. In our case it adds a trailing slash to a very common URL in our application because it is using the more specific version of the route (i.e. /a/b/c) to generate the less specific version (i.e. /a/b), which results in /a/b/.

[jroper]

Ok, well we have 3 fixes already, we'll cut 2.3.2 next week.