PathMatcher DSL: Map Matcher problem (?) #394

Closed
sbrugnoni opened this Issue Oct 15, 2016 · 7 comments

Projects

None yet

4 participants

@sbrugnoni

Given the following setup, describing two resources "/a" and "/aa", all requests to "/aa" get rejected.

val route = path(Map("a" -> 1, "aa" -> 2)) {
  res => get {
    complete(res.toString)
  }
}
Get("/a") ~> route ~> check { println(responseAs[String]) } // OK
Get("/aa") ~> route ~> check { println(responseAs[String]) } // rejected

According to the documentation, the Map[String, T] matcher should match "any of the keys and extracts the respective map value for it".
See Documentation

I assume the cause for this issue is common prefix (in this case "a") of the two resources.

If the order of the resources in the definition of the route is reversed, it works as expected:

val route = path(Map("aa" -> 2, "a" -> 1)) {
  res => get {
    complete(res.toString)
  }
}
Get("/a") ~> route ~> check { println(responseAs[String]) } // OK
Get("/aa") ~> route ~> check { println(responseAs[String]) } // OK

I have a unit test that reproduces this problem with akka-http-experimental 2.4.11

@sbrugnoni

I could have a look in the code to see if I can find and fix the problem.
However, it would be great if somebody could confirm that this is actually a bug :)

@jonas
Contributor
jonas commented Oct 15, 2016

Reproducible with the following test in PathDirectivesSpec.scala:

  "path(Map(\"sv\" -> 1, \"sv_SE\" -> 2, \"en\" -> 3))" should {
    val test = testFor(path(Map("sv"  1, "sv_SE"  2, "en"  3)) { echoCaptureAndUnmatchedPath })
    "accept [/sv]" inThe test("1:")
    "accept [/sv_SE]" inThe test("2:")
    "reject [/sv_FI]" inThe test()
    "reject [/fr]" inThe test()
  }

Looking at the Scaladoc it sounds like a bug, but I will let somebody from the Akka team answer that question.

Creates a PathMatcher from the given Map of path segments (prefixes) to extracted values.
If the unmatched path starts with a segment having one of the maps keys as a prefix
the matcher consumes this path segment (prefix) and extracts the corresponding map value.

@gosubpl
Contributor
gosubpl commented Oct 16, 2016 edited

@jonas I am not sure that the PathMatcher definition you have quoted allows actually for overlapping segments. It clearly states that the segments should be treated as prefixes to the extracted values.

This aspect is clearly reflected in the conversion implicit - https://github.com/akka/akka-http/blob/master/akka-http/src/main/scala/akka/http/scaladsl/server/PathMatcher.scala#L295-L298

On the other hand, looking into docs, it is stated (as @sbrugnoni has quoted) that one can use Map[String, T] of paths (not prefixes) to get them matched. This is also underlined here by the usage of the path (not pathPrefix) directive.

All in all looks like a complex interplay between path, PathMatcher and the behaviour of Scala Map. Please note that the order cannot be guaranteed for the Map built with standard immutable.Map builder object beyond 4 elements.

A couple of interesting test cases (run in the context of PathDirectivesSpec):

 """path(Map("a" -> 1, "ab" -> 2, "ba" -> 3, "b" -> 4, "c" -> 5, "d" -> 6, "da" -> 7))""" should {
    val test = testFor(path(Map("a" -> 1, "ab" -> 2, "ba" -> 3, "b" -> 4, "c" -> 5, "d" -> 6, "da" -> 7)) { echoCaptureAndUnmatchedPath })
    "accept [/a]" inThe test("1:")
    "accept [/ab]" inThe test("2:") // FAIL
    "accept [/ba]" inThe test("3:")
    "accept [/b]" inThe test("4:")
    "accept [/c]" inThe test("5:")
    "accept [/d]" inThe test("6:")
    "accept [/da]" inThe test("7:")
    "reject [/e]" inThe test()
    "reject [/ac]" inThe test()
  }

  """path(ListMap("a" -> 1, "ab" -> 2, "ba" -> 3, "b" -> 4, "c" -> 5, "d" -> 6, "da" -> 7))""" should {
    val test = testFor(path(ListMap("a" -> 1, "ab" -> 2, "ba" -> 3, "b" -> 4, "c" -> 5, "d" -> 6, "da" -> 7)) { echoCaptureAndUnmatchedPath })
    "accept [/a]" inThe test("1:")
    "accept [/ab]" inThe test("2:") // FAIL
    "accept [/ba]" inThe test("3:")
    "accept [/b]" inThe test("4:")
    "accept [/c]" inThe test("5:")
    "accept [/d]" inThe test("6:")
    "accept [/da]" inThe test("7:") // FAIL too!
    "reject [/e]" inThe test()
    "reject [/ac]" inThe test()
  }

  """path(ListMap("a" -> 1, "aa" -> 2, "bb" -> 3, "b" -> 4, "c" -> 5, "d" -> 6, "dd" -> 7))""" should {
    val test = testFor(path(ListMap("a" -> 1, "aa" -> 2, "bb" -> 3, "b" -> 4, "c" -> 5, "d" -> 6, "dd" -> 7)) { echoCaptureAndUnmatchedPath })
    "accept [/a]" inThe test("1:")
    "accept [/aa]" inThe test("2:") // FAIL
    "accept [/bb]" inThe test("3:") // PASS!
    "accept [/b]" inThe test("4:")  // PASS too!
    "accept [/c]" inThe test("5:")
    "accept [/d]" inThe test("6:")
    "accept [/dd]" inThe test("7:") // FAIL too!
    "reject [/e]" inThe test()
    "reject [/ac]" inThe test()
  }

  """path(Map("a" -> 1, "aa" -> 2, "bb" -> 3, "b" -> 4, "c" -> 5, "d" -> 6, "dd" -> 7))""" should {
    val test = testFor(path(Map("a" -> 1, "aa" -> 2, "bb" -> 3, "b" -> 4, "c" -> 5, "d" -> 6, "dd" -> 7)) { echoCaptureAndUnmatchedPath })
    "accept [/a]" inThe test("1:")
    "accept [/aa]" inThe test("2:") // FAIL
    "accept [/bb]" inThe test("3:") // FAIL now!
    "accept [/b]" inThe test("4:")  // PASS
    "accept [/c]" inThe test("5:")
    "accept [/d]" inThe test("6:")  // PASS
    "accept [/dd]" inThe test("7:") // PASS now!
    "reject [/e]" inThe test()
    "reject [/ac]" inThe test()
  }
@gosubpl
Contributor
gosubpl commented Oct 16, 2016

Fun fact: pasted those examples into spray. Got different pattern of failures (probably because the implementation is based on HLists in spray vs. Tuples in akka-http. Nota bene the spray-routing documentation page for PathMatcher DSL has the same text as akka-http for both Map[String, T] and PathMatcher sections (cf. http://spray.io/documentation/1.2.4/spray-routing/path-directives/pathmatcher-dsl/#pathmatcher-dsl ).

@sbrugnoni
sbrugnoni commented Oct 16, 2016 edited

@gosubpl
Thanks for the insights and the additional tests. I'm not sure how these overlaps should be handled. Disallowing them entirely makes the Matcher useless for use cases where the segment matching table is dynamically defined, since there is no way do guarantee that there are no overlaps.

I looked a bit more into it myself, and it seems that this behaviour is not exclusive to the valueMap matcher.

val route = path("a" | "aa") {
  get {  complete("success")  }
}
Get("/a") ~> route ~> check { println(responseAs[String]) } // pass
Get("/aa") ~> route ~> check { println(responseAs[String]) } // rejected. (scala.util.DynamicVariable at (DynamicVariable.scala:58))

whereas changing the segment order again results in the (at least for me) expected behaviour:

val route = path("aa" | "a") {
  get {  complete("success")  }
}
Get("/a") ~> route ~> check { println(responseAs[String]) } // pass
Get("/aa") ~> route ~> check { println(responseAs[String]) } // pass

Seems like having overlapping paths in general is a bad idea.

@jonas
Contributor
jonas commented Nov 7, 2016

@jrudolph It would be great to have your input on this. It looks trivial to fix for 10.0.0 if it is indeed a bug. Right now the result is nondeterministic (i.e. relying on order of map entries) so the outcome could be to document it.

@jonas jonas added a commit to jonas/akka-http that referenced this issue Nov 25, 2016
@jonas jonas Match path maps in order of longest matching key prefix (#394) 9ada011
@jonas jonas added a commit to jonas/akka-http that referenced this issue Nov 30, 2016
@jonas jonas Match path maps in order of longest matching key prefix (#394) d38a01e
@jonas jonas added a commit to jonas/akka-http that referenced this issue Dec 1, 2016
@jonas jonas Match path maps in order of longest matching key prefix (#394) 47a0321
@ktoso ktoso closed this in #586 Jan 5, 2017
@ktoso ktoso added the community label Jan 5, 2017
@jonas jonas was assigned by ktoso Jan 5, 2017
@ktoso ktoso added this to the 10.0.2 milestone Jan 5, 2017
@ktoso
Member
ktoso commented Jan 5, 2017

Solved by #586 will be in 10.0.2

@ktoso ktoso added the bug label Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment